Improve handling of legacy viewport meta tags

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kenneth Rohde Christiansen]

Support legacy viewport meta tags (Android supports at least HandheldFriendly and Windows Phone IE support HandheldFriendly and MobileOptimized.)

The prioritizing is done as on WP7:

HandheldFriendly
MobileOptimized (overrides HandheldFriendly)
Viewport (overrides both MobileOptimized and HandheldFriendly)

[Kenneth Rohde Christiansen]

Created attachment 84940 [details]
Patch

[WebKit Review Bot]

Attachment 84940 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/dom/ViewportArguments.h:113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 84940 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84940&action=review

Awesome patch! <3
Let's see what others think.

> Source/WebCore/dom/Document.cpp:2742
> +    if (features != "true" || m_viewportArguments.type > type)

Checking for "true" should be a case insensitive comparison.

[Kenneth Rohde Christiansen]

Created attachment 84942 [details]
Patch 2

[Joseph Pecoraro]

Comment on attachment 84942 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=84942&action=review

Nice, I didn't even know about these meta tags. A few questions below. Maybe
you can link to a spec in the bugzilla comments if one exists.

> Source/WebCore/dom/Document.cpp:2727
> +    bool ok;
> +    m_viewportArguments.width = features.toFloat(&ok);
> +
> +    // An absent value or a value of 0 means that the page width is the same as
> +    // the size of the screen. We handle invalid values the same way.
> +    if (features.isEmpty() || features == "0" || !ok)
> +        m_viewportArguments.width = ViewportArguments::ValueDeviceWidth;

Does MobileOptimized have the same numeric parsing as viewport? Should "123x" become 123 or ValueDeviceWidth?

> Source/WebCore/dom/Document.cpp:2743
> +    // Only override if same type, or if new type has higher priority.
> +    if (!equalIgnoringCase(features, "true") || m_viewportArguments.type > type)
> +        return;

Should content be trimmed or is this very strict? Would this be valid?

  <meta name="HandheldFriendly" content=" true">

[Alexey Proskuryakov]

What is the reason for adding these? If WebKit survived without legacy meta support for so long, won't adding it now cause more trouble than good?

[Kenneth Rohde Christiansen]

