Fix HTMLAllCollection

[annevk]

https://twitter.com/JustRogDigiTec/status/704539722362724352

<3

Tentatively assigning to @domenic since I like him so much.

[domenic]

@DigiTec can you clarify? Trying to piece this together from 140 character tweets is proving a bit difficult :). If I understand correctly:

• item()s arg needs to be optional since one browser supports document.all() and document.all.item() with no error. We'll want to test this cross-browser.
• HTMLCollection is not a correct return type (for both item and namedItem?) because Chrome/Firefox support document.all(index) and document.all.item(index)?

Blink's IDL has lots of FIXMEs for HTMLAllCollection: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/html/HTMLAllCollection.idl&sq=package:chromium

[domenic]

@so, this is not currently compatible cross browser, with no clear consensus. I am not sure we should change the current spec.

Test case: https://jsbin.com/kaxuqi/edit?html,output

	Firefox	Chrome	Edge	Safari
document.all()	error	ok	error	ok
document.all.item()	error	ok	ok	ok
document.all.namedItem()	error	error	error	ok
document.all(0)	null	HTMLHtmlElement	HTMLHtmlElement	HTMLHtmlElement
document.all["0"]	HTMLHtmlElement	HTMLHtmlElement	HTMLHtmlElement	HTMLHtmlElement
document.all("0")	HTMLSpanElement	HTMLHtmlElement	HTMLHtmlElement	HTMLHtmlElement
document.all.item(0)	HTMLHtmlElement	HTMLHtmlElement	HTMLHtmlElement	HTMLHtmlElement
document.all.item("0")	HTMLSpanElement	HTMLHtmlElement	HTMLSpanElement	HTMLHtmlElement
document.all("foo")	HTMLCollection	NodeList	HTMLCollection	NodeList
document.all.item("foo")	HTMLCollection	NodeList	HTMLCollection	NodeList
document.all.namedItem("foo")	HTMLCollection	NodeList	HTMLDivElement	NodeList
document.all.namedItem("0")	HTMLSpanElement	HTMLSpanElement	HTMLSpanElement	HTMLSpanElement
document.all.foo	HTMLCollection	NodeList	HTMLCollection	NodeList

We generally have a 2/2 browser split on these questions, with the exception that item() should have its argument become optional. So at this point that is the only change I would make; we would keep HTMLCollection as the return type.

@DigiTec, did this come up as a result of any compat work? E.g. is there evidence that Chrome's behavior is relied on by websites? That would help break the 2/2 split tie.

[DigiTec]

@domenic Just catching up on this. We apparently work offset shifts ;-)

We are working through this issue #210 and we have bandwidth to "get it right" so that is why these questions are coming up. Having a 2/2 browser split is not ideal. Especially if 2 of the larger market shares are split.

all() and all.item() are due to legacaller on the item getter. So the optional on that parameter would be required for Chrome/Edge/Safari to agree (and therefore Mobile and Desktop would have a majority sharehold and FireFox could be quick to fix).

Most of this request is about fixing the IDL itself. The original IDL did not compile for us since it specified two conflicting (in our parser) implementations of item that couldn't be resolved. So we prefer an option that makes this a single line.

legacycaller getter (HTMLCollection or Element)? item((unsigned long or DOMString) nameOrIndex);

• legacycaller means that all() works
• getter means that all[] works
• (HTMLCollection or Element)? means that it can return either of those, which seems to be universally true.
• item means that item and all work basically the same here and that item is the prototype for how the legacycaller on all works (which again seems to be the case mostly).
• optional (unsigned long or DOMString) nameOrIndex allows all() to accept strings as well as indices and also allows for all() and item() to returned undefined.

The current spec looks like this
getter Element? item(unsigned long index);
(HTMLCollection or Element)? item(DOMString name);
legacycaller getter (HTMLCollection or Element)? namedItem(DOMString name);

Since the only legacycaller is namedItem it means that all can only accept strings
item is an overloaded method which we could resolve as written above, but it is only to change the return type from being or'ed to being more specific. The simpler webidl I proposed seems better.
We'd remove legacycaller and getter on namedItem because it is already covered by item.

