Closed
Bug 1282902
Opened 5 years ago
Closed 5 years ago
Session store incorrectly scales about:firefox if device was rotated between saving and restoring that tab
Categories
(Firefox for Android Graveyard :: Toolbar, defect, P3)
Tracking
(firefox48 unaffected, firefox49 verified, firefox50 verified)
VERIFIED
FIXED
Firefox 50
Tracking | Status | |
---|---|---|
firefox48 | --- | unaffected |
firefox49 | --- | verified |
firefox50 | --- | verified |
People
(Reporter: JanH, Assigned: JanH)
References
Details
Attachments
(4 files, 3 obsolete files)
58 bytes,
text/x-review-board-request
|
kats
:
review+
gchang
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
kats
:
review+
gchang
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
kats
:
review+
gchang
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
sebastian
:
review+
gchang
:
approval-mozilla-aurora+
|
Details |
Splitting this off from bug 1279443: If I rotate my phone from portrait to landscape or vice versa between closing and restarting the browser, about:firefox is displayed at a wrong zoom level (zoomed in when going from portrait to landscape, zoomed out when going the other way). This is because while Firefox is running, that page behaves like a standard mobile page with "width=device-width", i.e. its resolution doesn't change when the window width changes on device rotation and instead always remains at ~0.6667. [1] When queried through getViewportInfo() however, that page is reported as autoSize = false [2], which is the signal for the session store to scale the stored zoom level if the window width has changed between saving and restoring of a tab. [1] <meta name="viewport" content="width=480; initial-scale=.6667; user-scalable=no"/> (https://dxr.mozilla.org/mozilla-central/rev/016e0f47e8ad66ba6eb11fe28958e3a69ef9e53d/mobile/android/chrome/content/about.xhtml#18) [2] Where I think that info is ultimately coming from: https://dxr.mozilla.org/mozilla-central/rev/016e0f47e8ad66ba6eb11fe28958e3a69ef9e53d/dom/base/nsDocument.cpp#8131 I think this basically boils down to: - Either the behaviour of autoSize isn't correct, and it ought to return "true" for about:firefox's meta viewport string, too. - Or else the session store needs some different property which properly models whether or not a document's resolution needs to be scaled when the window width changes. - And as a possible stopgap, make a specific exception for "about:firefox" in the session store's scaling code.
Assignee | ||
Comment 1•5 years ago
|
||
After some playing around, it seems that incorporating the viewport width change ratio into the calculation (compare the MVM's native code: https://dxr.mozilla.org/mozilla-central/rev/e45890951ce77c3df05575bd54072b9f300d77b0/layout/base/MobileViewportManager.cpp#234) might be the missing puzzle piece.
Assignee: nobody → jh+bugzilla
Assignee | ||
Comment 2•5 years ago
|
||
Argh, it turns out the session store's zoom restore function runs before dom-meta-added, meaning the viewport returned via getViewportInfo is bogus and cannot be used for recalculating the stored resolution level at that time. Maybe I'll look into how easily setRestoreResolution() could be expanded to also take the previous display and viewport widths as parameters after all and let the MVM worry about correctly recalculating that value.
Component: General → Graphics, Panning and Zooming
Assignee | ||
Comment 3•5 years ago
|
||
I've got an implementation that seems to be working now, albeit with one remaining problem: In order to have the session store use exactly the same values as the MVM (compare https://dxr.mozilla.org/mozilla-central/rev/b69a5bbb5e40bd426e35222baa600b481e50d265/layout/base/MobileViewportManager.cpp#344), I need the size of the content window in device pixels. However unfortunately all that I seem to be able to access from the JS side is window dimensions in CSS pixels, which introduces some rounding errors [1]. [1] There is window.gScreenWidth, however that is just the result of "window.outerWidth * window.devicePixelRatio" and therefore suffers from the same rounding problem.
Assignee | ||
Comment 4•5 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61630/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61630/
Assignee | ||
Comment 5•5 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61632/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61632/
Assignee | ||
Comment 6•5 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61634/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61634/
Assignee | ||
Comment 7•5 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61636/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61636/
Assignee | ||
Comment 8•5 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61638/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61638/
Comment 9•5 years ago
|
||
Can you give an example of what kind of values you're getting from window.devicePixelRatio and gScreenWidth and what the rounding error is?
Assignee | ||
Comment 10•5 years ago
|
||
On my phone, it's 1.5, so in landscape mode if I remember correctly: 800 display px -> 533 CSS px -> 799.5 display px according to gScreenWidth So the error is not very big, but it can mean that e.g. a typical mobile page, whose (unzoomed) resolution should always remain at 1 even after rotation, suddenly gets scaled slightly upon session restore if the device was rotated since the last session save. Actually thinking of it, this might happen even without rotating the phone simply because the viewport dimensions as calculated by the session store and the MVM differ (because getViewportInfo uses the display dimensions passed by the caller), so the cssViewportChangeRatio is suddenly != 1 and starts scaling the resolution. There doesn't seem to be anything available from DOMWindowUtils, however I've written a new helper methods which looks like it should work (haven't had the time to actually compile and try it yet, possibly tomorrow or so).
Assignee | ||
Comment 11•5 years ago
|
||
The intention is for the session store to record the current window size and then pass it to the MobileViewportManager for restoring, so the latter can correctly scale the stored resolution if the display width has since changed. However currently all window dimensions available from the JS side are measured in CSS pixels rounded to integers. Transforming those values back into display pixels by multiplying them with "window.devicePixelRatio" (or accessing window.gScreenWidth/Height, which does the same thing internally) unfortunately introduces some rounding errors. Therefore this patch introduces a new helper method in DOMWindowUtils which allows to access the content window width as measured in device pixels from the JS side, too. Review commit: https://reviewboard.mozilla.org/r/61938/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61938/
Attachment #8766915 -
Attachment description: Bug 1282902 - Extract resolution scaling into separate function → Bug 1282902 - Part 2 - Extract resolution scaling into separate function.
Attachment #8766917 -
Attachment description: Bug 1282902 - Pass previous display/viewport widths to MVM → Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring.
Attachment #8767470 -
Flags: review?(bugmail)
Attachment #8766915 -
Flags: review?(bugmail)
Attachment #8766917 -
Flags: review?(bugmail)
Assignee | ||
Comment 12•5 years ago
|
||
Comment on attachment 8766915 [details] Bug 1282902 - Part 2 - Extract resolution scaling into separate function. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61634/diff/1-2/
Assignee | ||
Comment 13•5 years ago
|
||
Comment on attachment 8766917 [details] Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61638/diff/1-2/
Assignee | ||
Updated•5 years ago
|
Attachment #8766913 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
Attachment #8766914 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
Attachment #8766916 -
Attachment is obsolete: true
Comment 14•5 years ago
|
||
Comment on attachment 8767470 [details] Bug 1282902 - Part 1 - Add a helper method for getting the content viewer size in device pixels. https://reviewboard.mozilla.org/r/61938/#review58842
Attachment #8767470 -
Flags: review?(bugmail) → review+
Updated•5 years ago
|
Attachment #8766915 -
Flags: review?(bugmail) → review+
Comment 15•5 years ago
|
||
Comment on attachment 8766915 [details] Bug 1282902 - Part 2 - Extract resolution scaling into separate function. https://reviewboard.mozilla.org/r/61634/#review58840 ::: layout/base/MobileViewportManager.h:62 (Diff revision 2) > /* Helper to clamp the given zoom by the min/max in the viewport info. */ > mozilla::CSSToScreenScale ClampZoom(const mozilla::CSSToScreenScale& aZoom, > const nsViewportInfo& aViewportInfo); > > + /* Helper to update the given resolution according to changed display and viewport widths. */ > + mozilla::LayoutDeviceToLayerScale ScaleResolutionWithDisplayWidth(const mozilla::LayoutDeviceToLayerScale& aRes, Stick a linebreak between the return type and the function name (and unindent the subsequent lines appropriately) so that the lines are less long.
Comment 16•5 years ago
|
||
Comment on attachment 8766917 [details] Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring. https://reviewboard.mozilla.org/r/61638/#review58844 So this looks fine, but I'd like to keep using the strongly-typed units as noted below, rather than using the raw types like float and uint32_t. ::: dom/base/nsDOMWindowUtils.cpp:568 (Diff revision 2) > nsIPresShell* presShell = GetPresShell(); > if (!presShell) { > return NS_ERROR_FAILURE; > } > > - presShell->SetRestoreResolution(aResolution); > + presShell->SetRestoreResolution(aResolution, aDisplayWidth, aDisplayHeight); I'd prefer if you wrapped the aDisplayWidth/Height values into a LayoutDeviceIntSize here and passed that to presShell->SetRestoreResolution instead. We're trying to use the strongly-typed units wherever possible rather than "float" and "int". The MVM function signature should also take a LayoutDeviceIntSize. ::: dom/interfaces/base/nsIDOMWindowUtils.idl:231 (Diff revision 2) > * Set a resolution on the presShell which is the "restored" from history. > * This resolution should be used when painting for the first time. Calling > * this too late may have no effect. Update comment to refer to the display size as well. ::: layout/base/MobileViewportManager.h:67 (Diff revision 2) > - mozilla::LayoutDeviceToLayerScale ScaleResolutionWithDisplayWidth(const mozilla::LayoutDeviceToLayerScale& aRes, > + mozilla::LayoutDeviceToLayerScale ScaleResolutionWithDisplayWidth(const float& aRes, > const float& aDisplayWidthChangeRatio, > - const mozilla::CSSSize& aNewViewport, > - const mozilla::CSSSize& aOldViewport); > + const float& aNewViewportWidth, > + const float& aOldViewportWidth); Here too I would like to keep using the strongly-typed units rather than "float". It looks like the call sites have the strongly-typed units, so I'm not sure if you had a good reason for changing this.
Attachment #8766917 -
Flags: review?(bugmail) → review-
Assignee | ||
Comment 17•5 years ago
|
||
https://reviewboard.mozilla.org/r/61638/#review58844 > Here too I would like to keep using the strongly-typed units rather than "float". It looks like the call sites have the strongly-typed units, so I'm not sure if you had a good reason for changing this. I possibly might have had a reason in a previous approach (where the session store directly stored only the display and viewport width), but you're right, I no longer have. Thanks for catching this.
Assignee | ||
Comment 18•5 years ago
|
||
Because we can't actually rotate the emulator within the test, we just manipulate the session store's stored display size to pretend that the device was rotated between tab data capturing and restoring. Review commit: https://reviewboard.mozilla.org/r/62158/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/62158/
Attachment #8767756 -
Flags: review?(bugmail)
Attachment #8766917 -
Flags: review- → review?(bugmail)
Assignee | ||
Comment 19•5 years ago
|
||
Comment on attachment 8767470 [details] Bug 1282902 - Part 1 - Add a helper method for getting the content viewer size in device pixels. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61938/diff/1-2/
Assignee | ||
Comment 20•5 years ago
|
||
Comment on attachment 8766915 [details] Bug 1282902 - Part 2 - Extract resolution scaling into separate function. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61634/diff/2-3/
Assignee | ||
Comment 21•5 years ago
|
||
Comment on attachment 8766917 [details] Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61638/diff/2-3/
Comment 22•5 years ago
|
||
Comment on attachment 8766917 [details] Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring. https://reviewboard.mozilla.org/r/61638/#review59064 Looks good thanks! ::: layout/base/MobileViewportManager.cpp:202 (Diff revision 3) > LayoutDeviceToLayerScale res(mPresShell->GetResolution()); > > if (mIsFirstPaint) { > CSSToScreenScale defaultZoom; > if (mRestoreResolution) { > - defaultZoom = CSSToScreenScale(mRestoreResolution.value() * cssToDev.scale); > + LayoutDeviceToLayerScale restoreResolution = You should be able to write more concisely as LayoutDeviceToLayerScale restoreResolution(mRestoreResolution.value());
Attachment #8766917 -
Flags: review?(bugmail) → review+
Comment 23•5 years ago
|
||
Comment on attachment 8767756 [details] Bug 1282902 - Part 4 - Include zoom level recalculation in session store test. https://reviewboard.mozilla.org/r/62158/#review59066 This looks fine to me but i have never written one of these tests before so if you want a more comprehensive review it would be good to get a second opinion as well. Your call.
Attachment #8767756 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 24•5 years ago
|
||
https://reviewboard.mozilla.org/r/61638/#review59064 > You should be able to write more concisely as LayoutDeviceToLayerScale restoreResolution(mRestoreResolution.value()); Thanks, I was wondering whether there wasn't some shorter syntax for this.
Updated•5 years ago
|
Priority: -- → P3
Assignee | ||
Comment 25•5 years ago
|
||
Comment on attachment 8766917 [details] Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61638/diff/3-4/
Attachment #8767756 -
Flags: review+ → review?(margaret.leibovic)
Assignee | ||
Comment 26•5 years ago
|
||
Comment on attachment 8767756 [details] Bug 1282902 - Part 4 - Include zoom level recalculation in session store test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62158/diff/1-2/
Updated•5 years ago
|
Attachment #8767756 -
Flags: review?(margaret.leibovic) → review?(s.kaspari)
Comment 27•5 years ago
|
||
Comment on attachment 8767756 [details] Bug 1282902 - Part 4 - Include zoom level recalculation in session store test. https://reviewboard.mozilla.org/r/62158/#review59706
Attachment #8767756 -
Flags: review?(s.kaspari) → review+
Assignee | ||
Comment 28•5 years ago
|
||
Comment on attachment 8767470 [details] Bug 1282902 - Part 1 - Add a helper method for getting the content viewer size in device pixels. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61938/diff/2-3/
Assignee | ||
Comment 29•5 years ago
|
||
Comment on attachment 8766915 [details] Bug 1282902 - Part 2 - Extract resolution scaling into separate function. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61634/diff/3-4/
Assignee | ||
Comment 30•5 years ago
|
||
Comment on attachment 8766917 [details] Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61638/diff/4-5/
Assignee | ||
Comment 31•5 years ago
|
||
Comment on attachment 8767756 [details] Bug 1282902 - Part 4 - Include zoom level recalculation in session store test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62158/diff/2-3/
Assignee | ||
Comment 32•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=737b63e3ed5e
Keywords: checkin-needed
Assignee | ||
Updated•5 years ago
|
Flags: needinfo?(jh+bugzilla)
Comment 33•5 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/68791be33a94 Part 1 - Add a helper method for getting the content viewer size in device pixels. r=kats https://hg.mozilla.org/integration/fx-team/rev/c3b2e397148f Part 2 - Extract resolution scaling into separate function. r=kats https://hg.mozilla.org/integration/fx-team/rev/93aa042c0fd8 Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring. r=kats https://hg.mozilla.org/integration/fx-team/rev/9329897c803d Part 4 - Include zoom level recalculation in session store test. r=sebastian
Keywords: checkin-needed
Comment 34•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/68791be33a94 https://hg.mozilla.org/mozilla-central/rev/c3b2e397148f https://hg.mozilla.org/mozilla-central/rev/93aa042c0fd8 https://hg.mozilla.org/mozilla-central/rev/9329897c803d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Comment 35•5 years ago
|
||
Jan, do you want to uplift this to Fx 49?
Assignee | ||
Comment 36•5 years ago
|
||
Yes, I just wanted to leave it a few days on Nightly to be on the safe side. But compared to bug 1279443 I think this one has less potential to break things in a subtle way, so I might as well request it now.
Flags: needinfo?(jh+bugzilla)
Assignee | ||
Comment 37•5 years ago
|
||
Comment on attachment 8767470 [details] Bug 1282902 - Part 1 - Add a helper method for getting the content viewer size in device pixels. Approval Request Comment [Feature/regressing bug #]: Mobile session store, bug 810981 [User impact if declined]: For some pages, a tab will be restored at an incorrect zoom level if the device is rotated between closing and restoring of that tab. This also applies to tabs unloaded in background or if the whole session is restored after Firefox has been killed. [Describe test coverage new/current, TreeHerder]: new and existing mochitests, manual [Risks and why]: Low, that code is well understood and only small refactoring was necessary to share the existing C++ implementation with the session store and remove some code duplication. [String/UUID change made/needed]: none
Attachment #8767470 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 38•5 years ago
|
||
Comment on attachment 8766915 [details] Bug 1282902 - Part 2 - Extract resolution scaling into separate function. (see above)
Attachment #8766915 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 39•5 years ago
|
||
Comment on attachment 8766917 [details] Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring. (see above)
Attachment #8766917 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 40•5 years ago
|
||
Comment on attachment 8767756 [details] Bug 1282902 - Part 4 - Include zoom level recalculation in session store test. (see above)
Attachment #8767756 -
Flags: approval-mozilla-aurora?
Comment 41•5 years ago
|
||
Verified as fixed in build 50.0a1 2016-07-14; Device: Motorola Razr (Android 4.4.4) and LG G4 (Android 5.1).
Comment 42•5 years ago
|
||
Comment on attachment 8766915 [details] Bug 1282902 - Part 2 - Extract resolution scaling into separate function. These patches fix a device rotating issue and also add tests. Take it in 49 aurora.
Attachment #8766915 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•5 years ago
|
Attachment #8766917 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•5 years ago
|
Attachment #8767470 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•5 years ago
|
Attachment #8767756 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 43•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/86acc0c87f52 https://hg.mozilla.org/releases/mozilla-aurora/rev/2d8cb00e2c85 https://hg.mozilla.org/releases/mozilla-aurora/rev/972c9d41b6d3 https://hg.mozilla.org/releases/mozilla-aurora/rev/e6ea844380a6
Comment 44•5 years ago
|
||
Verified as fixed in build 49.0a2 2016-07-29; Device: Motorola Razr (Android 4.4.4) and Nexus 5 (Android 6.0.1).
Status: RESOLVED → VERIFIED
Updated•4 months ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•