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

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

Attachment: 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.

Comment 1

[Markus Stange [:mstange]]

Attachment: 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

Comment 2

[Markus Stange [:mstange]]

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?

Comment 3

[Markus Stange [:mstange]]

Attachment: insane testcase with overflow:auto

Now this is just silly.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

FWIW clip-path doesn't clip position:fixed descendants, in Edge too.

Comment 6

[Michelle Funches - QA]

@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

Comment 7

[Kartikaya Gupta (email:[e-mail])]

I can still reproduce this on OS X nightly, leaving open.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

This is much worse in 48 with paint skipping.

Comment 9

[Botond Ballo [:botond]]

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.

Comment 10

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #9)
> I will do step (1), in a separate bug.

That will be bug 1267438.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Given comment 8, it seems to make sense to track this as a regression in 48.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Kats, which is the "paint skipping" bug you mention in comment 8?  Just to set the "depends on/block" better.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

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

Comment 14

[Markus Stange [: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.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

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

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 17

[Markus Stange [: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.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 19

[Markus Stange [:mstange]]

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

Comment 20

[Nathan Froyd [:froydnj]]

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?

Comment 21

[Markus Stange [:mstange]]

This has in fact been fixed by bug 1298218.

Comment 22

[margianig]

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

[Botond Ballo [:botond]]

(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

[margianig]

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

Comment 25

[Botond Ballo [:botond]]

(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

[margianig]

No, it says: "Asynchronous Pan/Zoom	none"

Comment 27

[Botond Ballo [:botond]]

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.