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

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I tried to set <meta name="viewport" content="device-heigth, width=device-width, initial-scale=1, user-scalable=no"> as the default viewport for Gaia apps in order to not have too much things to change in their CSS stylesheets to have them working with APZ but it seems like device-height does not work because the viewport height is altered at http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#613

For the settings apps minScale.scale == 0.5 and so the height normally expected with device-height is 460px, but it ends up beeing 920px. One work around is to add minimum-scale=1 in the initial viewport.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Botond, based on https://bugzilla.mozilla.org/show_bug.cgi?id=940691#c23 could it means that device-height is correctly honored and we really need to add minimum-scale=1, the bug here coming from the layout of Gaia apps ?

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Matt, do you know?

Comment 3

[Matt Brubeck (:mbrubeck)]

The problem is caused by logic that we added in bug 614353 to increase the viewport height if necessary to allow zooming out to the page width.  This logic violates the spec (which didn't exist at the time) and I think we should remove it.

According to the CSS device-adaptation spec, if width is set to "device-width" and "height" is either unspecified or set to "device-height", then the height should be equal to the device height.

Comment 4

[Botond Ballo [:botond]]

Let's use this bug for the fix.

Comment 5

[Botond Ballo [:botond]]

Attachment: bug942799.patch

Here's a patch that removes the expanding of the viewport height if user-scalable=no as Chris suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=940691#c34.

I'd like some feedback on the following:

  - Is the condition under which we expand the viewport height appropriate?

  - Should we be changing some other piece of code so that if we don't expand
    the viewport height, the minimum zoom is clamped to be no smaller than
    (screen height / page height)? In TabChild::HandlePossibleViewportChange()
    we don't use the computed 'minScale' for anything other than determining
    the viewport height, so I'm guessing that not being able to zoom out any
    further than that is enforced somewhere else?

Comment 6

[Botond Ballo [:botond]]

Attachment: bug942799.patch

Whoops, posted wrong patch by accident.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8339492 [details] [diff] [review]
bug942799.patch

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

::: dom/ipc/TabChild.cpp
@@ +611,5 @@
> +  // out) if the viewport tag doesn't specify user-scalable=no. This is because
> +  // this adjustment can increase the CSS viewport height beyond the screen
> +  // height, which means some content will be off-screen, and if we can't
> +  // zoom out we can't bring that content into view.
> +  if (viewportInfo.IsZoomAllowed()) {

I don't think we should keep this logic even in the user-scalable=yes case.  The constraining procedure described in the spec does not depend on the user-zoom (user-scalable) property.

I think we can just remove this code completely.  And the corresponding code in Fennec:
http://hg.mozilla.org/mozilla-central/file/6ecf0c4dfcbe/mobile/android/chrome/content/browser.js#l4026

(In reply to Botond Ballo [:botond] from comment #5)
> - Should we be changing some other piece of code so that if we don't expand
>   the viewport height, the minimum zoom is clamped to be no smaller than
>   (screen height / page height)? In TabChild::HandlePossibleViewportChange()
>   we don't use the computed 'minScale' for anything other than determining
>   the viewport height, so I'm guessing that not being able to zoom out any
>   further than that is enforced somewhere else?

Yes, it looks like this is enforced here, and it already handles both height-limited and width-limited cases correctly (no changes needed):
http://hg.mozilla.org/mozilla-central/file/5eb1c89fc2bc/gfx/layers/ipc/AsyncPanZoomController.cpp#l692

Comment 8

[Chris Lord [:cwiiis]]

Comment on attachment 8339492 [details] [diff] [review]
bug942799.patch

Removing f? for now, I think mbrubeck's feedback is more relevant than mine.

Comment 9

[Botond Ballo [:botond]]

Attachment: bug942799.patch

Comment 10

[Botond Ballo [:botond]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=5bfc89f30d2b

Comment 11

[Botond Ballo [:botond]]

(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> I think we can just remove this code completely.  And the corresponding code
> in Fennec:
> http://hg.mozilla.org/mozilla-central/file/6ecf0c4dfcbe/mobile/android/
> chrome/content/browser.js#l4026

Filed bug 944479 for Fennec.

Comment 12

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8340068 [details] [diff] [review]
bug942799.patch

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

Note that I'm not an official peer of this code (though I wrote the original JS code it was ported from), so you may want to get a DOM or B2G peer to sign off on this too.

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8340068 [details] [diff] [review]
bug942799.patch

Flagging for additional review as per Matt's suggestion.

Comment 14

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> Try push: https://tbpl.mozilla.org/?tree=Try&rev=5bfc89f30d2b

Looks clean.

Comment 15

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f725c8641830

Comment 16

[Carsten Book [:Tomcat]]

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