Closed
Bug 1255068
Opened 8 years ago
Closed 8 years ago
Header image on twitter APZ scrolls at twice the regular speed or jitters
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
VERIFIED
FIXED
mozilla48
People
(Reporter: mstange, Assigned: mstange)
References
Details
(Keywords: regression)
Attachments
(3 files, 1 obsolete file)
818 bytes,
text/html
|
Details | |
58 bytes,
text/x-review-board-request
|
kats
:
review+
mattwoodrow
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
1.15 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
STR: 1. Go to https://twitter.com/jlongster and wait for the page to load. 2. Scroll down slowly. Observe how the header image runs away. I suspect that this is a regression from bug 1238564 and that we're improperly nesting frame metrics.
Assignee | ||
Comment 1•8 years ago
|
||
Nothing to do with FrameMetrics nesting. It's an image layer with a scale, and this scale seems to throw off the APZ adjustment calculation.
Assignee | ||
Comment 2•8 years ago
|
||
It's not that either. It seems to have to do with how we treat empty transactions that only update layer transforms.
Assignee | ||
Comment 3•8 years ago
|
||
Comment 4•8 years ago
|
||
I said I'd look into this, assigning so I don't forget.
Assignee: mstange → bugmail.mozilla
Comment 5•8 years ago
|
||
diagnosis |
The problem here is that when APZ receives a layer transaction, it assumes that the scroll offset found on the layer's metrics, and the layer's content-side transform, are in sync. That is, when content scrolls a layer by modifying its content-side transform, APZ assumes the amount scrolled is reflected in the content-side scroll offset. It then takes any remaining difference between its async scroll offset and the content-side scroll offset, and sets a corresponding shadow transform on the layer. The problem with empty transactions is that they update the content-side transform (that happens here [1], which then takes effect here [2]), but do not make a corresponding update to the content-side scroll offset as stored by the FrameMetrics on the layer (that's computed in ContainerState::SetupScrollingMetadata(), which happens during frame-layer-building, which doesn't happen for empty transactions). As a result, APZ thinks content hasn't updated its scroll offset yet (even though it has internally, in the copy stored on the nsIScrollableFrame, and reflected it in the content-side transform), and sets a large shadow transform, which is added to the content-side transform, resulting in scrolling twice as fast. Doing a full paint restores the scroll offset to its correct position. I guess to fix this we need to update the scroll offset in the FrameMetrics in the empty transaction case too. It's not immediately clear where or how to do this. (The code that sets the content-side transform happens during restyle processing, and is not specific to scroll frames.) [1] https://dxr.mozilla.org/mozilla-central/rev/af7c0cb0798f5425d5d344cbaf0ac0ecb1a72a86/layout/generic/nsFrame.cpp#5316 [2] https://dxr.mozilla.org/mozilla-central/rev/af7c0cb0798f5425d5d344cbaf0ac0ecb1a72a86/gfx/layers/client/ClientLayerManager.cpp#279
Comment 6•8 years ago
|
||
Thanks for investigating! It's not obvious to me how to fix this problem either, will give it some more thought. We should fix it before attempting to uplift bug 1192910, probably.
Assignee | ||
Comment 7•8 years ago
|
||
I think there's the easy way and the hard way of fixing it. The easy way is: In nsIFrame::TryUpdateTransformOnly, check whether the layer has any frame metrics. For each frame metrics, look up the scroll frame, get a new frame metrics from it, compare the two frame metrics, and if they changed, return false and have the caller schedule a regular paint. The hard way is: Do the same thing as the easy way but don't bail out if the frame metrics changed. Instead, set the new frame metrics on the layer. Then in APZ land, do everything you can to support different "main thread frame metrics" for the same scroll frame on different layers. For each layer, the async adjustment would adjust by the difference of the current APZ scroll position and the main thread scroll position *for that layer*. However, letting different layers' frame metrics for the same scroll frame drift away from each other probably only works until there is a scroll position update forced by the main thread, which needs to clobber pending scroll position changes from APZ. In that case we would probably still have to force a full paint.
Assignee | ||
Comment 8•8 years ago
|
||
Oh, no, this won't work for transforms inside scrolled opacities. Layout sets the frame metrics on the opacity ContainerLayer, but for main thread scrolling, it only changes the transform of the transform ContainerLayer. Main thread scrolling does not move the opacity ContainerLayer. So the "easy way" needs to also walk the ancestor layers of the transform ContainerLayer and check their FrameMetrics as well, and bail out if anything changed. And the "hard way" gets much more complicated. We probably need to make APZ scrolling and main thread scrolling touch the same layers' transforms. Still thinking about how to do that.
Assignee | ||
Comment 9•8 years ago
|
||
Taking back. I'll implement the "easy way" approach.
Assignee: bugmail.mozilla → mstange
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8728526 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/39551/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39551/
Attachment #8729686 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 12•8 years ago
|
||
With this patch we do a full paint for transform changes after "paint skipped scrolls", and we still do empty transactions if the scroll position hasn't changed.
Comment 13•8 years ago
|
||
Comment on attachment 8729686 [details] MozReview Request: Bug 1255068 - Do not allow empty transaction transform changes if the scroll position has changed since the last paint. r?kats, r?mattwoodrow https://reviewboard.mozilla.org/r/39551/#review36227 This looks ok to me based on the previous analysis of the problem and your proposed solution. However I'm totally unfamiliar with empty transactions and this part of the code so it might be worth getting somebody else to look over it as well. ::: layout/generic/nsFrame.cpp:5282 (Diff revision 1) > + if (!scrollableFrame) { > + continue; Can you think of a scenario in which this might happen? I'm wondering if it's safer to return false in this case.
Attachment #8729686 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 14•8 years ago
|
||
https://reviewboard.mozilla.org/r/39551/#review36227 I'll ask Matt for another review. > Can you think of a scenario in which this might happen? I'm wondering if it's safer to return false in this case. I can't, and you're right, false will be safer. I'll return false.
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8729686 [details] MozReview Request: Bug 1255068 - Do not allow empty transaction transform changes if the scroll position has changed since the last paint. r?kats, r?mattwoodrow Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39551/diff/1-2/
Attachment #8729686 -
Attachment description: MozReview Request: Bug 1255068 - Do not allow empty transaction transform changes if the scroll position has changed since the last paint. r?kats → MozReview Request: Bug 1255068 - Do not allow empty transaction transform changes if the scroll position has changed since the last paint. r?kats, r?mattwoodrow
Attachment #8729686 -
Flags: review?(matt.woodrow)
Comment 16•8 years ago
|
||
Comment on attachment 8729686 [details] MozReview Request: Bug 1255068 - Do not allow empty transaction transform changes if the scroll position has changed since the last paint. r?kats, r?mattwoodrow https://reviewboard.mozilla.org/r/39551/#review36303 It seems like actually hitting this optimization is going to be super rare once APZ is enabled, since most layers will have the root scrollframes FM in their ancestor tree. On top of that, I really hate the empty transaction transform code, so maybe we should consider just killing it entirely? It was added for the original b2g homescreen (where scrolling was done via transform changes, to compensate for the lack of scroll snapping support in the compositor), which clearly isn't relevant any more.
Attachment #8729686 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 17•8 years ago
|
||
With this patch we're still allowing empty transaction for transform changes on layers with frame metrics if the scroll position in the frame metrics is up to date. That said, I'm not too attached to this code either.
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6ad26654ae6a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 20•8 years ago
|
||
Although this was exposed by bug 1192910, it is a pre-existing issue, so it affects 46 and 47 as well (any codeline with any form of paint skipping). Wontfix'ing for 46 because APZ will only be enabled there for a couple of weeks and it doesn't seem worth it. However we should uplift to 47.
status-firefox46:
--- → wontfix
status-firefox47:
--- → affected
Hi Markus, please nominate bug for uplift to Aurora47. Thanks!
Flags: needinfo?(mstange)
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8729686 [details] MozReview Request: Bug 1255068 - Do not allow empty transaction transform changes if the scroll position has changed since the last paint. r?kats, r?mattwoodrow Approval Request Comment [Feature/regressing bug #]: bug 1192910 (APZ paint skipping) [User impact if declined]: parts of the page moving unexpectedly during scrolling [Describe test coverage new/current, TreeHerder]: there is no test for this specific issue, but in general the test coverage in this area is okay. I should write a test for this bug. [Risks and why]: low - this patch causes us to skip an optimization in a case where it doesn't work and falls back to the proven working behavior [String/UUID change made/needed]: none
Flags: needinfo?(mstange)
Attachment #8729686 -
Flags: approval-mozilla-aurora?
Comment on attachment 8729686 [details] MozReview Request: Bug 1255068 - Do not allow empty transaction transform changes if the scroll position has changed since the last paint. r?kats, r?mattwoodrow Kats would like to see this uplifted, Aurora47+
Attachment #8729686 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 24•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/d84ed6588b23
Comment 25•8 years ago
|
||
backed out for causing android crashes/assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2213611&repo=mozilla-aurora
Flags: needinfo?(mstange)
Assignee | ||
Comment 26•8 years ago
|
||
Flags: needinfo?(mstange)
Attachment #8734508 -
Flags: review?(bugmail.mozilla)
Updated•8 years ago
|
Attachment #8734508 -
Flags: review?(bugmail.mozilla) → review+
Comment 27•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/372c2ef180b8
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a5d8c5b65333
Updated•8 years ago
|
Flags: qe-verify+
Comment 30•8 years ago
|
||
Reproduced with 2016-03-09 Nightly. Verified as fixed using Firefox 47 beta 6 and latest Aurora 48.0a2 2016-05-19 under Win 10 64-bit and Mac OS X 10.9.5.
You need to log in
before you can comment on or make changes to this bug.
Description
•