1012752 - Full-page invalidation when scrolling to the very bottom of a page with fractional height

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

STR:
 1. Enable paint flashing.
 2. Scroll to the bottom of this bug.

Nearly every website has a fractional scrollable height. When hitting the bottom of the scrollable area, the layer pixel alignment changes, so the scrollable layer contents get repainted with the new alignment.

Comment 1

[Markus Stange [:mstange]]

Attachment: round scrollable area to device pixels

Is there a reason for not just doing this?

When this increases the scrollable size we're leaving a fractional gap at the bottom, at least in the app unit representation, and I think I've seen a case where scrolling to the bottom changed the layer from opaque to non-opaque... so this aspect should be investigated more carefully before this patch can land.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8424948 [details] [diff] [review]
round scrollable area to device pixels

Review of attachment 8424948 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +4447,5 @@
> +  maximumScrollOffset = maximumScrollOffset.ToNearestPixels(appUnitsPerDevPixel).ToAppUnits(appUnitsPerDevPixel);
> +
> +  nsPoint scrolledAreaBottomRight = maximumScrollOffset + scrollPortSize;
> +  result.SetRightEdge(scrolledAreaBottomRight.x);
> +  result.SetBottomEdge(scrolledAreaBottomRight.y);

If we increase the scrollable height here, it's possible to scroll to a state where there's a row of pixels that's not covered by the background of the scrolled content. That's bad, not just because of opacity considerations but it also looks bad.

If we decrease the scrollable height here, in some cases it's not possible to scroll the bottom row of pixels into view. That's a problem if the scrolled content has a 1px border.

There may be a way to fix this by hacking the height of the scrolled content to be an integer number of device pixels, but I don't have a good idea for that. The problem is that any descendant of the scrollframe could be the frame whose border-box determines the bottom of the overflow area for the scrolled content, so we don't know until we've finished reflow which frame needs to be adjusted.

Comment 3

