436083 - Viewport meta tag

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement, P2)

Description

[Christian Sejersen]

Comment 1

[Bobby Holley (:bholley)]

Attachment: fennec chrome patch for viewport META support

Comment 2

[Bobby Holley (:bholley)]

Attachment: content patch for viewport META support

Comment 3

[Bobby Holley (:bholley)]

attached are two patches (one content, one chrome) that together implement support for the viewport <META> tag. Flagging stuart and jst for review.

There are still problems as relating to bug 443076. Currently, the viewport of a page is always applied to the _next_ page loaded because of some hackiness in the view manager visibility code, so you more or less need to refresh each page to get the proper layout. A workaround for that bug that lets you see the proper viewport behavior is on the bug page.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 327879 [details] [diff] [review]
fennec chrome patch for viewport META support

This looks pretty good, just a few nits...

>diff --git a/chrome/content/deckbrowser.xml b/chrome/content/deckbrowser.xml

>+        <xul:stack style="-moz-stack-sizing: ignore">

Shouldn't -moz-stack-sizing go on the hbox or browser? I wouldn't expect this to work - does it actually have an affect?

>+            <xul:browser anonid="browser"
>+                         flex="0"

Just omit the flex attribute?

>     <implementation>

>+      <!-- The zoom level. We make this a property so that we can clamp it
>+      between the maximimum and minimum zoom values and ignore new values if
>+      the document dissalows scaling. -->

typo: maximum, disallows

>+      <property name="_zoomLevel">
>+        <getter>
>+          return this._zoomLevelInternal;
>+        </getter>

Can just use an onget attribute for this.

>+        <setter>
>+          if (this._zoomEnabled) {
>+            var result = Math.max(Math.min(parseFloat(val), this._zoomMax),
>+                                  this._zoomMin);
>+            this._zoomLevelInternal = result;
>+          }
>+        </setter>

having conditionals like this in setters is confusing... moving the "if (this._zoomEnabled)" logic out to the callers would be clearer. Also, setters should return the value being set.

>+      <property name="_zoomFactor" readonly="true">

>+          if (bWidth > 0)
>+            return (w / bWidth) * this._zoomLevel;
>+          else
>+            return this._zoomLevel;

remove the else after return.

>+      <method name="_updateViewState">

>+          /* Reset the pan data. */

can you use // comments to match the rest of the file?

>+          /* Enable scaling. */
>+          this._zoomEnabled = 1;

>+          var vpUserScalable = 
>+            windowUtils.getDocumentMetadata("viewport-user-scalable");

>+          /* If the document disallows zooming, disable it. */
>+          if (vpUserScalable == "no")
>+            this._zoomEnabled = 0;

Just assign directly to _zoomEnabled (and make it a bool):

this._zoomEnabled = (vpUserScalable != "no");

Comment 5

[Bobby Holley (:bholley)]

Attachment: modifed patch to address gavin's concerns

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 327881 [details] [diff] [review]
content patch for viewport META support

- In ProcessViewportToken():

+  if (nsGkAtoms::height->Equals(key))
+    aDocument->SetHeaderData(nsGkAtoms::viewport_height, value);
+  else if (nsGkAtoms::width->Equals(key))
+    aDocument->SetHeaderData(nsGkAtoms::viewport_width, value);
+  else if (nsGkAtoms::initial_scale->Equals(key))
+    aDocument->SetHeaderData(nsGkAtoms::viewport_initial_scale, value);
+  else if (nsGkAtoms::minimum_scale->Equals(key))
+    aDocument->SetHeaderData(nsGkAtoms::viewport_minimum_scale, value);
+  else if (nsGkAtoms::maximum_scale->Equals(key))
+    aDocument->SetHeaderData(nsGkAtoms::viewport_maximum_scale, value);
+  else if (nsGkAtoms::user_scalable->Equals(key))
+    aDocument->SetHeaderData(nsGkAtoms::viewport_user_scalable, value);

This will do a string compare for each if check. I think you'd probably end up with faster code if you'd get the atom for the key up front and then relied on pointer compares in the if check to find out which header to set etc.

