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)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: mstange, Assigned: mstange)
References
Details
(Keywords: correctness, regression, Whiteboard: [gfx-noted])
Attachments
(3 files)
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.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
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.
Comment 6•9 years ago
|
||
@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)
Comment 7•9 years ago
|
||
I can still reproduce this on OS X nightly, leaving open.
status-firefox44:
affected → ---
status-firefox45:
--- → unaffected
status-firefox46:
--- → affected
status-firefox47:
--- → affected
Flags: needinfo?(bugmail.mozilla)
Comment 8•9 years ago
|
||
This is much worse in 48 with paint skipping.
Comment 9•9 years ago
|
||
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.
Updated•9 years ago
|
Keywords: polish → correctness
Comment 10•9 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #9)
> I will do step (1), in a separate bug.
That will be bug 1267438.
Assignee | ||
Updated•9 years ago
|
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
Updated•9 years ago
|
status-firefox49:
--- → affected
Keywords: regressionwindow-wanted
Kats, which is the "paint skipping" bug you mention in comment 8? Just to set the "depends on/block" better.
Flags: needinfo?(bugmail.mozilla)
Updated•9 years ago
|
Keywords: regressionwindow-wanted
Comment 13•8 years ago
|
||
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)
Assignee | ||
Comment 14•8 years ago
|
||
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)
Comment 15•8 years ago
|
||
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]).
Comment 16•8 years ago
|
||
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)
Assignee | ||
Comment 17•8 years ago
|
||
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)
Comment 18•8 years ago
|
||
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.
Updated•8 years ago
|
Assignee | ||
Comment 19•8 years ago
|
||
(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.
Updated•8 years ago
|
status-firefox52:
--- → affected
Priority: -- → P2
Updated•8 years ago
|
Updated•8 years ago
|
Comment 20•8 years ago
|
||
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)
Assignee | ||
Comment 21•8 years ago
|
||
This has in fact been fixed by bug 1298218.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: needinfo?(mstange)
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•8 years ago
|
status-firefox-esr52:
--- → wontfix
Comment 22•7 years ago
|
||
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...
Comment 23•7 years ago
|
||
(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?
Comment 24•7 years ago
|
||
At least in 56, all three test cases work equally smooth here.
Comment 25•7 years ago
|
||
(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.
Comment 26•7 years ago
|
||
No, it says: "Asynchronous Pan/Zoom none"
Comment 27•7 years ago
|
||
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.
Description
•