Closed Bug 1434250 Opened 6 years ago Closed 6 years ago

Small "margin" between the sticky header and the page's edge

Categories

(Core :: Panning and Zooming, defect, P3)

Product:
Core
Component:
Panning and Zooming
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: julienw, Assigned: botond)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(7 files, 1 obsolete file)

testcase-sticky.html
6.22 KB, text/html
Details
about-support.json
29.96 KB, text/plain
Details
bug-sticky.mp4
4.26 MB, video/mp4
Details
Bug 1434250 - Add a bare-bones Box class.
59 bytes, text/x-review-board-request
bas.schouten
: review+
Details
Bug 1434250 - Infrastructure for working with Box types in Gecko code.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1434250 - Use a Box, rather than a Rect, representation for position:sticky inner/outer rects in the Layers API.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1434250 - Reftest.
59 bytes, text/x-review-board-request
kats
: review+
Details
Attached file testcase-sticky.html — 
STR:
1. Load the attached page.
2. Scroll up the page.
3. Notice there is a slight margin above the header. We can see it clearly thanks to the underlying background.

Notes:
1. This works in Chrome (no margin).
2. This works also in Firefox as soon as we start the devtools.
3. Still in Firefox, if we load the page with the devtools opened, the bug happens until we interact with the devtools, then the issue disappears.
Also sometimes the top border disappear when the bug has disappeared, but we scroll back to the top.
> Also sometimes the top border disappear when the bug has disappeared, but we scroll back to the top.
That top border appears again at the next painting.
Attached file about:support dump (obsolete) — 

    
  

        Attachment #8946619 -
        Attachment mime type: text/plain → application/json

    
  

        Attachment #8946619 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        about-support.json
        — 
      

    


  

    
  

        Attachment #8946620 -
        Attachment mime type: text/plain → application/json

    
  

        Attachment #8946620 -
        Attachment mime type: application/json → text/plain

    
    

    
  
Actually the issue disappears as well when I want to take a capture with Firefox' internal tool.

    
    

    
  

      
      
      
      

        Attached video
          
        bug-sticky.mp4
        — 
      

    


  
Actually even with Gnome's tool the issue disappears right before the screenshot is taken. So here is a video.

    
    

    
  
I can reproduce (on my Ubuntu laptop, using two-finger scrolling on the touchpad at least).  But if disable APZ (layers.async-pan-zoom.enabled), then I cannot reproduce anymore.

Julien, could you confirm that the bug goes away if you turn off layers.async-pan-zoom.enabled and restart Firefox?

(--> Tentatively reclassifying to Panning and Zooming based on my testing.)
Component: Layout → Panning and Zooming

    
    

    
  
Yes I see the same as you do.

    
    

    
  
Definitely looks like an APZ problem. Setting apz.paint_skipping.enabled makes it barely noticeable.

    
    

    
  
