Last Comment Bug 707956 - Restore original zoom scale on double rotation?
: Restore original zoom scale on double rotation?
Status: VERIFIED FIXED
[QA]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 12 Branch
: ARM Android
: P4 normal with 1 vote (vote)
: Firefox 12
Assigned To: Patrick Walton (:pcwalton)
:
Mentors:
http://news.ycombinator.com/item?id=3...
: 710212 711258 715956 715982 718894 (view as bug list)
Depends on: 720364
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-06 09:00 PST by Aaron Train [:aaronmt]
Modified: 2013-12-10 10:00 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified


Attachments
Nightly (12/06) (318.40 KB, image/png)
2011-12-06 09:00 PST, Aaron Train [:aaronmt]
no flags Details
original page load in portrait (303.35 KB, image/jpeg)
2011-12-17 04:28 PST, Bill Gianopoulos [:WG9s]
no flags Details
original page load in landscape (493.74 KB, image/jpeg)
2011-12-17 04:32 PST, Bill Gianopoulos [:WG9s]
no flags Details
rotate to portrait (439.58 KB, image/jpeg)
2011-12-17 04:34 PST, Bill Gianopoulos [:WG9s]
no flags Details
rotate back to landscape (501.60 KB, image/jpeg)
2011-12-17 04:35 PST, Bill Gianopoulos [:WG9s]
no flags Details
Proposed patch. (6.30 KB, patch)
2012-01-19 21:47 PST, Patrick Walton (:pcwalton)
no flags Details | Diff | Splinter Review
Proposed patch, version 2. (5.74 KB, patch)
2012-01-19 21:51 PST, Patrick Walton (:pcwalton)
bugmail: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Aaron Train [:aaronmt] 2011-12-06 09:00:05 PST
Created attachment 579319 [details]
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
Comment 1 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-12-06 09:37:47 PST
related to bug 707934 ?
Comment 2 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-12-06 09:39:21 PST
Oh oops.  ignore comment 1 please.
Comment 3 Aaron Train [:aaronmt] 2011-12-13 07:24:03 PST
*** Bug 710212 has been marked as a duplicate of this bug. ***
Comment 4 Alex Keybl [:akeybl] 2011-12-13 07:43:52 PST
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.
Comment 5 Matt Brubeck (:mbrubeck) 2011-12-13 09:15:30 PST
Assigning to myself, but I haven't started on it yet -- if someone else wants to work on it, just let me know.
Comment 6 Bill Gianopoulos [:WG9s] 2011-12-17 04:28:20 PST
Created attachment 582519 [details]
original page load in portrait
Comment 7 Bill Gianopoulos [:WG9s] 2011-12-17 04:32:10 PST
Created attachment 582520 [details]
original page load in landscape
Comment 8 Bill Gianopoulos [:WG9s] 2011-12-17 04:34:23 PST
Created attachment 582521 [details]
rotate to portrait
Comment 9 Bill Gianopoulos [:WG9s] 2011-12-17 04:35:58 PST
Created attachment 582522 [details]
rotate back to landscape
Comment 10 Bill Gianopoulos [:WG9s] 2011-12-17 04:54:45 PST
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.
Comment 11 Bill Gianopoulos [:WG9s] 2011-12-17 06:46:19 PST
As additional information that might be helpful here, in landscape mode the screen width is 1280, in portrait mode it is 800.
Comment 12 John Hammink 2011-12-19 10:09:15 PST
*** Bug 711258 has been marked as a duplicate of this bug. ***
Comment 13 Aaron Train [:aaronmt] 2012-01-06 12:20:30 PST
A lot of users are talking about this being a bad user visible issue in today's testday on Aurora. Recommended for higher priority.
Comment 14 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-01-09 09:51:15 PST
*** Bug 715956 has been marked as a duplicate of this bug. ***
Comment 15 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-01-09 09:54:56 PST
*** Bug 715982 has been marked as a duplicate of this bug. ***
Comment 16 Aaron Train [:aaronmt] 2012-01-18 07:52:24 PST
*** Bug 718894 has been marked as a duplicate of this bug. ***
Comment 17 Aaron Train [:aaronmt] 2012-01-18 07:53:01 PST
Any decisions/updates to this bug?
Comment 18 Patrick Walton (:pcwalton) 2012-01-18 08:08:26 PST
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.
Comment 19 Matt Brubeck (:mbrubeck) 2012-01-18 09:50:06 PST
(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
Comment 20 Patrick Walton (:pcwalton) 2012-01-19 21:47:52 PST
Created attachment 590100 [details] [diff] [review]
Proposed patch.

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).
Comment 21 Patrick Walton (:pcwalton) 2012-01-19 21:51:04 PST
Created attachment 590101 [details] [diff] [review]
Proposed patch, version 2.

Patch version 2 removes an unused variable.
Comment 22 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-20 08:03:41 PST
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.
Comment 23 Patrick Walton (:pcwalton) 2012-01-20 21:12:45 PST
(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.
Comment 24 Patrick Walton (:pcwalton) 2012-01-20 21:15:16 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9d30f46b7d
Comment 25 Ed Morley [:emorley] 2012-01-22 12:34:00 PST
https://hg.mozilla.org/mozilla-central/rev/0e9d30f46b7d
Comment 26 Aaron Train [:aaronmt] 2012-01-26 12:49:06 PST
Aurora nom? This looks really bad in the product imo.
Comment 27 Aaron Train [:aaronmt] 2012-01-26 12:53:10 PST
Verified Fixed on M-C
Samsung Galaxy Nexus (Android 4.0.3)
20120126031113
http://hg.mozilla.org/mozilla-central/rev/402b394b6623
Comment 28 Matt Brubeck (:mbrubeck) 2012-01-26 12:56:38 PST
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.
Comment 29 Matt Brubeck (:mbrubeck) 2012-01-26 17:38:13 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/6dc4301a4511

Note You need to log in before you can comment on or make changes to this bug.