1316101 - [APZ] Position sticky element not behaving properly

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Cosentino]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20161107030203

Steps to reproduce:

Here is a page that shows the issue, it's a little difficult to explain.

https://www.lkwdspeechsolutions.com:8443/stickybug.html

It is the button at the bottom that isn't being positioned correctly when scrolling to the bottom of the page. It "jumps" when you then move your mouse cursor over the element and then is not positioned correctly when you scroll back up.

I found the regression window using mozregression and it seems that the change in behavior was caused by this:

https://bugzilla.mozilla.org/show_bug.cgi?id=1293125

Comment 1

[Alice0775 White]

setting layers.async-pan-zoom.enabled = false fixes the problem

Comment 2

[Botond Ballo [:botond]]

I'm at a standards meeting this week but I'll have a look as soon as I'm back.

Comment 3

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

Do we expect the fix to be simple enough to uplift all the way to beta?  If so, we can wait for Botond; otherwise, we may want to look for a fix this week, while 51 is still in aurora.

Comment 4

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

I'm not sure. I tried reproducing the bug on OS X Aurora and Nightly but was unable to. I think the ramp-up time for me to try to fix it will probably be high enough that it's worth waiting for Botond to come back. Markus might be able to fix it with lower ramp-up time but he's been overloaded with stuff for a long time so I don't think he'd have cycles for this right away anyway.

Comment 5

[Marcia Knous [:marcia]]

Tracking 52+ for this APZ issue.

Comment 6

[Botond Ballo [:botond]]

I also can't reproduce the issue.

