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

RESOLVED FIXED in Firefox 54

Status

()

P2
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

({correctness, regression})

48 Branch
mozilla54
correctness, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 unaffected, firefox46 wontfix, firefox47 wontfix, firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
Created attachment 8673015 [details]
testcase

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

3 years ago
Created attachment 8673016 [details]
same testcase using clip-path instead of clip - jittery in Firefox because clip-path isn't supported by the compositor, and unclipped in Chrome + Safari
(Assignee)

Comment 2

3 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

3 years ago
Created attachment 8673082 [details]
insane testcase with overflow:auto

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.
(Assignee)

Updated

3 years ago
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.
status-firefox44: affected → ---
status-firefox45: --- → unaffected
status-firefox46: --- → affected
status-firefox47: --- → affected
Flags: needinfo?(bugmail.mozilla)
Keywords: polish
Whiteboard: [gfx-noted]
(Assignee)

Updated

3 years ago
No longer blocks: 1209058
This is much worse in 48 with paint skipping.
status-firefox46: affected → wontfix
status-firefox48: --- → affected
Keywords: regression
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

3 years ago
Depends on: 1267438

Updated

3 years ago
Keywords: polish → correctness
(In reply to Botond Ballo [:botond] from comment #9)
> I will do step (1), in a separate bug.

That will be bug 1267438.

Updated

3 years ago
Blocks: 1230228
(Assignee)

Updated

3 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

2 years ago
status-firefox47: affected → wontfix
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)
Blocks: 1253860
Flags: needinfo?(bugmail.mozilla)
Keywords: regressionwindow-wanted

Updated

2 years ago
Blocks: 1272536
(Assignee)

Updated

2 years ago
Depends on: 1278656
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 14

2 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)
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)
(Assignee)

Comment 17

2 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)
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.
status-firefox48: affected → wontfix
status-firefox49: affected → fix-optional
status-firefox50: --- → affected
status-firefox49: fix-optional → wontfix
status-firefox50: affected → fix-optional
status-firefox51: --- → affected
(Assignee)

Updated

2 years ago
Depends on: 1298218
(Assignee)

Comment 19

2 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.
status-firefox50: fix-optional → wontfix
status-firefox51: affected → fix-optional
status-firefox52: --- → affected
Priority: -- → P2
status-firefox51: fix-optional → wontfix
status-firefox52: affected → fix-optional
status-firefox53: --- → affected
status-firefox52: fix-optional → wontfix
status-firefox53: affected → wontfix
status-firefox54: --- → affected
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

2 years ago
This has in fact been fixed by bug 1298218.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: affected → fixed
Flags: needinfo?(mstange)
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
status-firefox-esr52: --- → wontfix

Comment 22

11 months 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

11 months 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

11 months ago
At least in 56, all three test cases work equally smooth here.

Comment 25

11 months 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

11 months ago
No, it says: "Asynchronous Pan/Zoom	none"

Comment 27

11 months 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.