Simplify HTMLAllCollection as allowed by usage data

[foolip]

As mentioned in #775 (comment) and #780 (comment) I added use counters to Blink, and those have now reached the stable channel:

• DocumentAllItemNoArguments
• DocumentAllItemNamed
• DocumentAllItemIndexed
• DocumentAllItemIndexedWithNonNumber
• DocumentAllLegacyCallNoArguments
• DocumentAllLegacyCallNamed
• DocumentAllLegacyCallIndexed
• DocumentAllLegacyCallIndexedWithNonNumber
• DocumentAllLegacyCallTwoArguments

A few older ones for comparison:

• DocumentAll
• DocumentAllLegacyCall

I said:

To commit myself to some interpretation of the use counter data in advance, I'd say that code paths showing ≥0.0001% usage on most (or all) days can stay, and those that round to zero on most (or all) days can go. This is entirely arbitrary, and based on the fact that chromestatus.com rounds usage to 0.0001% increments.

The counters in question are the *NoArguments and *IndexedWithNonNumber ones. Of these none have risen to 0.0001%, and in fact looking at internal data the highest of them is just 0.00000035%.

Based on this, I would suggest making the argument non-optional and dropping the string-as-index handling, which I think matches Gecko.

First things first, has anyone made any changes here yet? Would everyone be OK converging on the current spec plus the suggested changes, or do you prefer to stick to the current spec?

@DigiTec @nox @travisleithead @bzbarsky @smaug----

[domenic]

From what I can tell all of those usage counters are basically zero, not just those two. If we're planning usage-data related simplification, we could drop a lot of behavior.

[DigiTec]

I've made a ton of changes ;-) Here is our current WebIDL. Note, we've found at least once consumer of every single behavior within our corpus. So I would consider the below the interoperable behavior. Some of the consumers were internal.

    /// @specs HTML5
    /// @tags TreeNavigation
    interface HTMLAllCollection
    {
        readonly attribute unsigned long length;
        getter Element? (unsigned long index);
        getter (HTMLCollection or Element)? namedItem(DOMString name);
        legacycaller (HTMLCollection or Element)? item(optional DOMString nameOrIndex);
    };

[DigiTec]

We could probably remove the optional on item though. I said we found consumers for all of the behaviors, but that is the one behavior we did not find any consumers for.

[domenic]

It's noteworthy that @DigiTec's IDL matches the current spec exactly :). I guess removing the optional is tracked by the DocumentAllLegacyCallIndexedWithNonNumber and DocumentAllItemNoArguments use counters.

[bzbarsky]

So what I'm hearing is that observed usage for the edge cases @DigiTec's IDL aims to support is basically 0, yes?

[domenic]

As far as I can tell based on the usage data we could get rid of the .item() method entirely. We'd still want to support a name-based legacycaller I guess? Although I don't understand where the difference between DocumentAllLegacyCallNamed (0-0.001%) and DocumentAllLegacyCall (~0.006%) comes from.

[foolip]

I think the data looks inconsistent because the stable channel rollout hasn't finished yet, numbers are still increasing.

Internally, again, DocumentAllLegacyCall ≈ DocumentAllLegacyCallNamed + DocumentAllLegacyCallIndexed, which makes sense.

DocumentAllLegacyCallTwoArguments actually has 3× the usage of any of the *NoArguments and *IndexedWithNonNumber counters, and I think we should try removing that from Blink, so my preference is to align with Gecko on these other details, but reaching interop is the top priority.

The biggest risk here is with making the argument non-optional, but the usage of document.all.item() is literally zero, document.all() something like 0.000002% and it already throws in Gecko.

So, @DigiTec, do you have any appetite for trying this, or do you want to see literally zero usage of the code paths to consider it?

[DigiTec]

Per the IDL, we already removed the two argument overloads. We no longer support those.

And I can remove the "optional" flavor of all() and all.item().

We have test and app data for every other variation that claims enough usage that we wouldn't want to remove. We didn't implement use counters though, so our data is only from regression analysis.

[foolip]

That's good, the no-argument form does nothing useful and cannot be intentional.

By regression analysis, do you mean that you've found pages that would break if the string-as-index handling were removed, or that string arguments with only digits show up in static searches of source code?

[DigiTec]

Let me use code to be clear. We have confirmed hits of real apps/sites using:

document.all(number);
document.all(string);
document.all.string;
document.all.item(number);
document.all.item(string);

So we've seen breakage and its painful to track it down. A mistake I made in unifying our collections implementations back in March is still resulting in slightly broken web behaviors as we keep finding new obscure use cases.

If we are committed to removing the optional though I think that's a great change. I also need to rerun us against the existing web-platform-test based document.all test and commit back additional updates that we've made.

[foolip]

I've written test for the currently spec'd behavior in web-platform-tests/wpt#4037

The tests "legacy caller with no argument" and "item method with no argument" are relevant for whether we make that argument required. Current behavior:

• Chrome 55 returns undefined instead of null, but this is a trivial difference.
• Edge 14 requires the argument for legacy caller, and behaves like document.all.item(0) for the item method.
• Firefox 51 requires the argument for both
• Safari 10 is like Chrome.

When the current tests have been reviewed and merged, I will propose a change to make those arguments required.

[foolip]

I tried to make Blink match what I'd want the spec to say, but ran into exactly what @bzbarsky said in #775 (comment). The array index handling is in the bindings layer, so either you'd need custom bindings or duplication of it. Given the usage and the fact that it doesn't work in Firefox, I'd be pretty hopeful about making that change in Blink.

[nox]

What's the status on this?

[annevk]

I'd prefer we close this and stop tweaking legacy objects further unless there's a really compelling benefit in doing so.

[foolip]

I think it might still be worthwhile getting rid of the nameOrIndex bit that requires replicating JavaScript semantics for what an array index outside of the JS engine. Without custom bindings that is not very difficult, but rather odd. And Gecko doesn't do it. Blink will have to change a little bit whatever we do here.

@bzbarsky, how much effort would you like me to put in to avoid Gecko having to implement the current spec?

[bzbarsky]

That's a hard question to answer free-response. Can we make it multiple choice? What are the options?

@annevk There may be a compelling benefit here, if we can minimize changes to existing UAs and have a simpler behavior. If we can't then we can't of course.

[foolip]

That's a hard question to answer free-response. Can we make it multiple choice? What are the options?

Sure, let's see. This is what I was hoping some months ago to change Blink's IDL into:

/* [LegacyUnenumerableNamedProperties] not in Blink, ignoring for this discussion */
interface HTMLAllCollection {
    readonly attribute unsigned long length;
    getter Element? (unsigned long index);
    getter (NodeList or Element)? namedItem(DOMString name);
    legacycaller (NodeList or Element)? item((DOMString or unsigned long) nameOrIndex);
};

In other words, different from the current spec in these ways:

• NodeList instead of HTMLCollection, an existing and separate issue
• The item method's argument is not optional
• The same argument has type (DOMString or unsigned long) instead of the array index handling in prose (non-generated C++).

So, it's really the last two points here. I could do both, either, or none of them, those are the options.

[foolip]

Well, more time has passed, and per https://wpt.fyi/html/infrastructure/common-dom-interfaces/collections/htmlallcollection.html Safari now passes all but one test. It looks like this happened in https://trac.webkit.org/changeset/216851/webkit, i.e. after we made the last changes here.

Given that, I don't want to change the spec again, causing WebKit to have to change again. Since I filed this I'll also close it, but if anyone feels otherwise please ask for it to be reopened, or file a new issue for specific changes.

[TimothyGu]

See also #2932, where an updated version of this discussion took place.