- In nsContentUtils::ProcessViewportInfo():

+    /* Advance tip past non-separator characters. */
+    while (!IS_SEPARATOR(*tip) && (tip != end))
+      ++tip;

If we the ++tip increment puts us at the end of the string then the !IS_SEPARATOR(*tip) check will look past the end of the string (which may or may not be ok, depending on the actual string passed in.

+
+    /* Our token consists of the characters between tail and tip. */
+    ProcessViewportToken(aDocument, Substring(tail, tip));
+
+    /* Skip separators. */
+    while (IS_SEPARATOR(*tip) && (tip != end))

Same here.

r=jst with that fixed.

Comment 7

[Bobby Holley (:bholley)]

Attachment: revised content patch to address jst's suggestions

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 328582 [details] [diff] [review]
revised content patch to address jst's suggestions

flagging jonas for sr

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

Updated this patch to apply and made a few fixes. This is going to need to be merged with my patch for bug 436059 once that lands.

Comment 10

[Bobby Holley (:bholley)]

Attachment: updated content patch to fix some bitrot

Comment 11

[Bobby Holley (:bholley)]

Attachment: mochitests for the content-side patch

Comment 12

[Bobby Holley (:bholley)]

Requesting checkin for the content-patch and mochitests patch. The current content patch consists of trivial updates to the SR-ed-but-obsoleted content patch, and I don't think mochitests need review. 

Comment 13

[Rob Arnold [:robarnold]]

Pushed the content and content test changes to mozilla-central:
http://hg.mozilla.org/index.cgi/mozilla-central/rev/ad66fd14f485
http://hg.mozilla.org/index.cgi/mozilla-central/rev/ec061b745244

Comment 14

[Robert Kaiser]

This broke compiling with MathML disabled, which is the default for Thunderbird.
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGkAtomList.h#1318 is #ifdef MOZ_MATHML while http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#4244 isn't, so we end up breaking in nsContentUtils.cpp:4244 with "error: ‘maximum_scale’ is not a member of ‘nsGkAtoms’"

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to comment #14)
> This broke compiling with MathML disabled
That's bug 445745, which has now been fixed.

Comment 16

[Stuart Parmenter]

moving to a1 :/  still waiting on 443076

Comment 17

[Bobby Holley (:bholley)]

a1 -> a2? Sigh. To think this was once slated for M3.

Comment 18

[Henri Sivonen (:hsivonen)]

I don't understand follow the comment "This information is ignored by the generic content sink because it only stores http-equiv meta tags."

Why isn't it appropriate to put the new ProcessMETATag() bits into nsContentSink instead of duplicating it in both XML and HTML content sinks?

(I'm trying to understand this in order to avoid breaking stuff in the HTML5 parser.)

Comment 19

[Bobby Holley (:bholley)]

Wow, it's been a while.

The comment refers to the fact that when the generic content sink does meta tag parsing, it only stores tags that are "http-equiv" flavor and not "name" flavor. This is why we have to do separate processing for them.

I don't remember why they're not combined into the superclass method. I think it may have to do with the fact that originally I was doing different stuff based on namespacing, but it then turned out that the behavior for xml and html was actually identical for this task. Assuming the information wouldn't screw anything up for other content sinks, I can't think of any reason (at the moment anyway) why they couldn't be factored out.

Comment 20

[Henri Sivonen (:hsivonen)]

OK. I'm proceeding by hoisting the viewport processing into nsContentSink so that I don't need yet another copy for HTML5. Thanks.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 329136 [details] [diff] [review]
updated patch

This has bitrotted quite a bit due to the canvasbrowser reworking.
bholley, are you going to be able to update it?

Comment 22

[Bobby Holley (:bholley)]

*shrug* - I could, though I don't really want to. I'm not working in mobile anymore, so it doesn't make a huge amount of sense for me to spend time figuring out the new canvas browser setup. I'd imagine it would take much less time for someone familiar with how things have progressed to port the patch.

Comment 23

[Ben Combee (:bcombee)]

Two websites useful for testing:

http://www.hopstop.com/pda - uses both Viewport and HandheldFriendly
http://m.precentral.net/ - only uses viewport

blassey said this bug was on his radar

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: mobile patch

Comment 25

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

Comment on attachment 400792 [details] [diff] [review]
mobile patch

If a website does not have the "viewport" metadata, we still do some work. Can you wrap the viewport math in a check for valid viewport strings?

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: mobile patch v.2

addresses gavin and mfinkle's comments

Comment 27

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

Comment on attachment 400866 [details] [diff] [review]
mobile patch v.2

>diff --git a/chrome/content/BrowserView.js b/chrome/content/BrowserView.js

>+    let vpWidthStr = windowUtils.getDocumentMetadata("viewport-width"); 
>+    let vpHeightStr = windowUtils.getDocumentMetadata("viewport-height");
>+    let vpInitialScale = parseFloat(windowUtils.getDocumentMetadata("viewport-initial-scale"));
>+    let vpUserScalable = windowUtils.getDocumentMetadata("viewport-user-scalable");

Add a line break here
It be cool if you could just use viewportW and viewportH (even as the string versions) and then just reuse them when you parseInt below

And basically "vp" -> "viewport"

>+    let [w, h] = BrowserView.Util.getBrowserDimensions(browser);

browserW and browserH please

>+    var sw = window.screen.width;
>+    var sh = window.screen.height;
>+    var aspectRatio = sw / sh;

screenW and screenH please

>+    let newZoomLevel = BrowserView.Util.pageZoomLevel(this.getVisibleRect(), w, h);
>+    let newWidth = w;
>+    let newHeight = h;
>+    let newTextZoom =  browser.markupDocumentViewer.textZoom;

Do we want to touch textZoom at all?

Also, since I'm being so annal about this: newWidth -> newW and newHeight -> newH ?

>       delete  browser.handheld;
>-      let [w, h] = BrowserView.Util.getBrowserDimensions(browser);
>-      this.setZoomLevel(BrowserView.Util.pageZoomLevel(this.getVisibleRect(), w, h));
>     }
>+    if (vpWidthStr != "" && vpHeightStr !="") {
>+      let vpWidth, vpHeight;
>+      if (vpWidthStr == "device-width")

Add line break before the "if" block

>+      if ((vpWidth > 0) && (vpHeight > 0)) {
>+        browser.vpWidth = newWidth = vpWidth;
>+        browser.vpHeight = newHeight = vpHeight;

Please use browser.viewportWidth and browser.viewportHeight to be a little consistent with the other xxxWidth and xxxHeight properties.

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>-          browserStyle.height = ((browsers[i].hasOwnProperty("handheld") && 
>-                                  browsers[i].handheld) ? 
>-                                 scaledScreenH : scaledDefaultH) + "px";
>+	  let newHeight = scaledDefaultH;

Same annal naming here


Mostly all naming nits (sorry), except for the textZoom thing. Do we want to use it?

r+ with that answered

Comment 28

[Stuart Parmenter]

Comment on attachment 400866 [details] [diff] [review]
mobile patch v.2

this code needs to live somewhere other than zoomToPage.  probably needs to live off of one of the networkStart notifications, if we think we'd have this data at that point.

Comment 29

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.3

Comment 30

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

Attachment: Updated to trunk

Updated to trunk with some changes to Util.contentIsHandheld

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 409989 [details] [diff] [review]
Updated to trunk

>diff --git a/chrome/content/BrowserView.js b/chrome/content/BrowserView.js

>   getZoomForPage: function getZoomForPage() {

>+    let metaData = Util.contentIsHandheld(browser);
>+    if (metaData.reason == "handheld")
>       return 1;
>+    else if (metaData.reason == "viewport")
>+      return parseFloat(metaData.scale);

shouldn't we do some form of validation here? Seems like bad things will happen if this returns NaN.

>diff --git a/chrome/content/Util.js b/chrome/content/Util.js

>   contentIsHandheld: function contentIsHandheld(browser) {
>     let doctype = browser.contentDocument.doctype;
>     if (doctype && /(WAP|WML|Mobile)/.test(doctype.publicId))
>-      return true;
>+      return {reason: "doctype", result: true};

no one seems to check for reason == "doctype", doesn't that regress bug 524900?

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>   endLoading: function() {

>+      let validW = !isNaN(viewportW) && viewportW > 0;
>+      let validH = !isNaN(viewportH) && viewportH > 0;

isNaN checks are redundant, NaN > 0 is false.

>+      } else if(!validW && validH){

nit: space after "if", before "{"

Comment 32

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 409989 [details] [diff] [review]
Updated to trunk

eturn {reason: "handheld", result: true};
> 
>-    return false;
>+    let viewportScale = windowUtils.getDocumentMetadata("viewport-initial-scale");
>+    if (viewportScale != "") {
>+      return {

changing this to:
    let viewportScale = parseFloat(windowUtils.getDocumentMetadata("viewport-initial-scale"));
    if (viewportScale > 0)

should alleviate Gavin's concern. (obviously you wouldn't re-parse also)

Comment 33

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

Attachment: updated for comment

Updated based on review comments

Comment 34

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 410009 [details] [diff] [review]
updated for comment

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>   endLoading: function() {

>+      } else if (!validW && validH){

still missing a space here, before "{" :)

Comment 35

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

pushed with changes:
https://hg.mozilla.org/mobile-browser/rev/a212232f29a9

Comment 36

[Joel Maher ( :jmaher ) (UTC -8)]

any tips for how to verify this bug?

Comment 37

[Brad Lassey [:blassey] (use needinfo?)]

go to touch.facebook.com, you'll notice that it lays out 320px wide (unfortunately), which is the width specified by its viewport meta tag.

Comment 38

[Joel Maher ( :jmaher ) (UTC -8)]

verified with 20091106 1.9.2 nightly build on n810

Comment 39

[Martijn Wargers (dead)]

Url of the Apple documentation changed to here:
http://developer.apple.com/safari/library/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/MetaTags.html

Comment 40

[Martijn Wargers (dead)]

Mdc doesn't really have documentation:
https://developer.mozilla.org/Firefox_Mobile_for_Developers#Viewport_meta_tag

I saw there were some mochitests for this bug, so I guess we can mark in-testsuite+ for this or do we need more tests?

Comment 41

[Martijn Wargers (dead)]

More documentation here:
http://developer.apple.com/safari/library/documentation/AppleApplications/Reference/SafariWebContent/UsingtheViewport/UsingtheViewport.html

Comment 42

[Tony Chung [:tchung]]

automated test candidate, not manual.  in-litmus-

Comment 43

[Joel Maher ( :jmaher ) (UTC -8)]

I thought we had tests for this:
http://mxr.mozilla.org/mobile-browser/source/chrome/tests/browser_viewport.js

Could we verify this and if not, please add specifics of what we need to get automated coverage for?

Comment 44

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

We have automated tests in fennec

Comment 45

[Eric Shepherd [:sheppy]]

Documented here:

https://developer.mozilla.org/en/Mobile/Viewport_meta_tag

Comment 46

[Nicolas FROIDURE]

The Fennec Emulator seems to not take in count the meta viewport feature, so i can't have my little HTML5 game working on the Fennec emulator : https://github.com/nfroidure/CasseBriques

Comment 47

[Brad Lassey [:blassey] (use needinfo?)]

Nicolas, what emulator are you referring to?

Comment 48

[Nicolas FROIDURE]

The program available here, i run it under Debian GNU/Linux : http://ftp.mozilla.org/pub/mozilla.org/mobile/releases/4.0.1/linux-i686/fennec-4.0.1.en-US.linux-i686.tar.bz2

I realize it's not really an emulator. It would be great to give a way to override my desktop screen size.

Is it planned ?

Comment 49

[Brad Lassey [:blassey] (use needinfo?)]

We stopped making desktop builds of fennec a while back (that build is 13 months old). You may want to run fennec in the ARM android emulator included in their sdk [http://developer.android.com/sdk/index.html].

Just to warn you, it will be sluggish. However, once bug 750366 is fixed, you'll be able to run fennec in the x86 android emulator, which is actually quite fast. You can do that today if you don't mind producing your own build.