Bug 72722 – Parse the viewport meta tag more robustly

• Component: DOM
• Status: RESOLVED LATER

[Peter Beverloo]

Previous bugs on this subject:
https://bugs.webkit.org/show_bug.cgi?id=40204 (RESOLVED WONTFIX)
https://bugs.webkit.org/show_bug.cgi?id=47607 (RESOLVED INVALID)
https://bugs.webkit.org/show_bug.cgi?id=53705 (RESOLVED FIXED)

Following the popularity of the Android browser, several websites such as CNN, Flickr and Reddit have adapted the semi-colon as a separator in meta viewport. Because of this, Microsoft Internet Explorer and Mozilla Firefox also support the semi-colon as a separator, and the CSS Device Adaption specification has been updated to reflect this following discussion on www-style, albeit in a non-normative section.

This patch brings WebKit's behavior en par with Internet Explorer, Firefox and the specification.

http://dev.w3.org/csswg/css-device-adapt/#parsing-algorithm

Other sources:
http://blogs.msdn.com/b/iemobile/archive/2010/11/22/the-ie-mobile-viewport-on-windows-phone-7.aspx
http://hg.mozilla.org/mozilla-central/file/447556784745/content/base/src/nsContentUtils.cpp#l4506
http://lists.w3.org/Archives/Public/www-style/2011Oct/0689.html
http://lists.w3.org/Archives/Public/www-style/2011Oct/att-0689/viewport-tags.csv (download)

[Peter Beverloo]

Created attachment 115796 [details]
Proposed patch

Proposed patch. Code is Kenneth's from bug 47607. I updated the description and added two tests. Kenneth, I can change the author if you'd prefer that.

As expected results need to be updated, this will need at least another iteration. I'm sure there will be a fair amount of discussion as well.

[Kenneth Rohde Christiansen]

I'm fine with the change, but as I wrote part of the code someone else should probably review it.

[Alexey Proskuryakov]

It appears that bug description and ChangeLog substantially misrepresent reasons for this change. 

I'm concerned whether the situation was misrepresented to the CSS working group as well. Please provide a link to the www-style discussion.

> CNN
"initial-scale=1.0; maximum-scale=1.0; user-scalable=0;

> Flickr
"width=device-width; initial-scale=1.0; maximum-scale=1.0; user-scalable=0;"

> Reddit
"width=800, initial-scale=1"

None of these sites uses semicolon as separator. CNN and Flickr use "; ", which always worked in iOS, and Reddit uses comma.

Assuming the discussion in previous bugs is accurate, only one old version of Android supported semicolon as separator.

I think that one possible reason to make this change would be "paving the cowpaths". Semicolon with space works as a separator by accident, yet many authors expect it to work. An obvious downside is that most sites were never tested in any browser that treated lone semicolon as separator, so we can break sites for very little gain.

Matching IE or Firefox should not be a consideration in this decision in my opinion. It's other engines that were trying to match WebKit, apparently misunderstanding what it does. We need not create a cycle of reverse engineering.

[Alexey Proskuryakov]

> Please provide a link to the www-style discussion.

I'm sorry, I somehow didn't notice it among the other links. Reading now.

[Alexey Proskuryakov]

OK, so the data in the csv file linked from www-style discussion confirms that sites do not use semicolon as separator. There was very clear understanding on www-style of why semicolon with space works.

The discussion mentions the principle of "Be liberal in what you accept, and conservative in what you send." In almost all cases, it's the wrong principle for web browser engines in this age. We should stick to interoperable specs, not surprise authors and security researchers with badly defined behaviors.

I think that we can still consider paving the cowpath. I suggest that the patch is reviewed by Dave Kilzer or Joe Pecoraro (CC'ed on the  bug). One way or another, please correct the ChangeLog.

[Alexey Proskuryakov]

The reason why other engines felt the pressure to support semicolon as separator is likely that their number parser does not accept trailing garbage. If we decide to support semicolon explicitly, we should strongly consider using a number parsing mode that does not accept trailing garbage for interoperability.

[Peter Beverloo]

Created attachment 116617 [details]
Patch

> > CNN
> "initial-scale=1.0; maximum-scale=1.0; user-scalable=0;
> 
> > Flickr
> "width=device-width; initial-scale=1.0; maximum-scale=1.0; user-scalable=0;"
> 
> > Reddit
> "width=800, initial-scale=1"
> 
> None of these sites uses semicolon as separator. CNN and Flickr use "; ", which always worked in iOS, and Reddit uses comma.

I do not completely with your assertion. While, on a technical level, they indeed do not use the semicolon as the separator, I'm sure that the authors believe they do. Especially in the case of "width=device-width;" this is the case, because "device-width;" is invalid and thus the entire key-value pair gets disregarded. The intended behavior is only observed due to the other added properties.

> The discussion mentions the principle of "Be liberal in what you accept, and conservative in what you send." In almost all cases, it's the wrong principle for web browser engines in this age. We should stick to interoperable specs, not surprise authors and security researchers with badly defined behaviors.
> 
> I think that we can still consider paving the cowpath. I suggest that the patch is reviewed by Dave Kilzer or Joe Pecoraro (CC'ed on the  bug). One way or another, please correct the ChangeLog.

Ok. I amended the description and labelled current usage as an "assumed separator", while also adding more background. In the addendum of "the behavior which authors expect" I specifically refer to my earlier statement that authors think they are separators, and disfunctionality of the keywords is often compensated for by additional properties.

[Alexey Proskuryakov]

Yes, authors might assume that viewport has CSS syntax.

[Peter Beverloo]

Hello Dave, Joe,

Would one of you have a moment to review the patch?

Thanks.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> Hello Dave, Joe,
> 
> Would one of you have a moment to review the patch?

I'm concerned about introducing regressions with this change.  Is there any web site that doesn't work because we're not parsing semi-colons?

[Joseph Pecoraro]

(In reply to comment #10)
> (In reply to comment #9)
> > Hello Dave, Joe,
> > 
> > Would one of you have a moment to review the patch?
> 
> I'm concerned about introducing regressions with this change.
> Is there any web site that doesn't work because we're not parsing semi-colons?

There could be a site that has "width=device-width; ..." that might change
because the device-width would actually start working. I don't know how
many sites actually have this problem though.

[Peter Beverloo]

Comment on attachment 116617 [details]
Patch

Removing r? for now until we can give clarify on the asked questions, which is in progress internally.

[Adam Barth]

> Removing r? for now until we can give clarify on the asked questions, which is in progress internally.

Peter any update on these questions?

IMHO, we should match whatever iOS does here.

[Adam Barth]

By the way, the spec still seems to say that ; is a separator.

[Adam Barth]

I'm going to try to get to the bottom of this issue.

[Adam Barth]

IMHO, we should fix Bug 45652 first so that we can actually test this code in a port-agnostic manner.

[Adam Barth]

Is the iOS code that parses the viewport open source?  It's lame to have to reverse engineer a WebKit implementation in the WebKit project.  :-/

[Adam Barth]

(In reply to comment #17)
> Is the iOS code that parses the viewport open source?  It's lame to have to reverse engineer a WebKit implementation in the WebKit project.  :-/

Apparently the iOS parsing code is in http://opensource.apple.com/source/WebCore/WebCore-1298.39/dom/ViewportArguments.cpp

[Adam Barth]

I've verified that WebKit trunk currently matches the iOS parsing behavior.  I don't think we should change it at the moment for the reason expressed in Comment #10.  Like it or not, we probably need to emulate iOS quirks in the mobile world with the same amount of zeal that we emulate IE quirks in the desktop world.

[Adam Barth]

Created attachment 151469 [details]
Patch

[Peter Beverloo]

This actually is an Android Browser quirk, which no longer applies in more recent versions of the browser (not entirely deliberately). Support for the semi-colon as a separator is supported by Mobile IE and Fennec (Firefox), but not by WebKit. In order to align with browser and developer behavior, the spec was updated by Florian last year, albeit in a non-normative section.

While I'm still in favor of actually supporting the separator, we have no data (nor bandwidth to gather it) to determine whether there are significant regressions at this moment. Chrome on Android does ship with support for the semi-colon as a separator.

[John Mellor]

(In reply to comment #19)
> Like it or not, we probably need to emulate iOS quirks in the mobile world with the same amount of zeal that we emulate IE quirks in the desktop world.

I don't think rigidly following iOS is optimal here. We've established that a high proportion of major sites use semi-colons in their viewport tag [1]. Most of these sites do happen to render correctly in Mobile Safari, because the developers have tested them and hackily added redundant information to the viewport until it worked. But there are still problems:

1. It's harder to achieve parity between WebKit ports, as some sites using semi-colons may only work in Mobile Safari due to iOS quirks such as [2]. Other ports can end up with a different rendering, even though if they had parsed the semi-colons they would have done what the author intended (hence match Mobile Safari).

2. This isn't an acceptable state for the web platform to remain in. By the principle of least astonishment, behavior should not be completely different depending on whether you separate terms with ";" or "; " [3], or add a trailing semi-colon [4], or add redundant properties. Unlike emulating IE quirks, the author's intent is very clear when they accidentally use semi-colons, and we should just parse them sensibly rather than letting them stumble around undefined/accidental behavior.

Meanwhile I've yet to see any known downside of supporting semi-colons...

[1]: See the csv linked from the first post; I'm happy to update this if it's felt useful.
[2]: http://adactio.com/journal/5088/
[3]: "width=device-width; initial-scale=1" works but "width=device-width;initial-scale=1" doesn't.
[4]: "width=device-width" works but "width=device-width;" doesn't.

[Adam Barth]

> This actually is an Android Browser quirk, which no longer applies in more recent versions of the browser (not entirely deliberately).

Which behavior is an Android Browser quirk?  According to my reading of http://opensource.apple.com/source/WebCore/WebCore-1298.39/dom/ViewportArguments.cpp, trunk matches the iOS behavior.

> Support for the semi-colon as a separator is supported by Mobile IE and Fennec (Firefox),

Those browsers don't have enough usage on the mobile web to matter for these sorts of compatibility issues.

> but not by WebKit. In order to align with browser and developer behavior, the spec was updated by Florian last year, albeit in a non-normative section.

In general, specs should match implementations, especially when there's a popular implementation that's been shipping for years.

> While I'm still in favor of actually supporting the separator, we have no data (nor bandwidth to gather it) to determine whether there are significant regressions at this moment. Chrome on Android does ship with support for the semi-colon as a separator.

Until we have data to the contrary, we should match the most popular implementation, which, in this case, is iOS.  Introducing non-interoperability is not a path to better compatibility.

> I don't think rigidly following iOS is optimal here. We've established that a high proportion of major sites use semi-colons in their viewport tag [1].

That's even more reason to match what the most popular browser does with those inputs!

> Most of these sites do happen to render correctly in Mobile Safari, because the developers have tested them and hackily added redundant information to the viewport until it worked.

Exactly.  That's how the web works.

> 1. It's harder to achieve parity between WebKit ports, as some sites using semi-colons may only work in Mobile Safari due to iOS quirks such as [2]. Other ports can end up with a different rendering, even though if they had parsed the semi-colons they would have done what the author intended (hence match Mobile Safari).

The solution is to become ever more identical with iOS WebKit, not to introduce a bunch of non-interoperability in the hopes that it will all balance out.

> 2. This isn't an acceptable state for the web platform to remain in. By the principle of least astonishment, behavior should not be completely different depending on whether you separate terms with ";" or "; " [3], or add a trailing semi-colon [4], or add redundant properties. Unlike emulating IE quirks, the author's intent is very clear when they accidentally use semi-colons, and we should just parse them sensibly rather than letting them stumble around undefined/accidental behavior.

That's not how the web works.  Authors copy and paste all manner of garbage syntax and the only way to make browsers render the content the same way is to have them work exactly the same.  In this case, that means processing the meta viewport exactly the way it is processed on iOS, starting with the parsing.

> Meanwhile I've yet to see any known downside of supporting semi-colons...

The downside is that doing should would make different ports of WebKit process the meta viewport in different ways.  That is a large downside.  This mess was created by the Android Browser diverging from the original iOS implementation.  The only path to sanity is to converge again.

This story has played itself out many, many times in the history of the web.

[Peter Beverloo]

(In reply to comment #23)
> Which behavior is an Android Browser quirk?  According to my reading of http://opensource.apple.com/source/WebCore/WebCore-1298.39/dom/ViewportArguments.cpp, trunk matches the iOS behavior.

Recognizing semi-colons as a separator. Android Browser did this, ToT/iOS never did.

> Those browsers don't have enough usage on the mobile web to matter for these sorts of compatibility issues.

That does not mean we can ignore them, interoperability between browsers is no different than interoperability between WebKit ports..

> In general, specs should match implementations, especially when there's a popular implementation that's been shipping for years.

The specification matches what authors expect, and what is implemented by a majority of implementations when quantitatively speaking. Practically speaking, the share of iOS Safari and the recent Android Browser builds have a vast majority of usage, voiding my argument.

> Until we have data to the contrary, we should match the most popular implementation, which, in this case, is iOS.  Introducing non-interoperability is not a path to better compatibility.

This is about interoperability between WebKit ports v.s. interoperability between different mobile rendering engines. I wish there was more insight behind the rationales why IE and Firefox decided to support this.

I do support not landing any new separator until there is data to back up its need.

[Adam Barth]

> > Those browsers don't have enough usage on the mobile web to matter for these sorts of compatibility issues.
> 
> That does not mean we can ignore them, interoperability between browsers is no different than interoperability between WebKit ports..

Sure it does.  The goal here is to produce an implementation that correctly renders web pages.  Most folks with mobile web sites do not test their site in Fennec, which means that matching Fennec won't cause us to render the site correctly.  By contrast, most folks do test their mobile site in iOS, which means that if we match what Mobile Safari does, those sites will render correctly.

> > In general, specs should match implementations, especially when there's a popular implementation that's been shipping for years.
> 
> The specification matches what authors expect, and what is implemented by a majority of implementations when quantitatively speaking. Practically speaking, the share of iOS Safari and the recent Android Browser builds have a vast majority of usage, voiding my argument.

It's usage that matters because that's what causes site authors to test their web sites in a given implementation.

> > Until we have data to the contrary, we should match the most popular implementation, which, in this case, is iOS.  Introducing non-interoperability is not a path to better compatibility.
> 
> This is about interoperability between WebKit ports v.s. interoperability between different mobile rendering engines. I wish there was more insight behind the rationales why IE and Firefox decided to support this.

Interoperability with IE and Firefox on mobile currently doesn't matter.  They'll run into compat problems and end up matching iOS too.

[Peter Beverloo]

The specification is moving towards deprecating (though not dropping support for) the viewport meta tag in favor of CSS' @viewport at-rule, which should take care of these differences in a more persistent way. For Android, part of the problem will be sites which send different viewports based on the user agent, assuming support for semi-colons as separators.

While matching the expected behavior of authors would be ideal, which is, as earlier stated, what other browsers do, you're right in that usage share outweighs this argument. Without additional data, I'm wary that the ship has already sailed and we should drop this as you proposed.

[John Mellor]

(In reply to comment #23)
> Until we have data to the contrary, we should match the most popular implementation, which, in this case, is iOS.

Technically the most used implementation seems to be Opera; and of the WebKit ports it has been Android Browser since May 2011 (http://gs.statcounter.com/#mobile_browser-ww-monthly-201101-201206).

> > 1. It's harder to achieve parity between WebKit ports, as some sites using semi-colons may only work in Mobile Safari due to iOS quirks such as [2]. Other ports can end up with a different rendering, even though if they had parsed the semi-colons they would have done what the author intended (hence match Mobile Safari).
> 
> The solution is to become ever more identical with iOS WebKit, not to introduce a bunch of non-interoperability in the hopes that it will all balance out.

It's not desirable to reproduce all iOS bugs. For example, we shouldn't copy iOS's bug that if you rotate a "width=device-width, initial-scale=1.0" page from portrait to landscape you end up at an arbitrary zoom level and have to fix it by manually double-tapping (http://adactio.com/journal/5088/); instead iOS should just fix that.

> > 2. This isn't an acceptable state for the web platform to remain in. By the principle of least astonishment, behavior should not be completely different depending on whether you separate terms with ";" or "; " [3], or add a trailing semi-colon [4], or add redundant properties. Unlike emulating IE quirks, the author's intent is very clear when they accidentally use semi-colons, and we should just parse them sensibly rather than letting them stumble around undefined/accidental behavior.
> 
> That's not how the web works.  Authors copy and paste all manner of garbage syntax and the only way to make browsers render the content the same way is to have them work exactly the same.  In this case, that means processing the meta viewport exactly the way it is processed on iOS, starting with the parsing.
> (...)
> This story has played itself out many, many times in the history of the web.

The web tends to evolve by "paving the cowpaths". This is a cowpath: well-trodden, despite being unspec'd. We need to pave it with something sensible that matches existing websites. While requiring all browsers to reverse-engineer every last iOS bug is one way of standardising this, it's far from ideal. My experience to date is that 99% of websites using semi-colons behave the same whether or not you parse the semi-colons (and of the remainder, some render worse but others render better); if we can get data that confirms that, then it would be fairly safe to pave this cowpath in a way that won't constantly trip up web developers: supporting semi-colons.

(In reply to comment #25)
> Interoperability with IE and Firefox on mobile currently doesn't matter.  They'll run into compat problems and end up matching iOS too.

I had a skim through bugzilla.mozilla.org and couldn't find any viewport bugs caused by supporting semi-colons...

[Kenneth Rohde Christiansen]

(In reply to comment #18)
> (In reply to comment #17)
> > Is the iOS code that parses the viewport open source?  It's lame to have to reverse engineer a WebKit implementation in the WebKit project.  :-/
> 
> Apparently the iOS parsing code is in http://opensource.apple.com/source/WebCore/WebCore-1298.39/dom/ViewportArguments.cpp

That is not really what parses the 'content' argument of the meta tag. The whole discussion started as some browsers supported ; as a separator in 'content' which does not match the specification of meta tags (HTML spec?). I am pretty OK with us not supporting ; as a separator, IE etc will just have to change their implementations.

[Adam Barth]

Comment on attachment 151469 [details]
Patch

John, I'm happy to continue discussing this topic with you.  It's important that we come to agreement so that we can synchronous the upstream behavior here with the code on the chromium-android branch.

In the meantime, I'm inclined to land this patch given that it just introduces tests for this behavior.  If we later decide to use ; as a separator, we can always update the tests.

[WebKit Review Bot]

Comment on attachment 151469 [details]
Patch

Clearing flags on attachment: 151469

Committed r122464: <http://trac.webkit.org/changeset/122464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[John Mellor]

(In reply to comment #29)
> (From update of attachment 151469 [details])
> John, I'm happy to continue discussing this topic with you.  It's important that we come to agreement so that we can synchronous the upstream behavior here with the code on the chromium-android branch.
> 
> In the meantime, I'm inclined to land this patch given that it just introduces tests for this behavior.  If we later decide to use ; as a separator, we can always update the tests.

Sure, the patch LGTM. It seems clear that upstream won't change without data; perhaps we should put this bug on hold until data can be produced (and if that doesn't happen in time I guess we can revert this on the chromium-android branch)?

[Adam Barth]

I'm happy to keep this bug open while we gather data, but I would like to synchronize this behavior between upstream and the chromium-android branch.

Maybe the thing to do is to make the chromium-android branch match upstream and then be on the lookout for compatibility issues.  If we find compat issues, that will be useful data that we can use to resolve this bug.

[John Mellor]

Assigning this to myself to I remember to gather data.

[Adam Barth]

No longer blocks bug 66687.

[Ahmad Saleem]

[e-mail] - There are only just test cases in the patch, do we need them or WPT is enough for these cases? Thanks!

[Ryosuke Niwa]

At this point, this is Later.