707956 - Restore original zoom scale on double rotation?

Status: VERIFIED FIXED

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

Description

[Aaron Train [:aaronmt]]

Attachment: 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)]

related to bug 707934 ?

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Oh oops.  ignore comment 1 please.

Comment 4

[Alex Keybl [:akeybl]]

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)]

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]]

Attachment: original page load in portrait

Comment 7

[Bill Gianopoulos [:WG9s]]

Attachment: original page load in landscape

Comment 8

[Bill Gianopoulos [:WG9s]]

Attachment: rotate to portrait

Comment 9

[Bill Gianopoulos [:WG9s]]

Attachment: rotate back to landscape

Comment 10

[Bill Gianopoulos [:WG9s]]

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]]

As additional information that might be helpful here, in landscape mode the screen width is 1280, in portrait mode it is 800.

Comment 13

[Aaron Train [:aaronmt]]

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 17

[Aaron Train [:aaronmt]]

Any decisions/updates to this bug?

Comment 18

[Patrick Walton (:pcwalton)]

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)]

(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)]

Attachment: 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)]

Attachment: Proposed patch, version 2.

Patch version 2 removes an unused variable.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

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)]

(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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9d30f46b7d

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0e9d30f46b7d

Comment 26

[Aaron Train [:aaronmt]]

Aurora nom? This looks really bad in the product imo.

Comment 27

[Aaron Train [:aaronmt]]

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)]

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)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6dc4301a4511