1046631 - Zoom level changes on some mobile sites on pressing the back button

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[VN]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release)
Build ID: 20140731061946

Steps to reproduce:

1. Open Firefox Beta for Android
2. Go to http://m.dw.de/english
3. Click on any article link to go to the article
4. Click the back button to go to the main dw.de page 


Actual results:

1. The mobile site http://m.dw.de/english is zoomable using pinch-to-zoom.
2. After going back to the main page in step 4, the main page is zoomed-in a lot; you have to zoom out fully using pinch-to-zoom to reset it.

Screenshots: https://imgur.com/a/Wi9B6


Expected results:

1. The mobile site http://m.dw.de/english should not have been zoomable[#] using pinch-to-zoom. 
2. After going back to the main page in step 4, the original zoom level should have persisted.

[#] Note: I am not a developer or a UX expert, but I have read that by convention, pinch-to-zoom is (should be?) disabled on mobile-optimized sites. I don't know whether this is done at the browser level or the site-level.

Comment 1

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

I can reproduce on nightly as well. The page being zoomable is fine because that is specified at the website level and this site chose to make themselves zoomable. However the zoom shouldn't change when going back. Eugen, is this a known issue?

Comment 2

[Eugen Sawin [:esawin]]

It's not reproducing on Fennec 31, here I am hitting bug 1003800 instead.
It is reproducing consistently on Fennec 32+.

It could be a regression on bug 1002426, I can check for that next week.

Comment 3

[Eugen Sawin [:esawin]]

It's a regression caused by bug 1003962.

Line http://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4263 sets |browserWidth| to the default browser width, which is then used in http://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4475 to compute the wrong |zoomScale|.

Here's a comment from bug 1003962.
> This code is has been a source of problems like this for a while. In this
> case I *think* the right solution might be to call 
> 
> this.setBrowserSize(kDefaultCSSViewportWidth, kDefaultCSSViewportHeight);
> 
> inside this block:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js?rev=5be519918f65#4155
> 
> I suspect that will fix this problem but I have no idea if it will break
> other things.

So this seems to something that it breaks. I could find a workaround for this, or we find a safer solution for the original issue. What do you think, Kats?

Comment 4

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

I don't think it should be going into that block at all when you hit 'back'. |zoom && aInitialLoad| should be true, and it should skip over that block, I think. Mostly likely it's getting called with aInitialLoad=false, do you know from where?

Comment 5

[Eugen Sawin [:esawin]]

We do call |updateViewportSize| with |aInitialLoad=false| for each page load, but that's not what is causing the regression, I think.
What I see happening, is that we update the zoom by multiplying it with a wrong zoom scale right after clicking on a link and before loading the linked page. That wrongly assigned zoom is being saved for the session and loaded when going back.

Here is what happens in more detail (portrait mode on Nexus 7, (*) shows the value has been regressed).

A. Initial page load (http://m.dw.de):
1. DOMMetaAdded -> updateMetadata -> updateViewportMetadata -> updateViewportSize:
   aOldScreenWidth = 1200
   aInitialLoad = false
   oldBrowserWidth = 980 (*)
   zoomScale = 1.53 (*)
   viewportW = 640
   restoredSessionZoom() = null
   metadata.defaultZoom = undefined/null/0
2. setResolution: 1.22 -> 1.88
3. before-first-paint:
   restoredSessionZoom() = null
   zoom = 1.22
4. setResolution: 1.88 -> 1.22
5. updateViewportMetadata -> updateViewportSize:
   aOldScreenWidth = 1200
   aInitialLoad = true
   oldBrowserWidth = 980 (*)
   zoomScale = 1.53 (*)
   viewportW = 640
   restoredSessionZoom() = null
   metadata.defaultZoom = undefined/null/0
6. setResolution: 1.22 -> 1.88

B. Clicking on an article link:
1. DOMMetaAdded -> updateMetadata -> updateViewportMetadata -> updateViewportSize:
   aOldScreenWidth = 1200
   aInitialLoad = false
   oldBrowserWidth = 980 (*)
   zoomScale = 1.53 (*)
   viewportW = 640
   restoredSessionZoom() = null
   metadata.defaultZoom = undefined/null/0
2. setResolution: 1.88 -> 2.87
3. before-first-paint:
   restoredSessionZoom() = null
   zoom = 1.22
4. setResolution: 2.87 -> 1.22
5. updateViewportMetadata -> updateViewportSize:
   aOldScreenWidth = 1200
   aInitialLoad = true
   oldBrowserWidth = 980 (*)
   zoomScale = 1.53 (*)
   viewportW = 640
   restoredSessionZoom() = null
   metadata.defaultZoom = undefined/null/0
6. setResolution: 1.22 -> 1.88

C. Clicking back button:
1. DOMMetaAdded -> updateMetadata -> updateViewportMetadata -> updateViewportSize:
   aOldScreenWidth = 1200
   aInitialLoad = false
   oldBrowserWidth = 980 (*)
   zoomScale = 1.53 (*)
   viewportW = 640
   restoredSessionZoom() = null
   metadata.defaultZoom = undefined/null/0
2. setResolution: 1.88 -> 2.87
3. before-first-paint:
   restoredSessionZoom() = 2.87
   zoom = 2.87
4. setResolution: 2.87 -> 2.87
5. updateViewportMetadata -> updateViewportSize:
   aOldScreenWidth = 1200
   aInitialLoad = true
   restoredSessionZoom() = 2.87
6. setResolution: 2.87 -> 2.87

Comment 6

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

Nice description, thanks!

A couple of things: (1) It looks like the DOMMetaAdded for a page is getting fired before the first-paint stuff. That seems unexpected, and might be the "root" cause here. (2) The restoredSessionZoom() that returns 2.87 (second-last line in comment 5) - when does that zoom get saved? Is it between C1 and C2?

If my theory is correct then when we get the DOMMetaAdded event, we should only process it if BrowserApp.isBrowserContentDocumentDisplayed() is true. That way in the cases where we get the DOMMetaAdded before the first-paint (A1, B1, and C1 in your flow) we will just ignore it and do nothing. The correct zoom will be set as part of A5, B5, and C5. It looks like all of A1, B1, and C1 set a wrong zoom value anyway which we shouldn't be doing.

Does that make sense?

Comment 7

[Eugen Sawin [:esawin]]

> A couple of things: (1) It looks like the DOMMetaAdded for a page is getting
> fired before the first-paint stuff. That seems unexpected, and might be the
> "root" cause here.
I haven't paid much attention to this fact, since this has been the case for the past few months (at least) and it didn't seem to cause other issues yet.

> (2) The restoredSessionZoom() that returns 2.87
> (second-last line in comment 5) - when does that zoom get saved? Is it
> between C1 and C2?
From what I could read from the debug output, it is being set in B2; so *after* clicking on the link, but before the before-first-paint handling.

> If my theory is correct then when we get the DOMMetaAdded event, we should
> only process it if BrowserApp.isBrowserContentDocumentDisplayed() is true.
> That way in the cases where we get the DOMMetaAdded before the first-paint
> (A1, B1, and C1 in your flow) we will just ignore it and do nothing. The
> correct zoom will be set as part of A5, B5, and C5. It looks like all of A1,
> B1, and C1 set a wrong zoom value anyway which we shouldn't be doing.
> 
> Does that make sense?

That sounds good and does indeed fix the issue, I will test it some more to see if it breaks anything else.
However, I'm still uneasy about oldBrowserWidth always holding the default width value because of bug 1003962. Is that expected behavior?

Comment 8

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

Yeah I think that is expected. oldBrowserWidth is supposed to only matter for changes in size/CSS viewport after the initial page load. So for example if you load a page and rotate the device, the oldBrowserWidth helps compute the new zoom that we should display the content at after rotation. For the initial stages of page load oldBrowserWidth should not be carrying over random values from the previous page, and bug 1003962 makes that happen.

Comment 9

[Eugen Sawin [:esawin]]

Attachment: Fix meta data update

Try: https://tbpl.mozilla.org/?tree=Try&rev=42e29203d218

Comment 10

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

Comment on attachment 8471585 [details] [diff] [review]
Fix meta data update

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

r=me with the change below. Also please change the commit message, it's not very descriptive of the patch. Something more like "Don't process meta-viewport tags from a new page load before displaying the new page"

::: mobile/android/chrome/content/browser.js
@@ +6201,5 @@
>            break;
>          let document = target.ownerDocument;
>          let browser = BrowserApp.getBrowserForDocument(document);
>          let tab = BrowserApp.getTabForBrowser(browser);
> +        if (tab && BrowserApp.isBrowserContentDocumentLoaded())

Actually since we have the tab here already we can just do tab.contentDocumentIsDisplayed instead of BrowserApp.isBrowserContentDocumentLoaded()

Comment 11

[Eugen Sawin [:esawin]]

Attachment: Fix meta data update

Try: https://tbpl.mozilla.org/?tree=Try&rev=c8539aacf7ca

Comment 12

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

Comment on attachment 8471780 [details] [diff] [review]
Fix meta data update

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

::: mobile/android/chrome/content/browser.js
@@ +6201,5 @@
>            break;
>          let document = target.ownerDocument;
>          let browser = BrowserApp.getBrowserForDocument(document);
>          let tab = BrowserApp.getTabForBrowser(browser);
> +        if (tab && tab.contentDocumentIsDisplayed())

It's not a function

Comment 13

[Eugen Sawin [:esawin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> Comment on attachment 8471780 [details] [diff] [review]
> Fix meta data update
> 
> Review of attachment 8471780 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +6201,5 @@
> >            break;
> >          let document = target.ownerDocument;
> >          let browser = BrowserApp.getBrowserForDocument(document);
> >          let tab = BrowserApp.getTabForBrowser(browser);
> > +        if (tab && tab.contentDocumentIsDisplayed())
> 
> It's not a function

Oh wow, thanks for catching that.

Comment 14

[Eugen Sawin [:esawin]]

Attachment: Fix meta data update

Try: https://tbpl.mozilla.org/?tree=Try&rev=bf77f77a1bec

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/afb4e5f28df4

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/afb4e5f28df4

Comment 17

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: noticeable regression

Eugen we only have 2 more betas before 32 goes out the door. Would you request approval aurora and beta on this patch? Else explain why we should not uplift it.e

Comment 18

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in build:
Firefox for Android 34.0a1 (2014-08-17)
Device: Samsung S3 (Android 4.3)

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

tracking+ for 32 and 33 as this is a visible regression.

Eugen - This has been on m-c for 4 days. Can you nom for beta and aurora today so that we can get this into beta8?

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

We can still get this into beta10 on Monday. Eugen, can I get an approval request?

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8471811 [details] [diff] [review]
Fix meta data update

Sorry: Eugen has been on PTO.

Approval Request Comment
[Feature/regressing bug #]: bug 1003962
[User impact if declined]: Bad zooming behavior
[Describe test coverage new/current, TBPL]: Several days on m-c and a verification
[Risks and why]: moderate risks given the area of code, but no regression have been filed yet after a week or so of Nightly usage.
[String/UUID change made/needed]: none

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8471811 [details] [diff] [review]
Fix meta data update

Thanks Mark. Approved for Aurora and Beta. This fix will be in 32 beta10.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c2ead550888e
https://hg.mozilla.org/releases/mozilla-beta/rev/548fff9b12df

Comment 24

[Eugen Sawin [:esawin]]

Thanks Mark! I'll make sure to update my profile name on my next PTO.