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

RESOLVED FIXED in Firefox 66

Status

()

defect
P2
normal
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: csheany, Assigned: botond)

Tracking

(Blocks 2 bugs, {regression})

63 Branch
mozilla66
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)

Details

Attachments

(5 attachments)

Reporter

Description

6 months ago
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.
Reporter

Updated

6 months ago
Flags: needinfo?(botond)
Assignee

Comment 1

6 months ago
This is caused by bug 1498812.
Depends on: 1498812
Flags: needinfo?(botond)
Assignee

Updated

6 months ago
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
Priority: -- → P2
So do you think that may be solved in bug 1498812?
Assignee

Comment 4

6 months ago
(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
Assignee

Comment 8

5 months ago
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)
Assignee

Updated

5 months ago
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
Assignee

Comment 10

5 months ago
(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
Assignee

Comment 11

5 months ago
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.
Assignee

Comment 13

5 months ago
(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.
Assignee

Comment 14

5 months ago

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.

Assignee

Comment 15

5 months ago
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.
Assignee

Comment 16

5 months ago
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

Updated

5 months ago
Assignee: nobody → botond
Assignee

Updated

5 months ago
Blocks: 1519214
Assignee

Comment 17

5 months ago

(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.

Comment 18

5 months ago
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
Assignee

Comment 20

5 months ago

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)
Assignee

Updated

5 months ago
Depends on: 1519594
Assignee

Updated

5 months ago
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
Assignee

Comment 22

4 months ago

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)
Assignee

Comment 24

4 months ago

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.

Assignee

Comment 26

4 months ago

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
Assignee

Comment 27

4 months ago
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
Assignee

Comment 28

4 months ago

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)

Comment 29

4 months ago
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.

Assignee

Comment 32

4 months ago

(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.