Compat usually breaks when you lock things down, not when you open them up. For Edge we'd have to

• fix the errors due to optional for all() and item()
• fix namedItem to return an HTMLCollection.
• We'd also need to fix the namedItem case where something isn't found to return null instead of undefined (I think we return undefined).
• Fix our HTMLAllCollection to no longer derive from HTMLCollection (still does today)

If we choose not to change the spec in this way to match the Chrome/Safari behavior more closely then we would like, if possible, to get a committed bug to correct the behavior for Chrome/Safari to avoid future interop issues.

Did this result from compat work? No, it resulted from Interop work found while tearing HTMLAllCollection from HTMLCollection. Note, this may result in some HTMLCollection feedback as well.

[nox]

legacycaller getter (HTMLCollection or Element)? item((unsigned long or DOMString) nameOrIndex);

I'm pretty sure this is not spec-compliant and that both Gecko and Servo choke on it.

What about an anonymous legacycaller?

getter Element? item(unsigned long index);
(HTMLCollection or Element)? item(DOMString name);
getter (HTMLCollection or Element)? namedItem(DOMString name);
legacycaller (HTMLCollection or Element)? ((unsigned long or DOMString) nameOrIndex);

[DigiTec]

@nox Do you mean not spec compliant per WebIDL standards? It did pass our parser and even went all the way through TypeScript d.ts creation through our XML format and nothing flagged along the way.

What restrictions would disallow the one liner from working? I couldn't find anything during a spec read.

[nox]

http:https://heycam.github.io/webidl/#idl-indexed-properties

Indexed property getters must be declared to take a single unsigned long argument. Indexed property setters must be declared to take two arguments, where the first is an unsigned long.

getter type identifier(unsigned long identifier);
setter type identifier(unsigned long identifier, type identifier);

http:https://heycam.github.io/webidl/#idl-named-properties

Named property getters and deleters must be declared to take a single DOMString argument. Named property setters must be declared to take two arguments, where the first is a DOMString.

getter type identifier(DOMString identifier);
setter type identifier(DOMString identifier, type identifier);
deleter type identifier(DOMString identifier);

getter type (DOMString identifier);
setter type (DOMString identifier, type identifier);
deleter type (DOMString identifier);

[domenic]

OK, cool. So it sounds like you're not too concerned about NodeList vs. HTMLCollection? Then we can focus on the other issues.

I've updated https://jsbin.com/kaxuqi/edit?html,output to include document.all(0) and document.all.item(0) so we can figure out whether browsers allow indices (would return HTMLHtmlElement) or convert to strings (would return null since there's no element with id "0"). The table above is updated too. It looks like there is a majority that does allow indices (Firefox being the exception). So we should fix that.

I agree we also want to allow document.all.item() (i.e. no arguments) since that's a majority opinion.

To achieve these fixes I think @nox's solution is going to be the best one. It seems pretty clean to me.

[nox]

So:

getter Element? item(unsigned long index);
(HTMLCollection or Element)? item(optional DOMString name);
getter (HTMLCollection or Element)? namedItem(DOMString name);
legacycaller (HTMLCollection or Element)? ((unsigned long or DOMString) nameOrIndex);

With an optional argument in the non-getter item operation.

[domenic]

@DigiTec how does that sound to you? Happy to make the change today if you agree.

[DigiTec]

The getter/setter restrictions seem odd to me from the spec, but I'll pass the rest through our parser later today and if I run into any issues will let you know. I'm guessing the rationale is for code auto-gen and overload resolution to work properly the restrictions need to be in place.

@domenic NodeList "seems" wrong since you are disallowing authors from poly-filling APIs properly and the spec clearly states HTMLCollection and has for a very long time. Would that be an area Chrome would be willing to take a bug? The HTMLAllCollection WPT tests will need to encapsulate this behavior and I think the right test for return type should be HTMLCollection and thus Chrome would fail. Is that okay?

NodeList doesn't have namedItem and a bunch of other stuff. There seems to be a lot of bugs here though across browsers and maybe even within the spec.

per HTMLCollection semantics this should work and it does work in Edge/FireFox
console.log(document.all.item("foo").namedItem("foo"))

[domenic]