(In reply to comment #6)
> What is the reason for adding these? If WebKit survived without legacy meta support for so long, won't adding it now cause more trouble than good?

We are actually supporting them in our legacy WebKit based browsers and so does Android. As what I understood from Andreas Kling, iOS actually also inserts a viewport when a XHTML MP document is encountered.

We have similar code on our branch:

--- a/WebCore/dom/Document.cpp
+++ b/WebCore/dom/Document.cpp
@@ -661,7 +661,8 @@ void Document::setDocType(PassRefPtr<DocumentType> docType)
         m_docType->setDocument(this);
 
     if (m_docType && !ownerElement() && m_docType->publicId().startsWith("-//wapforum//dtd xhtml mobile 1.", false)) {
         m_viewportArguments.width = ViewportArguments::ValueDeviceWidth;

[Andreas Kling]

(In reply to comment #7) 
> As what I understood from Andreas Kling, iOS actually also inserts a viewport when a XHTML MP document is encountered.

I'm not sure iOS does this for XHTML MP documents, but Android's WebKit does for sure.

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> (From update of attachment 84942 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84942&action=review
> 
> Nice, I didn't even know about these meta tags. A few questions below. Maybe
> you can link to a spec in the bugzilla comments if one exists.

I couldn't find all of the docs, but searching a bit I found these:

http://msdn.microsoft.com/en-us/library/ms890014.aspx
http://blogs.msdn.com/b/iemobile/archive/2010/11/22/the-ie-mobile-viewport-on-windows-phone-7.aspx
http://blogs.msdn.com/b/iemobile/archive/2011/01/21/managing-the-browser-viewport-in-windows-phone-7.aspx
http://docs.blackberry.com/en/developers/deliverables/6176/HTML_ref_meta_564143_11.jsp
http://starkravingfinkle.org/blog/2009/10/fennec-1-0-beta-4-for-maemo/

> > Source/WebCore/dom/Document.cpp:2727
> > +    bool ok;
> > +    m_viewportArguments.width = features.toFloat(&ok);
> > +
> > +    // An absent value or a value of 0 means that the page width is the same as
> > +    // the size of the screen. We handle invalid values the same way.
> > +    if (features.isEmpty() || features == "0" || !ok)
> > +        m_viewportArguments.width = ViewportArguments::ValueDeviceWidth;
> 
> Does MobileOptimized have the same numeric parsing as viewport? Should "123x" become 123 or ValueDeviceWidth?

I didn't find this documented any where, but I will try to test it when I get my hand on one of these phones.

> > Source/WebCore/dom/Document.cpp:2743
> > +    // Only override if same type, or if new type has higher priority.
> > +    if (!equalIgnoringCase(features, "true") || m_viewportArguments.type > type)
> > +        return;
> 
> Should content be trimmed or is this very strict? Would this be valid?
> 
>   <meta name="HandheldFriendly" content=" true">

I will try to test it.

(In reply to comment #6)
> What is the reason for adding these? If WebKit survived without legacy meta support for so long, won't adding it now cause more trouble than good?

We have had support for these quite some time on our development branches and our testers have reported no regressions so far. Pages that support both viewport and say handheldfriendly, will use the viewport meta tag, independently of the order they are declared.

[Alexey Proskuryakov]

It's good to know that no regressions have been found yet, although it's unclear if the same would hold true for iOS. Are there any known pages that benefit from this?

[Kenneth Rohde Christiansen]

(In reply to comment #10)
> It's good to know that no regressions have been found yet, although it's unclear if the same would hold true for iOS. Are there any known pages that benefit from this?

There are (though I don't have them handy right now) as we got bug reports of some of these, which led me to implement this feature.

[Simon Hausmann]

Comment on attachment 84942 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=84942&action=review

> Source/WebCore/dom/Document.cpp:2712
> +    static ViewportArguments::Type type = ViewportArguments::MobileOptimizedMeta;

A variable here seems overkill IMHO

[Simon Hausmann]

Comment on attachment 84942 [details]
Patch 2

Patch looks okay to me, but we need clarity on the impact or a list of sites that use this before landing.

[Hugo Parente Lima]

So, what about this patch? Can it be landed?

The thread about it on www-style[1] is almost dead but has no objections.

BTW, the patch needs to be rebased to apply on current WebKit trunk.

[1] http://lists.w3.org/Archives/Public/www-style/2012May/0371.html

[Kenneth Rohde Christiansen]

I think that under the LEGACY_VIEWPORT? flag, landing it would be quite OK.

It will also be required for fully upstreaming the Android port.

[Peter Beverloo]

+johnme

Yes, we're also using the MobileOptimized and HandheldFriendly meta tags, however, for the MobileOptimized meta, we completely ignore the content and I don't think we set the initial scale to 1 for HandheldFriendly.

This does look like a much more sustainable way than what we have right now. John, could you have a look as well please?

[Kenneth Rohde Christiansen]

(In reply to comment #16)
> +johnme
> 
> Yes, we're also using the MobileOptimized and HandheldFriendly meta tags, however, for the MobileOptimized meta, we completely ignore the content and I don't think we set the initial scale to 1 for HandheldFriendly.

Let's try to do the same. Do you have any tests?
 
> This does look like a much more sustainable way than what we have right now. John, could you have a look as well please?

John, you don't seem to auto complete in the cc field. Are you in the committers.py?

[Hugo Parente Lima]

Created attachment 144626 [details]
Patch

[Hugo Parente Lima]

I did the requested changes on Kenneth's patch[1] and rebased it.

The changes:

- Ignore content value of MobileOptimized.
- Don't set initial-scale on HandheldFriendly (need to do the same on MobileOptimized?)

BTW... height=device-height need to be set? what the behavior on other browsers?

[1] Kenneth asked me to do it weeks ago, so it wasn't an intrusive patch =]

[Kenneth Rohde Christiansen]

(In reply to comment #19)
> I did the requested changes on Kenneth's patch[1] and rebased it.
> 
> The changes:
> 
> - Ignore content value of MobileOptimized.
> - Don't set initial-scale on HandheldFriendly (need to do the same on MobileOptimized?)
> 
> BTW... height=device-height need to be set? what the behavior on other browsers?
> 
> [1] Kenneth asked me to do it weeks ago, so it wasn't an intrusive patch =]

John Mellor recently sent an email to www-style explaining the existing behavior. Maybe you can dig it up? I even think you were cc'ed.

[Hugo Parente Lima]

(In reply to comment #20)
> (In reply to comment #19)
> > I did the requested changes on Kenneth's patch[1] and rebased it.
> > 
> > The changes:
> > 
> > - Ignore content value of MobileOptimized.
> > - Don't set initial-scale on HandheldFriendly (need to do the same on MobileOptimized?)
> > 
> > BTW... height=device-height need to be set? what the behavior on other browsers?
> > 
> > [1] Kenneth asked me to do it weeks ago, so it wasn't an intrusive patch =]
> 
> John Mellor recently sent an email to www-style explaining the existing behavior. Maybe you can dig it up? I even think you were cc'ed.

Yes I am and read it, but the email is just about widths, not heights.

[Adam Barth]

Comment on attachment 144626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144626&action=review

> Source/WebCore/dom/ViewportArguments.h:62
> +#if USE(LEGACY_VIEWPORT_ADAPTION)

This should probably be ENABLE(LEGACY_VIEWPORT_ADAPTION)

[Adam Barth]

Comment on attachment 144626 [details]
Patch

This looks like a good start at least.  We can iterate on the exact parsing rules in followup patches, if needed.

[Alexey Proskuryakov]

Comment on attachment 144626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144626&action=review

> Source/WebCore/dom/ViewportArguments.h:63
> +        XHTMLMobileProfile,

XHTMLMP again?..

[Adam Barth]

Comment on attachment 144626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144626&action=review

>> Source/WebCore/dom/ViewportArguments.h:63
>> +        XHTMLMobileProfile,
> 
> XHTMLMP again?..

If you look at the patch, you'll see that we're already processing "-//wapforum//dtd xhtml mobile 1."  The patch doesn't add support for anything new related to XHTMLMP.  It just creates an enum value for what we're already doing.

[Hugo Parente Lima]

(In reply to comment #25)
> (From update of attachment 144626 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144626&action=review
> 
> >> Source/WebCore/dom/ViewportArguments.h:63
> >> +        XHTMLMobileProfile,
> > 
> > XHTMLMP again?..
> 
> If you look at the patch, you'll see that we're already processing "-//wapforum//dtd xhtml mobile 1."  The patch doesn't add support for anything new related to XHTMLMP.  It just creates an enum value for what we're already doing.

Yep, and the enum is needed just to know when processViewport should overwrite the current viewport settings or not.

[Hugo Parente Lima]

(In reply to comment #22)
> (From update of attachment 144626 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144626&action=review
> 
> > Source/WebCore/dom/ViewportArguments.h:62
> > +#if USE(LEGACY_VIEWPORT_ADAPTION)
> 
> This should probably be ENABLE(LEGACY_VIEWPORT_ADAPTION)

The "USE" was suggested and included in a previous patch, so this patch just continue using USE instead of ENABLE.

Anyway I think the semantic difference between both isn't to big and IMO webkit would have just "USE" or just "ENABLE" to keep things simple.

[WebKit Review Bot]

Comment on attachment 144626 [details]
Patch

Clearing flags on attachment: 144626

Committed r119256: <http://trac.webkit.org/changeset/119256>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

> The "USE" was suggested and included in a previous patch, so this patch just continue using USE instead of ENABLE.
> 
> Anyway I think the semantic difference between both isn't to big and IMO webkit would have just "USE" or just "ENABLE" to keep things simple.

Yeah, the two are very similar, and it's not clear what we gain by differentiating them.  My understanding is that USE is for selecting among different ways of implementing features (e.g., which library to use or whether to use an in-memory cache) whereas ENABLE is about controlling which features are visible to the platform (e.g., IndexedDB or various meta tags).

It's possible that other folks have a different understanding, and it's certainly not a big deal one way or the other.

[Hugo Parente Lima]

(In reply to comment #30)
> > The "USE" was suggested and included in a previous patch, so this patch just continue using USE instead of ENABLE.
> > 
> > Anyway I think the semantic difference between both isn't to big and IMO webkit would have just "USE" or just "ENABLE" to keep things simple.
> 
> Yeah, the two are very similar, and it's not clear what we gain by differentiating them.  My understanding is that USE is for selecting among different ways of implementing features (e.g., which library to use or whether to use an in-memory cache) whereas ENABLE is about controlling which features are visible to the platform (e.g., IndexedDB or various meta tags).
> 
> It's possible that other folks have a different understanding, and it's certainly not a big deal one way or the other.

I can do the switch to ENABLE in another patch as well and add you as reviewer.

Regards

[Adam Barth]

> I can do the switch to ENABLE in another patch as well and add you as reviewer.

Thanks Hugo.

[John Mellor]

Sorry, missed this. Belated review: looks great, thanks for adding support for these, and seems a nice clean way to do priorities.

It seems a bit odd that depending on the source we use "width=device-width, height=device-height", "width=device-width" or "width=device-width, initial-scale=1" as the implied viewport tag. Would anyone object if I changed these all to be just "width=device-width", on the assumption that initial-scale and device-height both default to those values once width=device-width is set?

[Adam Barth]

John, you might want to open a new bug and attach a patch.  That way folks can comment without feeling like they need to read 33 previous comments.

[Kenneth Rohde Christiansen]

I don't mind.