position:sticky element scrolls at twice the regular speed with APZ in this testcase

RESOLVED FIXED in Firefox 52

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mstange, Assigned: botond)

Tracking

({correctness})

Trunk
mozilla52
correctness
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 disabled, firefox52 fixed)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
Created attachment 8778719 [details]
testcase

STR:
 1. Load the testcase.
 2. Scroll up or down a bit.
 3. Do something that causes a main thread paint, e.g. hovering the scrollbar.

The blue square snaps back to the place that it's supposed to be at, which is about halfway along the way that APZ scrolled it.
(Assignee)

Comment 1

2 years ago
ni?myself to look into this
Flags: needinfo?(botond)

Updated

2 years ago
Whiteboard: [gfx-noted]
(Assignee)

Comment 2

2 years ago
Layout is populating the Layers API with bad sticky position data:

[isStickyPosition scrollId=3 outer=(-8947849.000,74.000)-(8947849.000,8947848.000) inner=(-8947849.000,-326.000)-(8947849.000,8947849.000)]

Observe that inner.y < outer.y, violating the property that 'inner' is inside 'outer'. This is turn causes calculations in AsyncCompositionManager to produce bad results.

I presume this is a regression?
Component: Panning and Zooming → Layout
Flags: needinfo?(botond)
(Assignee)

Comment 3

