Add HTMLAllCollection [[Call]], remove legacycaller special operation #2932

tobie · 2017-08-16T17:13:00Z

domenic · 2017-08-18T22:12:17Z

source

@@ -7091,6 +7092,11 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
  different (ab)uses of its methods to all end up returning something, and that it can be called as
  a function as an alternative to property access.</p>

+  <p>Objects that implement the <code>HTMLAllCollection</code> are <span data-x="legacy platform


Missing word "interface"

domenic · 2017-08-18T22:12:58Z

source

+  <p>Objects that implement the <code>HTMLAllCollection</code> are <span data-x="legacy platform
+  object">legacy platform objects</span> with an additonal <a
+  href="#HTMLAllCollection-call">[[Call]]</a> internal method described in the subsection
+  below.</p>


I'd rather the <a> be "the subsection below". Also, probably "section" instead of "subsection"; who knows how deep it is nested (and thus how many "sub"s are necessary).

domenic · 2017-08-18T22:13:32Z

source

@@ -7163,7 +7169,8 @@ interface <dfn>HTMLAllCollection</dfn> {
  element(s)</span> from this <code>HTMLAllCollection</code> given <var>name</var>.</p>

  <p>The <dfn><code data-x="dom-HTMLAllCollection-item">item(<var>nameOrIndex</var>)</code></dfn>
-  method (and the <code data-x="">legacycaller</code> behavior) must run the following steps:</p>
+  method (and the <a href="#HTMLAllCollection-call">[[Call]]</a> internal method) must run the


I wouldn't include this here as it makes there be two normative definitions of [[Call]]

domenic · 2017-08-19T01:04:17Z

source

+  <ol>
+   <li><p>Let <var>result</var> be the result of performing the steps listed in the description of
+   the <code data-x="dom-HTMLAllCollection-item">item</code> method on <var>thisArgument</var> with
+   <var>argumentsList</var>[0] as argument.</p></li>


I guess we should really refactor these steps into a separate named algorithm.

Done, though I'm not sure of the name.

annevk · 2017-08-19T13:13:09Z

I think you have to rebase on master. Sorry about that.

Depends on whatwg/webidl#412.

TimothyGu

Another thing is that I'd like to address whatwg/webidl#407 if possible and convenient. In other words, deciding if document.all is a constructor. Chrome says yes, Firefox says no.

TimothyGu · 2017-08-19T13:23:16Z

source

@@ -7101,7 +7106,7 @@ interface <dfn>HTMLAllCollection</dfn> {
  readonly attribute unsigned long <span data-x="dom-HTMLAllCollection-length">length</span>;
  getter <span>Element</span>? (unsigned long index);
  getter (<span>HTMLCollection</span> or <span>Element</span>)? <span data-x="dom-HTMLAllCollection-namedItem">namedItem</span>(DOMString name);
-  legacycaller (<span>HTMLCollection</span> or <span>Element</span>)? <span data-x="dom-HTMLAllCollection-item">item</span>(optional DOMString nameOrIndex);
+  (<span>HTMLCollection</span> or <span>Element</span>)? <span data-x="dom-HTMLAllCollection-item">item</span>(optional DOMString nameOrIndex);


Would be good to add a comment in IDL about the fact that HTMLAllCollection instances implement a [[Call]]. Even better if it's linked like for "intentionally" in HTMLUnknownElement's IDL.

TimothyGu · 2017-08-19T13:55:27Z

source

+  <ol>
+   <li><p>Let <var>result</var> be the result of <span
+   data-x="concept-get-all-named-or-indexed">getting the "all"-named or indexed element(s)</span>
+   from this <code>HTMLAllCollection</code> given <var>argumentsList</var>[0].</p></li>


If argumentsList's length is 0, it might be undefined behavior. Infra says, "The index cannot be out-of-bounds, except when used with exists." We don't use "exists" here, but there is a "was not provided" check in the algorithm. Either way I'd say it'll save us some headache if we just say "argumentsList[0] if argumentsList[0] exists".

OTOH, if argumentsList[0] is provided, it must first be converted to a IDL DOMString.

Another point of interest/headache is if argumentsList[0] is the ES value undefined. Per Web IDL right now it must be ignored as the parameter is optional. But on both Chrome and Firefox undefined is treated as 'undefined'. Additionally Firefox throws if no argument is passed to document.all or document.all.item, against the spec; i.e. it's not treating the argument as optional at all.

A piece of good news is that branding check for thisArgument as a Document seems unnecessary though.

Either way I'd say it'll save us some headache if we just say "argumentsList[0] if argumentsList[0] exists".

That's a good point. I thought I'd somehow get away with being hand-wavy here, but looks like I didn't. :-P

Another point of interest/headache is if argumentsList[0] is the ES value undefined. Per Web IDL right now it must be ignored as the parameter is optional. But on both Chrome and Firefox undefined is treated as 'undefined'. Additionally Firefox throws if no argument is passed to document.all or document.all.item, against the spec; i.e. it's not treating the argument as optional at all.

So Firefox's behavior is actually implementing legacycaller (HTMLCollection or Element)? item(DOMString nameOrIndex);, right (i.e. without the optional argument)? Safari seems to be doing the same thing as Chrome, with a little twist: document.all.item() is the same as document.all.item(undefined) i.e. it treats it as "undefined", but document.all() always returns undefined.

Also worth noting that Chrome and Safari return undefined instead of null when they can't find a node.

Do we know what IE does?

A piece of good news is that branding check for thisArgument as a Document seems unnecessary though.

Probably worth testing.

Do we know what IE does?

I wrote a test file to be run on Edge. Seems like it does the exact same thing as Chrome.

A piece of good news is that branding check for thisArgument as a Document seems unnecessary though.

Probably worth testing.

Yep, the same behavior is present on all popular browsers regarding document.all()'s branding check.

The full results are available at: https://github.com/TimothyGu/document-all-undefined/blob/master/index.html. There are multiple instances of convergence we can spec.

There are multiple instances of convergence we can spec.

Do you want to just go-ahead and push those to this PR?

@tobie No commit access to this repo 😕

@TimothyGu oh, right. I assumed checking "Allow edits from maintainers" was enough. Let me fix that.

@tobie Thanks! @annevk has also offered write access to whatwg/html so that should be enough.

Unfortunately I'll be going on vacation until next Wednesday :( I will assign this to myself, but do not hesitate to make more improvements during this time!

bzbarsky · 2017-08-29T20:26:03Z

Is the goal here just to keep someone else from putting legacycaller on some interface?

It's not clear to me why the spec (and possibly implementation) churn here is that desirable. In practice, at least for Gecko, we'd just leave legacycaller in our IDL, because the thing that needs to spit out the [[Call]] uses the IDL as input anyway...

domenic · 2017-08-29T20:27:17Z

Is the goal here just to keep someone else from putting legacycaller on some interface?

That, plus perhaps a general principle that constructs used for a single object on the platform should not get dedicated IDL syntax.

bzbarsky · 2017-08-29T20:27:19Z

And if the worry is that people will misuse legacycaller, then note that the misuse potential of random specs defining arbitrary internal methods however they want is much greater. Having that precedent is pretty suboptimal, even if we do kind of have it for Location....

tobie · 2017-08-29T20:28:56Z

@bzbarsky this was a proposed solution to whatwg/webidl#408, based on the fact that HTMLAllCollection is the only legacycaller with [[Call]].

bzbarsky · 2017-08-29T20:29:20Z

Yes, I understand that.

bzbarsky · 2017-08-29T20:32:03Z

source

+
+  <ol>
+   <li><p>Let <var>result</var> be the result of <span
+   data-x="concept-get-all-named-or-indexed">getting the "all"-named or indexed element(s)</span>


argumentsList is a list of ES Values. concept-get-all-named-or-indexed takes as input an IDL value. This is the main problem with trying to hand-define this instead of using IDL: you end up having to carefully reinvent parts of the IDL spec...

tobie · 2017-08-29T20:34:40Z

I don't mind backing out of this. We just need to address whatwg/webidl#408 differently.

bzbarsky · 2017-08-29T21:01:49Z

I guess we can do this on the principle that one-offs don't belong in IDL. We do need to make the actual algorithm make sense in terms of ES vs IDL values, though.

TimothyGu · 2017-08-31T03:55:30Z

Having that precedent is pretty suboptimal

I think document.all (and Location for that matter) is understood to be legacy enough to not be a good precedent for... anything.

We do need to make the actual algorithm make sense in terms of ES vs IDL values, though.

Agreed. I'll try to fix this while tackling #2932 (comment).

TimothyGu · 2017-09-01T11:28:44Z

I have addressed the issues @bzbarsky and I noted earlier. I have also made the change to return undefined instead of null for document.all() and document.all.item(), as is the behavior in Chrome, Edge, and released Safari. The change to the latter isn't real clean because of the fact that IDL doesn't work well with undefined, but it's there for review.

However, I also found even more complications to Web compatibility regarding this excrescence.

It seems at least Chrome supports a weird two-argument form of document.all() legacy caller:

  // If there is a second argument it is the index of the item we want.

The good news is that this feature is used very very rarely (0.000002% according to Chrome's UseCounter), and this special treatment does not extend to document.all.item(). I will just ignore this new development.

Another thing I noticed is that relatively recently WebKit changed its behavior regarding HTMLAllCollection to be spec-compliant, and therefore not consistent with any other browsers except for Firefox (mostly). 😕

CC'ing @samweinig who made that change.

domenic · 2017-09-02T01:48:48Z

@TimothyGu I think we should stick with the existing spec/what was in IDL for null vs. undefined. It's been in the spec a long time, and we shouldn't be punishing Firefox and WebKit for following the spec. We need some WPTs for it, to be sure, but we shouldn't change it.

domenic · 2017-09-02T01:51:54Z

source

@@ -7247,9 +7250,15 @@ interface <dfn>HTMLAllCollection</dfn> {
  <h6 id="HTMLAllCollection-call">[[Call]] ( <var>thisArgument</var>,  <var>argumentsList</var> )</h6>

  <ol>
+   <li><p>If <var>argumentsList</var>'s length is zero, return null.</p></li>


Lists have sizes, not lengths, per Infra.

domenic · 2017-09-02T01:52:12Z

So things to test so far:

item() out of bounds behavior
[[Call]] out of bounds behavior
No [[Construct]] --- both directly, and using https://esdiscuss.org/topic/isconstructor#content-11

what else?

TimothyGu · 2017-09-02T06:14:18Z

@domenic

Sounds fair.

Note that even Firefox isn't 100% compliant right now, as it throws when there's no argument. I'm unable to test latest Safari due to its unavailability on Sauce Labs and my not having a Mac.

Some more test cases I thought of:

(from @domenic) no [[Construct]]
undefined as lone argument
null as lone argument
empty string
not an "all"-named element but has a name
(from @annevk) no brand check for [[Call]]
(optionally) second argument doesn't make a difference

The following tests I thought of were already tested:

no arguments behavior for [[Call]] and item()
array index out of bounds
integer index but not array index argument (treated as ID/name; for both when ID/name exists and doesn't exist)
fake array index (e.g. '01', '0xff', '-1', treated as an ID/name)
nonexistent name/ID

annevk · 2017-09-02T06:36:54Z

I think another thing is this being incorrect. I'd imagine the current specified behavior for that is to throw as it is for other IDL-defined members. Tested by https://github.com/TimothyGu/document-all-undefined/blob/master/index.html.

TimothyGu · 2017-09-02T06:49:34Z

@annevk Yes, that would be something to test as well. However, current Web IDL spec doesn't do such a check, nor does any of the browsers I tested.

This reverts commit b3f7d55.

TimothyGu · 2017-09-05T06:40:57Z

All, a new batch of stuff.

Reverted the change to return undefined in favor of the current spec (returning null) per Add HTMLAllCollection [[Call]], remove legacycaller special operation #2932 (comment) / d6ee730
Use Infra terminology: Add HTMLAllCollection [[Call]], remove legacycaller special operation #2932 (review) / 152c2c0
Make document.all.item(undefined) return the element with id "undefined" if there is one / ec03b38. See compat report.

TimothyGu · 2017-09-05T07:11:56Z

Tests are now available at web-platform-tests/wpt#7254!

domenic · 2017-09-07T21:36:43Z

source

@@ -7101,7 +7106,11 @@ interface <dfn>HTMLAllCollection</dfn> {
  readonly attribute unsigned long <span data-x="dom-HTMLAllCollection-length">length</span>;
  getter <span>Element</span>? (unsigned long index);
  getter (<span>HTMLCollection</span> or <span>Element</span>)? <span data-x="dom-HTMLAllCollection-namedItem">namedItem</span>(DOMString name);
-  legacycaller (<span>HTMLCollection</span> or <span>Element</span>)? <span data-x="dom-HTMLAllCollection-item">item</span>(optional DOMString nameOrIndex);
+  (<span>HTMLCollection</span> or <span>Element</span>)? <span data-x="dom-HTMLAllCollection-item-noargs">item</span>();
+  (<span>HTMLCollection</span> or <span>Element</span>)? <span data-x="dom-HTMLAllCollection-item">item</span>(DOMString nameOrIndex);


Given the browser incompatibility here, I think we can align with Safari (1/4) and do optional arguments. I will push a commit.

Oh, I see, Safari doesn't do optional arguments, it coerces undefined to a string. So... let's align with Firefox then.

No, wait, Safari tech preview does optional arguments.

Basically given the non-interop here let's converge on Safari tech preview, which has done the best job staying up to date with the spec. Still working on it.

domenic

So I changed this to align with the existing spec and with Safari. I think that's better than changing the spec out from under them, as they've been the ones that kept most up to date.

I'll work on the tests next.

annevk · 2017-09-08T09:38:03Z

source

+  data-x="">Function.prototype.call.call(document.all, null, "x")</code> will still search for
+  elements. (<code data-x="">document.all.call</code> does not exist, since <code
+  data-x="">document.all</code> does not inherit from <code
+  data-x="">Function.prototype</code>.)</p>


You mean HTMLAllCollection does not inherit from Function.prototype? How is HTMLAllCollection different from other interfaces in that regard? Can't be due to the custom [[Call]] or is it?

Yeah that's it.

TimothyGu · 2017-09-08T09:52:09Z

So I changed this to align with the existing spec and with Safari. I think that's better than changing the spec out from under them, as they've been the ones that kept most up to date.

Sure, but note: all browsers were consistently against the spec regarding undefined -- Chrome, Edge, Firefox, Safari -- before WebKit became spec-compliant. It kind of sucks, but I'm of the opinion that changing WebKit back to what it was before for this particular case is easier than changing all the other browsers.

This is different from #2932 (comment), where Safari TP and Firefox act the same way. The undefined behavior is only Safari TP.

domenic · 2017-09-08T15:41:29Z

I think it's more important to play nicely with browsers that have taken pains to be spec-compliant, than it is to conform to the majority of browsers which haven't been paying attention---at least in cases where we don't believe there to be any compat concerns.

Would either of you mind reviewing this and the tests, now that I've stuck my fingers in them and so it'd be good to get a review of my work?

TimothyGu · 2017-09-08T16:01:18Z

@domenic Your changes LGTM.

domenic · 2017-09-08T21:28:31Z

This ended up being editorial since we preserved the spec behavior (which was last touched in 93cc395). But I'll take this opportunity to file browser bugs now that @TimothyGu has contributed a great comprehensive test suite.

This makes document.all(undefined) behave like document.item(undefined). There was already a test for this, that got out of sync with the spec: https://github.com/w3c/web-platform-tests/blob/b6d42d0174fd24bda0802f8a55ee46381092ef80/html/infrastructure/common-dom-interfaces/collections/htmlallcollection.html#L209 #2932 Safari passes this test: https://wpt.fyi/html/infrastructure/common-dom-interfaces/collections/htmlallcollection.html?sha=13eaad17a4

tobie force-pushed the rm-legacycaller branch from 17b8b73 to 9865895 Compare August 16, 2017 17:21

tobie mentioned this pull request Aug 16, 2017

[[Call]] for objects w/o legacycaller whatwg/webidl#408

Closed

domenic assigned bzbarsky Aug 18, 2017

domenic added the do not merge yet Pull request must not be merged per rationale in comment label Aug 18, 2017

domenic reviewed Aug 19, 2017

View reviewed changes

tobie added 4 commits August 19, 2017 15:17

Add HTMLAllCollection [[Call]], remove legacycaller special operation

a234dd0

Depends on whatwg/webidl#412.

fixup! fix incorrect end tag

8473e7b

mixup! fix broken link and line breaks

0102dc9

fixup! Address comments

0917433

tobie force-pushed the rm-legacycaller branch from 6971c5b to 0917433 Compare August 19, 2017 13:19

TimothyGu reviewed Aug 19, 2017

View reviewed changes

TimothyGu assigned bzbarsky and TimothyGu and unassigned bzbarsky Aug 25, 2017

bzbarsky reviewed Aug 29, 2017

View reviewed changes

TimothyGu added 2 commits September 1, 2017 17:51

Fix some non-controversial spec issues

5b80212

Make document.all[.item] return undefined instead of null

b3f7d55

domenic added the needs tests Moving the issue forward requires someone to write tests label Sep 2, 2017

domenic reviewed Sep 2, 2017

View reviewed changes

TimothyGu added 3 commits September 5, 2017 12:25

Revert "Make document.all[.item] return undefined instead of null"

d6ee730

This reverts commit b3f7d55.

s/length/size/

152c2c0

Make item(undefined) act the same way as item("undefined")

ec03b38

TimothyGu mentioned this pull request Sep 5, 2017

Add more test cases for document.all web-platform-tests/wpt#7254

Merged

TimothyGu removed the needs tests Moving the issue forward requires someone to write tests label Sep 5, 2017

annevk mentioned this pull request Sep 7, 2017

Add a [[IsHTMLDDA]] internal slot to document.all #3015

Closed

annevk added the topic: javascript label Sep 7, 2017

domenic mentioned this pull request Sep 7, 2017

Remove legacy callers whatwg/webidl#412

Merged

domenic reviewed Sep 7, 2017

View reviewed changes

Align on Safari/spec semantics

4a517ec

domenic approved these changes Sep 7, 2017

View reviewed changes

domenic removed the topic: javascript label Sep 8, 2017

annevk reviewed Sep 8, 2017

View reviewed changes

domenic merged commit 0679448 into whatwg:master Sep 8, 2017

tobie mentioned this pull request Sep 8, 2017

Track PR #412 whatwg/webidl#434

Closed

7 tasks

TimothyGu deleted the rm-legacycaller branch September 9, 2017 00:27

TimothyGu mentioned this pull request Jan 13, 2018

Simplify HTMLAllCollection as allowed by usage data #1379

Closed

foolip mentioned this pull request Jan 24, 2018

Treat undefined as no arguments for document.all(undefined) #3392

Merged

Add HTMLAllCollection [[Call]], remove legacycaller special operation #2932

Add HTMLAllCollection [[Call]], remove legacycaller special operation #2932

Conversation

tobie commented Aug 16, 2017

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 commented Aug 19, 2017

TimothyGu 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

tobie Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzbarsky commented Aug 29, 2017

domenic commented Aug 29, 2017

bzbarsky commented Aug 29, 2017

tobie commented Aug 29, 2017

bzbarsky commented Aug 29, 2017

Choose a reason for hiding this comment

tobie commented Aug 29, 2017

bzbarsky commented Aug 29, 2017

TimothyGu commented Aug 31, 2017

TimothyGu commented Sep 1, 2017

domenic commented Sep 2, 2017

Choose a reason for hiding this comment

domenic commented Sep 2, 2017

TimothyGu commented Sep 2, 2017 • edited Loading

annevk commented Sep 2, 2017

TimothyGu commented Sep 2, 2017

TimothyGu commented Sep 5, 2017

TimothyGu commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Sep 8, 2017

domenic commented Sep 8, 2017

TimothyGu commented Sep 8, 2017

domenic commented Sep 8, 2017 • edited Loading

tobie Aug 25, 2017 •

edited

Loading

TimothyGu commented Sep 2, 2017 •

edited

Loading

domenic commented Sep 8, 2017 •

edited

Loading