Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enums that does not throw? #893

Open
saschanaz opened this issue Jun 13, 2020 · 12 comments
Open

Enums that does not throw? #893

saschanaz opened this issue Jun 13, 2020 · 12 comments

Comments

@saschanaz
Copy link
Member

saschanaz commented Jun 13, 2020

w3c/webauthn@a133711 (w3c/webauthn#1392)

Web Authentication just dropped their uses of enums because it's not forward compatible; older implementation throws when passing a newly added enum value. They now use DOMString and have proses to ignore unknown enum values:

Client platforms MUST ignore unknown values, treating an unknown value as if the member does not exist.

Should this be supported in IDL level? Is there any other specification that does the same thing?

@bathos
Copy link
Contributor

bathos commented Jun 13, 2020

Is there any other specification that does the same thing?

I think this is what pretty much every attribute reflecting an “enumerated” HTML content attribute does. The Web IDL attributes are typed as DOMString, and there are additional rules for their behavior defined in HTML, though many of these cases are a bit more complex (the validity of tokens is potentially dynamic vis a vis other state, if I understand right):

@saschanaz
Copy link
Member Author

saschanaz commented Jun 13, 2020

Yes, some HTML IDL attributes e.g. dir may be able to benefit from this feature.

@annevk
Copy link
Member

annevk commented Jun 14, 2020

IDL does not throw for enums passed to setters. It only does so for methods and constructors.

@jcjones could you elaborate a bit on w3c/webauthn#1376? It's a little surprising to see specifications deviate from the platform norms.

@jcjones
Copy link

jcjones commented Jun 15, 2020

Sure, @annevk: The bug was https://bugzilla.mozilla.org/show_bug.cgi?id=1573245 for the webcompat issue with Chrome. When they added the enum type and began using it, Gecko threw Uncaught (in promise) TypeError: CredentialsContainer.get: 'cable' (value of 'transports' member of PublicKeyCredentialDescriptor) is not a valid value for enumeration AuthenticatorTransport. while parsing the scripts.

All uses of WebAuthn are through passing dictionaries to the methods navigator.credentials.get or navigator.credentials.create, which presumably is why they throw.

It sounds like it would have been more future-proof if Credentials Management had instead returned an interface which could have been manipulated with setters, and had some sort of execute method.

@annevk
Copy link
Member

annevk commented Jun 16, 2020

Thanks, I can see how this might be useful for certain dictionaries, although normally I'd expect the caller to do some kind of feature testing. For IDL we should probably wait until more use cases show up around methods/dictionaries.

@bathos
Copy link
Contributor

bathos commented Jun 18, 2020

@annevk

IDL does not throw for enums passed to setters. It only does so for methods and constructors.

Thanks for clarifying/correcting this. I’d never noticed the special casing in create-an-attribute-setter and had assumed HTML not using IDL enums for reflected content attributes was about not-throwing. Is the use of DOMString for these in HTML just cause of the invalid/missing value defaults + sometimes-stateful valid value sets?

@annevk
Copy link
Member

annevk commented Jun 18, 2020

@bathos I think it's also that HTML has to deal with equivalent setAttribute() calls and those will always take DOMString.

@saschanaz
Copy link
Member Author

Just found that DataTransfer#dropEffect/DataTransfer#effectAllowed also have the same ignoring behavior.

@dontcallmedom
Copy link
Contributor

linking to the similar request in #491 for record keeping

@saschanaz
Copy link
Member Author

linking to the similar request in #491 for record keeping

Should we close either one?

@caraitto
Copy link

For developing FLEDGE [0], we'd be interested in enums that don't throw, since we have several "enums" that will grow as we add new functionality. Often, feature detection seems like it needn't be necessary, since ignoring unsupported enum fields on older browsers seems sufficient for our use-cases?

For now, due to this issue, we're using strings instead of enums.

[0] https://github.com/WICG/turtledove/blob/main/FLEDGE.md

aarongable pushed a commit to chromium/chromium that referenced this issue Feb 28, 2023
Fix the naming of the IDL members to match the naming-convention
documented in [0]. This is a breaking change, so I intend to cherry-pick
this CL after landing into M112 to avoid breaking the feature [1]
shipping in that release in stable. (There will be a breaking change in
other channels, but I believe that should be OK, especially given that
the M112 feature code only landed recently and shouldn't have any usage
yet).

Also, as part of this change, stop throwing exceptions for unsupported
enum fields -- ignore them instead. This lets FLEDGE API users use new
fields as they are introduced, without needing to provide feature
detection APIs, or require checking browser versions.

This CL doesn't use the "enum" WebIDL type because:

1) "enum" doesn't support forward compatibility as described above -- it
   always throws for unsupported fields [2].

2) The Chromium WebIDL bindings for enum have some backward
   compatibility issues -- you cannot have an enum with both "camelCase"
   and "dash-naming", since "fooBar" and "foo-bar" both result in
   kFooBar in the generated C++ enum, causing a compile error due to the
   duplication. So, if you already shipped "fooBar", you cannot also
   support "foo-bar" as an alias without switching to some other WebIDL
   type, like one of the string types.

[0] https://webidl.spec.whatwg.org/#idl-enums
[1] crrev.com/c/4219559, crrev.com/c/4261810
[2] whatwg/webidl#893

Change-Id: Id52412b7101837a5923359a927584da2d4c7d630
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4295454
Commit-Queue: Caleb Raitto <caraitto@chromium.org>
Reviewed-by: Russ Hamilton <behamilton@google.com>
Cr-Commit-Position: refs/heads/main@{#1111168}
caraitto added a commit to caraitto/turtledove that referenced this issue Mar 9, 2023
group-by-origin should be used instead to match the naming documented in [0].

Also, switch executionMode to "USVString" to avoid the compatibility issue with "enum" described in [1].

[0] https://webidl.spec.whatwg.org/#idl-enums

[1] whatwg/webidl#893
@zcorpan
Copy link
Member

zcorpan commented Jun 5, 2023

For integers we have Clamp and EnforceRange extended attributes to control throwing or changing the value. If specs want to use enums but not throw it seems useful to add an extended attribute to opt in to no throwing. Should it only work when setting a default value, where unknown values are converted to the default value?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

7 participants