Closed Bug 1002426 Opened 6 years ago Closed 5 years ago

Zoom level mangled after restoring session

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox29 --- unaffected
firefox30 --- unaffected
firefox31 + verified
firefox32 + verified
fennec 31+ ---

People

(Reporter: CristinaM, Assigned: esawin)

References

Details

(Keywords: regression, reproducible)

Attachments

(2 files, 4 obsolete files)

Environment:
Build: Nightly (2014-04-28)
Devices: Galaxy Nexus (Android 4.2.1) and Samsung S3 (Android 4.3)

Steps to reproduce:
1. Start Fennec and navigate to a page (e.g. www.cnn.com);
3. Tools-> New Guest Session;
4. Exit Guest Session.

Expected results:
The page opened before guest mode is displayed normally.

Actual results:
The page opened before guest mode is zoomed in.

Notes:
I'm not able to reproduce this on tablets.
Blocks: 611556
tracking-fennec: --- → ?
Summary: Leaving guest session displays the page opened before zoomed in → Zoom level mangled after restoring session
This is reproducible on other sites such as kernel.org, I was able to reproduce this on my Nexus 5 (Android 4.4.2). Likely a regression from recent landing bug 611556, I think. Eugen?
Flags: needinfo?(esawin)
This is reproducing on tablets, too, it's just unnoticeable on the common screen sizes.

Bug 611556 enables zoom session history, which means we override the resolution when moving in history. In this case, we override the resolution with the default value (1.0), because the resolution has not been properly restored from disk.

It's related to bug 697858, scrolling position is not being properly restored either (tested on Fennec 28+). I can provide a workaround to disable zoom restoring from disk, but ultimately we need to fix the |SessionStorage| to provide complete state restoring from the persistent session storage, including scroll position and zooming.
Flags: needinfo?(esawin)
That's just a hotfix to disable zoom level overriding with the default zoom by setting the default value to an invalid range.
Assignee: nobody → esawin
I think the original default (1.0) is good because it's a valid default resolution. But given a valid default resolution, there is no way for us to determine whether the session restoring (from disk or online) was successful or not.

Having the default resolution set to an invalid range (0.0) is good because that signals us that we don't have a valid resolution restored from a previous session and it's up to the frontend to determine something sensible for the given device and document.

In the end, we want to add zoom restoring from session storage, but having a way to know that it was successful would still be useful, as contrary to scroll position, there is no general default resolution across all devices and documents.
Attachment #8413937 - Attachment is obsolete: true
Attachment #8414854 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8414854 [details] [diff] [review]
Set uninitialized resolution to invalid range

Review of attachment 8414854 [details] [diff] [review]:
-----------------------------------------------------------------

I think this will cause regressions on other platforms that also use the resolution code but don't have Fennec's front-end zoom-restoring code.
Attachment #8414854 - Flags: feedback?(bugmail.mozilla) → feedback-
tracking-fennec: ? → 31+
That's right, a lot of desktop code assumes a valid range for the default value, we can't change that easily.

This patch uses the existing scroll frame restore status to trigger resolution overriding. We have to make |isHistoryRestored| visible to the frontend for that. I've put it into |nsDOMWidowUtil| where the resolution is accessed through, is there a better location for that?

This should fix all situations, where we unexpectedly restore to the default resolution.
Attachment #8414854 - Attachment is obsolete: true
Attachment #8416806 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8416806 [details] [diff] [review]
(WIP) Check restore status before overriding zoom

Review of attachment 8416806 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like a reasonable approach to me.
Attachment #8416806 - Flags: feedback?(bugmail.mozilla) → feedback+
Just did some clean up. This should be good, unless we don't want to expose |isHistoryRestored| via |nsDOMWindowUtils|.
Attachment #8416806 - Attachment is obsolete: true
Attachment #8417499 - Flags: review?(bugmail.mozilla)
Comment on attachment 8417499 [details] [diff] [review]
Check restore status before overriding zoom

Review of attachment 8417499 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/base/nsDOMWindowUtils.cpp
@@ +591,5 @@
> +
> +  nsIScrollableFrame* sf = presShell->GetRootScrollFrameAsScrollable();
> +  if (sf) {
> +    *aIsHistoryRestored = sf->DidHistoryRestore();
> +  }

