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

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

(Depends on 2 bugs, Blocks 1 bug)

48 Branch
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox50 affected, firefox51 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
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.
Attachment #8424948 - Flags: feedback?(roc)
Blocks: 1008819
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.
Attachment #8424948 - Flags: feedback?(roc) → feedback-
(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?
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
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?
What do the other browsers do for this?
(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.
Blocks: 1009306
(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?
(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.
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?
(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.
No longer blocks: 1009306
This doesn't sound specific to APZ so I'm not convinced it needs to block APZ.
No longer blocks: all-aboard-apz
Posted patch wip patch (obsolete) — Splinter Review
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.
Attachment #8424948 - Attachment is obsolete: true
Posted patch wip tests (obsolete) — Splinter Review
More tests need to be added for all kinds of snapping, and for the missing parts of the patch.
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/
Attachment #8759781 - Flags: review?(tnikkel)
Attachment #8742497 - Attachment is obsolete: true
Attachment #8742498 - Attachment is obsolete: true
This is a new regression in 48, right?
Version: Trunk → 48 Branch
Assignee: nobody → mstange
Flags: needinfo?(bugs)
(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.
Attachment #8759781 - Flags: review?(tnikkel) → review+
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39dabd617b12
Snap scrolled area to layer pixels. r=tnikkel
Flags: needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/39dabd617b12
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1285532
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.
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.
Flags: needinfo?(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.
Flags: needinfo?(mstange)
(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)?
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.
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')
Flags: needinfo?(mstange)
If this lands (again), please reopen bug 1286674, or resolve as fixed (right now, it's resolved as worksforme)
:tomcat, should this still be resolved->fixed, or should we change the status due to a backout?
Flags: needinfo?(cbook)
This is still in the tree, I haven't re-backed out it yet. I'm going to do it once inbound reopens.
Flags: needinfo?(cbook)
Ah, thanks.  I saw comment 29 and thought that was the backout.
(In reply to Pulsebot from comment #34)
> Backout by mstange@themasta.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/440f999f965f
> Back out for causing bug 1285532 and bug 1286674.

So reopening
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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;
>     }
>   }
Flags: needinfo?(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 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.
Attachment #8759781 - Flags: review?(mstange)
Attachment #8759781 - Flags: review?(tnikkel)
Attachment #8759781 - Flags: review?(mstange)
Attachment #8759781 - Flags: review+
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.
Attachment #8759781 - Flags: review?(tnikkel) → review+
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?
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.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1dbce81bf3b
Snap scrolled area to layer pixels. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/b1dbce81bf3b
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1293031
Depends on: 1292460
No longer blocks: 1267885
Duplicate of this bug: 1267885
Markus, are you going to request for an uplift to aurora/beta? Thanks
Flags: needinfo?(mstange)
Duplicate of this bug: 1258307
(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.
Flags: needinfo?(mstange)
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.
No longer depends on: 1292460
Depends on: 1303761
I just filed bug 1319517, which appears to be a regression from the fix for this bug.
See Also: → 1327350
Depends on: 1327195
You need to log in before you can comment on or make changes to this bug.