Closed Bug 1214146 Opened 9 years ago Closed 8 years ago

Laggy clip rect with position: fixed inside clip: rect(...)

Categories

(Core :: Panning and Zooming, defect, P2)

48 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox45 --- unaffected
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Keywords: correctness, regression, Whiteboard: [gfx-noted])

Attachments

(3 files)

Attached file testcase (deleted) —
While scrolling the testcase, no red areas should be revealed. This effect is used on http://www.bbc.co.uk/news/resources/idt-248d9ac7-9784-4769-936a-8d3b435857a8 . This is a case we hadn't considered when we worked out the background-attachment:fixed scrolling clip thing.
As far as I can tell, the "clip" property has been deprecated in favor of "clip-path". However, it looks like this case is something where Webkit's / Blink's interpretation of "clip-path" differs from their interpretation of "clip" - they don't apply clip-path clipping to position:fixed descendants. Roc, do you know what the spec says on this matter? Also, do you know what our current plans are with respect to dropping "clip" support from Gecko?
Flags: needinfo?(roc)
Attached file insane testcase with overflow:auto (deleted) —
Now this is just silly.
(In reply to Markus Stange [:mstange] from comment #2) > As far as I can tell, the "clip" property has been deprecated in favor of > "clip-path". I wasn't aware of that, but you're right. http://www.w3.org/TR/css-masking/#clip-property That doesn't say what 'clip' clips, but it appears to me that per spec 'clip-path' should clip fixed-pos descendants, and by analogy 'clip' the reader would assume 'clip' does too. Per spec: > This includes any content, background, borders, text decoration, outline and visible scrolling mechanism of the element to > which the clipping path is applied, and those of its descendants. The fixed-pos element is definitely a descendant. > However, it looks like this case is something where Webkit's / > Blink's interpretation of "clip-path" differs from their interpretation of > "clip" - they don't apply clip-path clipping to position:fixed descendants. That seems like a bug in their implementation of 'clip-path', if I'm reading the spec rightly. Do you want to raise this on www-style for clarification, or shall I? > Roc, do you know what the spec says on this matter? Also, do you know what > our current plans are with respect to dropping "clip" support from Gecko? No plans. The spec says "UAs must support the 'clip' property.", and I imagine that it's important for web compat, so I would be surprised if anyone's planning to remove it. Is it hard for us to make clip and clip-path support position:fixed descendants?
Flags: needinfo?(roc)
FWIW clip-path doesn't clip position:fixed descendants, in Edge too.
Blocks: 1209058
@Kats: this can probably be closed... Name Firefox Version 47.0a1 Build ID 20160201030241 User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0
Flags: needinfo?(bugmail.mozilla)
I can still reproduce this on OS X nightly, leaving open.
Flags: needinfo?(bugmail.mozilla)
Keywords: polish
Whiteboard: [gfx-noted]
No longer blocks: 1209058
This is much worse in 48 with paint skipping.
Markus and I discussed this. The trick here is to accurately communicate to the compositor which clip rects are scrolled by which scroll frames. Certain combinations, includes ones that come up in these testcases, cannot currently be represented by the Layers API. We plan to fix this in two steps: (1) Modify the Layers API to make the representation of clip rects and what they are scrolled by more flexible. (2) Modify Layout code to take advantage of the modified API to set up the correct representation when the "clip" property is used. I will do step (1), in a separate bug. Markus then plans to do step (2), in this bug.
Depends on: 1267438
Keywords: polishcorrectness
(In reply to Botond Ballo [:botond] from comment #9) > I will do step (1), in a separate bug. That will be bug 1267438.
Blocks: 1230228
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Given comment 8, it seems to make sense to track this as a regression in 48.
Version: Trunk → 48 Branch
Kats, which is the "paint skipping" bug you mention in comment 8? Just to set the "depends on/block" better.
Flags: needinfo?(bugmail.mozilla)
Blocks: 1253860
Flags: needinfo?(bugmail.mozilla)
Blocks: 1272536
Depends on: 1278656
No longer depends on: 1278656
Depends on: 1280344
No longer depends on: 1280344
I tried disabling paint-skipping in the presence of a clip, but from the test cases it looks like the clip element can be anywhere inside the scrollframe, so my naive approach [1] didn't work. I don't know if there's an efficient way to detect the presence of a clip'd element anywhere in the subtree. Markus, do you know? [1] https://github.com/staktrace/gecko-dev/commit/6dfe75f4ff9c2fc9f746d6ca2154a88626d273a5
Flags: needinfo?(mstange)
I know how to detect this specific case that breaks, during display list building, specifically in the call to "clipState.SetScrollClipForContainingBlockDescendants" if a certain set of conditions is true. I don't think we can check for this condition ahead of time, unless we want lots of false positives. So what I think we could do is: During display list building, if we detect the case that goes wrong, set a flag on the scroll frame that sticks around forever, and just check that flag when we want to paint skip.
Flags: needinfo?(mstange)
I copied bits of your patch from the pastebin and basically stumbled around the code until I got something that seemed to work: https://github.com/staktrace/gecko-dev/commit/2690e545cbd4e7df64a835a6b6623275bee7100e This turns off paint-skipping in the basic test case (attachment 8673015 [details]) and the "insane" test case (attachment 8673082 [details]) but it doesn't work for the clip-path test case (attachment 8673016 [details]).
Markus, could you take a look at the patch above and see if I'm doing something wrong? (Or better yet, steal the patch and finish it off :)). Do we need different code to catch the clip-path test case?
Flags: needinfo?(mstange)
Your patch looks good. We should file a new bug about clip-path (or mask or filter); that one will need a completely different fix, and I haven't really thought much about how to do it (short of supporting all those properties on the compositor), so I think we should handle it in a separate bug.
Flags: needinfo?(mstange)
With bug 1284586 fixed and uplifted to 48/49, I'm marking this bug wontfix/fix-optional for those releases as the bad behaviour is less bad. We should still try to fix this properly but the proper fix can ride the trains when it lands.
Depends on: 1298218
(In reply to Robert O'Callahan (:roc) (email my personal email if necessary) from comment #4) > > However, it looks like this case is something where Webkit's / > > Blink's interpretation of "clip-path" differs from their interpretation of > > "clip" - they don't apply clip-path clipping to position:fixed descendants. > > That seems like a bug in their implementation of 'clip-path', if I'm reading > the spec rightly. I agree, and it looks like this has been fixed in Webkit in the meantime. Chrome Canary still has the bug, and it's more serious than I had thought; even just applying position:relative will make the clip-path no longer apply to the element. > Is it hard for us to make clip and clip-path support position:fixed > descendants? It turned out to be rather hard, but bug 1298218 implements it for 'clip' at least. 'clip-path' will require support on the compositor, which will probably be implemented in bug 1234485.
Markus, it looks like the blocking bugs from comment 19 have been resolved here. Is there a chance of getting this fixed for 54 or 55, or should we just classify this as a backlog bug?
Flags: needinfo?(mstange)
This has in fact been fixed by bug 1298218.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: needinfo?(mstange)
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
In "Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0", the clip-path variant is again very laggy, while clip works well for me...
(In reply to margianig from comment #22) > In "Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0", > the clip-path variant is again very laggy, while clip works well for me... clip-path should be fixed in Firefox 58 by bug 1382534. I'm a bit surprised by the "again" part, though - was there a previous version where it wasn't laggy?
At least in 56, all three test cases work equally smooth here.
(In reply to margianig from comment #24) > At least in 56, all three test cases work equally smooth here. Do you have APZ enabled in 56? You can check "Asynchronous Pan/Zoom" in about:support to see.
No, it says: "Asynchronous Pan/Zoom none"
Ok, I see. APZ has been available since Firefox 48, but in some configurations it was disabled due to addons or accessibility reasons until Firefox 57 (more specifically, multi-process (e10s) was disabled for those reasons, and APZ depends on e10s). Without APZ, the whole page potentially scrolls more slowly / less smoothly than with APZ, but at least the lag observed in these testcases (caused by the location of the clip rect and the location of the element being out of sync) doesn't occur. So basically, what you're seeing in Firefox 57 is expected. People who have had e10s enabled since Firefox 48, have been seeing the same thing since Firefox 48. The clip-path testcase will scroll smoothly *and* in sync in Firefox 58.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: