| Summary: | Tweak window.open features argument tokenizer to match HTML standard and Edge | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Pieters (:zcorpan) <zcorpan> |
| Component: | DOM | Assignee: | Chris Dumez <cdumez> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | achristensen, aestes, ap, buildbot, cdumez, commit-queue, darin, dbates, esprehn+autocc, ggaren, kangil.han, rniwa, sam, simon.fraser, youennf |
| Priority: | P2 | ||
| Version: | Safari Technology Preview | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | 171313 | ||
| Bug Blocks: | |||
| Attachments: | |||
|
Description
Simon Pieters (:zcorpan)
2017-04-06 08:06:51 PDT
Created attachment 308209 [details]
WIP Patch
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. (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. Created attachment 308211 [details]
WIP Patch
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 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
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 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
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 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
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 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
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 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
Created attachment 308274 [details]
WIP Patch
Created attachment 308281 [details]
Patch
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 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
Created attachment 308293 [details]
Patch
Comment on attachment 308293 [details]
Patch
r=me
(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 (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. (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. 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 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
Created attachment 308318 [details]
Patch
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.
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 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
(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. 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> (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. (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>. (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>. (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. > 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.
Created attachment 308404 [details]
Patch
Patch is ready for review. 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. Committed r215945: <http://trac.webkit.org/changeset/215945> |