Fix some bugs in scrollToVisual

RESOLVED FIXED in Firefox 67

Status

()

defect
P2
normal
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: botond, Assigned: botond)

Tracking

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 wontfix, firefox67 fixed, firefox68 fixed)

Details

(Whiteboard: [geckoview:p2])

Attachments

(2 attachments)

While working on bug 1531531, Randall and I discovered a couple of bugs in the main thread mechanism for scrolling the visual viewport (nsIDOMWindowUtils.scrollToVisual / nsIPresShell::SetPendingVisualScrollUpdate):

  • The pending visual update is sent to APZ on the next transaction (paint). However, the call to set a pending update doesn't schedule a paint, so if nothing else has scheduled a paint, the update won't happen until arbitrarily far in the future.

  • The transaction that sends the visual update still has the displayport at the old position. Since APZ composites the visual update right away, if the target position is far away from the previous scroll position, we can checkerboard. Moreover, the checkerboarding can be permanent because APZ assumes that when the main thread sends a scroll update, it has already painted the desired displayport for it.

A possible alternative would be to have the main thread already paint a
displayport at the target position of a requested visual update as part of
the same transaction that requests the update.

There are a couple of reasons we may not want to do that:

  1. APZ could reject the requested visual update under certain conditions,
    e.g. if there is a higher-priority layout update.

  2. It would break the property that the displayport in the main thread's
    scroll metadata is relative to the scroll offset in said metadata.
    Various places assume this and untangling that seems tricky.

This does mean that if the main thread requests a visual update to "far way"
(outside the existing displayport), we can get temporary checkerboarding
before the content at the target position is painted. However, it's
straightforward for callers to work around that, by changing the layout scroll
offset and scheduling a visual update if they wish to visual-scroll far
away.

Depends on D23901

P2 since it seems like this is desired for GV work.

Priority: -- → P2
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/847ee44069dc
Schedule a paint when setting a pending visual scroll update. r=kats
https://hg.mozilla.org/integration/autoland/rev/7635315e975f
Make sure we repaint after accepting a visual scroll update. r=kats
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Botond, do you think we should uplift this scrolling fix for Fenix MVP? GV 67 (now in Beta) is the GV version that will ship in Fenix MVP.

Flags: needinfo?(botond)
Whiteboard: [geckoview:p2]

Yes, we should. I will request uplift after a few days of baking on Nightly.

Comment on attachment 9051774 [details]
Bug 1536157 - Schedule a paint when setting a pending visual scroll update. r=kats

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1507279
  • User impact if declined: In some cases, workflows that involve the session store (e.g. reloading a page) may not restore the scroll position until after a tap or other action that triggers a repaint.
  • Is this code covered by automated tests?: Unknown
  • 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): Fairly low risk, small targeted fix. Note that while this doesn't have direct test coverage, bug 1531531 will be exercising it in codepaths that do have test coverage.
  • String changes made/needed:
  • Do you want to request approval of these patches as well?: on
Flags: needinfo?(botond)
Attachment #9051774 - Flags: approval-mozilla-beta?

Comment on attachment 9051775 [details]
Bug 1536157 - Make sure we repaint after accepting a visual scroll update. r=kats

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: None
  • User impact if declined: (Same as other patch)
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • 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):
  • String changes made/needed:
  • Do you want to request approval of these patches as well?: on
Attachment #9051775 - Flags: approval-mozilla-beta?

Comment on attachment 9051774 [details]
Bug 1536157 - Schedule a paint when setting a pending visual scroll update. r=kats

Low risk targetted fix that impacts our Fenix MVP, uplift approved for 67 beta 7, thanks!

Attachment #9051774 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9051775 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.