Specify the tokenizer for window.open's features argument #2476

zcorpan · 2017-03-27T20:19:13Z

This was specified in CSSOM View but the "noopener" feature did
not use the same tokenizer as the legacy features. Fixes #2474.

Also specify the aliases screenx, screeny, innerwidth, innerheight
for left, top, width, and height, respectively. Part of #2464.
Closes w3c/csswg-drafts#1128.

This needs updates in cssom-view as well.

Maybe the aliases should be a separate commit... or maybe they should be in cssom-view still, not sure.

annevk

Lots of nits, looks reasonable overall.

annevk · 2017-03-28T05:44:00Z

source

+
+   <li>
+    <p>For each <var>token</var> of <var>tokens</var>:</p>
+    <ol>


Newline before <ol>.

annevk · 2017-03-28T05:44:19Z

source

+   <li><p>Let <var>map</var> be a new <span>ordered map</span>.</p></li>
+
+   <li>
+    <p>For each <var>token</var> of <var>tokens</var>:</p>


Link "For each".

annevk · 2017-03-28T05:45:05Z

source

+
+     <li><p>Let <var>position</var> point at the first character of <var>input</var>.</p></li>
+
+     <li><p><span>Skip ASCII whitespace</span>.


You need to pass this parameters these days.

annevk · 2017-03-28T05:45:19Z

source

+   <li>
+    <p>For each <var>token</var> of <var>tokens</var>:</p>
+    <ol>
+     <li><p>Let <var>input</var> be <var>token</var>.


Is this needed?

annevk · 2017-03-28T05:45:29Z

source

+
+     <li><p><span>Skip ASCII whitespace</span>.
+
+     <li><p><span>Collect a sequence of code points</span> that are not <span>ASCII


This also needs to be passed more arguments.

annevk · 2017-03-28T05:45:54Z

source

+     <var>name</var>.</p></li>
+
+     <li><p>If <var>tokenizedFeatures</var>[<var>name</var>] <span data-x="map
+     exists">exists</span>, then continue.</p></li>


So you can't overwrite existing values?

Right. This matches Gecko but not WebKit or Chromium.

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4986

What does Edge do?

Edge overwrites, but does not appear to support left (I ended up overwriting width).

annevk · 2017-03-28T05:46:04Z

source

+     <li><p>If <var>tokenizedFeatures</var>[<var>name</var>] <span data-x="map
+     exists">exists</span>, then continue.</p></li>
+
+     <li><p><span>Skip ASCII whitespace</span>.</p></li>


Missing arguments again.

zcorpan · 2017-03-28T07:22:21Z

source

+     whitespace</span> nor U+003D (=). Let <var>name</var> be the collected characters,
+     <span>converted to ASCII lowercase</span>.</p></li>
+
+     <li><p>Set <var>name</var> to the result of <span>normalizing the feature name</span>


Hmm maybe the name shouldn't be normalized during tokenization.

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4987

Gecko picks the "left" value in both cases. Chromium and WebKit pick the last one specified.

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4988

Here Chromium and WebKit still pick the last one specified and use 0. Gecko has different result for the two cases...

annevk

LGTM with minor nit.

annevk · 2017-03-28T12:46:58Z

source

+   <li><p>Let <var>tokenizedFeatures</var> be a new <span>ordered map</span>.</p></li>
+
+   <li>
+    <p>For each <var>token</var> of <var>tokens</var>:</p>


"For each" should be a link to https://infra.spec.whatwg.org/#list-iterate

See whatwg/html#2476 Also change the behavior for left=foo to act as if left=0 to align with the behavior of at least WebKit and Chromium.

lyzadanger · 2017-03-29T17:26:23Z

@zcorpan I was thinking making test coverage better in this area of the spec (7.3.1) — I'm not sure how the process typically works but would you like me to help out with writing tests for this?

zcorpan · 2017-03-30T08:29:12Z

@lyzadanger that would be awesome! Thanks!

I think this could be automated by checking https://drafts.csswg.org/cssom-view/#dom-window-screenx et al. Although the features for window.open needs to be defined in terms of https://drafts.csswg.org/cssom-view/#web-exposed-screen-area as well so they match up (and to fix the privacy leak for window.open). (Edit: w3c/csswg-drafts@7e04f5c )

Another problem is that doing anything with features is optional (other than noopener), but I guess we can add metadata to the tests to that effect.

See whatwg/html#2476 (comment)

lyzadanger · 2017-03-30T18:19:19Z

@zcorpan Sounds good. Just finishing up some travel; will get on this tomorrow (Friday).

zcorpan · 2017-04-03T11:01:53Z

source

+     <var>position</var>.</p></li>
+
+     <li><p>If <var>position</var> is not past the end of <var>token</var> and the character at
+     <var>position</var> is U+003D (=), then advance <var>position</var> by 1.</p></li>


Hmm there is a bug here. If it's not past the end and the character is not =, we shouldn't collect a value. Maybe it would be better to split on the first = and then trim leading and trailing whitespace from name and value?

zcorpan · 2017-04-03T12:31:37Z

I looked at the code in Chromium and reverse-engineered Edge. It appears there is almost perfect interop between Edge and Chromium and WebKit in how to tokenize. The only difference seems to be handling of U+0000 (Edge cuts the value at the first U+0000, Chromium treats it as a separator like space, WebKit treats it as a non-separator like x). I'll specify Chromium's tokenizer.

For example ==width==450==height==450 "works".

annevk · 2017-04-03T12:32:50Z

What does Chrome do for U+0001? I don't see why we'd treat U+0000 as a separator.

zcorpan · 2017-04-03T15:06:58Z

It's a non-separator. Happy to spec WebKit-like instead on that point.

Another interesting bit is U+000C, which only Gecko treats as a separator character. Likely it is Web-compatible to treat it as a separator so this is more consistent with everything else.

zcorpan · 2017-04-03T17:20:11Z

I found a case that is not interoperable between Edge and WebKit/Chromium, see ca2aa5d

Implementation of the proposed spec with a few tests: ~~http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4998~~ (edit: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/5001 )

lyzadanger · 2017-04-04T15:19:02Z

source

+
+      <ol>
+       <li><p>If the code point at <var>position</var> in <var>features</var> is U+002C (,), or if
+       it is not a <span>window features separator</span>, then break.</p></li>


Does this break to

While position is not past the end of features:

(line 78754)?

Yes, per https://infra.spec.whatwg.org/#iteration-break -- @annevk @domenic should we xref these terms?

I mean no. It breaks the nearest iteration.

Which is

While position is not past the end of features and the code point at position in features is not U+003D (=):

??

In that case, does this imply that any ',' characters between the name and the first instance of '=' are ignored? e.g. the case:

'noopener ,=foo,'

How does that tokenize?

log: noopener ,=foo, log: { "noopener": "", "foo": "" }

I could step through the spec but maybe it's simpler to step through the JS implementation with some browser's devtools? :-)

Ah, yes, good idea (I'm new to the part of this that involves prototyping implementation via JS). I'll walk through that more carefully and do some of my own tests against it there. In the end, I want to be sure that the spec language is in accordance to the implementation (obviously :) ). More soon.

tl;dr I think things are fine as-is.

My confusion was that, at this point in the spec, we are nested a few levels deep, but if I evaluate those levels, it's while (a) > while (b) > if. The break statement in that if conditional will return control to while (a), which is as it should be, but I had to think more programmatically than I was initially. 👍 and thanks for your patience.

IMO yes we should xref those terms. We're taking an incremental approach to introducing infra into HTML, so it's good to use this opportunity to introduce it here.

lyzadanger · 2017-04-04T15:42:49Z

source

+       </li>
+
+       <li><p><span>Collect a sequence of code points</span> that are not <span data-x="window
+       features separator">window features separators</span> code points from <var>features</var>


What happens to anything after the last non-separator code point in value? e.g. in the case 'noopener=yes yes,foo=bar', I believe you end up with ('noopener', 'yes') for the first tokenized feature, but what happens to the other 'yes'? It seems like this might put you into the next loop of tokenization with some remaining characters before the next ','?

...or does this tokenize to:

name 'noopener', value 'yes'

name 'yes', value ''

name 'foo', value 'bar'

Yes. Added that case in http://software.hixie.ch/utilities/js/live-dom-viewer/saved/5001 to confirm. (Also fixed a typo asciiLowercase -> asciiLowerCase from 4998.)

This was specified in CSSOM View but the "noopener" feature did not use the same tokenizer as the legacy features. Fixes #2474. Also specify the aliases screenx, screeny, innerwidth, innerheight for left, top, width, and height, respectively. Part of #2464. Closes w3c/csswg-drafts#1128. The tokenizer specified here closely follows Chromium and WebKit. Difference from Chromium: U+0000 is not a separator. Difference from WebKit and Chromium: U+000C is a separator.

Specifically, for the input `width toolbar=450, height=450`, Edge tokenizes like `width, toolbar=450, height=450` while WebKit/Chromium like `width=450, height=450`. The Edge behavior seems better.

zcorpan · 2017-04-05T10:51:15Z

I've cleaned up the history and rebased to resolve conflicts. I left the like-Edge change as a separate commit to make it clearer what the difference is from like-Chromium and -WebKit.

annevk · 2017-04-05T11:07:16Z

When you land you'll squash these I assume? (The second commit doesn't seem to have enough context on its own.)

zcorpan · 2017-04-05T11:30:35Z

Yes

zcorpan · 2017-04-05T13:33:35Z

Tests for noopener are (almost) ready: web-platform-tests/wpt#5306

@annevk can you do another round of editorial review? It's possible I made a mistake when rewriting history and resolving conflicts. I'll file browser bugs tomorrow.

annevk

Apart from the annotated nit, I also noticed that you had one dfn where you use "window features", but the other dfns just have feature(s). It might be nice to make that consistent, but since these terms are only for internal usage I'm okay either way.

annevk · 2017-04-05T13:52:35Z

source

+  </ol>
+
+  <p>A code point is a <dfn>window features separator</dfn> if it is one of <span>ASCII
+  whitespace</span>, U+003D (=), or U+002C (,).</p>


zcorpan · 2017-04-05T16:33:57Z

Fixed nit and renamed to "feature separator"

See whatwg/html#2476 Also change the behavior for left=foo to act as if left=0 to align with the behavior of at least WebKit and Chromium. Tests: web-platform-tests/wpt#5306 web-platform-tests/wpt#5390

Follows whatwg/html#2476

zcorpan · 2017-04-06T15:22:53Z

https://bugs.webkit.org/show_bug.cgi?id=170548
https://bugs.chromium.org/p/chromium/issues/detail?id=709037
https://bugzilla.mozilla.org/show_bug.cgi?id=1354174
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11544316/

See whatwg/html#2476 (comment)

zcorpan added the do not merge yet Pull request must not be merged per rationale in comment label Mar 27, 2017

annevk reviewed Mar 28, 2017

View reviewed changes

zcorpan commented Mar 28, 2017

View reviewed changes

annevk approved these changes Mar 28, 2017

View reviewed changes

zcorpan added the needs tests Moving the issue forward requires someone to write tests label Mar 28, 2017

zcorpan mentioned this pull request Mar 28, 2017

[cssom-view] Move tokenizing of window.open's features arg to HTML w3c/csswg-drafts#1138

Merged

zcorpan added a commit to zcorpan/csswg-drafts that referenced this pull request Mar 30, 2017

[cssom-view] Make window.open's features use Web-exposed screen area

7e04f5c

See whatwg/html#2476 (comment)

zcorpan mentioned this pull request Mar 30, 2017

[cssom-view] Make window.open's features use Web-exposed screen area w3c/csswg-drafts#1143

Merged

lyzadanger mentioned this pull request Mar 31, 2017

HTML: Add tests for tokenization of noopener window.open feature web-platform-tests/wpt#5306

Merged

zcorpan commented Apr 3, 2017

View reviewed changes

lyzadanger reviewed Apr 4, 2017

View reviewed changes

zcorpan added 2 commits April 5, 2017 12:49

Tokenize more like Edge

e74dc9a

Specifically, for the input `width toolbar=450, height=450`, Edge tokenizes like `width, toolbar=450, height=450` while WebKit/Chromium like `width=450, height=450`. The Edge behavior seems better.

zcorpan force-pushed the zcorpan/window-open-features-tokenize branch from dcc2580 to e74dc9a Compare April 5, 2017 10:49

zcorpan removed do not merge yet Pull request must not be merged per rationale in comment needs tests Moving the issue forward requires someone to write tests labels Apr 5, 2017

annevk approved these changes Apr 5, 2017

View reviewed changes

Address comments

51cadfa

lyzadanger mentioned this pull request Apr 5, 2017

HTML: Add tests for optional window.open position, size features web-platform-tests/wpt#5390

Merged

zcorpan merged commit b649435 into master Apr 6, 2017

zcorpan deleted the zcorpan/window-open-features-tokenize branch April 6, 2017 14:54

zcorpan pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 6, 2017

HTML: Add tests for tokenization of noopener window.open feature

e9c860b

Follows whatwg/html#2476

zcorpan added a commit to zcorpan/csswg-drafts that referenced this pull request Apr 6, 2017

[cssom-view] Make window.open's features use Web-exposed screen area

ed8e602

See whatwg/html#2476 (comment)

zcorpan added a commit to w3c/csswg-drafts that referenced this pull request Apr 6, 2017

[cssom-view] Make window.open's features use Web-exposed screen area

80c0561

See whatwg/html#2476 (comment)

This was referenced Apr 27, 2017

noopener feature in window.open inconsistent with other boolean features #2600

Closed

[css-device-adapt] Meta viewport tokenizer should re-use window.open() features tokenizer w3c/csswg-drafts#1302

Open


		<li><p>Let <var>position</var> point at the first character of <var>input</var>.</p></li>

		<li><p><span>Skip ASCII whitespace</span>.


		<li><p><span>Skip ASCII whitespace</span>.

		<li><p><span>Collect a sequence of code points</span> that are not <span>ASCII

Specify the tokenizer for window.open's features argument #2476

Specify the tokenizer for window.open's features argument #2476

Conversation

zcorpan commented Mar 27, 2017

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyzadanger commented Mar 29, 2017

zcorpan commented Mar 30, 2017 • edited Loading

lyzadanger commented Mar 30, 2017

Choose a reason for hiding this comment

zcorpan commented Apr 3, 2017

annevk commented Apr 3, 2017

zcorpan commented Apr 3, 2017

zcorpan commented Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyzadanger Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcorpan commented Apr 5, 2017

annevk commented Apr 5, 2017

zcorpan commented Apr 5, 2017

zcorpan commented Apr 5, 2017

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcorpan commented Apr 5, 2017

zcorpan commented Apr 6, 2017

zcorpan commented Mar 30, 2017 •

edited

Loading

zcorpan commented Apr 3, 2017 •

edited

Loading

lyzadanger Apr 4, 2017 •

edited

Loading