Closed Bug 707956 Opened 9 years ago Closed 9 years ago

Restore original zoom scale on double rotation?

Categories

(Firefox for Android :: General, defect, P4)

12 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- fixed
firefox12 --- verified

People

(Reporter: aaronmt, Assigned: pcwalton)

References

()

Details

(Whiteboard: [QA])

Attachments

(6 files, 1 obsolete file)

Attached image Nightly (12/06)
Currently when one double rotates (e.g, portrait to landscape, landscape to portrait), the originally shown zoom-scale from initial page load is not restored. As far as I can see, from landscape to portrait, the zoom level is valued at what it was modified from after the landscape rotation.

Should a rotation back to portrait restore the zoom level to what it was prior to first rotation to landscape? 

See attached screenshot: first frame - page load in portrait, second frame - landscape, third frame - portrait.

Used the URL: http://news.ycombinator.com/item?id=3319384 

--
Samsung Nexus S (Android 2.3.6)
20111206080625
http://hg.mozilla.org/projects/birch/rev/5d205868839c
No longer blocks: 704738
Priority: -- → P4
Duplicate of this bug: 710212
This feels like one of those low hanging fruit whose inconsistent behavior will confuse users. On most other mobile browsers today this is taken for granted.
Assigning to myself, but I haven't started on it yet -- if someone else wants to work on it, just let me know.
Assignee: nobody → mbrubeck
Now that bug 708307 has landed, I decided to do some more testing on this.
I used my homepage for the test because I am not planning on making major changes to the layout.

These screenshots are all on a Samsung Galaxy Tab 8.9 running Android 3.1.

The page I used for testing is at http://www.wg9s.com/

Attachment 582519 [details] shows how the page appears if you originally load it in portrait mode.  Attachment 582520 [details] shows how the page looks if you originally load it in landscape.  Note that in both cases the page formats correctly to fit the screen width.

Attachment 582521 [details] shows how the page appears when you load it in landscape more and then switch to portrait.  Not that this does not look like the case of originally loading in portrait.  The page is not sized to fit the width, but is instead zoomed to have very large text.  This sounds like the problem already marked fixed in Bug 704738.

Attachment 582522 [details] shows the result form a second rotation back to landscape showing how the original zoom scale is not restored.
As additional information that might be helpful here, in landscape mode the screen width is 1280, in portrait mode it is 800.
Duplicate of this bug: 711258
Version: unspecified → Firefox 11
A lot of users are talking about this being a bad user visible issue in today's testday on Aurora. Recommended for higher priority.
Whiteboard: [QA]
Version: Firefox 11 → Firefox 12
Duplicate of this bug: 715982
Duplicate of this bug: 718894
Any decisions/updates to this bug?
I've been working on this on Friday and Tuesday. My patch is working on sites with no <meta viewport>, but it's breaking sites that have width set to device-width. The solution is most likely going to be to just disable the logic for those sites.

Assigning to self.
Assignee: mbrubeck → pwalton
Status: NEW → ASSIGNED
(In reply to Patrick Walton (:pcwalton) from comment #18)
> My patch is working on sites with no <meta viewport>, but it's breaking sites
> that have width set to device-width. The solution is most likely going to be to
> just disable the logic for those sites.

Yeah, that is excatly what we did in XUL fennec:
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser.js#3355
Attached patch Proposed patch. (obsolete) — Splinter Review
This patch adjusts zoom proportionally after rotation, matching what other browsers generally do. Two different paths are needed, one for sites that don't autosize (this one is in Java) and one for sites that do autosize (in JS). Tested on cnn.com (for a non-autosizing site) and on the mobile version of techcrunch.com (for an autosizing site).
Attachment #590100 - Flags: review?(bugmail.mozilla)
Patch version 2 removes an unused variable.
Attachment #590100 - Attachment is obsolete: true
Attachment #590100 - Flags: review?(bugmail.mozilla)
Attachment #590101 - Flags: review?(bugmail.mozilla)
Attachment #590101 - Attachment description: Propsoed patch, version 2. → Proposed patch, version 2.
Comment on attachment 590101 [details] [diff] [review]
Proposed patch, version 2.

r=me, but had a question below.

>+
>+    // Avoid having the scroll position jump around after device rotation.
>+    let win = this.browser.contentWindow;
>+    this.userScrollPos.x = win.scrollX;
>+    this.userScrollPos.y = win.scrollY;

Is this needed here? updateViewport below sets the viewport, and the viewport setter updates these after calling scrollTo. The only case I think it might be needed in is if viewportW == oldBrowserWidth so this code bails out of the guard below.
Attachment #590101 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (:kats) from comment #22)
> Comment on attachment 590101 [details] [diff] [review]
> Proposed patch, version 2.
> 
> r=me, but had a question below.
> 
> >+
> >+    // Avoid having the scroll position jump around after device rotation.
> >+    let win = this.browser.contentWindow;
> >+    this.userScrollPos.x = win.scrollX;
> >+    this.userScrollPos.y = win.scrollY;
> 
> Is this needed here? updateViewport below sets the viewport, and the
> viewport setter updates these after calling scrollTo. The only case I think
> it might be needed in is if viewportW == oldBrowserWidth so this code bails
> out of the guard below.

Yes, it's needed to prevent the scroll listener from triggering. You can see the bug if you scroll to the bottom right of cnn.com and rotate the device a few times.
https://hg.mozilla.org/mozilla-central/rev/0e9d30f46b7d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Depends on: 720364
Aurora nom? This looks really bad in the product imo.
tracking-fennec: --- → ?
Verified Fixed on M-C
Samsung Galaxy Nexus (Android 4.0.3)
20120126031113
http://hg.mozilla.org/mozilla-central/rev/402b394b6623
Comment on attachment 590101 [details] [diff] [review]
Proposed patch, version 2.

[Approval Request Comment]
User impact if declined: Rotating the device results in unexpected panning/zooming.

Testing completed (on m-c, etc.): Fix has been on m-c since 1/22.

Risk to taking this patch (and alternatives if risky): Patch is mobile-only and fairly localized.  Unexpected bugs in the patch could cause regressions in zooming on page load / screen rotation.  There is one known cosmetic issue about extra redraws on rotation (bug 720364) which is not yet fixed; I don't think it needs to block this patch shipping on Aurora.
Attachment #590101 - Flags: approval-mozilla-aurora?
Attachment #590101 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.