Bug 170548 – Tweak window.open features argument tokenizer to match HTML standard and Edge

• Component: DOM
• Status: RESOLVED FIXED

[Simon Pieters (:zcorpan)]

See the following spec changes and tests in web-platform-tests:
https://github.com/whatwg/html/pull/2476
https://github.com/w3c/csswg-drafts/pull/1138
https://github.com/w3c/web-platform-tests/pull/5306
https://github.com/w3c/web-platform-tests/pull/5390

The PR for whatwg/html has a commit that effectively shows what the diff is between WebKit and Edge (ignoring Edge's behavior for U+0000):
https://github.com/whatwg/html/pull/2476/commits/e74dc9ad3872a89cc8f2fa62e160c076741170e6

The test that fails because of not tokenizing like Edge is
https://github.com/w3c/web-platform-tests/pull/5306/files#diff-f218f27c4d582eebff6fcf48d4f4e219R65

Apart from implementing that diff, WebKit also needs to add U+000C as a "feature separator" character.
https://github.com/w3c/web-platform-tests/pull/5306/files#diff-f218f27c4d582eebff6fcf48d4f4e219R24

[Chris Dumez]

Created attachment 308209 [details]
WIP Patch

[Chris Dumez]

I am still trying to figure out from https://html.spec.whatwg.org/#concept-window-open-features-tokenize why '\u000Cnoopener' would turn on no-opener feature given that \u000 is not a feature separator.

[Chris Dumez]

(In reply to Chris Dumez from comment #2)
> I am still trying to figure out from
> https://html.spec.whatwg.org/#concept-window-open-features-tokenize why
> '\u000Cnoopener' would turn on no-opener feature given that \u000 is not a
> feature separator.

Ah, it is \u000C, never mind. We are indeed missing this separator.

[Chris Dumez]

Created attachment 308211 [details]
WIP Patch

[Build Bot]

Comment on attachment 308211 [details]
WIP Patch

Attachment 308211 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3607528

New failing tests:
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-001.html

[Build Bot]

Created attachment 308216 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308211 [details]
WIP Patch

Attachment 308211 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3607680

New failing tests:
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-001.html

[Build Bot]

Created attachment 308220 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308211 [details]
WIP Patch

Attachment 308211 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3607725

New failing tests:
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-001.html

[Build Bot]

Created attachment 308222 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308211 [details]
WIP Patch

Attachment 308211 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3607718

New failing tests:
fast/viewport/viewport-122.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-001.html
fast/viewport/viewport-121.html

[Build Bot]

Created attachment 308225 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308211 [details]
WIP Patch

Attachment 308211 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3608140

New failing tests:
fast/viewport/viewport-122.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-001.html
fast/viewport/viewport-121.html

[Build Bot]

Created attachment 308233 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 308274 [details]
WIP Patch

[Chris Dumez]

Created attachment 308281 [details]
Patch

[Build Bot]

Comment on attachment 308281 [details]
Patch

Attachment 308281 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3612288

New failing tests:
fast/viewport/viewport-117.html
fast/viewport/viewport-126.html
fast/viewport/viewport-74.html
fast/viewport/viewport-113.html
fast/viewport/viewport-87.html
fast/viewport/viewport-111.html
fast/viewport/viewport-108.html
fast/viewport/viewport-121.html
fast/viewport/viewport-116.html
fast/viewport/viewport-114.html
fast/viewport/viewport-115.html
fast/viewport/viewport-110.html
fast/viewport/viewport-109.html
fast/viewport/viewport-75.html
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-noopener.html
fast/viewport/viewport-70.html
fast/viewport/viewport-122.html
fast/viewport/viewport-73.html

[Build Bot]

Created attachment 308292 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 308293 [details]
Patch

[Geoffrey Garen]

Comment on attachment 308293 [details]
Patch

r=me

[Chris Dumez]

(In reply to Build Bot from comment #17)
> Comment on attachment 308281 [details]
> Patch
> 
> Attachment 308281 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: http://webkit-queues.webkit.org/results/3612288
> 
> New failing tests:
> fast/viewport/viewport-117.html
> fast/viewport/viewport-126.html
> fast/viewport/viewport-74.html
> fast/viewport/viewport-113.html
> fast/viewport/viewport-87.html
> fast/viewport/viewport-111.html
> fast/viewport/viewport-108.html
> fast/viewport/viewport-121.html
> fast/viewport/viewport-116.html
> fast/viewport/viewport-114.html
> fast/viewport/viewport-115.html
> fast/viewport/viewport-110.html
> fast/viewport/viewport-109.html
> fast/viewport/viewport-75.html
> imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-
> creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-
> noopener.html
> fast/viewport/viewport-70.html
> fast/viewport/viewport-122.html
> fast/viewport/viewport-73.html

I currently have no idea about those fast/viewport/ on iOS yet. Those tests are not using window.open() at all so they look unrelated to my change.

The diffs look like:
--- /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/layout-test-results/retries/fast/viewport/viewport-70-expected.txt
+++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/layout-test-results/retries/fast/viewport/viewport-70-actual.txt
@@ -1,3 +1,3 @@
 CONSOLE MESSAGE: line 3: Viewport argument key "x" not recognized and ignored.
-ALERT: viewport size 100x110 scale 3.2 with limits [3.2, 5] and userScalable true
+ALERT: viewport size 320x352 scale 1 with limits [1, 5] and userScalable true

[Chris Dumez]

(In reply to Chris Dumez from comment #21)
> (In reply to Build Bot from comment #17)
> > Comment on attachment 308281 [details]
> > Patch
> > 
> > Attachment 308281 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> > Output: http://webkit-queues.webkit.org/results/3612288
> > 
> > New failing tests:
> > fast/viewport/viewport-117.html
> > fast/viewport/viewport-126.html
> > fast/viewport/viewport-74.html
> > fast/viewport/viewport-113.html
> > fast/viewport/viewport-87.html
> > fast/viewport/viewport-111.html
> > fast/viewport/viewport-108.html
> > fast/viewport/viewport-121.html
> > fast/viewport/viewport-116.html
> > fast/viewport/viewport-114.html
> > fast/viewport/viewport-115.html
> > fast/viewport/viewport-110.html
> > fast/viewport/viewport-109.html
> > fast/viewport/viewport-75.html
> > imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-
> > creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-
> > noopener.html
> > fast/viewport/viewport-70.html
> > fast/viewport/viewport-122.html
> > fast/viewport/viewport-73.html
> 
> I currently have no idea about those fast/viewport/ on iOS yet. Those tests
> are not using window.open() at all so they look unrelated to my change.
> 
> The diffs look like:
> ---
> /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/layout-test-
> results/retries/fast/viewport/viewport-70-expected.txt
> +++
> /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/layout-test-
> results/retries/fast/viewport/viewport-70-actual.txt
> @@ -1,3 +1,3 @@
>  CONSOLE MESSAGE: line 3: Viewport argument key "x" not recognized and
> ignored.
> -ALERT: viewport size 100x110 scale 3.2 with limits [3.2, 5] and
> userScalable true
> +ALERT: viewport size 320x352 scale 1 with limits [1, 5] and userScalable
> true

Ok, so this is legit. It looks like the processFeaturesString() which I aligned with the HTML spec is also used to parse viewport meta tag values such as:
<meta name="viewport" content="width=100 x initial-scale=1">

I am not sure yet if we want different parsing behavior for viewport and window features.

[Chris Dumez]

(In reply to Chris Dumez from comment #22)
> (In reply to Chris Dumez from comment #21)
> > (In reply to Build Bot from comment #17)
> > > Comment on attachment 308281 [details]
> > > Patch
> > > 
> > > Attachment 308281 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> > > Output: http://webkit-queues.webkit.org/results/3612288
> > > 
> > > New failing tests:
> > > fast/viewport/viewport-117.html
> > > fast/viewport/viewport-126.html
> > > fast/viewport/viewport-74.html
> > > fast/viewport/viewport-113.html
> > > fast/viewport/viewport-87.html
> > > fast/viewport/viewport-111.html
> > > fast/viewport/viewport-108.html
> > > fast/viewport/viewport-121.html
> > > fast/viewport/viewport-116.html
> > > fast/viewport/viewport-114.html
> > > fast/viewport/viewport-115.html
> > > fast/viewport/viewport-110.html
> > > fast/viewport/viewport-109.html
> > > fast/viewport/viewport-75.html
> > > imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-
> > > creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-
> > > noopener.html
> > > fast/viewport/viewport-70.html
> > > fast/viewport/viewport-122.html
> > > fast/viewport/viewport-73.html
> > 
> > I currently have no idea about those fast/viewport/ on iOS yet. Those tests
> > are not using window.open() at all so they look unrelated to my change.
> > 
> > The diffs look like:
> > ---
> > /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/layout-test-
> > results/retries/fast/viewport/viewport-70-expected.txt
> > +++
> > /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/layout-test-
> > results/retries/fast/viewport/viewport-70-actual.txt
> > @@ -1,3 +1,3 @@
> >  CONSOLE MESSAGE: line 3: Viewport argument key "x" not recognized and
> > ignored.
> > -ALERT: viewport size 100x110 scale 3.2 with limits [3.2, 5] and
> > userScalable true
> > +ALERT: viewport size 320x352 scale 1 with limits [1, 5] and userScalable
> > true
> 
> Ok, so this is legit. It looks like the processFeaturesString() which I
> aligned with the HTML spec is also used to parse viewport meta tag values
> such as:
> <meta name="viewport" content="width=100 x initial-scale=1">
> 
> I am not sure yet if we want different parsing behavior for viewport and
> window features.

The spec for meta viewport seems to be at https://drafts.csswg.org/css-device-adapt/#parsing-algorithm. It seems to have its own algorithm so I may need to use different algorithms for parsing meta viewport and window features.

[Build Bot]

Comment on attachment 308293 [details]
Patch

Attachment 308293 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3614308

New failing tests:
fast/viewport/viewport-117.html
fast/viewport/viewport-126.html
fast/viewport/viewport-74.html
fast/viewport/viewport-113.html
fast/viewport/viewport-87.html
fast/viewport/viewport-111.html
fast/viewport/viewport-108.html
fast/viewport/viewport-121.html
fast/viewport/viewport-116.html
fast/viewport/viewport-114.html
fast/viewport/viewport-115.html
fast/viewport/viewport-110.html
fast/viewport/viewport-109.html
fast/viewport/viewport-75.html
fast/viewport/viewport-73.html
fast/viewport/viewport-70.html
fast/viewport/viewport-122.html

[Build Bot]

Created attachment 308309 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 308318 [details]
Patch

[Chris Dumez]

Comment on attachment 308318 [details]
Patch

Requesting review again because I had to update my change to maintain background compatibility for the meta tag viewport parsing.

[Build Bot]

Comment on attachment 308318 [details]
Patch

Attachment 308318 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3615446

New failing tests:
workers/wasm-hashset-many-2.html

[Build Bot]

Created attachment 308324 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Chris Dumez]

(In reply to Build Bot from comment #28)
> Comment on attachment 308318 [details]
> Patch
> 
> Attachment 308318 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/3615446
> 
> New failing tests:
> workers/wasm-hashset-many-2.html

Please disregard this failure, it looks unrelated.

[Daniel Bates]

Comment on attachment 308318 [details]
Patch

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

> Source/WebCore/page/WindowFeatures.cpp:84
> +// Viewport: https://drafts.csswg.org/css-device-adapt/#parsing-algorithm

From my understanding this is not the spec. that we follow for viewport. We implement the behavior described in <https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariWebContent/UsingtheViewport/UsingtheViewport.html>

[Chris Dumez]

(In reply to Daniel Bates from comment #31)
> Comment on attachment 308318 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308318&action=review
> 
> > Source/WebCore/page/WindowFeatures.cpp:84
> > +// Viewport: https://drafts.csswg.org/css-device-adapt/#parsing-algorithm
> 
> From my understanding this is not the spec. that we follow for viewport. We
> implement the behavior described in
> <https://developer.apple.com/library/content/documentation/AppleApplications/
> Reference/SafariWebContent/UsingtheViewport/UsingtheViewport.html>

Well, this is *a* spec which document's Apple's behavior. I am not convinced pointing to the Apple documentation is better given that it does not document the actual algorithm.

[Daniel Bates]

(In reply to Chris Dumez from comment #32)
> (In reply to Daniel Bates from comment #31)
> > Comment on attachment 308318 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=308318&action=review
> > 
> > > Source/WebCore/page/WindowFeatures.cpp:84
> > > +// Viewport: https://drafts.csswg.org/css-device-adapt/#parsing-algorithm
> > 
> > From my understanding this is not the spec. that we follow for viewport. We
> > implement the behavior described in
> > <https://developer.apple.com/library/content/documentation/AppleApplications/
> > Reference/SafariWebContent/UsingtheViewport/UsingtheViewport.html>
> 
> Well, this is *a* spec which document's Apple's behavior. 

No, it documents an attempt at reverse engineering Apple's behavior:

"Below is an algorithm for parsing the content attribute of the <META> tag produced from testing Safari on the iPhone."

Notice the key phrase "produced from testing Safari on the iPhone". Apple has never contributed to this spec. and the closet thing to a spec that Apple has is the description in <https://developer.apple.com/library/content/documentation/AppleApplications/

> I am not convinced pointing to the Apple documentation is better given that it does not document the actual algorithm.

I have not verified your code change. If your code change does not change the behavior of the meta viewport tag AND the algorithm used is identical to the algorithm described in <https://drafts.csswg.org/css-device-adapt/#parsing-algorithm> then it seems OK to link to <https://drafts.csswg.org/css-device-adapt/#parsing-algorithm>. Otherwise, the definitive reference for the meta viewport syntax that we want to implement is at <https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/MetaTags.html#//apple_ref/doc/uid/TP40008193-SW6>.

[Daniel Bates]

(In reply to Daniel Bates from comment #33)
[...]
> Apple
> has never contributed to this spec. and the closet thing to a spec that
> Apple has is the description in
> <https://developer.apple.com/library/content/documentation/AppleApplications/
> 

This should read:

Apple has never contributed to this spec. and the closet thing to a spec that Apple has is the description in <https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/MetaTags.html#//apple_ref/doc/uid/TP40008193-SW6> and <https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariWebContent/UsingtheViewport/UsingtheViewport.html>.

[Chris Dumez]

(In reply to Daniel Bates from comment #33)
> (In reply to Chris Dumez from comment #32)
> > (In reply to Daniel Bates from comment #31)
> > > Comment on attachment 308318 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=308318&action=review
> > > 
> > > > Source/WebCore/page/WindowFeatures.cpp:84
> > > > +// Viewport: https://drafts.csswg.org/css-device-adapt/#parsing-algorithm
> > > 
> > > From my understanding this is not the spec. that we follow for viewport. We
> > > implement the behavior described in
> > > <https://developer.apple.com/library/content/documentation/AppleApplications/
> > > Reference/SafariWebContent/UsingtheViewport/UsingtheViewport.html>
> > 
> > Well, this is *a* spec which document's Apple's behavior. 
> 
> No, it documents an attempt at reverse engineering Apple's behavior:
> 
> "Below is an algorithm for parsing the content attribute of the <META> tag
> produced from testing Safari on the iPhone."
> 
> Notice the key phrase "produced from testing Safari on the iPhone". Apple
> has never contributed to this spec. and the closet thing to a spec that
> Apple has is the description in
> <https://developer.apple.com/library/content/documentation/AppleApplications/
> 
> > I am not convinced pointing to the Apple documentation is better given that it does not document the actual algorithm.
> 
> I have not verified your code change. If your code change does not change
> the behavior of the meta viewport tag AND the algorithm used is identical to
> the algorithm described in
> <https://drafts.csswg.org/css-device-adapt/#parsing-algorithm> then it seems
> OK to link to
> <https://drafts.csswg.org/css-device-adapt/#parsing-algorithm>. Otherwise,
> the definitive reference for the meta viewport syntax that we want to
> implement is at
> <https://developer.apple.com/library/content/documentation/AppleApplications/
> Reference/SafariHTMLRef/Articles/MetaTags.html#//apple_ref/doc/uid/
> TP40008193-SW6>.

It seemed to match our behavior but i will double check tomorrow. I’ll definitely not point to it if it does not match exactly.

[Simon Pieters (:zcorpan)]

> The spec for meta viewport seems to be at https://drafts.csswg.org/css-device-adapt/#parsing-algorithm. It seems to have its own algorithm so I may need to use different algorithms for parsing meta viewport and window features.

Ugh. My knee-jerk reaction is that we should continue to use the same parser for both, unless web compat requires anything else. The CSS spec can be updated to hook into HTML's feature tokenizer. I can file an issue for css-device-adapt.

[Simon Pieters (:zcorpan)]

Filed https://github.com/w3c/csswg-drafts/issues/1302

[Chris Dumez]

Created attachment 308404 [details]
Patch

[Chris Dumez]

Patch is ready for review.

[Geoffrey Garen]

Comment on attachment 308404 [details]
Patch

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

r=me

> Source/WebCore/page/WindowFeatures.cpp:51
> +    switch (mode) {
> +    case FeatureMode::Viewport:
> +        return character == ' ' || character == '\t' || character == '\n' || character == '\r' || character == '=' || character == ',';
> +    case FeatureMode::Window:
> +        break;
> +    }

I think this would be clearer as an if.

[Chris Dumez]

Committed r215945: <http://trac.webkit.org/changeset/215945>