Markus Stange [:mstange] Reporter
	Description • 9 years ago

The blue box in the testcase should always stay fixed.

	Markus Stange [:mstange] Reporter
	Comment 2 • 9 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 3 • 9 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 4 • 8 years ago

That patch is in bug 1231538 and it doesn't fix this bug.

	Botond Ballo [:botond] Assignee
	Comment 5 • 8 years ago

Let's fix this.

	Botond Ballo [:botond] Assignee
	Comment 6 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/61406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61406/

	Botond Ballo [:botond] Assignee
	Comment 7 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/61408/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61408/

	Botond Ballo [:botond] Assignee
	Comment 8 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/61410/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61410/

	Botond Ballo [:botond] Assignee
	Comment 9 • 8 years ago

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.

	Markus Stange [:mstange] Reporter
	Comment 10 • 8 years ago

Comment on attachment 8766567 [details]
Bug 1214151 - Reftest for the common case.

https://reviewboard.mozilla.org/r/61406/#review62588

	Markus Stange [:mstange] Reporter
	Comment 11 • 8 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 12 • 8 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=293a3fe8623a

	Botond Ballo [:botond] Assignee
	Comment 13 • 8 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 14 • 8 years ago

(In reply to Botond Ballo [:botond] from comment #13)
> Anyways, I'll file a follow-up for the edge case.

Filed bug 1288210.

	Pulsebot
	Comment 15 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 16 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ff261b9d118a
https://hg.mozilla.org/mozilla-central/rev/2c5aecb17cb2

	Petruta Horea [:phorea], Desktop QA
	Comment 17 • 8 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 18 • 8 years ago

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

	Petruta Horea [:phorea], Desktop QA
	Comment 19 • 8 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 20 • 8 years ago

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

	Petruta Horea [:phorea], Desktop QA
	Comment 21 • 8 years ago

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.