(In reply to Matt Cosentino from comment #0)
> It is the button at the bottom that isn't being positioned correctly when
> scrolling to the bottom of the page.

Is that the "Save" button?

A screenshot or screencast illustrating the the incorrect positioning / behaviour would be helpful.

Comment 7

[Alice0775 White]

Screen capture: https://youtu.be/-bokiY31Njg

Steps To Reproduce:
0. Open https://www.lkwdspeechsolutions.com:8443/stickybug.html
1. Scroll to Bottom by mouse wheel
2. Mouse hover over scroll thumb
3. Scroll to Top by mouse wheel
4. Mouse hover over scroll thumb

Actual Results:
Position sticky element([Save]button) does not stick when scroll page by mouse wheel.
Position sticky element([Save]button) jumps when mouse over scroll thumb.

Expected Results:
Position sticky element([Save]button) should be fixed position.
Position sticky element([Save]button) should not jump when mouse over scroll thumb.

Comment 8

[Botond Ballo [:botond]]

Thank you, Alice!

The page looks completely different for me; it looks like my browser isn't loading some of the page's CSS.

Comment 9

[Alice0775 White]

Attachment: testpage for the record

open index.html

Comment 10

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

(In reply to Botond Ballo [:botond] [standards meeting Nov 7-12] from comment #8)
> Thank you, Alice!
> 
> The page looks completely different for me; it looks like my browser isn't
> loading some of the page's CSS.

Same here.

Comment 11

[Botond Ballo [:botond]]

(In reply to Alice0775 White from comment #9)

Thanks - I can see the issue in the testcase in comment 9.

Comment 12

[Matt Cosentino]

Thanks, Alice. I've fixed the link as well.

Comment 13

[Botond Ballo [:botond]]

(In reply to Milan Sreckovic [:milan] from comment #3)
> Do we expect the fix to be simple enough to uplift all the way to beta?  If
> so, we can wait for Botond; otherwise, we may want to look for a fix this
> week, while 51 is still in aurora.

Since the testcase regressed in this bug was found "in the wild", while the testcase fixed by bug 1293125 was not (as Markus confirmed in bug 1293125 comment 21), I think we can simply back out the fix for bug 1293125 from 51 if necessary.

Comment 14

[Gerry Chang [:gchang]]

Track 51+ as new regression.

Comment 15

[Botond Ballo [:botond]]

Attachment: Reduced testcase

Comment 16

[Botond Ballo [:botond]]

The clamping added in bug 1316101 is inappropriate in the presence of a negative margin.

Comment 17

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #16)
> The clamping added in bug 1316101

In bug 1293125, sorry.

Comment 18

[Botond Ballo [:botond]]

Attachment: Bug 1316101 - Avoid excessive clamping in StickyScrollContainer::GetScrollRanges().

Excessive clamping can cause incorrect behaviour in the presence of negative
margins.

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

Comment 19

[Markus Stange [:mstange]]

Comment on attachment 8810637 [details]
Bug 1316101 - Avoid excessive clamping in StickyScrollContainer::GetScrollRanges().

https://reviewboard.mozilla.org/r/92920/#review92870

::: layout/generic/StickyScrollContainer.cpp:315
(Diff revision 1)
> +  // Make sure |inner| does not extend outside of |outer|. (The consumers of
> +  // the Layers API, to which this information is propagated, expect this
> +  // invariant to hold.) The calculated value of |inner| can sometimes extend
> +  // outside of |outer|, for example due to margin collapsing, since
> +  // GetNormalPosition() returns the actual position after margin collapsing,
> +  // whil e|contain| is calculated based on the frame's GetUsedMargin() which

"whil e|contain|" -> "while |contain|"

::: layout/generic/StickyScrollContainer.cpp:317
(Diff revision 1)
> +  // invariant to hold.) The calculated value of |inner| can sometimes extend
> +  // outside of |outer|, for example due to margin collapsing, since
> +  // GetNormalPosition() returns the actual position after margin collapsing,
> +  // whil e|contain| is calculated based on the frame's GetUsedMargin() which
> +  // is pre-collapsing.
> +  *aInner = aInner->Intersect(*aOuter);

It's not really obvious that this intersection will solve all problems coming from the discrepancy between the two. The intersection does ensure (trivially) that the Layers API contract is upheld, though, so I think we should land this patch. But I fear that there might be bugs caused by comparing pre-collapsing and post-collapsing rectangles that this intersection does not fix.

I'm not really sure what to do about that feeling. We could try to create testcases for all combinations of making {inner, outer} {smaller, larger} due to collapsed margins, and then check whether the sticky element stays fixed in the correct ranges.

Comment 20

[Botond Ballo [:botond]]

(In reply to Markus Stange [:mstange] from comment #19)
> > +  // Make sure |inner| does not extend outside of |outer|. (The consumers of
> > +  // the Layers API, to which this information is propagated, expect this
> > +  // invariant to hold.) The calculated value of |inner| can sometimes extend
> > +  // outside of |outer|, for example due to margin collapsing, since
> > +  // GetNormalPosition() returns the actual position after margin collapsing,
> > +  // whil e|contain| is calculated based on the frame's GetUsedMargin() which
> 
> "whil e|contain|" -> "while |contain|"

I can't quite seem to get that word right, can I? :p

> But I fear that there might be bugs caused by
> comparing pre-collapsing and post-collapsing rectangles that this
> intersection does not fix.

Yes, I think that's likely. My patch in bug 1293125 characterized the situation as "This is hard to fix properly (TODO)".

However, I think those bugs are not likely to be regressions (except in the "APZ regressed this" sense).

I'll add back such a TODO comment, to make it clear that we haven't solved the margin collapsing problem properly.

(I briefly explored the idea of pursuing a proper fix in bug 1293125 before settling on my workaround, but some discussion with :dholbert and :dbaron led me to conclude that it would be fairly involved.)

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8810637 [details]
Bug 1316101 - Avoid excessive clamping in StickyScrollContainer::GetScrollRanges().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92920/diff/1-2/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/343cef5aeb2b
Avoid excessive clamping in StickyScrollContainer::GetScrollRanges(). r=mstange

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/343cef5aeb2b

Comment 24

[Botond Ballo [:botond]]

Markus, what are your thoughts on uplifting this to 51 vs. backing out bug 1293125 from 51?

Comment 25

[Markus Stange [:mstange]]

I'd prefer backing out bug 1293125 for 51, just because it's a little safer.

Comment 26

[Botond Ballo [:botond]]

Comment on attachment 8810637 [details]
Bug 1316101 - Avoid excessive clamping in StickyScrollContainer::GetScrollRanges().

Sounds good - I'll back bug 1293125 out from 51.

We still need to uplift this fix to 52.

Approval Request Comment
[Feature/regressing bug #]:
  Bug 1293125

[User impact if declined]:
  In some cases when a position:sticky element has a negative margin,
  during async scrolling the element can be moved incorrectly, resulting
  in a jump/jitter when the page is repainted.

[Describe test coverage new/current, TreeHerder]:
  The patch adds a reftest that covers this case.

[Risks and why]:
  Low but nonzero. We're touching the same code as the regressing
  patch, which, well, caused a regression.

[String/UUID change made/needed]:
  None.

Comment 27

[Julien Cristau [:jcristau]]

Comment on attachment 8810637 [details]
Bug 1316101 - Avoid excessive clamping in StickyScrollContainer::GetScrollRanges().

fix clamping issue with async-pan-zoom, for aurora52

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea5bb8b2aaa0

Comment 29

[Ryan VanderMeulen [:RyanVM]]

The backout of bug 1293125 has landed for Fx51b3.