Closed Bug 1509575 Opened 2 years ago Closed 2 years ago

Returning to a page that was zoomed in on the right side is opened on the left

Categories

(Core :: Panning and Zooming, defect, P2)

63 Branch
All
Android
defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed

People

(Reporter: csheany, Assigned: botond)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(5 files)

User Agent: Mozilla/5.0 (Android 7.1.1; Tablet; rv:65.0) Gecko/65.0 Firefox/65.0

Steps to reproduce:

1. Open https://www.mozilla.org/en-US/
2. Zoom in on the right side
3. Open a new tab
4. Return to the original tab


Actual results:

The left side of the page is in view.


Expected results:

The location should be restored.
Flags: needinfo?(botond)
This is caused by bug 1498812.
Depends on: 1498812
Flags: needinfo?(botond)
Component: General → Session Restore
Summary: Reurning to a page that was zoomed in on the right side is opened on the left → Returning to a page that was zoomed in on the right side is opened on the left
Ugh, so far I thought this only happens when the session store restores the scroll position (browser restart, restoring a recently closed tab, restoring a tab that was discarded to save memory), or when session history comes into play when navigating forwards/backwards through a tab's session history.

Apparently merely switching away from a tab and then back again is enough to trigger this as well, though.
Status: UNCONFIRMED → NEW
Component: Session Restore → General
Ever confirmed: true
Keywords: regression
OS: Unspecified → Android
Hardware: Unspecified → All
Version: Firefox 65 → Firefox 63
So do you think that may be solved in bug 1498812?
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #3)
> So do you think that may be solved in bug 1498812?