Yeah, Chrome should be happy to take a bug and fail the relevant WPTs. Maybe @foolip can confirm since I believe he was the one who added the FIXME comments to the IDL.

[DigiTec]

// Allows item(0) and [0] to return element or null`
getter Element? item(unsigned long index);
// Allows item("foo") to work and return collection, element or null
(HTMLCollection or Element)? item(optional DOMString name);
// Allows namedItem(str) and [str] to return collection, elment or null
getter (HTMLCollection or Element)? namedItem(DOMString name);
// Had to add optional here for all() to work
legacycaller (HTMLCollection or Element)? (/*added*/optional (unsigned long or DOMString) nameOrIndex);

I think we need anonymous getter's and not item mapping. Why? Because [num] (getter syntax) returns undefined while item(num) returns null. So these can't be overloads of one another.

console.log("non-existent", document.all(5000), document.all.item(5000), document.all[5000])

Probably same for namedItem since that is even more weird (it doesn't ever return null in Chrome, always undefined).

Test cases here (2 more added to the top list)
https://jsfiddle.net/cjjhj2ms/

QQ: Is it possible that we can't fully describe the right behavior in the WebIDL? That the undefined vs null behavior cannot be captured in the current specification of the language?

[nox]

Please use backticks in your message because it is quite hard to read as such with Markdown not helping.

[nox]

I think we need anonymous getter's and not item mapping. Why? Because [num] (getter syntax) returns undefined while item(num) returns null. So these can't be overloads of one another.

Not a problem. The undefined is returned by the fact that the passed num was outside the set of supported property indices:

If an indexed property getter was specified using an operation with an identifier, then the value returned when indexing the object with a given supported property index is the value that would be returned by invoking the operation, passing the index as its only argument.

See how the operation isn't used at all if the index wasn't supported to begin with.

The platform already relies on this in CSSStyleDeclaration: the operation associated with the getter returns an empty string, while using the getter syntax it returns undefined.

var div = document.createElement("div");
console.log(div.style["foo"]);
console.log(div.style.item("foo"));

[DigiTec]

Adding @travisleithead as well.

If the odd behaviors of undefined are necessary then we likely need to define all parts of this API separately (no collapsing).

Two anonymous legacycaller's one for the numeric and one for string
Two anonymous getters one for numeric and one for string
item and namedItem as their own thing

I'm not even sure if that is entirely possible.

We also believe that to return undefined for some set of scenarios we need to or in void to some of the return value lists.

(void or HTMLCollection or Element) // Used when we need undefined
(HTMLCollection or Element)? // Used when we need null

I also just saw @nox respond with some new information. Let me drop this comment as is and then reflect on his updates. I think they do make some thing simpler, but still the fact that Chrome has dramatically different null vs undefined behavior in similar conditions for string vs num would likely still need something calling it out for THAT condition. That might at least mean that we need different syntax for DOMString vs unsigned long indexing.

[nox]

Returning undefined is mandated by the specification of getters, while returning null is mandated by the specification of the associated operation, nothing WebIDL cannot currently handled, so I don't think they should be split.

There is nothing forbidding either Chrome or Edge to write their IDL definitions differently as long as the observable effects are the same though.

[DigiTec]

The problem is this test:

console.log("non-existent num", document.all(5000), document.all.item(5000), document.all[5000])
console.log("non-existent str", document.all("whammy"), document.all.item("whammy"), document.all["whammy"])

This returns

null, null, undefined
undefined, undefined, undefined

Which means when using DOMString on item that the value is not (HTMLCollection or Element)? it is instead (void or HTMLCollection or Element). So this is separate from getter syntax altogether. So while the last value in the list can be defined by the getter prose, it doesn't handle the return type for the non getter case.

Also, the legacycaller acts like the method, while the getter acts differently. I'm not sure if the spec clarifies that. That legacycaller is actually more like the method and less like the getter. I know that in EdgeHTML we map legacycaller -> GetOwnProperty (edit: Which I should clarify is the getter). So this can lead to pretty big interop discrepancies.

[nox]

So your first line (null, null, undefined) is just the usual business I described.

As for the second line… Now that's a problem. We can't put void in an union so we can't do what you suggested. :(

[DigiTec]

Ugh, that was the only thing @travisleithead and I could come up with :-(

[nox]

getter Element? item(unsigned long index);
any item(optional DOMString name);
getter (HTMLCollection or Element)? namedItem(DOMString name);
legacycaller any ((unsigned long or DOMString) nameOrIndex);

[nox]

I think this cover all the cases, but we have to use any.

[domenic]

It's null, null, undefined in Firefox if that helps.

[nox]

Oh! I didn't check. That combination is just OK too with the current WebIDL rules and my previous snippet without any. What does Edge do? Oh you just said it just returns undefined all three times.

[DigiTec]

Right now edge does

undefined, null, undefined // per my notes about legacycaller mapping to GetOwnProperty 
undefined, null, undefined // for the same reason

We auto-gen caller entry points and so GetOwnProperty was the only thing we could easily map to and it "works" in the majority of cases. Also legacycaller is now pretty much only on HTMLAllCollection. We dropped support for it in all other cases I believe with the following caveats:

• Until I make this change HTMLCollection has it because HTMLAllCollection derives it
• HTMLOptionsCollection has it, but I believe that is a bug.
• HTMLFormsElement has it, but that is DEFINITELY a bug
• HTMLSelectElement has it because it used to be the return value for the options property

I think other browsers also support it for HTMLObjectElement and HTMLEmbedElement for legacy reasons that we no longer have.

Willing to custom generate the CallerEntryPoint instead of auto-generation if it really is true that legacycaller is ONLY on the HTMLAllCollection now.

[nox]

I see that document.all() returns undefined too, so I think using any and a separate legacycaller is the way to go:

getter Element? item(unsigned long index);
(HTMLCollection or Element)? item(optional DOMString name);
getter (HTMLCollection or Element)? namedItem(DOMString name);
legacycaller any (optional (unsigned long or DOMString) nameOrIndex);

[DigiTec]

We can mark it any, but then we need to clarify the expected values in the specification. I also have to come up with a TypeScript solution that constrains the types to only those expected to be returned. any is too tolerant and doesn't allow for tool chains to provide any intellisense. As noted earlier though, this is now ONLY HTMLAllCollection.

With this change would we take the stance that all 3 browsers are compliant even though they have different return values? Or would we lock down the WPT tests to one of the behaviors? There are some clear behaviors we CAN lock down (such as Edge not returning an HTMLCollection in the namedItem case when it should be, which I have fixed locally). Tests for null vs undefined is really not a huge deal I guess since basic truthy/falsey still works (though strict comparisons would fail) and we could choose to accept both values.

Of all the proposals I'm liking this one the best so far. Its close enough and we can wrangle the rest. Here is what the Edge webIDL would look like (we don't yet have overload resolution) and I had to add support for anonymous legacycaller since this is the first time we've had this.

any item(optional DOMString name);
[msOverload] getter Element? item(unsigned long index);
getter (HTMLCollection or Element)? namedItem(DOMString name);
legacycaller any (optional (unsigned long or DOMString) nameOrIndex);

[domenic]

With this change would we take the stance that all 3 browsers are compliant even though they have different return values? Or would we lock down the WPT tests to one of the behaviors?

No, the spec would require an unambiguous answer defined in prose. Return types in Web IDL are pretty useless anyway; they don't impose any normative requirements that the prose does not already. (That is, their only function from a spec perspective is generating "spec segfaults" when the prose tells you to return something other than the Web IDL return type.)

What about just going with

getter Element? item(unsigned long index);
(HTMLCollection or Element)? item(optional DOMString name);
getter (HTMLCollection or Element)? namedItem(DOMString name);
legacycaller (HTMLCollection or Element)? ((unsigned long or DOMString) nameOrIndex);

and saying that nobody is allowed to return undefined (except via the getters)? That might be against the majority (haven't run full tests), but it seems most sane, and the only edge-case difference is null vs. undefined.

[domenic]

or similar, while weird, but valid. I use the -1 off the front of the list to get back null/undefined. But I could errantly now get an extra element whose id is "-1".

As I mentioned in #780 (comment) this would require changing the type from unsigned long to something else (long? long long? unrestricted double?). Is that really what you want? It's another 2/2 split issue, again.

[foolip]

Getting exactly the ToUint32 behavior for any value is going to be impossible when operating on the result of ToString, I think. It seems entirely within reach to find something that's good enough for compat, though.

Here are some more tests for string-to-int conversion:
http:https://software.hixie.ch/utilities/js/live-dom-viewer/saved/3956

Instead of a table I'll try to summarize in prose.

Chrome interprets the string as an array index if it consists of only digits and is in the range [0, 4294967294]. (That's 2³²-2 because an optimized overflow check.) If the string begins with a 0, that must also be the only digit. No - or + is accepted. Implemented in StringToArrayIndex.

Safari is like Chrome, and I think it's implemented in parseIndex. The only difference is with document.all["100"] and similar, where it seems to fall back to named property lookup. I think we can ignore this quirk.

Firefox only does the conversion for document.all["..."], where numbers up to 2³¹-1 are treated as array indexes. It also doesn't allowing leading +, - or extra zeros. This is presumably not custom code for HTMLAllElement at all, maybe @bzbarsky or @Ms2ger can confirm.

Edge is rather different. It seems like anything that can be interpreted as a number takes the indexed lookup path. Leading +, - and extra zeros are all tolerated. Even huge numbers that exceed 2⁶⁴ (tested separately) are OK.

[foolip]

OK, so #775 (comment) was about strings as the input, on to cases like document.all.item(-2) discussed in #780 (comment) and onward.

Tests: http:https://software.hixie.ch/utilities/js/live-dom-viewer/saved/3958

Chrome and Safari will never treat negative numbers as indexes, and instead does lookup by string.

Edge will always treat negative numbers as indexes, and return null/undefined.

Firefox will treat negative numbers as indexes only for document.all.item(-1) or similar. Again, the document.all[-1] form is probably not specific for HTMLAllCollection.

I'll comment in the pull request with what I think we should do.

[bzbarsky]

Firefox doesn't have any custom code for this stuff at all. It just has the IDL in http:https://mxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLAllCollection.webidl and normal bindings for that IDL.

I'm not sure what problem you're trying to solve with this discussion of "convert it all to a string and then parse indices from that string", but I don't think we want to go there....

[annevk]

@bzbarsky if the bindings are normal, why does it say /* Emulates undefined through Codegen.py. */?

[bzbarsky]

I meant normal bindings in terms of method behavior. The "emulates undefined" bit is to satisfy the prose HTML spec requirement for typeof and ToBoolean and doesn't affect the behavior of property access or methods on the interface.

[foolip]

@bzbarsky

AFAICT, Gecko's IDL is equivalent to the current spec, only arranged differently.

The new thing at the IDL level in the current #780 proposal is the legacycaller form for unsigned long index, because document.all(0) works roughly like document.all[0] in all all engines except Gecko.

Then there's the curious string-to-int handling, added because document.all("0") also gets the first element of the collection in all engines except Gecko, and document.item.all("0") does too in Blink/WebKit, but not Edge.

If document.all("0") could be made to work with IDL only, that would be nice, and if not, it's just as well to give document.item.all("0") the same behavior, I think.

[bzbarsky]

Ah, I see.

Making document.all(0) work seems pretty reasonable, yes.

document.all("0") is harder. Do the other browsers actually do string to integer parsing? Or do they convert the call into a JS get instead and then just go through the normal codepath that would treat 0 and "0" as identical property names (because in JavaScript they are)? Anyway, I don't think there's a way to do this purely via IDL if we want "0" to work like 0....

I'm not sure what you mean by document.item.all("0"); there is no such thing.

[foolip]

Oops, I mean document.all.item("0").

Currently Blink's string-to-integer parsing is in custom bindings and not very explicit. #775 (comment) and #775 (comment) touch on this, but basically for both item and the legacy caller, it first tries to interpret the argument as an array index, and only if that fails does it fall back to named lookup. I can't see how to preserve exactly this behavior without custom bindings. Of course, it probably doesn't need to be exactly this for compat.

[bzbarsky]

Firefox only does the conversion for document.all["..."], where numbers up to 2^31-1 are treated as array indexes. It also doesn't allowing leading +, - or extra zeros. This is presumably not custom code for HTMLAllElement at all.

Oh, missed this part. So yeah, for that case Gecko is just doing the ES6 definition of "array index" (see http:https://www.ecma-international.org/ecma-262/6.0/#sec-object-type ) but then it ends up going through an int32_t at some point; there are some XXX comments about it. This is all (except for the int32_t silliness) specced in IDL at http:https://heycam.github.io/webidl/#dfn-array-index-property-name which aims to match the ES6 "array index" definition. Importantly, this is done on the binding layer before we ever get into the prose; by the time we get into prose we have either an index or a string, and even more annoyingly it's a string of a type for which we can't easily run the "is an array index" algorithm (which operates on property ids anyway, not on strings).

I should also note that we don't have custom bindings (in the sense of things that aren't already specced in IDL) and would really rather avoid adding them...

[foolip]

We would also like to get rid of as much custom bindings as possible!

The least bad idea so far is to have an extra string-to-int in prose. The current one is pretty good, but it would be fantastic if it were exactly in sync with ES6 "array index", and it looks like the only difference is that redundant leading zeros shouldn't be allowed ("canonical numeric String") and the range should be 0 ≤ i < 2³²−1, which is off-by-one compared to #780.

[foolip]

FWIW, I tried implemented string parsing equivalent to ES6 "array index" outside of bindings and the results look really promising, giving the same behavior for document.all[x] and document.all(x) for the tests in #775 (comment) and http:https://software.hixie.ch/utilities/js/live-dom-viewer/saved/3958

[domenic]

@foolip would you mind speccing it by pushing new commits to #780? Assuming that's the consensus we come to...

[foolip]

Sure, I have updated #780 to match what I implemented. I reverted back to an earlier state because the unsigned long or DOMString wouldn't make much sense in combination with that.

Of course, this isn't meant to end the discussion, we can revert back and forth for however long it takes to figure this out.

[foolip]

In https://codereview.chromium.org/1756963002/ I'm adding use counters to Blink. The test shows pretty clearly what is being measured and doesn't require any understanding of Blink bindings, so if anyone sees some interesting behavior left untested, please let me know.

I'd also like to reiterate that we don't have to wait for this (or similar) data before proceeding, if someone wants to change sooner there are reasonable guesses we can make about what's likely to work out. Otherwise, I'll report back here when the results are in, some time in June.

[DigiTec]

Thanks @foolip I'll report back sometime next week on exactly where we end up landing as well. I think we will end up matching the spec for the HTMLAllCollection. Which should also mean I'm able to provide good WPT tests for the issue that I logged there based on the latest prose.

For the "index" parsing behavior our plan is currently to use something like strtoul (though not exactly that) and if parsing fails, fall back to string indexing. It may be more subtle, since it might be our current index parsing + added range check instead.

[domenic]

Great :D. #780 looks good to me after @foolip's changes, so I guess we should go for another round of signoffs. @DigiTec is looking into it next week. @bzbarsky, I guess it might not be completely to your liking since it requires checking if a string is a valid "all"-index... @nox, thoughts?

[nox]

SGTM with @foolip changes too.

[bzbarsky]

I'm not super-happy with it, but if we really do want to support item("0") and the like then there's really no other way around it.

I would somewhat prefer it if we didn't add a third definition of what it means to be an index, given that ES and Web IDL already have such definitions. It's a bit unfortunate that those definitions have to work on ES strings and not on IDL strings, so I see why we're doing things this way, but I would also vastly prefer it if we could somehow not end up with the various definitions getting out of sync with each other.

I'm not convinced about the term "all-indexed element", because there's nothing there that's a property of the element, unlike the "all-named" case. Why is the "indexth element in the collection" wording we use for NodeList and HTMLCollection not ok here?

[foolip]

Oh, it's in WebIDL too :( Would it help if the note points to http:https://heycam.github.io/webidl/#dfn-array-index-property-name as well?

[bzbarsky]

@foolip It would help some, but the bigger problem is what happens if ES6 or Web IDL changes its definition of index (something ES has definitely talked about doing, so this is not theoretical).