Closed Bug 1282902 Opened 4 years ago Closed 4 years ago

Session store incorrectly scales about:firefox if device was rotated between saving and restoring that tab

Categories

(Firefox for Android :: Toolbar, defect, P3)

All
Android
defect

Tracking

()

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)

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.
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
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
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.
Can you give an example of what kind of values you're getting from window.devicePixelRatio and gScreenWidth and what the rounding error is?
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).
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)
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/
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/
Attachment #8766913 - Attachment is obsolete: true
Attachment #8766914 - Attachment is obsolete: true
Attachment #8766916 - Attachment is obsolete: true
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+
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 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-
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.
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)
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/
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/
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 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 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+
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.
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)
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/
Attachment #8767756 - Flags: review?(margaret.leibovic) → review?(s.kaspari)
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+
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/
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/
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/
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/
Flags: needinfo?(jh+bugzilla)
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
Jan, do you want to uplift this to Fx 49?
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)
Blocks: 810981
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?
Comment on attachment 8766915 [details]
Bug 1282902 - Part 2 - Extract resolution scaling into separate function.

(see above)
Attachment #8766915 - Flags: approval-mozilla-aurora?
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?
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?
Verified as fixed in build 50.0a1 2016-07-14;
Device: Motorola Razr (Android 4.4.4) and LG G4 (Android 5.1).
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+
Attachment #8766917 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8767470 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8767756 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
You need to log in before you can comment on or make changes to this bug.