I expect so, yes.
Duplicate of this bug: 1513905
Too late to fix in 64. Marking this issue as fix-optional for 65; if you land a patch in nightly and think it's low-risk for beta, please request uplift.
(In reply to Botond Ballo [:botond] from comment #4)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #3)
> > So do you think that may be solved in bug 1498812?
> 
> I expect so, yes.

1. Bug 1498812 won't help here, because session history isn't involved in this problem as far as I can tell. After all, this happens even when merely switching to a different tab and back again.
2. For completeness, this also happens when going back/forward restores a page from the bfcache, where session history (the PresState) is effectively bypassed as far as the scroll position is concerned, because we simply take the previously rendered page out of the bfcache.
3. I tried poking around a bit with a debugger - somehow the repainting/layer updating that happens when re-selecting a tab (or fishing a page out of the bfcache) clobbers APZ's scroll position (and therefore the visual viewport) with the scroll position of the layout viewport. Beyond that however it would probably be better for someone more familiar with APZ to take this over.
Blocks: 1423011
Component: General → Panning and Zooming
No longer depends on: 1498812
Flags: needinfo?(botond)
Product: Firefox for Android → Core
Version: Firefox 63 → 63 Branch
Thanks for the analysis.

I did some debugging, and it looks like APZ is getting a "first paint" transaction when we switch back to the page, which is not particularly surprising seeing as browser.js sets that flag is contentDocumentChanged() [1]. The visual scroll position does indeed get clobbered during a "first paint" transaction.

I also see that the session history is being bypassed, because the scroll frame object itself remains alive.

I still think there's a relation to bug 1498812, in the following sense: whatever mechanism we use to restore the visual scroll position when restoring a page from the session history, we should use the same mechanism to restore the visual scroll position after (or during) a "first paint" transaction.

[1] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/mobile/android/chrome/content/browser.js#1082
Flags: needinfo?(botond)
Hmm. According to the documentation at [1], the isFirstPaint flag can be reset to effectively get a notification of when the next paint happens, and clobbering the visual viewport position seems a rather unfortunate side effect.
We're not resetting the pinch-zoom, either, when (re-)doing a first paint, so is it really necessary to reset the (visual) scroll offset?

To me, I have a feeling that getting session history (or even worse, the session store) involved with this could turn out rather awkward.

I hacked together a small patch, which at a first glance seems to work: Getting a page from the bfcache or manually resetting isFirstPaint (as Fennec does) no longer clobbers the visual viewport position, but navigating to a new page still correctly resets the visual viewport back to its default position.

[1] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/dom/interfaces/base/nsIDOMWindowUtils.idl#237-244
Attachment #9034205 - Flags: feedback?(botond)
See Also: → 1498812
(In reply to Jan Henning [:JanH] from comment #9)
> We're not resetting the pinch-zoom, either, when (re-)doing a first paint,
> so is it really necessary to reset the (visual) scroll offset?

Agreed, it's not necessary.

> To me, I have a feeling that getting session history (or even worse, the
> session store) involved with this could turn out rather awkward.

Sorry, I wasn't very clear. What I meant was, both session history and isFirstPaint should use something like scrollToVisual() (the API to be added in bug 1507279) to restore the visual scroll position.
Depends on: 1507279
Comment on attachment 9034205 [details] [diff] [review]
preserve_visual_viewport.patch

Review of attachment 9034205 [details] [diff] [review]:
-----------------------------------------------------------------

So this is actually starting to look like the implementation of bug 1507279, with just the details being a bit different.

Give me a chance to put some patches up in bug 1507279, and the fix here should become straightforward.
Attachment #9034205 - Flags: feedback?(botond)
(In reply to Botond Ballo [:botond] from comment #10)
> > To me, I have a feeling that getting session history (or even worse, the
> > session store) involved with this could turn out rather awkward.
> 
> Sorry, I wasn't very clear. What I meant was, both session history and
> isFirstPaint should use something like scrollToVisual() (the API to be added
> in bug 1507279) to restore the visual scroll position.

By "isFirstPaint", do you mean that this should "just work", i.e. front-end code could simply reset the flag without any ill effects on the (visual) scroll position, or that the front-end code that was setting isFirstPaint back to true should also be responsible for subsequently calling the new bug 1507279-API to restore the previous visual viewport scroll position?

It's the latter alternative that I'd potentially find rather awkward.

As for session history, I didn't debug this in detail, but just to clarify, this only affects the case when the page is loaded from the bfcache and the normal session history code for restoring the scroll position is bypassed. When the page doesn't come from the bfcache, the scroll restoration code in nsGfxScrollFrame runs and everything works fine, although of course that code will have to be switched to the new API from bug 1507279, too.

> Give me a chance to put some patches up in bug 1507279, and the fix here should become straightforward.

Okay, I was already working on getting a test case working for this.

One more thing, though - right now I just commented out the code in Fennec that was resetting isFirstPaint and while this does break the UI rather severely because we're never clearing the clear colour we're drawing front of the content until we've painted something, I can still reproduce this problem.
Through getting my test case to work I've confirmed that resetting the isFirstPaint flag on the chrome window (at least in Fennec - I still need to get the test working on Desktop with e10s) does indeed get APZ into the code path where it overwrites its own scroll position with the scroll position from the incoming FrameMetrics, which courtesy of ComputeScrollMetadata contains the layout viewport position.

However it seems that switching tabs or getting a page from the bfcache is sufficient to trigger the same issue even when aIsFirstPaint is false. So while my patch would be sufficient for Fennec (which one way or another apparently is resetting isFirstPaint for all the relevant cases), it doesn't yet cover all potential cases.
(In reply to Jan Henning [:JanH] from comment #12)
> (In reply to Botond Ballo [:botond] from comment #10)
> > > To me, I have a feeling that getting session history (or even worse, the
> > > session store) involved with this could turn out rather awkward.
> > 
> > Sorry, I wasn't very clear. What I meant was, both session history and
> > isFirstPaint should use something like scrollToVisual() (the API to be added
> > in bug 1507279) to restore the visual scroll position.
> 
> By "isFirstPaint", do you mean that this should "just work", i.e. front-end
> code could simply reset the flag without any ill effects on the (visual)
> scroll position, or that the front-end code that was setting isFirstPaint
> back to true should also be responsible for subsequently calling the new bug
> 1507279-API to restore the previous visual viewport scroll position?
> 
> It's the latter alternative that I'd potentially find rather awkward.

I imagined something like this:

  - The JS API nsIDOMWindowUtils.scrollToVisual() would be backed by
    a C++ API on nsIPresShell.

  - When sending a transaction with isFirstPaint=true, the pres shell
    (which knows this via nsIPresShell::mIsFirstPaint) would invoke
    the C++ API directly.

I don't think there's a need for front-end code to be involved.

> As for session history, I didn't debug this in detail, but just to clarify,
> this only affects the case when the page is loaded from the bfcache and the
> normal session history code for restoring the scroll position is bypassed.
> When the page doesn't come from the bfcache, the scroll restoration code in
> nsGfxScrollFrame runs and everything works fine, although of course that
> code will have to be switched to the new API from bug 1507279, too.

Yup, understood.

> Okay, I was already working on getting a test case working for this.

Cool, thanks for working on a test case! We should be able to use it regardless of the exact fix.

> One more thing, though - right now I just commented out the code in Fennec
> that was resetting isFirstPaint and while this does break the UI rather
> severely because we're never clearing the clear colour we're drawing front
> of the content until we've painted something, I can still reproduce this
> problem.
> Through getting my test case to work I've confirmed that resetting the
> isFirstPaint flag on the chrome window (at least in Fennec - I still need to
> get the test working on Desktop with e10s) does indeed get APZ into the code
> path where it overwrites its own scroll position with the scroll position
> from the incoming FrameMetrics, which courtesy of ComputeScrollMetadata
> contains the layout viewport position.
> 
> However it seems that switching tabs or getting a page from the bfcache is
> sufficient to trigger the same issue even when aIsFirstPaint is false. So
> while my patch would be sufficient for Fennec (which one way or another
> apparently is resetting isFirstPaint for all the relevant cases), it doesn't
> yet cover all potential cases.

Interesting. We can investigate this as a follow-up.
Depends on: 1517976

In this Try push, I have a WIP patch for bug 1507279, and on top of it patches for this bug which make the test from bug 1517976 pass.

So that it's easily available during painting.

The flag is set based on nsIPresShell::mIsFirstPaint, but the pres shell
flag is cleared at the beginning of the paint, so we can't query it from
the pres shell during the paint.
During a "first paint" transaction, compositor-side state such as APZ's copy
of the visual viewport offset is overwritten. However, the scroll frame may
persist on the main thread, and in such a case we want to restore the visual
viewport offset stored in the scroll frame. This comes into play during e.g.
navigation back to a page.

Depends on D16237
Assignee: nobody → botond
Blocks: 1519214

(In reply to Botond Ballo [:botond] from comment #13)

One more thing, though - right now I just commented out the code in Fennec
that was resetting isFirstPaint and while this does break the UI rather
severely because we're never clearing the clear colour we're drawing front
of the content until we've painted something, I can still reproduce this
problem.
Through getting my test case to work I've confirmed that resetting the
isFirstPaint flag on the chrome window (at least in Fennec - I still need to
get the test working on Desktop with e10s) does indeed get APZ into the code
path where it overwrites its own scroll position with the scroll position
from the incoming FrameMetrics, which courtesy of ComputeScrollMetadata
contains the layout viewport position.

However it seems that switching tabs or getting a page from the bfcache is
sufficient to trigger the same issue even when aIsFirstPaint is false. So
while my patch would be sufficient for Fennec (which one way or another
apparently is resetting isFirstPaint for all the relevant cases), it doesn't
yet cover all potential cases.

Interesting. We can investigate this as a follow-up.

Filed bug 1519214 to track this.

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7dcfa876f76c
Expose the isFirstPaint flag on LayerManager. r=kats
https://hg.mozilla.org/integration/autoland/rev/245d6855cd3b
Restore the visual viewport offset from the main thread during a first paint. r=kats

What appears to be happening here is that a visual scroll offset update is clobbering a smooth scroll being requested in the same transaction. The smooth scroll should probably take precedence.

Flags: needinfo?(botond)
Depends on: 1519594
No longer depends on: 1519594
Attachment #9036018 - Attachment description: Bug 1509575 - Give a smooth scroll update precedence over a visual scroll offset update. r=kats → Bug 1509575 - Extend the internal visual scroll API to allow specifying "restore" vs. regular priority. r=kats

These patches seem to be causing test_session_scroll_position.html and test_session_scroll_visual_viewport.html to time out:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=396d148b35471a240f94e6641efca54b3a3b499f

(In reply to Botond Ballo [:botond] from comment #22)

These patches seem to be causing test_session_scroll_position.html and test_session_scroll_visual_viewport.html to time out:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=396d148b35471a240f94e6641efca54b3a3b499f

Flags: needinfo?(jh+bugzilla)

It looks like the isFirstPaint mechanism is interfering with the session store mechanism.

Hmm, the thing that breaks seems to be session history not involving the bfcache.
I'm now seeing the same thing locally with my own patch as well, but unfortunately I cannot remember whether I never tested that scenario, or whether something else landed in the meantime that broke this again.

I believe I have a fix for this:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=041b8c572c9d13f4876ad877c089f73e07a1f2bc

(The tests in question are passing in this push. The other failures are unrelated.)

Attachment #9035731 - Attachment description: Bug 1509575 - Expose the isFirstPaint flag on LayerManager. r=kats → Bug 1509575 - Expose the isFirstPaint flag on LayerManager. r?kats
Attachment #9036018 - Attachment description: Bug 1509575 - Extend the internal visual scroll API to allow specifying "restore" vs. regular priority. r=kats → Bug 1509575 - Extend the internal visual scroll API to allow specifying "restore" vs. regular priority. r?kats
This allows us to e.g. avoid sending a value that's (0,0) because it hasn't
been set yet in a visual scroll update.

Depends on D16346
Attachment #9035733 - Attachment description: Bug 1509575 - Restore the visual viewport offset from the main thread during a first paint. r=kats → Bug 1509575 - Restore the visual viewport offset from the main thread during a first paint. r?kats

To expand a little more on what the problem was: the tests were exercising a case where the pres shell does not persist across the navigation, so the visual offset to be restored was not stored in nsIPresShell::mVisualViewportOffset (that variable was just at its default value of (0,0)). Nonetheless, the first-paint mechanism was sending that (0,0) over as a visual scroll update, and it rode the same transaction as the layout scroll update from ScrollToRestoredPosition(). They were both eRestore and incompatible, so the (0,0) won.

My solution was to track whether nsIPresShell::mVisualViewportOffset has been set, and avoid sending a visual update based on it if it has not been set.

Flags: needinfo?(jh+bugzilla)
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ecba81cf5e74
Expose the isFirstPaint flag on LayerManager. r=kats
https://hg.mozilla.org/integration/autoland/rev/52501b577855
Extend the internal visual scroll API to allow specifying "restore" vs. regular priority. r=kats
https://hg.mozilla.org/integration/autoland/rev/eba0c068601d
Make nsIPresShell::mVisualViewportOffset a Maybe so we can tell if it's ever been set. r=kats
https://hg.mozilla.org/integration/autoland/rev/041f9c2bfce5
Restore the visual viewport offset from the main thread during a first paint. r=kats

Given that we're less than a week away from the 65 RC and this isn't a new issue, I think this should just ride the trains to release. Feel free to nominate for approval if you feel strongly otherwise.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #31)

Given that we're less than a week away from the 65 RC and this isn't a new issue, I think this should just ride the trains to release.

+1

You need to log in before you can comment on or make changes to this bug.