1531057 - Reloading page gets viewports stuck in strange position, unable to scroll to see whole page

Status: RESOLVED FIXED

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

Description

[Kartikaya Gupta (email:[e-mail])]

This has been happening for a while and I think there might be another bug on file already that covers this but I figured I should file something anyway to be sure.

STR, on Fennec Nightly (Xperia XZ1 Compact):

• Go to a page treeherder, e.g. https://treeherder.mozilla.org/#/jobs?repo=try&author=kgupta%40mozilla.com
• Zoom in to the right side of the page, click on one of the per-push action dropdowns, and select "Set as bottom of range".

This reloads the page with that push set to the bottom of the page. The visual viewport is positioned where it was before (zoomed in to the right side of the page), but you can't scroll to the left any more. You have to zoom all the way out in order to force the bad state to clear, before you can see the left side of the page again.

In current release Fennec, when the page is reloaded, it loads zoomed out so this problem doesn't happen.

Comment 1

[Botond Ballo [:botond]]

The "can't scroll to the left any more" symptom sounds like bug 1519007, which will be fixed by bug 1516056.

I've been trying to prioritize bug 1516056 as much as I can, but the dependency on the session store stuff in bug 1517895 has been unfortunate. Hopefully both can land pretty soon.

Comment 2

[csheany]

The APZ Mini Map paints an interesting picture.

Comment 3

[csheany]

Attachment: Screenshot_20190228-020140.jpg

Comment 4

[csheany]

Attachment: Screenshot_20190228-020336.jpg

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

If this issue still exists it may have to wait for 68/67.

Comment 6

[Botond Ballo [:botond]]

This is pretty bad from a usability perspective, and I'd like to fix this for 67.

However, bug 1516056 is not going to be fixed for 67.

So, I'm going to explore a more targeted fix here.

Comment 7

[csheany]

Did the screenshots help?

Do you you know why zoom is different in Release and Nightly?

Comment 8

[Botond Ballo [:botond]]

(In reply to csheany from comment #7)

Did the screenshots help?

Yes, the screenshots illustrate the issue quite nicely. Thank you for posting them.

In this case, I already knew what the problem was from bug 1519007, but if I hadn't, the screenshots would have led to a quick diagnosis.

Do you you know why zoom is different in Release and Nightly?

Let's keep this bug focused on the issue of getting stuck in a state where you can't scroll back leftward. If you think there might be another issue related to the zoom level, please feel free to file a different bug.

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1531057 - In ScrollToRestoredPosition(), clamp the layout scroll offset being restored to the layout scroll range. r=kats

Comment 10

[Botond Ballo [:botond]]

The posted patch fixes the bug for cases where the scroll restoration is triggered via "session history", which includes reloads and back/forward navigation.

It does not fix the bug in cases where the scroll restoration is triggered via the "session store", which includes closing the tab followed by "Undo close tab", or restarting the browser. I'm afraid the fix for those scenarios will have to come from bug 1516056.

Comment 11

[csheany]

I only asked based on the description. I'm not sure which is the intended behavior.

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1531057 - Add a test to check that session history respects the layout scroll range. r=JanH

Depends on D23725

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0bc9e7cdfe1
In ScrollToRestoredPosition(), clamp the layout scroll offset being restored to the layout scroll range. r=kats

Comment 14

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/e0bc9e7cdfe1

Comment 15

[Pascal Chevrel:pascalc (PTO until April 26)]

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

This is pretty bad from a usability perspective, and I'd like to fix this for 67.

However, bug 1516056 is not going to be fixed for 67.

So, I'm going to explore a more targeted fix here.

Tracking as this was marked as P2 and your targeted fix landed in 68. Botond do you feel that this is safe enough for uplifting to beta? Also, would it benefit from a manual verification from QE? Thanks

Comment 16

[Botond Ballo [:botond]]

(In reply to Pascal Chevrel:pascalc from comment #15)

Tracking as this was marked as P2 and your targeted fix landed in 68. Botond do you feel that this is safe enough for uplifting to beta?

Yes, I'd like to uplift this to 67. I'll let it bake on Nightly for a couple of more days, and then request uplift. Leaving needinfo on me so I don't forget.

Also, would it benefit from a manual verification from QE?

I think baking on Nightly for a while should be sufficient.

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 9051391 [details]
Bug 1531057 - In ScrollToRestoredPosition(), clamp the layout scroll offset being restored to the layout scroll range. r=kats

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1498812
• User impact if declined: After zooming in on certain pages and reloading (or certain other workflows that trigger scroll restoration from session history), the user can get stuck in a state where they can't scroll back to the top / left of the page.
• Is this code covered by automated tests?: Yes
• 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): This is a targeted fix that avoids most of the regression risk that the full fix (which is in bug 1516056, and targets 68) has.
• String changes made/needed:
• Do you want to request approval of these patches as well?: on

Comment 18

[Botond Ballo [:botond]]

(The regressed-by bug here is bug 1498812.)

Comment 19

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9051391 [details]
Bug 1531057 - In ScrollToRestoredPosition(), clamp the layout scroll offset being restored to the layout scroll range. r=kats

Targeted fix for 67 fixing a scrolling bug, covered by tests and fix baked on nightly for a week with no new regression reported, uplift approved for 67 beta 6, thanks!

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d5a9d2da215f

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6c4db66365c
Add a test to check that session history respects the layout scroll range. r=JanH

Comment 22

[Botond Ballo [:botond]]

Attachment: Bug 1531057 - Fix eslint failure due to trailing whitespace (CLOSED TREE). r=arthuriakab

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70c50ff305d7
Fix eslint failure due to trailing whitespace (CLOSED TREE).

Comment 24

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/b6c4db66365c
https://hg.mozilla.org/mozilla-central/rev/70c50ff305d7

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e2f09a8bd87c
https://hg.mozilla.org/releases/mozilla-beta/rev/086b3323f453