Zoom not restored when going back in history

VERIFIED FIXED in Firefox 55

Status

()

Firefox for Android
Toolbar
P3
normal
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: kats, Assigned: JanH)

Tracking

({regression})

43 Branch
Firefox 56
All
Android
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox49 wontfix, firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 verified, firefox56 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

Nightly 52 on a nexus 4. Go to https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX_50_0b8_RELEASE&tochange=FIREFOX_50_0b9_RELEASE, zoom in and click on one of the csets. Go back.

ER: zoom and scroll position restored.
AR: page is zoomwd out
I bisected this using mozregression. While bisecting I found another issue where sometimes after hitting back, the zoom value would be restored but the scroll position was not restored properly. I counted those builds as "good" since this bug is specifically about zoom. The bisection got as far as this before barfing:

INFO: Last good revision: 8a6045d14d6bd348a3b5bfeb55a9321e680cc93e (2015-08-24)
INFO: First bad revision: 04b8c412d9f58fb6194c58dcaa66bf278bbd53cf (2015-08-25)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8a6045d14d6bd348a3b5bfeb55a9321e680cc93e&tochange=04b8c412d9f58fb6194c58dcaa66bf278bbd53cf

So it's most likely either bug 1197592 or bug 1180267.
status-firefox49: --- → wontfix
status-firefox50: --- → wontfix
status-firefox51: --- → fix-optional
status-firefox52: --- → affected
Keywords: regressionwindow-wanted
OS: Unspecified → Android
Priority: -- → P3
Hardware: Unspecified → All
Version: 50 Branch → 43 Branch
status-firefox53: --- → affected
status-firefox51: fix-optional → wontfix
status-firefox52: affected → fix-optional
(Assignee)

Updated

a year ago
See Also: → bug 1265818
(Assignee)

Updated

a year ago
Blocks: 1265818
See Also: bug 1265818
status-firefox54: --- → affected
status-firefox55: --- → affected
kats, still on your radar? I notice no one is assigned. 
We can let this fall off the platform triage if it's considered backlog.
status-firefox52: fix-optional → wontfix
status-firefox53: affected → wontfix
status-firefox54: affected → fix-optional
Yeah let's consider it backlog.
status-firefox55: affected → fix-optional
(Assignee)

Comment 4

11 months ago
So the actual issue is quite simple: The resolution is restored on the PresShell before the first paint, so it subsequently gets clobbered by the MobileViewportManager's default zoom calculation.
Assignee: nobody → jh+bugzilla
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 7

11 months ago
mozreview-review
Comment on attachment 8882002 [details]
Bug 1312605 - Part 0 - Fix indentation in MVM.

https://reviewboard.mozilla.org/r/153058/#review158326

Thanks for fixing this! (And for putting in a separate patch!)
Attachment #8882002 - Flags: review?(bugmail) → review+
(Reporter)

Comment 8

11 months ago
mozreview-review
Comment on attachment 8882003 [details]
Bug 1312605 - Part 1 - Don't clobber resolution changes that happen before first paint on Fennec.

https://reviewboard.mozilla.org/r/153060/#review158328
Attachment #8882003 - Flags: review?(bugmail) → review+
Comment hidden (mozreview-request)

Comment 10

11 months ago
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/8a07aa497968
Part 0 - Fix indentation in MVM. r=kats
https://hg.mozilla.org/integration/autoland/rev/e1283673d436
Part 1 - Don't clobber resolution changes that happen before first paint on Fennec. r=kats
Backed out for failing mobile/android/tests/browser/chrome/test_session_scroll_position.html:

https://hg.mozilla.org/integration/autoland/rev/c8ef01f4ce1070c574b69734031813c682195843
https://hg.mozilla.org/integration/autoland/rev/1f2c1539b344aee13dfddf8931b979e9429ad6af

Failure log (Android was busted during the push for this bug): https://treeherder.mozilla.org/logviewer.html#?job_id=110619566&repo=autoland

