Viewport meta tag

VERIFIED FIXED in fennec1.0b5

Status

Fennec Graveyard
General
P2
enhancement
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: Christian Sejersen, Assigned: blassey)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

Trunk
fennec1.0b5
Other
Maemo
dev-doc-complete
Dependency tree / graph
Bug Flags:
wanted-fennec1.0 +
in-testsuite +
in-litmus -

Details

(URL)

Attachments

(3 attachments, 9 obsolete attachments)

12.94 KB, patch
Details | Diff | Splinter Review
6.13 KB, patch
Details | Diff | Splinter Review
4.91 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
 

Updated

10 years ago
Flags: wanted-fennec1.0+
Priority: -- → P2
(Reporter)

Updated

10 years ago
Assignee: nobody → pavlov
Assignee: pavlov → bholley
Status: NEW → ASSIGNED
Created attachment 327879 [details] [diff] [review]
fennec chrome patch for viewport META support
Attachment #327879 - Flags: review?
Created attachment 327881 [details] [diff] [review]
content patch for viewport META support
Attachment #327881 - Flags: review?
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.
Depends on: 443076
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");
Attachment #327879 - Flags: review?
Attachment #327881 - Flags: review? → review?(jst)
Created attachment 328072 [details] [diff] [review]
modifed patch to address gavin's concerns
Attachment #327879 - Attachment is obsolete: true
Attachment #328072 - Flags: review?
Attachment #328072 - Flags: review? → review?(gavin.sharp)
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.
Attachment #327881 - Flags: review?(jst) → review+
Created attachment 328582 [details] [diff] [review]
revised content patch to address jst's suggestions
Attachment #327881 - Attachment is obsolete: true
Comment on attachment 328582 [details] [diff] [review]
revised content patch to address jst's suggestions

flagging jonas for sr
Attachment #328582 - Flags: superreview?(jonas)
Attachment #328582 - Flags: superreview?(jonas) → superreview+
Created attachment 329136 [details] [diff] [review]
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.
Attachment #328072 - Attachment is obsolete: true
Attachment #328072 - Flags: review?(gavin.sharp)

Updated

10 years ago
Attachment #329136 - Flags: review?(gavin.sharp)

Updated

10 years ago
Target Milestone: Fennec M5 → Fennec M6
Created attachment 329744 [details] [diff] [review]
updated content patch to fix some bitrot
Attachment #328582 - Attachment is obsolete: true
Created attachment 329745 [details] [diff] [review]
mochitests for the content-side patch
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. 
Keywords: checkin-needed
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

Keywords: checkin-needed

Updated

10 years ago
Depends on: 445745

Comment 14

10 years ago
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’"
(In reply to comment #14)
> This broke compiling with MathML disabled
That's bug 445745, which has now been fixed.

Updated

9 years ago
Target Milestone: Fennec M6 → Fennec M7

Comment 16

9 years ago
moving to a1 :/  still waiting on 443076
Target Milestone: Fennec M7 → Fennec A1

Updated

9 years ago
Target Milestone: Fennec A1 → Fennec A2
a1 -> a2? Sigh. To think this was once slated for M3.
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.)
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.
OK. I'm proceeding by hoisting the viewport processing into nsContentSink so that I don't need yet another copy for HTML5. Thanks.
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?
Attachment #329136 - Attachment is obsolete: true
Attachment #329136 - Flags: review?(gavin.sharp)
*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.
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
Created attachment 400792 [details] [diff] [review]
mobile patch
Attachment #400792 - Flags: review?(combee)
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?
Created attachment 400866 [details] [diff] [review]
mobile patch v.2

addresses gavin and mfinkle's comments
Assignee: bobbyholley → bugmail
Attachment #400792 - Attachment is obsolete: true
Attachment #400866 - Flags: review?(mark.finkle)
Attachment #400792 - Flags: review?(combee)
Attachment #400866 - Flags: review?(mark.finkle) → review+
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

Updated

8 years ago
Attachment #400866 - Flags: review+ → review-

Comment 28

8 years ago
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.
Created attachment 408814 [details] [diff] [review]
patch v.3
Attachment #400866 - Attachment is obsolete: true
Attachment #408814 - Flags: review?(mark.finkle)
Created attachment 409989 [details] [diff] [review]
Updated to trunk

Updated to trunk with some changes to Util.contentIsHandheld
Attachment #408814 - Attachment is obsolete: true
Attachment #409989 - Flags: review?(gavin.sharp)
Attachment #408814 - Flags: review?(mark.finkle)
Attachment #409989 - Flags: review?(bugmail)
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 "{"
Attachment #409989 - Flags: review?(gavin.sharp) → review-
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)
Created attachment 410009 [details] [diff] [review]
updated for comment

Updated based on review comments
Attachment #409989 - Attachment is obsolete: true
Attachment #410009 - Flags: review?(gavin.sharp)
Attachment #409989 - Flags: review?(bugmail)
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 "{" :)
Attachment #410009 - Flags: review?(gavin.sharp) → review+
pushed with changes:
https://hg.mozilla.org/mobile-browser/rev/a212232f29a9
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: A2 → B5
any tips for how to verify this bug?
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.
verified with 20091106 1.9.2 nightly build on n810
Status: RESOLVED → VERIFIED
Url of the Apple documentation changed to here:
http://developer.apple.com/safari/library/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/MetaTags.html
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?
Flags: in-testsuite?
Flags: in-litmus?
Keywords: dev-doc-needed

Updated

8 years ago
Depends on: 561202

Updated

8 years ago
Depends on: 561203
More documentation here:
http://developer.apple.com/safari/library/documentation/AppleApplications/Reference/SafariWebContent/UsingtheViewport/UsingtheViewport.html
automated test candidate, not manual.  in-litmus-
Flags: in-litmus? → in-litmus-
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?
We have automated tests in fennec
Flags: in-testsuite? → in-testsuite+
Documented here:

https://developer.mozilla.org/en/Mobile/Viewport_meta_tag
Keywords: dev-doc-needed → dev-doc-complete

Comment 46

6 years ago
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
Nicolas, what emulator are you referring to?

Comment 48

6 years ago
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 ?
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.
You need to log in before you can comment on or make changes to this bug.