2 years ago
(In reply to Botond Ballo [:botond] from comment #2)
> I presume this is a regression?

If it is, it's not a very recent regression: 48 is affected (with e10s+apz enabled, of course).
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected
(Assignee)

Updated

2 years ago
Keywords: correctness
Priority: -- → P3
(Assignee)

Comment 4

2 years ago
(In reply to Botond Ballo [:botond] from comment #2)
> Layout is populating the Layers API with bad sticky position data:
> 
> [isStickyPosition scrollId=3
> outer=(-8947849.000,74.000)-(8947849.000,8947848.000)
> inner=(-8947849.000,-326.000)-(8947849.000,8947849.000)]
> 
> Observe that inner.y < outer.y, violating the property that 'inner' is
> inside 'outer'. This is turn causes calculations in AsyncCompositionManager
> to produce bad results.

I talked about this briefly with :dholbert. It may be the case that inner protruding from outer is legitimate in some cases, and AsyncCompositionManager just needs to handle it.
(Assignee)

Comment 5

2 years ago
(In reply to Botond Ballo [:botond] from comment #4)
> I talked about this briefly with :dholbert. It may be the case that inner
> protruding from outer is legitimate in some cases, and
> AsyncCompositionManager just needs to handle it.

After further investigation, I've come to believe that, while it may be legitimate for inner to protrude from outer in some cases, that's not the case in this testcase. In this testcase, Layout is computing wrong values for inner and outer.
(Assignee)

Comment 6

2 years ago
Created attachment 8798257 [details]
Testcase, tweaked to disable margin collapsing

Here's a modified version of the testcase, with a single "a" character added before the div.

Observe that it does not reproduce the bug, i.e. it async-scrolls correctly!

How, you ask? It appears that the reason the Layout calculation of |inner| and |outer| is incorrect, is that part of the calculation fails to account for "margin collapsing", where the margin on a child element (the position:sticky <div> in this case) is transferred to be on the parent (the <body> in this case) instead. The addition of the "a" disables margin collapsing, so we don't encounter the bug.
(Reporter)

Comment 7

2 years ago
Nice detective work!
(Assignee)

Comment 8

2 years ago
(In reply to Markus Stange [:mstange] from comment #7)
> Nice detective work!

The credit belongs to :dholbert, who's been patiently helping me debug this!
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
It looks like a proper solution to this is hard (among other things, it's not even clear how position:sticky is supposed to interact with margin collapsing), but we can apply a workaround that fixes this and similar testcases; that's what the posted patch does.
(Assignee)

Updated

2 years ago
Assignee: nobody → botond
(Reporter)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8799065 [details]
Bug 1293125 - Work around a layout issue that causes StickyScrollContainer::GetScrollRanges() to compute malformed rects.

https://reviewboard.mozilla.org/r/84366/#review83608

::: layout/generic/StickyScrollContainer.cpp:288
(Diff revision 1)
>    aOuter->SetRect(nscoord_MIN/2, nscoord_MIN/2, nscoord_MAX, nscoord_MAX);
>    aInner->SetRect(nscoord_MIN/2, nscoord_MIN/2, nscoord_MAX, nscoord_MAX);
>  
> -  const nsPoint normalPosition = firstCont->GetNormalPosition();
> +  // Due to margin collapsing, |firstCont->GetNormalPosition()| can sometimes
> +  // fall otuside of |contain|. (This is because GetNormalPosition() returns
> +  // the actual position after margin collapsing, which |contain| is

typo: which -> while
(Reporter)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8799065 [details]
Bug 1293125 - Work around a layout issue that causes StickyScrollContainer::GetScrollRanges() to compute malformed rects.

https://reviewboard.mozilla.org/r/84366/#review83610
Attachment #8799065 - Flags: review?(mstange) → review+

Comment 14

2 years ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99a36fecb7ab
Work around a layout issue that causes StickyScrollContainer::GetScrollRanges() to compute malformed rects. r=mstange

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/99a36fecb7ab
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Is this something you would like to uplift?
status-firefox48: affected → wontfix
status-firefox49: affected → wontfix
(Assignee)

Comment 17

2 years ago
Comment on attachment 8799065 [details]
Bug 1293125 - Work around a layout issue that causes StickyScrollContainer::GetScrollRanges() to compute malformed rects.

It seems safe enough to uplift to aurora. Given that the issue hasn't been reported in the wild (as far as I'm aware), and that we're late in the beta cycle, I'm not going to request uplift to beta unless someone feels strongly that we should.

Approval Request Comment
[Feature/regressing bug #]:
  APZ

[User impact if declined]:
  On certain pages where a position:sticky element has a margin
  and that margin is collapsed with the margin of a parent
  element, the element can be positioned incorrectly during
  async scrolling, resulting in a jitter or jump when the page
  is repainted.

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

[Risks and why]: 
  Low but nonzero.
 
[String/UUID change made/needed]:
  None.
Attachment #8799065 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 years ago
status-firefox50: affected → wontfix
Comment on attachment 8799065 [details]
Bug 1293125 - Work around a layout issue that causes StickyScrollContainer::GetScrollRanges() to compute malformed rects.

Fix an issue related to layout. Take it in 51 aurora.
Attachment #8799065 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 19

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/4b705aaa6f5a
status-firefox51: affected → fixed
Flags: in-testsuite+

Updated

2 years ago
Depends on: 1316101
(Assignee)

Comment 20

2 years ago
Markus, was the testcase in this bug reduced from a page you encountered on the web?
Flags: needinfo?(mstange)
(Reporter)

Comment 21

2 years ago
It was reduced from something I encountered while trying to create a standalone python-docs-style sticky sidebar.
Flags: needinfo?(mstange)
We may want to back this out from 51 since it caused another regression (in bug 1316101).
(Assignee)

Comment 23

2 years ago
As discussed in bug 1316101 comment 25, we're going to back this out from 51.
(Assignee)

Comment 24

2 years ago
Created attachment 8812230 [details] [diff] [review]
Back out the fix from Firefox 51
Attachment #8812230 - Flags: review?(mstange)
(Reporter)

Updated

2 years ago
Attachment #8812230 - Flags: review?(mstange) → review+
(Assignee)

Comment 25

2 years ago
Comment on attachment 8812230 [details] [diff] [review]
Back out the fix from Firefox 51

Requesting beta approval for the backout:

Approval Request Comment
[Feature/regressing bug #]:
  The patch that landed on 51 in this bug.

[User impact if declined]:
  The regression in bug 1316101 will affect beta.

[Describe test coverage new/current, TreeHerder]:
  N/A

[Risks and why]:
  Very low - we are reverting to a previous known state.

[String/UUID change made/needed]:
  None.
Attachment #8812230 - Flags: approval-mozilla-beta?
Comment on attachment 8812230 [details] [diff] [review]
Back out the fix from Firefox 51

Back out patch for fixing bug 1316101. Beta51+. Should be in 51 beta 2.

Hi Florin,
Could you please help verify if bug 1316101 is fixed once the patch is backed out?
Flags: needinfo?(florin.mezei)
Attachment #8812230 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Moving the needinfo over to Petruta who's the main QA for APZ related stuff - Petruta can you check that bug 1316101 works fine after the back out from this bug lands (we expect that will be 51 Beta 2)?
Flags: needinfo?(florin.mezei) → needinfo?(petruta.rasa)
Bug 1316101 is still reproducible on Firefox 51 beta 2, Win 10 64-bit.
It seems that the patch was not backed out, leaving the needinfo for verification in 51 beta 3.
Flags: needinfo?(petruta.rasa)

Updated

2 years ago
status-firefox51: fixed → affected

Comment 29

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/ef45a0a73643
status-firefox51: affected → disabled
Moving again to Petruta as this should be in Beta 3 tomorrow.
Flags: needinfo?(petruta.rasa)
Checked that bug 1316101 is fixed using Firefox 51 beta 3 under Win 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.10.
Flags: needinfo?(petruta.rasa)
You need to log in before you can comment on or make changes to this bug.