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

RESOLVED FIXED in Firefox 67

Status

()

defect
P2
normal
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: kats, Assigned: botond)

Tracking

(Regression, {regression})

unspecified
mozilla68
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox65 unaffected, firefox66 wontfix, firefox67+ fixed, firefox68 fixed)

Details

Attachments

(5 attachments)

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

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.

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.

Depends on: 1516056

The APZ Mini Map paints an interesting picture.

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

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.

Assignee: nobody → botond
No longer depends on: 1516056

Did the screenshots help?

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

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

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.

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

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e0bc9e7cdfe1
In ScrollToRestoredPosition(), clamp the layout scroll offset being restored to the layout scroll range. r=kats
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

(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

Flags: needinfo?(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.

Blocks: 1519013

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
Flags: needinfo?(botond)
Attachment #9051391 - Flags: approval-mozilla-beta?

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

Blocks: 1498812

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!

Attachment #9051391 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b6c4db66365c
Add a test to check that session history respects the layout scroll range. r=JanH
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/70c50ff305d7
Fix eslint failure due to trailing whitespace (CLOSED TREE).
Flags: qe-verify-
No longer blocks: 1498812
Regressed by: 1498812
You need to log in before you can comment on or make changes to this bug.