I would just do
*aIsHistoryRestored = (sf && sf->DidHistoryRestore());

and get rid of the assignment to false up at the top. If there is an error return code the bool return value won't be accessible anyway.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +231,5 @@
>    void getResolution(out float aXResolution, out float aYResolution);
>  
>    /**
> +   * Whether the current window has been restored from session history.
> +   * This gives a way to check whether the provided resolution and scroll

This file needs a UUID bump

::: mobile/android/chrome/content/browser.js
@@ +4208,5 @@
> +    let res = {x: {}, y: {}};
> +    let cwu = this.browser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> +    cwu.getResolution(res.x, res.y);
> +    let zoom =  res.x.value * window.devicePixelRatio;
> +    return zoom;

nit: no need for 'zoom' local var, just return the result of multiplication. If you want to leave the local var in, at least remove the double-space after '='

@@ +4224,2 @@
>        return null;
>      }

To make this easier to read, invert the condition:

if (this._restoreZoom && cwu.isHistoryRestored) {
   return this._getGeckoZoom();
}
return null;

Or use ?:
Attachment #8417499 - Flags: review?(bugmail.mozilla) → review+
Thanks for the super fast review!
I've addressed all the issues, but left the local zoom variable for readability (will be optimized out) and added some more const-correctness.

Carrying over the r+.
Attachment #8417669 - Flags: review+
Attachment #8417499 - Attachment is obsolete: true
Duplicate of this bug: 1004488
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/7a96c878cdf8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Verified as fixed on:

Build: Nightly (2014-05-08);

Device: Galaxy Nexus (Android 4.2.1).

This is still reproducible on Aurora (2014-05-05).
Status: RESOLVED → VERIFIED
The proposed fix depends on a correctly set |isHistoryRestored| flag, which may not be the set when we restore via the fastback cache (bfcache).

I would like to reopen the bug to propose a patch, which depends on a new flag specifically designed to signal whether the resolution has been set by the user or not, to avoid any side effects with the bfcache.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
This fix just adds and uses the |isResolutionSet| flag instead of |isHistoryRestored| to avoid any issues with bfcache, the logic stays the same.
Attachment #8421121 - Flags: review?(bugmail.mozilla)
Comment on attachment 8421121 [details] [diff] [review]
(Fix) Check resolution restore status via specialized flag

Review of attachment 8421121 [details] [diff] [review]:
-----------------------------------------------------------------

This looks ok to me, but I'd like a layout peer to look too.
Attachment #8421121 - Flags: review?(tnikkel)
Attachment #8421121 - Flags: review?(bugmail.mozilla)
Attachment #8421121 - Flags: review+
Attachment #8421121 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/mozilla-central/rev/3ba939c4a277
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
I'm still able to reproduce this on Aurora (2014-05-27). On Nightly (2014-05-27) is not reproducible using Alcatel One Touch (Android 4.1.2).
Keywords: verifyme
(In reply to cristina.madaras from comment #22)
> I'm still able to reproduce this on Aurora (2014-05-27). On Nightly
> (2014-05-27) is not reproducible using Alcatel One Touch (Android 4.1.2).

This has not been been uplifted to Aurora.
NI to request uplift
Flags: needinfo?(esawin)
Comment on attachment 8417669 [details] [diff] [review]
Check restore status before overriding zoom

Review of attachment 8417669 [details] [diff] [review]:
-----------------------------------------------------------------

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 611556
User impact if declined: Restoring the previous tab session or returning from a guest session would render the page at a non-standard resolution. On most smartphone devices, this would open the pages at zoom level which is neither the default zoom level for the page nor the zoom level last used by the user.
Testing completed (on m-c, etc.): It's been in m-c for a few weeks now, no issues.
Risk to taking this patch (and alternatives if risky): Low, well tested in m-c.
String or UUID changes made by this patch: nsIDOMWindowUtils.
Attachment #8417669 - Flags: approval-mozilla-aurora?
Attachment #8421121 - Flags: approval-mozilla-aurora?
Flags: needinfo?(esawin)
Attachment #8417669 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8421121 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in:
Build: 31.0a2 (2014-06-04);
Device: Galaxy Nexus (Android 4.2.1).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.