Closed Bug 944479 Opened 7 years ago Closed 4 years ago

<meta name="viewport" content="height=device-height"> is not respected on Fennec

Categories

(Firefox for Android :: Toolbar, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: botond, Unassigned)

Details

Attachments

(1 file)

See bug 942799 for the corresponding issue on B2G. Matt Brubeck suggested to make a similar change to Fennec (https://bugzilla.mozilla.org/show_bug.cgi?id=942799#c7).

+++ This bug was initially created as a clone of Bug #942799 +++
Component: Panning and Zooming → Graphics, Panning and Zooming
OS: Gonk (Firefox OS) → Android
Product: Core → Firefox for Android
Attached patch PatchSplinter Review
Note that I'm removing the update to screenW even though screenW is used later. I can do this because the horizontal margins are always zero (the margins is only used for the dynamic toolbar which is a vertical thing).
Assignee: nobody → bugmail.mozilla
Attachment #8344845 - Flags: review?(mbrubeck)
Attachment #8344845 - Flags: review?(chrislord.net)
Attachment #8344845 - Flags: review?(mbrubeck) → review+
Unfortunately this patch seems to consistently cause M-2 and M-3 to fail on android 4.0 opt. https://tbpl.mozilla.org/?tree=Try&rev=2a457a170aa9 and the try run above. I will try to take a look.
Comment on attachment 8344845 [details] [diff] [review]
Patch

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

I'm removing my r? because of the test failures, but I have questions.

Along with the question below, I'm worried about your comment on screenW. Horizontal margins are known to just not work at all, so I'm not too bothered, but is that change something that ought to be commented on in the code? (or maybe we need to add a comment about horizontal margins being known broken?)

::: mobile/android/chrome/content/browser.js
@@ -4028,4 @@
>      }
> -    minScale = this.clampZoom(minScale);
> -    viewportH = Math.max(viewportH, screenH / minScale);
> -    this.setBrowserSize(viewportW, viewportH);

I'm not convinced that the clampZoom and setBrowserSize are unnecessary, especially as clampZoom gets used further down in this function. Can you explain this change a bit more?
Attachment #8344845 - Flags: review?(chrislord.net)
(In reply to Chris Lord [:cwiiis] from comment #4)
> Along with the question below, I'm worried about your comment on screenW.
> Horizontal margins are known to just not work at all, so I'm not too
> bothered, but is that change something that ought to be commented on in the
> code? (or maybe we need to add a comment about horizontal margins being
> known broken?)

Yeah I can add a comment there. I can also leave the screenW and screenH assignments in if you prefer; it shouldn't make a difference one way or another - it's just dead code.

> I'm not convinced that the clampZoom and setBrowserSize are unnecessary,
> especially as clampZoom gets used further down in this function. Can you
> explain this change a bit more?

The clampZoom function is used a bit later, yes, but the minScale variable (which is the thing I'm changing) is not used later. Taking out that setBrowserSize call is the entire goal of this bug, because that is what makes us resize the CSS viewport height to be bigger than what the meta tag says it should be. It's just that taking out the setBrowserSize makes a bunch of variable assignments unused so I remove them as well.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> (In reply to Chris Lord [:cwiiis] from comment #4)
> > Along with the question below, I'm worried about your comment on screenW.
> > Horizontal margins are known to just not work at all, so I'm not too
> > bothered, but is that change something that ought to be commented on in the
> > code? (or maybe we need to add a comment about horizontal margins being
> > known broken?)
> 
> Yeah I can add a comment there. I can also leave the screenW and screenH
> assignments in if you prefer; it shouldn't make a difference one way or
> another - it's just dead code.

That's fine, let's not leave in dead code.

> > I'm not convinced that the clampZoom and setBrowserSize are unnecessary,
> > especially as clampZoom gets used further down in this function. Can you
> > explain this change a bit more?
> 
> The clampZoom function is used a bit later, yes, but the minScale variable
> (which is the thing I'm changing) is not used later. Taking out that
> setBrowserSize call is the entire goal of this bug, because that is what
> makes us resize the CSS viewport height to be bigger than what the meta tag
> says it should be. It's just that taking out the setBrowserSize makes a
> bunch of variable assignments unused so I remove them as well.

I thought the goal of this bug was to respect height=device-height? That call does that, but it also changes the browser size when the toolbar becomes unpinned, so with this change, the browser size will always be as if the toolbar was permanently visible (which will likely cause absolutely positioned content to layout incorrectly, and may be the cause of your test failures?).

I'm not sure you can so easily just remove that call. Strictly speaking, it's the "minScale = screenW / pageWidth;" line that causes the expansion, I think.
After staring at the code some more and considering what you wrote, I agree that my patch is wrong. To fix this bug we need to get rid of the minScale bit. However, we also need to keep the call to setBrowserSize to update the CSS viewport after figuring out if the margins are excluded or not. However, the minScale code is masking some potentially complicated logic to figure out the correct CSS viewport height given the meta-viewport tag and the margins. If we take out the minScale we will need to put this complicated logic in.

In other words, in order to correctly compute the desired CSS viewport height at [1], we need to re-run some of the code at [2] with the new screen height at [3] and *then* call setBrowserSize with that.

At this point I'm just going to try and find a way to fix bug 940889 in a way that doesn't depend on this bug.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=0491b6ab227d#4032
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=0491b6ab227d#3966
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=0491b6ab227d#4024
Attachment #8344845 - Flags: review+ → review-
Assignee: bugmail.mozilla → nobody
No longer blocks: 940889
This should be working fine now that we have switched to the MobileViewportManager code on Fennec.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.