above-the-fold animations in scrollport (of e.g. 'height') can cause visible elements to jitter, with scroll anchoring
Categories
(Core :: Layout: Scrolling and Overflow, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox65 | --- | unaffected |
firefox66 | + | fixed |
firefox67 | --- | fixed |
People
(Reporter: StefanG_QA, Assigned: rhunt)
References
(Blocks 2 open bugs, )
Details
Attachments
(4 files, 2 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
video/ogg
|
Details | |
(deleted),
text/x-phabricator-request
|
jcristau
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-phabricator-request
|
Details |
STR:
- Launch Firefox.
- Navigate to https://output.jsbin.com/tumuru/quiet and slowly scroll the content so only the text is visible.
- 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.
Updated•6 years ago
|
Comment 1•6 years ago
|
||
Comment 2•6 years ago
|
||
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.
Comment 3•6 years ago
|
||
[removing "transform" from summary because transforms aren't actually relevant here; it reproduces with just an animated height.]
Assignee | ||
Comment 4•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 5•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 8•6 years ago
|
||
(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.
Assignee | ||
Comment 9•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Comment 10•6 years ago
|
||
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.
Assignee | ||
Comment 11•6 years ago
|
||
(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.
Assignee | ||
Comment 12•6 years ago
|
||
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.
Assignee | ||
Comment 13•6 years ago
|
||
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.
Assignee | ||
Comment 14•6 years ago
|
||
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 15•6 years ago
|
||
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.
Assignee | ||
Comment 16•6 years ago
|
||
Assignee | ||
Comment 17•6 years ago
|
||
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
Assignee | ||
Comment 18•6 years ago
|
||
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.
Comment 19•6 years ago
|
||
Pushed by rhunt@eqrion.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/893f2ec05d7a Don't round scroll anchor adjustments to device pixels. r=dholbert
Comment 20•6 years ago
|
||
bugherder |
Happy to uplift this for beta 8 if you can get the request in.
Assignee | ||
Comment 23•6 years ago
|
||
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
Comment 25•6 years ago
|
||
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
Comment 26•6 years ago
|
||
Pushed by rhunt@eqrion.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/31252f85aa36 Add test that we don't round subpixel scroll anchoring adjustments. r=dholbert
Comment 27•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Comment 28•6 years ago
|
||
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
Updated•6 years ago
|
Comment 29•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/92af0592bc43
https://hg.mozilla.org/releases/mozilla-beta/rev/1d2492f5e16c
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15444 for changes under testing/web-platform/tests
Upstream PR merged
Description
•