(In reply to Botond Ballo [:botond] from comment #9)
> Setting apz.paint_skipping.enabled makes it barely noticeable.

Sorry, setting that pref to *false* makes it barely noticeable.

    
    

    
  
I modified the testcase locally to remove the border to simplify calculations (the bug still reproduces).

With that modification, the initial offset of the sticky element from the top is 8px (from the <body> element's margin).

Conceptually, this 8px is "the amount by which the sticky element can scroll before it needs to start being fixed". For APZ to position the sticky element correctly in the presence of async scrolling, it needs to know this quantity.

Unfortunately, thanks to the contortions we put this number through en route from Layout to APZ, it becomes 7px by the time it arrives at APZ.

  - We start off with the 8px represented as |sticky.y| being 480
    app units in StickyScrollContainer::GetScrollRanges() [1].

  - That then gets encoded in |aInner| [2], with
      aInner.y = -536870911 app units, and
      aInner.height = 536871391 app units
    making aInner.YMost() be 480 app units.

  - In nsDisplayStickyPosition::BuildLayer(), that nsRect is
    converted into float pixels. The correct result would be
      y = -8947848.51667 and
      height = 8947856.51667
    which would yield a YMost() of 8 pixels, but due to
    float's limited precision, it becomes
      y = -8947849
      height = 8947856
    which narrows the difference to 7 pixels.

  - When APZ then queries that YMost() value [4], it sees 7 pixels,
    and uses that to position the sticky element.
      

[1] https://searchfox.org/mozilla-central/rev/e06af9c36a73a27864302cd2f829e6200dee8541/layout/generic/StickyScrollContainer.cpp#289
[2] https://searchfox.org/mozilla-central/rev/e06af9c36a73a27864302cd2f829e6200dee8541/layout/generic/StickyScrollContainer.cpp#303
[3] https://searchfox.org/mozilla-central/rev/e06af9c36a73a27864302cd2f829e6200dee8541/layout/painting/nsDisplayList.cpp#7498
[4] https://searchfox.org/mozilla-central/rev/e06af9c36a73a27864302cd2f829e6200dee8541/gfx/layers/composite/AsyncCompositionManager.cpp#538

    
    

    
  
This is an example of a place where using an x1/y1/x2/y2 ("box") representation for the "inner" rect would solve the problem, since then we would not be trying to represent a small number that is divisible by the app unit ratio (480) as the difference between two large numbers which are not.
Priority: -- → P3
Whiteboard: [gfx-noted]

    
    

    
  
(In reply to Botond Ballo [:botond] from comment #12)
> This is an example of a place where using an x1/y1/x2/y2 ("box")
> representation for the "inner" rect would solve the problem, since then we
> would not be trying to represent a small number that is divisible by the app
> unit ratio (480) as the difference between two large numbers which are not.

To clarify a bit, divisibility by the app unit ratio is a red herring. I tried to fix the issue by rounding  nscoord_MIN and nscoord_MAX to the nearest value that's divisible by the app unit ratio, but that only fixes the issue at unit full zoom. As soon as you have a full zoom in the mix, the quantities are no longer divisible by the app unit ratio and the problem is back.

Rather, the issue is representing a small number as the difference of two large numbers at all (and specifically, the numbers being so large that they eat up all of the mantissa bits in a float, causing a fractional part (which is important in this case) to be discarded after division).

(The conclusion that using a "box" representation would fix the problem still holds. I'm thinking of trying a solution along those lines, introducing a very bare-bones Box class for the purpose.)

    
    

    
      
  


  

    
  
Assignee: nobody → botond

    
    

    
  
Comment on attachment 8949115 [details]
Bug 1434250 - Infrastructure for working with Box types in Gecko code.

https://reviewboard.mozilla.org/r/218510/#review224290

::: gfx/src/nsCoordBox.h:24
(Diff revision 1)
> +  nsCoordBox() : Super() {}
> +  nsCoordBox(nscoord aX1, nscoord aY1, nscoord aX2, nscoord aY2) :
> +      Super(aX1, aY1, aX2, aY2) {}
> +};
> +
> +#endif /* NSCOORD_H */

fix comment

        Attachment #8949115 -
        Flags: review?(bugmail) → review+

    
    

    
  
Comment on attachment 8949116 [details]
Bug 1434250 - Use a Box, rather than a Rect, representation for position:sticky inner/outer rects in the Layers API.

https://reviewboard.mozilla.org/r/218512/#review224298

::: layout/generic/StickyScrollContainer.cpp:304
(Diff revision 1)
> -  aInner->SetRect(nscoord_MIN/2, nscoord_MIN/2, nscoord_MAX, nscoord_MAX);
> +  aInner->SetBox(gUnboundedNegative, gUnboundedNegative, gUnboundedPositive, gUnboundedPositive);
>  
>    const nsPoint normalPosition = firstCont->GetNormalPosition();
>  
>    // Bottom and top
> -  if (stick.YMost() != (nscoord_MAX + (nscoord_MIN/2))) {
> +  if (stick.y2 != gUnboundedPositive) {

You added XMost()/YMost() functions on BaseBox, so you could keep the old calls to that instead of using x2/y2 directly here. Doesn't matter much but reduces the diff somewhat.

        Attachment #8949116 -
        Flags: review?(bugmail) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> Comment on attachment 8949115 [details]
> Bug 1434250 - Infrastructure for working with Box types in Gecko code.
> 
> https://reviewboard.mozilla.org/r/218510/#review224290
> 
> ::: gfx/src/nsCoordBox.h:24
> (Diff revision 1)
> > +  nsCoordBox() : Super() {}
> > +  nsCoordBox(nscoord aX1, nscoord aY1, nscoord aX2, nscoord aY2) :
> > +      Super(aX1, aY1, aX2, aY2) {}
> > +};
> > +
> > +#endif /* NSCOORD_H */
> 
> fix comment

Fixed.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #18)
> Comment on attachment 8949116 [details]
> Bug 1434250 - Use a Box, rather than a Rect, representation for
> position:sticky inner/outer rects in the Layers API.
> 
> https://reviewboard.mozilla.org/r/218512/#review224298
> 
> ::: layout/generic/StickyScrollContainer.cpp:304
> (Diff revision 1)
> > -  aInner->SetRect(nscoord_MIN/2, nscoord_MIN/2, nscoord_MAX, nscoord_MAX);
> > +  aInner->SetBox(gUnboundedNegative, gUnboundedNegative, gUnboundedPositive, gUnboundedPositive);
> >  
> >    const nsPoint normalPosition = firstCont->GetNormalPosition();
> >  
> >    // Bottom and top
> > -  if (stick.YMost() != (nscoord_MAX + (nscoord_MIN/2))) {
> > +  if (stick.y2 != gUnboundedPositive) {
> 
> You added XMost()/YMost() functions on BaseBox, so you could keep the old
> calls to that instead of using x2/y2 directly here. Doesn't matter much but
> reduces the diff somewhat.

Indeed, I meant to change these back after I added XMost()/YMost(). Fixed.

    
    

    
      
  


  

    
    

    
  
Comment on attachment 8949114 [details]
Bug 1434250 - Add a bare-bones Box class.

https://reviewboard.mozilla.org/r/218508/#review224544

I don't mind this patch but this needs a better name, I discussed some with Jeff and think this needs a better name, let's refactor Rect soon :).

        Attachment #8949114 -
        Flags: review?(bas) → review+

    
    

    
  
Any chance of making x1, x2, y1, y2 protected?

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Milan Sreckovic [:milan] from comment #29)
> Any chance of making x1, x2, y1, y2 protected?

Yes, good idea! Changed in the updated patches.

    
    

    
  
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/19c2bf005486
Add a bare-bones Box class. r=bas
https://hg.mozilla.org/integration/autoland/rev/d2a0298fb0fd
Infrastructure for working with Box types in Gecko code. r=kats
https://hg.mozilla.org/integration/autoland/rev/4b018dee7323
Use a Box, rather than a Rect, representation for position:sticky inner/outer rects in the Layers API. r=kats
https://hg.mozilla.org/integration/autoland/rev/7ad46a63909c
Reftest. r=kats

    
  
Blocks: 1442767

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: