Bug 40204 – Need to recognize ";" as separator for arguments of viewport tag

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Gyuyoung Kim]

When arguments of viewport's tag are processed, ";" is not recognized as separator as below,

static bool isSeparator(UChar c)
{
    return c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == '=' || c == ',' || c == '\0';
}

However, some website is using ";" as separator.
For example, in http://m.flickr.com/#/home, it is using ";" for arguments of viewport as below,
<meta name="viewport" content="width=device-width; initial-scale=1.0; maximum-scale=1.0; user-scalable=0;">

So, the "device-width;" argument is not processed by processArguments() function because the "deveice-width;" contains ";".
In my opinion, we need to add ";" to isSeparator() as below,

static bool isSeparator(UChar c)
{
    return c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == '=' || c == ',' || c == '\0' || c == ';';
}

If there are better solutions, please let me know.

[Gyuyoung Kim]

Created attachment 57963 [details]
patch-1

Please review a patch attached.

[Peter Beverloo]

After doing some quick research, the Safari documentation[1] clearly demonstrates that multiple tags should be divided by commas instead of semicolons. Other websites, including some Opera's introduction about the viewport meta[2] and a fair amount of other websites describing the possibility, talk solely about dividing by commas as well. The CSS3 Media Queries spec[3], which certainly has some similarities with viewport, does not list a semicolon in its syntax either.

Are you sure a change like this is desirable? A much easier solution probably is asking Flickr to fix their meta-tag.

[1] http://developer.apple.com/safari/library/documentation/appleapplications/reference/safarihtmlref/articles/metatags.html#//apple_ref/html/const/viewport
[2] http://dev.opera.com/articles/view/opera-mobile-9-5-the-developer-angle/
[3] http://www.w3.org/TR/css3-mediaqueries/#syntax

[Gyuyoung Kim]

Thank you for your your opinion. It seems I need to investigate this specification further.

[Viatcheslav Ostapenko]

(In reply to comment #2)
> After doing some quick research, the Safari documentation[1] clearly demonstrates that multiple tags should be divided by commas instead of semicolons. Other websites, including some Opera's introduction about the viewport meta[2] and a fair amount of other websites describing the possibility, talk solely about dividing by commas as well. The CSS3 Media Queries spec[3], which certainly has some similarities with viewport, does not list a semicolon in its syntax either.
> 
> Are you sure a change like this is desirable? A much easier solution probably is asking Flickr to fix their meta-tag.
> 
> [1] http://developer.apple.com/safari/library/documentation/appleapplications/reference/safarihtmlref/articles/metatags.html#//apple_ref/html/const/viewport
> [2] http://dev.opera.com/articles/view/opera-mobile-9-5-the-developer-angle/
> [3] http://www.w3.org/TR/css3-mediaqueries/#syntax

That's all true, but iphone-4 and android both support comma and semicolon for viewport tag separator. It seems also, that android support space as separator also, but iphone-4 doesn't.

The patch is not quite complete.  In addition to modifying isSeparator() it should also modify two lines in the function that calls isSeparator(), Document::processArguments(), to replace

if (buffer[i] == ',' || i >= length)

with

if (buffer[i] == ',' || buffer[i] == ';' || i >= length)

[Gyuyoung Kim]

Created attachment 84902 [details]
Proposed Patch

Thank you for your comment. I add ';' to the two lines. Iphone and Android already support semicolon as separator, I think we can add semicolon to separator. I add comments to this patch.

[Kenneth Rohde Christiansen]

Android has removed support for ; and iOS does not support it. What iOS (and WebKit trunk now) supports is trailing garbage. ie the ; in

width = 230;

is considered garbage and thus ignored. This means that width will indeed become 230.

Closing as wontfix.

[Eric Seidel (no email)]

Comment on attachment 84902 [details]
Proposed Patch

Cleared review? from attachment 84902 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Gyuyoung Kim]

(In reply to comment #8)
> (From update of attachment 84902 [details])
> Cleared review? from attachment 84902 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

Kenneth said we don't need this patch. Do you think this patch can be needed ?