Closed Bug 1293125 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- disabled
firefox52 --- fixed

People

(Reporter: mstange, Assigned: botond)

References

Details

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

Attachments

(4 files)

testcase
(deleted), text/html
Details
Testcase, tweaked to disable margin collapsing
(deleted), text/html
Details
Bug 1293125 - Work around a layout issue that causes StickyScrollContainer::GetScrollRanges() to compute malformed rects.
(deleted), text/x-review-board-request
mstange
: review+
gchang
: approval-mozilla-aurora+
Details
Back out the fix from Firefox 51
(deleted), patch
mstange
: review+
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached file testcase (deleted) — 
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.
ni?myself to look into this
Flags: needinfo?(botond)
Whiteboard: [gfx-noted]
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)
(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
Keywords: correctness
Priority: -- → P3
(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.
(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.
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.
Nice detective work!
(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!
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: nobody → botond
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
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+
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
https://hg.mozilla.org/mozilla-central/rev/99a36fecb7ab
Status: NEW → RESOLVED
Closed: 8 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
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?
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+
Depends on: 1316101
Markus, was the testcase in this bug reduced from a page you encountered on the web?
Flags: needinfo?(mstange)
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).
As discussed in bug 1316101 comment 25, we're going to back this out from 51.

        Attachment #8812230 -
        Flags: review?(mstange)

    
  

        Attachment #8812230 -
        Flags: review?(mstange) → review+

    
    

    
  
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)

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: