Investigate banning nullable enums

[annevk]

I cannot think of a single scenario where that would be preferable over an additional enum value. Of course, it might be too late to change this.

[foolip]

Should optional dictionary members of enum type also be required to have a default value? Otherwise undefined can mean something other than one of the enum values.

[annevk]

I think that would break Request objects.

[bzbarsky]

Optional enum values (in dictionaries, and as arguments) may make sense as a "use the default" kind of thing, especially when the default is computed in a complicated way from other arguments or dictionary members, or depends on prior state or whatnot.

I looked at where nullable enums are used in Gecko's IDL. Apart from codegen tests and some nonstandard APIs, they are not used.

In the nonstandard APIs, there are some cases in which null is used to indicate "object is not in a state that can provide this data right now". And boy are there a lot of uses like that. :( Those APIs were all written by JS hackers, for what it's worth, so it's a data point in terms of what "authors" expect.

[foolip]

I found a nullable enum in Blink and Gecko:
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/modules/peerconnection/RTCSessionDescription.idl
https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/RTCSessionDescription.webidl

Found via https://crbug.com/662898 because it doesn't match the spec.

[bzbarsky]

Yes, I found that too and filed https://bugzilla.mozilla.org/show_bug.cgi?id=1313966 on Gecko at the time. I didn't include it in #213 (comment) as a result.

[foolip]

Do you recall if there were any others that are web exposed and could be risky to change?

[bzbarsky]

There aren't any.

[foolip]

Great, thanks for checking!

[foolip]

Here's a new one: https://wicg.github.io/media-capabilities/#screen-extension

The colorGamut attribute is a nullable enum. In this case, I suppose that the empty string could be used instead, but would that be an improvement? Note that luminance is also nullable but of interface type.

[foolip]

@mounirlamouri ^

[annevk]

Yeah, I don't see why using two types is better than a single type for colorGamut.

[tobie]

I suppose that the empty string could be used instead, but would that be an improvement?

From a JS dev perspective, empty strings as falsy values in enums are icky (see canPlay).

[annevk]

@tobie that's also an argument against null though.

[tobie]

that's also an argument against null though.

Well, not really. null in that case pretty much means: "object is not in a state that can provide this data right now" as @bzbarsky mentioned above, or "this has not be set yet." The empty string on the other hand is just super weird (and hacky in a way I wouldn't want to see Web APIs hacky). So if I had no other consideration in mind than "how does that feel to a Web dev," I'd say null is OK, but "" should be banned.

[annevk]

Null is just as falsy though, so if you want to avoid that you should do something else.

[tobie]

Null is just as falsy though, so if you want to avoid that you should do something else.

Well, all of this is pretty subjective anyway. The point I was trying to make is that replacing null by "" isn't an improvement from a JS dev perspective. I'd even argue it's worse. So (still from a JS dev's perspective—as I said earlier, there might be other sound reasons to choose differently), my suggestion would be to keep nullable enums (if those are needed) but to disallow empty strings in enums.

[foolip]

I noticed that colorGamut from #213 (comment) is no longer nullable.

[foolip]

Found some more nullable enums:
https://drafts.csswg.org/web-animations/#dictdef-basecomputedkeyframe
https://drafts.csswg.org/web-animations/#dictdef-basepropertyindexedkeyframe
https://drafts.csswg.org/web-animations/#dictdef-basekeyframe
https://w3c.github.io/web-nfc/#dom-nfcwatchoptions
https://w3c.github.io/webrtc-pc/#dom-rtcicecandidate
https://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver

Edit: I found these by elaborate grepping in https://github.com/tidoust/reffy-reports/tree/master/whatwg/idl: for name in `git grep -hoE 'enum \w*\s*\{' | awk '{print $2}'`; do git grep -F "$name?"; done

[bzbarsky]

https://drafts.csswg.org/web-animations/#dictdef-basecomputedkeyframe
https://drafts.csswg.org/web-animations/#dictdef-basepropertyindexedkeyframe
https://drafts.csswg.org/web-animations/#dictdef-basekeyframe

Yeah, this one is a bit weird. It might have been nicer to have an explicit "default" value or something... @birtles do you recall why null was picked here? I should have asked this in https://bugzilla.mozilla.org/show_bug.cgi?id=1429671...

https://w3c.github.io/web-nfc/#dom-nfcwatchoptions

This one hasn't had wide review yet, so hard to say what the story there is. The prose talks about a default value of "", the IDL has no default value at all. I would assume that this IDL has nothing to do with whatever reality exists here, if any.

https://w3c.github.io/webrtc-pc/#dom-rtcicecandidate
https://w3c.github.io/webrtc-pc/#dom-rtcrtptransceiver

I think here null is used to mean "we don't know". Not sure why that couldn't be an enum value, but its possible that these specs don't fully control the value set of these enums or something... @jan-ivar do you know?

[foolip]

@alexshalamov, can you comment on the Web NFC case? Since it's an optional dictionary member, I believe just using undefined to mean missing would be fine. I've filed w3c/web-nfc#149.

[birtles]

https://drafts.csswg.org/web-animations/#dictdef-basecomputedkeyframe
https://drafts.csswg.org/web-animations/#dictdef-basepropertyindexedkeyframe
https://drafts.csswg.org/web-animations/#dictdef-basekeyframe

Yeah, this one is a bit weird. It might have been nicer to have an explicit "default" value or something... @birtles do you recall why null was picked here? I should have asked this in https://bugzilla.mozilla.org/show_bug.cgi?id=1429671...

We could have a "default" value, I guess. The reasoning at the time is mostly covered in w3c/csswg-drafts#2072 (comment), where consistency with offset, which uses null for "auto"-like values, seems to be the main reason. (The second reason listed there concerns whether we represent auto-values by lack of a value or an explicit sentinel value--but either null or "default" would work for that).

That is you can write:

elem.animate({
  backgroundColor: ['red', 'green', 'blue', 'orange', 'purple'],
  offset: [0, null, null, 0.95, 1],
}, 1000);

(where null means, "Calculate the offset for me").

So, it made sense that composite should use the same notation (where null means, "Use the composite mode specified on the effect"):

i.e.

elem.animate({
  backgroundColor: ['red', 'green', 'blue', 'orange', 'purple'],
  offset: [0, null, null, 0.95, 1],
  composite: ['replace', null, null, null, 'replace'],
}, 1000);

With a default value that would become:

elem.animate({
  backgroundColor: ['red', 'green', 'blue', 'orange', 'purple'],
  offset: [0, null, null, 0.95, 1],
  composite: ['replace', 'auto', 'auto', 'auto', 'replace'],
}, 1000);

Which could work.

We haven't shipped composite modes in any browser but we are planning to ship getKeyframes() in Firefox 63 which exposes BaseComputedKeyframe, i.e. will return objects with composite set to null. However we'd still be in time to change this if we want to.

[marcoscaceres]

“auto” looks way more legible and obvious, fwiw. I’d encourage you to change it.

[birtles]

Ok, I've filed a PR to change Web animations and written a patch for Firefox and wpt should that PR be accepted.

[birtles]

I've updated Web Animations (and Firefox implementation) so that it no longer uses nullable enums.