[task 2017-06-29T02:08:46.099383Z] 02:08:46     INFO -  218 INFO TEST-PASS | mobile/android/tests/browser/chrome/test_session_scroll_position.html | zoom restored correctly
[task 2017-06-29T02:08:46.099766Z] 02:08:46     INFO -  219 INFO TEST-PASS | mobile/android/tests/browser/chrome/test_session_scroll_position.html | scrollX restored correctly
[task 2017-06-29T02:08:46.100214Z] 02:08:46     INFO -  220 INFO TEST-PASS | mobile/android/tests/browser/chrome/test_session_scroll_position.html | scrollY restored correctly
[task 2017-06-29T02:08:46.100630Z] 02:08:46     INFO -  221 INFO TEST-PASS | mobile/android/tests/browser/chrome/test_session_scroll_position.html | can go back
[task 2017-06-29T02:08:46.101061Z] 02:08:46     INFO -  222 INFO Now waiting for pageshow event from browser
[task 2017-06-29T02:08:46.101441Z] 02:08:46     INFO -  223 INFO Now waiting for scroll event from browser
[task 2017-06-29T02:08:46.101746Z] 02:08:46     INFO -  Buffered messages logged at 02:08:38
[task 2017-06-29T02:08:46.102051Z] 02:08:46     INFO -  224 INFO Received event pageshow from browser
[task 2017-06-29T02:08:46.102460Z] 02:08:46     INFO -  225 INFO Received event scroll from browser
[task 2017-06-29T02:08:46.103008Z] 02:08:46     INFO -  Buffered messages finished
[task 2017-06-29T02:08:46.103242Z] 02:08:46     INFO -  226 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_session_scroll_position.html | undefined assertion name - got true, expected "zoom restored correctly"
[task 2017-06-29T02:08:46.103615Z] 02:08:46     INFO -  SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
[task 2017-06-29T02:08:46.103885Z] 02:08:46     INFO -  test_sessionStoreScrollPositionAndZoomLevel@chrome://mochitests/content/chrome/mobile/android/tests/browser/chrome/test_session_scroll_position.html:214:5
[task 2017-06-29T02:08:46.104530Z] 02:08:46     INFO -  onFulfilled@chrome://mochikit/content/tests/SimpleTest/SpawnTask.js:69:15
Flags: needinfo?(jh+bugzilla)
(Assignee)

Comment 12

11 months ago
I think I got "is" and "ok" confused there, but it's now even failing on try even with the correct test. Strangely enough earlier on I managed to get an unexpected pass with the test unchanged. Unfortunately I can't run tests locally right now, so will look into it next week when I get back.
Flags: needinfo?(jh+bugzilla)
(Assignee)

Comment 13

11 months ago
(In reply to Jan Henning [:JanH] from comment #12)
> I think I got "is" and "ok" confused there, but it's now even failing on try
> even with the correct test

...because I did a try push in artifact mode (duh). Should be fine now...
Comment hidden (mozreview-request)

Comment 15

11 months ago
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/cb4445c53902
Part 0 - Fix indentation in MVM. r=kats
https://hg.mozilla.org/integration/autoland/rev/53ba9f0f61da
Part 1 - Don't clobber resolution changes that happen before first paint on Fennec. r=kats
https://hg.mozilla.org/mozilla-central/rev/cb4445c53902
https://hg.mozilla.org/mozilla-central/rev/53ba9f0f61da
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
(Assignee)

Comment 17

11 months ago
Comment on attachment 8882003 [details]
Bug 1312605 - Part 1 - Don't clobber resolution changes that happen before first paint on Fennec.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1180267
[User impact if declined]: Zoom level is not restored when navigating back/forward through a tab's session history
[Is this code covered by automated tests?]: Partially.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This just feeds resolution changes that happen before first paint into the same code path that is already being used by the session store.
[String changes made/needed]: none
Attachment #8882003 - Flags: approval-mozilla-beta?
Comment on attachment 8882003 [details]
Bug 1312605 - Part 1 - Don't clobber resolution changes that happen before first paint on Fennec.

Nice, let's uplift the fix! I'm sure many people who use zoom (for example for accessibility/vision issues) will appreciate zoom level being sticky as they go back through the history.
Attachment #8882003 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox54: fix-optional → wontfix

Comment 19

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/b4344f79a38a
status-firefox55: fix-optional → fixed
Flags: in-testsuite+
Verified as fixed on build 56.0a1 (7-20) and 55.0b10 with devices: HTC 10 (Android 7.0)
Honor 8 (Android 6.0).
Following the steps from description and another pages, zoom was restored when going back in history.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
status-firefox56: fixed → verified
You need to log in before you can comment on or make changes to this bug.