[Markus Stange [:mstange]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> If we increase the scrollable height here, it's possible to scroll to a
> state where there's a row of pixels that's not covered by the background of
> the scrolled content. That's bad, not just because of opacity considerations
> but it also looks bad.

I can't think of a case where this can actually happen. Can you show me one?

> If we decrease the scrollable height here, in some cases it's not possible
> to scroll the bottom row of pixels into view.

We snap borders and backgrounds during rendering. When when would that snapping choose a different snap direction than the snapping of the scrollable area?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It happens when the scrollable container is itself at a fractional position.

<div id='d' style="overflow:hidden; height:10px; position:absolute; top:0.9px; outline:1px solid black">
  <div id='e' style="height:10.5px; background:blue;">
  </div>
</div>
<script>
d.scrollTop = 100; // Scroll to the bottom
</script>

The bottom edge of e's background is at 0.9 - 1 + 10.5, i.e. 10.4, snapping to 10. Unfortunately the top edge of d's bottom border is at 10.9, snapping to 11.

The problem is that snapping happens relative to the viewport (or transformed ancestor, but that's not important here). Trying to make snapping aware of scrollframes would mean adding "overflow" would change the rendering.

Comment 5

[Markus Stange [:mstange]]

Thank you.

Can we take the scrollable container's position into account when snapping the size of the scrollable content? That would mean that the frame's scroll range can differ depending on its position, but would that break something?

Comment 6

[Matt Woodrow (:mattwoodrow)]

What do the other browsers do for this?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Markus Stange [:mstange] from comment #5)
> Can we take the scrollable container's position into account when snapping
> the size of the scrollable content? That would mean that the frame's scroll
> range can differ depending on its position, but would that break something?

This is probably a good idea. A bit tricky to implement though. It might be worth focusing on the case where you're scrolling the viewport of some document and you can prove that the viewport origin is device pixel aligned --- obviously, a very common case --- and clamping the scroll range to whole device pixels in that case.

Comment 8

[Markus Stange [:mstange]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> you can prove that the viewport origin is device pixel aligned

I think this is not even the case with the current Firefox Mac theme. Is this worth fixing on its own?

Comment 9

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #5)
> That would mean that the frame's scroll range can differ depending on its position

It would also depend on the layer resolution, since we'll want to take that into account when snapping, right? So e.g. zooming on B2G will change a page's scroll range in app units.

Comment 10

[Chris Lord [:cwiiis]]

An idea;

Rather than changing the scroll range to take into account pixel grid snapping, and indeed changing the scroll position to take into account the same, could we have a GetScrollPositionSnapped-style function that we can use when we need a snapped scroll position (so during display-list/layer building and such, I guess?) that would take in the necessary parameters to do the right snapping for the particular context?

Is this feasible, and if so, would it be any easier or harder than what we're doing right now?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Markus Stange [:mstange] from comment #8)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> > you can prove that the viewport origin is device pixel aligned
> 
> I think this is not even the case with the current Firefox Mac theme. Is
> this worth fixing on its own?

Probably yes...

(In reply to Markus Stange [:mstange] from comment #9)
> (In reply to Markus Stange [:mstange] from comment #5)
> > That would mean that the frame's scroll range can differ depending on its position
> 
> It would also depend on the layer resolution, since we'll want to take that
> into account when snapping, right? So e.g. zooming on B2G will change a
> page's scroll range in app units.

True.

(In reply to Chris Lord [:cwiiis] from comment #10)
> Rather than changing the scroll range to take into account pixel grid
> snapping, and indeed changing the scroll position to take into account the
> same, could we have a GetScrollPositionSnapped-style function that we can
> use when we need a snapped scroll position (so during display-list/layer
> building and such, I guess?) that would take in the necessary parameters to
> do the right snapping for the particular context?

I don't think this would avoid the problem in comment #4.

Comment 12

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

This doesn't sound specific to APZ so I'm not convinced it needs to block APZ.

Comment 13

[Markus Stange [:mstange]]

Attachment: wip patch

This has some unnecessary debugging stuff in it but works, iirc. The case it doesn't handle is the one where we need to adjust the scroll position because scroll area snapping has changed from snap-outward to snap-inward and the current scroll position is outside the allowed range. This can happen if the scrolled frame moves, or when its parent scrollable scrolls in a way that changes the parent scrollable's scroll position layer pixel alignment.

Comment 14

[Markus Stange [:mstange]]

Attachment: wip tests

More tests need to be added for all kinds of snapping, and for the missing parts of the patch.

Comment 15

[Markus Stange [:mstange]]

Attachment: testcase with findings from cross-browser testing

Comment 16

[Markus Stange [:mstange]]

Attachment: Bug 1012752 - Snap scrolled area to layer pixels.

We want the maximum scroll position to be aligned with layer pixels. That way
we don't have to re-rasterize the scrolled contents once scrolling hits the
edge of the scrollable area.

Here's how we determine the maximum scroll position: We get the scroll port
rect, snapped to layer pixels. Then we get the scrolled rect and also snap
that to layer pixels. The maximum scroll position is set to the difference
between right/bottom edges of these rectangles.
Now the scrollable area is computed by adding this maximum scroll position
to the unsnapped scroll port size.
The underlying idea here is: Pretend we have overflow:visible so that the
scrolled contents start at (0, 0) relative to the scroll port and spill over
the scroll port edges. When these contents are rendered, their rendering is
snapped to layer pixels. We want those exact pixels to be accessible by
scrolling.

This way of computing the snapped scrollable area ensures that, if you scroll
to the maximum scroll position, the right/bottom edges of the rendered
scrolled contents line up exactly with the right/bottom edges of the scroll
port. The scrolled contents are neither cut off nor are they moved too far.
(This is something that no other browser engine gets completely right, see the
testcase in bug 1012752.)

There are also a few disadvantages to this solution. We snap to layer pixels,
and the size of a layer pixel can depend on the zoom level, the document
resolution, the current screen's scale factor, and CSS transforms. The snap
origin is the position of the reference frame. So a change to any of these
things can influence the scrollable area and the maximum scroll position.
This patch does not make us adjust the current scroll position in the event
that the maximum scroll position changes such that the current scroll position
would be out of range, unless there's a reflow of the scrolled contents. This
means that we can sometimes render a slightly inconsistent state where the
current scroll position exceeds the maximum scroll position. We can fix this
once it turns out to be a problem; I doubt that it will be a problem because
none of the other browsers seems to prevent this problem either.

The size of the scrollable area is exposed through the DOM properties
scrollWidth and scrollHeight. At the moment, these are integer properties, so
their value is rounded to the nearest CSS pixel. Before this patch, the
returned value would always be within 0.5 CSS pixels of the value that layout
computed for the content's scrollable overflow based on the CSS styles of the
contents.
Now that scrollWidth and scrollHeight also depend on pixel snapping, their
values can deviate by up to one layer pixel from what the page might expect
based on the styles of the contents. This change requires a few changes to
existing tests.
The fact that scrollWidth and scrollHeight can change based on the position of
the scrollable element and the zoom level / resolution may surprise some web
pages. However, this also seems to happen in Edge. Edge seems to always round
scrollWidth and scrollHeight upwards, possibly to their equivalent of layout
device pixels.

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

Comment 17

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

This is a new regression in 48, right?

Comment 18

[Markus Stange [:mstange]]

(In reply to Milan Sreckovic [:milan] from comment #17)
> This is a new regression in 48, right?

We've had this bug for a long time but APZ makes the resulting jiggle a lot easier to see.

Comment 19

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8759781 [details]
Bug 1012752 - Snap scrolled area to layer pixels.

https://reviewboard.mozilla.org/r/57656/#review59242

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39dabd617b12
Snap scrolled area to layer pixels. r=tnikkel

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/39dabd617b12

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Do you want to request uplift? I notice 48 is affected. We could bring this to aurora 49, since we expect the e10s/APZ using instances to increase once 49 goes to release.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Markus, comment 22 was meant for you. Looks like bug 1267885 also depends on the work here, so if we uplift this we could bring up the work in bug 1267885 as well.

Comment 24

[Markus Stange [:mstange]]

I would like to uplift, but it seems this bug caused a pretty serious pageload performance regression on Windows 8, see bug 1285532. So I would wait until we have more information about what's happening there.

Comment 25

[Chris Lord [:cwiiis]]

(In reply to Markus Stange [:mstange] from comment #24)
> I would like to uplift, but it seems this bug caused a pretty serious
> pageload performance regression on Windows 8, see bug 1285532. So I would
> wait until we have more information about what's happening there.

Could the regression just have been caused by the extra processing that now happens when you call GetScrolledRect (which I'm guessing may be called quite a lot during loading)? Maybe it'd be helped by caching the values somehow (or finding out what's calling it so much and caching the values there)?

Comment 26

[Markus Stange [:mstange]]

I backed this out because of bug 1285532 and bug 1286674.

(In reply to Chris Lord [:cwiiis] from comment #25)
> Could the regression just have been caused by the extra processing that now
> happens when you call GetScrolledRect (which I'm guessing may be called
> quite a lot during loading)?

Unlikely, none of those calculations are particularly expensive.

It's possible that bug 1286674 is responsible for the slowdowns - if we fail to detect the layer as being opaque, lots of things get more expensive.

Comment 27

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b98df554a6f0
Back out for causing bug 1285532 and bug 1286674.

Comment 28

[Carsten Book [:Tomcat]]

markus has to back this backout out since this caused merge conflicts with mozilla-central like warning: conflicts while merging layout/generic/nsGfxScrollFrame.h! (edit, then use 'hg resolve --mark')

Comment 29

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0d8bd0a8dd6
Backed out changeset b98df554a6f0

Comment 30

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

If this lands (again), please reopen bug 1286674, or resolve as fixed (right now, it's resolved as worksforme)

Comment 31

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

:tomcat, should this still be resolved->fixed, or should we change the status due to a backout?

Comment 32

[Markus Stange [:mstange]]

This is still in the tree, I haven't re-backed out it yet. I'm going to do it once inbound reopens.

Comment 33

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

Ah, thanks.  I saw comment 29 and thought that was the backout.

Comment 34

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/440f999f965f
Back out for causing bug 1285532 and bug 1286674.

Comment 35

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Pulsebot from comment #34)
> Backout by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/440f999f965f
> Back out for causing bug 1285532 and bug 1286674.

So reopening

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/440f999f965f

Comment 37

[Markus Stange [:mstange]]

For bug 1286674, the relevant code is this one in ContainerState::ComputeOpaqueRect. We want to hit *aOpaqueForAnimatedGeometryRootParent = true. When the scrollable area is snapped to become bigger, then the scrolled canvas background color no longer covers the scroll port (non-APZ) or the displayport (APZ).

>   nsIScrollableFrame* sf = nsLayoutUtils::GetScrollableFrameFor(*aAnimatedGeometryRoot);
>   if (sf) {
>     nsRect displayport;
>     bool usingDisplayport =
>       nsLayoutUtils::GetDisplayPort((*aAnimatedGeometryRoot)->GetContent(), &displayport,
>         RelativeTo::ScrollFrame);
>     if (!usingDisplayport) {
>       // No async scrolling, so all that matters is that the layer contents
>       // cover the scrollport.
>       displayport = sf->GetScrollPortRect();
>     }
>     nsIFrame* scrollFrame = do_QueryFrame(sf);
>     displayport += scrollFrame->GetOffsetToCrossDoc(mContainerReferenceFrame);
>     if (opaque.Contains(displayport)) {
>       *aOpaqueForAnimatedGeometryRootParent = true;
>     }
>   }

Comment 38

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f82832d45b2

Comment 39

[Markus Stange [:mstange]]

Comment on attachment 8759781 [details]
Bug 1012752 - Snap scrolled area to layer pixels.

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

Comment 40

[Markus Stange [:mstange]]

Comment on attachment 8759781 [details]
Bug 1012752 - Snap scrolled area to layer pixels.

https://reviewboard.mozilla.org/r/57656/#review66400

Re-requesting review. The only parts that changed are: (1) In addition to clipping to the display port, we intersect that clip with the unsnapped scrolled rect, so that if you have something transparent over the edge of a scrolled background color item, we don't make the layer transparent because the transparent thing will now be clipped to the exact app units size of the background color item below (the true scrolled rect). (2) When FrameLayerBuilder computes the opaqueness for the AGR parent, compare rectangles in layer pixel space instead of app units.

Comment 41

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8759781 [details]
Bug 1012752 - Snap scrolled area to layer pixels.

https://reviewboard.mozilla.org/r/57656/#review66562

The interdiff link doesn't work, it shows a bunch of spurious changes. Diffing the two patches locally made it easy though.

Comment 42

[Timothy Nikkel (:tnikkel)]

https://reviewboard.mozilla.org/r/57656/#review66564

::: layout/generic/nsGfxScrollFrame.cpp:3378
(Diff revision 2)
> -      DisplayListClipState::AutoSaveRestore displayPortClipState(aBuilder);
> +      // Clip our contents to the unsnapped scrolled rect. This makes sure that
> +      // we don't have display items over the subpixel seam at the edge of the
> +      // scrolled area.
> +      DisplayListClipState::AutoSaveRestore scrolledRectClipState(aBuilder);
> +      nsRect scrolledRectClip =
> +        GetScrolledRectInternal(mScrolledFrame->GetScrollableOverflowRect(),

Can you just call GetScrolledRect here instead?

Comment 43

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/57656/#review66564

> Can you just call GetScrolledRect here instead?

Not really - the point is to use the unsnapped version, and GetScrolledRect will return the snapped version.
I can add a new function called GetTrueScrolledRect if you'd prefer that, but I don't think it'd help much.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1dbce81bf3b
Snap scrolled area to layer pixels. r=tnikkel

Comment 45

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

https://hg.mozilla.org/mozilla-central/rev/b1dbce81bf3b

Comment 47

[Sylvestre Ledru [:Sylvestre]]

Markus, are you going to request for an uplift to aurora/beta? Thanks

Comment 50

[Markus Stange [:mstange]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #47)
> Markus, are you going to request for an uplift to aurora/beta? Thanks

The patch caused a perf regression (bug 1293031), I need to fix it first before we can uplift.

Comment 51

[Liz Henry (:lizzard) (relman/hg->git project)]

It is getting late for a fix for beta, as we are heading into beta 9 and then the RC build on Monday. We could still potentially take a patch for 50 once you iron out perf issues.

Comment 52

[Dirkjan Ochtman (:djc)]

I just filed bug 1319517, which appears to be a regression from the fix for this bug.