Closed Bug 1214151 Opened 4 years ago Closed 3 years ago

Async scroll adjustment wrong for position:fixed elements inside scrolling position:sticky elements

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox44 --- affected
firefox50 --- verified

People

(Reporter: mstange, Assigned: botond)

References

Details

(Keywords: polish, Whiteboard: [gfx-noted])

Attachments

(6 files, 2 obsolete files)

Attached file testcase (obsolete) —
The blue box in the testcase should always stay fixed.
Attached file testcase (obsolete) —
Attachment #8673028 - Attachment is obsolete: true
Attached file testcase
Added -webkit-sticky. Safari 9 fails the testcase in the same way we do, but they don't correct their mistake through a new main thread transaction the way we do - it just stays wrong, until you reload the page, which keeps the scroll position and fixes up the positioning.
Attachment #8673029 - Attachment is obsolete: true
OS: Unspecified → All
Hardware: Unspecified → All
Matt has a patch to make us always use ContainerLayers for fixed position elements. That might make this bug easier to fix, or even fix it on its own.
That patch is in bug 1231538 and it doesn't fix this bug.
Keywords: polish
Whiteboard: [gfx-noted]
Let's fix this.
Assignee: nobody → botond
The first patch turns the testcase posted to this bug into a reftest.

The second patch fixes the issue, in most but not all cases, including the testcase in the first patch.

The third patch demonstrates an edge case for which the patch does not fix the issue, and a more sophisticated patch would be necessary.

The issue here is that when we need to apply an un-adjustment corresponding to async scroll to a fixed or sticky layer, a fixed layer "consumes" the entire un-adjustment, while a sticky layer may consume all, part, or none of it, depending on where we are relative to its sticky scroll range. In the case where a sticky layer did not consume all of it, we need to give a possible fixed or sticky descendant of the sticky layer a chance to consume the rest.

My patch just passes the entire un-adjustment down to descendant layers whenever a sticky layer did not consume all of it. This is correct when the sticky layer consumed *none* of the un-adjustment, but it's incorrect when the sticky layer consumed *part* of it; in this case, we should be passing down only the un-consumed portion. This is non-trivial to calculate (all the transformations we performed to derive the un-adjustment from |aPreviousTransformForRoot| and |aCurrentTransformForRoot| would need to be performed in reverse to come up with a new value for |aCurrentTransformForRoot| that reflects the partially consumed un-adjustment), so I'm inclined to punt on it (or at least defer it to a follow-up) unless someone feels it's an important case.
Comment on attachment 8766568 [details]
Bug 1214151 - If a sticky element does not consume all of the un-adjustment for an async transform, allow a descendant fixed or sticky element to consume it.

https://reviewboard.mozilla.org/r/61408/#review62590

Looks good. For some reason I thought this patch would be bigger.

I'm still wondering if we can simplify the transform handling code at some point, because AFAIK the root scroll frame container is the only scenario where there can be a transform between a fixed layer and the first unscrolled ancestor layer. Doing so might make fixing the "edge case" easier.
Attachment #8766568 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #11)
> I'm still wondering if we can simplify the transform handling code at some
> point, because AFAIK the root scroll frame container is the only scenario
> where there can be a transform between a fixed layer and the first
> unscrolled ancestor layer. Doing so might make fixing the "edge case" easier.

I definitely recall the localTransform part being necessary for correctness. The ancestorTransform part was as well at some point, although that was quite a while ago and things may have changed since then (introduction of containerless scrolling and such). Anyways, I'll file a follow-up for the edge case.
Blocks: 1288210
(In reply to Botond Ballo [:botond] from comment #13)
> Anyways, I'll file a follow-up for the edge case.

Filed bug 1288210.
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff261b9d118a
Reftest for the common case. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c5aecb17cb2
If a sticky element does not consume all of the un-adjustment for an async transform, allow a descendant fixed or sticky element to consume it. r=mstange
https://hg.mozilla.org/mozilla-central/rev/ff261b9d118a
https://hg.mozilla.org/mozilla-central/rev/2c5aecb17cb2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Flags: qe-verify+
Attached image blue box.gif
Botond, the blue box is still a little jumping on Firefox 50 beta 9 with e10s enabled. Is this the case fixed in bug 1288210? Can you please have a look? 

Let me know if I can help in further investigating this.
Flags: needinfo?(botond)
(In reply to Petruta Rasa [QA] [:petruta] from comment #17)
> Botond, the blue box is still a little jumping on Firefox 50 beta 9 with
> e10s enabled. Is this the case fixed in bug 1288210? Can you please have a
> look? 

Yes, the jumping depicted in this video is bug 1288210.

(If you look at the behaviour in Firefox 49, you can see that there, the blue box scrolls even at the beginning, before the black line approaches it. That's what was fixed in this bug.)

Thanks for checking!
Flags: needinfo?(botond)
Attached image missing blue box.gif
I've spotted another issue that was not present when I previously tested this.

Now, on e10s builds only, the blue box disappears or is partially hidden under url bar. This reproduces with Firefox 50 beta 11 and Developer Edition 51.0a2.
Flags: needinfo?(botond)
(In reply to Petruta Rasa [QA] [:petruta] from comment #19)
> Created attachment 8805963 [details]
> missing blue box.gif
> 
> I've spotted another issue that was not present when I previously tested
> this.
> 
> Now, on e10s builds only, the blue box disappears or is partially hidden
> under url bar. This reproduces with Firefox 50 beta 11 and Developer Edition
> 51.0a2.

Thanks for the continued testing :) 

I had a look, and this is still bug 1288210.

After a downward scroll, the box can be incorrectly positioned lower than it should be (this is depicted in the screen capture in comment 17). Similarly, after an upward scroll, the box can be incorrectly positioned higher than it should be (this is depicted in the screen capture in comment 19). In cases where this high position intersects with the URL bar, the box is cut off (because web content is clipped to the content area), and in cases where the high position is above the content area altogether, it looks as if the box disappeared.

In both cases, the incorrect positioning only occurs when the scrolling starts at a place where the element is near the sticky-position boundary (where the black line stops moving and starts staying in one position), and in both cases, the positioning is corrected by a repaint (e.g. by clicking on the page, or hovering over the scrolbar).

Due to the aggressive paint-skipping implemented in bug 1257641 and similar bugs, the incorrect positioning can persist for a long time (as depicted in both screen captures) if the user doesn't do anything that forces a repaint. (Previously, we would repaint after every scroll offset change, so any incorrect positioning would typically only last for a fraction of a second.) This was a tradeoff we were aware of when implementing the aggressive paint skipping: that in some edge cases, incorrect rendering would persist for longer, in exchange for increased responsiveness from keeping the main thread less busy painting.

I'm still open to uplifting bug 1288210 to 51 (but not 50, it's too late for that), if a user encounters this issue in the wild.
Flags: needinfo?(botond)
Thanks for clarifying this!

Considering the above comments and the fact that the remaining issues are fixed in follow-up bug 1288210, I'm marking this bug as verified.

I used Firefox 50 beta 11 under Win 10 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
See Also: → 1514291
You need to log in before you can comment on or make changes to this bug.