Closed Bug 1316101 Opened 3 years ago Closed 3 years ago

[APZ] Position sticky element not behaving properly

Categories

(Core :: Layout, defect)

51 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 + fixed
firefox52 + fixed
firefox53 --- fixed

People

(Reporter: mattcoz, Assigned: botond)

References

Details

(Keywords: regression)

Attachments

(3 files)

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
Component: Untriaged → Layout
Product: Firefox → Core
Version: 52 Branch → 51 Branch
Blocks: 1293125
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
setting layers.async-pan-zoom.enabled = false fixes the problem
Flags: needinfo?(botond)
Summary: Position sticky element not behaving properly → [APZ] Position sticky element not behaving properly
I'm at a standards meeting this week but I'll have a look as soon as I'm back.
Assignee: nobody → botond
Flags: needinfo?(botond)
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.
Flags: needinfo?(bugmail)
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.
Flags: needinfo?(bugmail)
Tracking 52+ for this APZ issue.
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.
Flags: needinfo?(mattcoz)
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.
Thank you, Alice!

The page looks completely different for me; it looks like my browser isn't loading some of the page's CSS.
open index.html
(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.
(In reply to Alice0775 White from comment #9)

Thanks - I can see the issue in the testcase in comment 9.
Flags: needinfo?(mattcoz)
Thanks, Alice. I've fixed the link as well.
(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.
Track 51+ as new regression.
Attached file Reduced testcase
The clamping added in bug 1316101 is inappropriate in the presence of a negative margin.
(In reply to Botond Ballo [:botond] from comment #16)
> The clamping added in bug 1316101

In bug 1293125, sorry.
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.
Attachment #8810637 - Flags: review?(mstange) → review+
(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.)
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/343cef5aeb2b
Avoid excessive clamping in StickyScrollContainer::GetScrollRanges(). r=mstange
https://hg.mozilla.org/mozilla-central/rev/343cef5aeb2b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Markus, what are your thoughts on uplifting this to 51 vs. backing out bug 1293125 from 51?
Flags: needinfo?(mstange)
I'd prefer backing out bug 1293125 for 51, just because it's a little safer.
Flags: needinfo?(mstange)
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.
Attachment #8810637 - Flags: approval-mozilla-aurora?
Comment on attachment 8810637 [details]
Bug 1316101 - Avoid excessive clamping in StickyScrollContainer::GetScrollRanges().

fix clamping issue with async-pan-zoom, for aurora52
Attachment #8810637 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The backout of bug 1293125 has landed for Fx51b3.
You need to log in before you can comment on or make changes to this bug.