STR:

1. Launch Firefox.
2. Navigate to https://output.jsbin.com/tumuru/quiet and slowly scroll the content so only the text is visible.
3. Observe.

ER: Solid scroll position.
AR: The page jitter.

This issue does not reproduce in the competitor browser.

Disabling layout.css.scroll-anchoring.enabled makes the page jumping up and down.

Here's a screencast.

You can see the subtly jiggling text in Firefox in the first part (particularly noticeable at the edge of the viewport, where the characters are cut off), whereas there's no jiggling in Chrome in the second part.

[removing "transform" from summary because transforms aren't actually relevant here; it reproduces with just an animated height.]

I wonder if we’re not adjusting on a subpixel level correctly. I think I saw a similar issue locally. I’ll take a closer look tonight or tomorrow.

Yup, we're adjusting by rounded device pixels when I think we should be adjusting by just plain app-units. This causes us to drift apart on subpixel changes.

There's no good API for this on nsIScrollableFrame or ScrollFrameHelper (which is why I originally wrote it with device pixels, missing that we were rounding them). I'll see how we can hook this up better.

(In reply to Ryan Hunt [:rhunt] from comment #5)

Yup, we're adjusting by rounded device pixels when I think we should be
adjusting by just plain app-units. This causes us to drift apart on subpixel
changes.

There's no good API for this on nsIScrollableFrame or ScrollFrameHelper
(which is why I originally wrote it with device pixels, missing that we were
rounding them). I'll see how we can hook this up better.

Follow up, there actually is an API as we were using the ScrollFrameHelper directly and not the nsIScrollableFrame interface.

Matt, this change might cause us to start applying scroll adjustments that leave us at different subpixel offsets in the scroll frame. Is this a concern for our painting performance?

If it is, I could rewrite the patch to be more smart and track the difference between the pixel snapped destination and the desired destination. On following adjustments, we could use that difference to avoid accumulating error over time.

Now that I think about it, that might only solve bug 1519510 and not this issue.

Indeed it is. With non-WR, we invalidate our PaintedLayers if the scroll offset changes by a subpixel amount, which is why the existing scroll code does some extra work to avoid it if possible.

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)

Indeed it is. With non-WR, we invalidate our PaintedLayers if the scroll
offset changes by a subpixel amount, which is why the existing scroll code
does some extra work to avoid it if possible.

I'll give the 'smart' approach a try then.

I've written a patch that will scroll to pixel snapped coordinates, but then accumulate the remainder over time to prevent drift. This solves bug 1519510, but not this animation.

I'm going to unblock the initial release of scroll anchoring, as I think the experience is no worse than without scroll anchoring.

It may still make it into the initial release, it just shouldn't block.

So I took a look at just adjusting by subpixel amounts again, as that solves this issue, bug 1523866, and bug 1519510.

The concern here is that subpixel differences in scroll offset between paints can cause whole layer invalidation (comment 10).

I'm not sure that's actually an issue with scroll anchoring, as it seems like we should only care about subpixel differences of frames relative to the scroll port, and anchor adjustments should preserve that.

For example, if the anchor frame is offset from the scroll port by 10.5px and we have a div above expand by 10.5px, performing a 10.5px scroll would keep the anchor frame at the same relative subpixel position.

I tested this out with different snapping methods and paint flashing and wasn't able to see a difference between any of them.

With this, I think we should try and adjust by subpixel amounts.

This test might be overkill, but I was tired when writing it and couldn't think
of a better way. It feels like there should be, though.

Depends on D19107

I've got an update for the test, but I'm going to land the patch to allow subpixel adjustments first as it's relatively simple and high priority.

https://hg.mozilla.org/mozilla-central/rev/893f2ec05d7a

We need to uplift this to beta.

Happy to uplift this for beta 8 if you can get the request in.

Comment on attachment 9042381 [details]
Bug 1519553 - Don't round scroll anchor adjustments to device pixels. r?dholbert

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Scroll anchoring

User impact if declined

Certain animations and adjustments to pages can cause us to perform scroll anchor adjustments by rounded pixels. These adjustments can cause undesired 'jittering'. This commit causes us to adjust by the exact unrounded amount to avoid this issue.

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

It changes us to stop rounding scroll positions.

String changes made/needed

Thanks for the reminder, the needinfo got a bit lost.

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See How Do You Triage for more information

https://hg.mozilla.org/mozilla-central/rev/31252f85aa36

Comment on attachment 9042381 [details]
Bug 1519553 - Don't round scroll anchor adjustments to device pixels. r?dholbert

scroll anchoring fix, approved for 66.0b9

https://hg.mozilla.org/releases/mozilla-beta/rev/92af0592bc43
https://hg.mozilla.org/releases/mozilla-beta/rev/1d2492f5e16c