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

Enum value naming and consistency with the web platform #1725

Open
annevk opened this issue Mar 15, 2023 · 28 comments
Open

Enum value naming and consistency with the web platform #1725

annevk opened this issue Mar 15, 2023 · 28 comments

Comments

@annevk
Copy link
Member

annevk commented Mar 15, 2023

An issue came up when designing a WebDriver extension for the HTML Standard. Apparently some WebDriver extensions use enum value naming that is inconsistent with https://w3ctag.github.io/design-principles/#casing-rules.

E.g., w3c/secure-payment-confirmation#219.

Can we still address that? This is blocking web-platform-tests/wpt#35792 so if this issue could be expedited I'd appreciate it.

cc @javifernandez @stephenmcgruer @dontcallmedom @foolip

@jgraham
Copy link
Member

jgraham commented Mar 16, 2023

I don't quite understand the request here.

WebDriver can't really change existing APIs at this point.

We always use camel case for property names. That isn't what's in the TAG document, but I believe it's totally consistent throughout the spec.

For values we are less consistent. For example error codes just use normal text e.g. "no such element". Arguably we sometimes use all lowercase values e.g. "fullscreen", although arguably that's just treating it as a single word (and also I think that value isn't used anywhere). BiDi uses this for type names when serializing e.g. "htmlcollection". We probably just about have the ability to change that. We don't really use dash separated anywhere; BIDi has one instance which is for realm types e.g. "dedicated-worker". Given that afaik nothing supports non-Window realms yet we probably can change that. There is also one case where we use camel case for property values: in action types e.g. "pointerMove". That's also widely deployed and can't reasonably be changed. Underscore separated values are totally unused.

So insofar as there's a WebDriver style, it's probably space-separated values, or all-lowercase values.

@jgraham
Copy link
Member

jgraham commented Mar 16, 2023

Oh and BiDi "method" names are using a consistent but slightly different convention (modulename.methodName) because they're supposed to look like JS methods.

@javifernandez
Copy link

I don't quite understand the request here.

I guess the main problem is that WebDriver extension commands are usually defined in the spec of the feature that fist had the need of automating a particular behavior. Hence, it may happen that the WebDriver's syntax may conflicts with the spec's syntax. This is the case of the RPH Registration Mode defined in the HTML spec.

It's worth mentioning that despite the argument of following the Design Principles makes sense, there were not strong opinions against following a WebDriver specific criteria if needed. So, this issue is to get an agreement about how to resolve possible conflicts between the WebDriver syntax and the spec's where extensions commands are defined. This agreement could be used in future discussions.

Perhaps it'd be a good idea to add something in the WebDriver spec's chapter about the extension commands, in order to ease the resolution of other potential conflicts in the future.

@javifernandez
Copy link

javifernandez commented Mar 31, 2023

So insofar as there's a WebDriver style, it's probably space-separated values, or all-lowercase values.

So If I've understood it correctly @jgraham suggests to use "all-lowercase" for enum values for Web Driver extensions commands defined in external specs. Is this correct ? Bear in mind that this would imply changes in both, the HTML and Secure Payment Confirmation specs.

It's worth considering that this has been discussed already in the issue 219 of the Secure Payment confirmation spec. Back then, it was decide to use camelCase instead. Recently @foolip commented that if we get an agreement between WebDriver implementers, we should follow it.

It'd be interesting to know whether @stephenmcgruer would be willing to change the Secure Payment Confirmation spec to align with this proposal. As far as I know only Chrome implements such feature. From Chrome we have @domenic's opinion about the extension command added to the HTML spec, who slightly prefer to use dash-separated (following the Design Principles and change Secure Payment Confirmation spec.

Finally, although Safari doesn't implement either of these 2 driver extension commands, it'd be ideal to know @annevk's opinion about this issue. His opinion is also relevant for the HTML spec.

From a practical perspective, using camelCase would be the easier approach, in terms of implementation effort. We wouldn't need to change at all the Secure Payment Confirmation feature and we would only need to change the HTML spec, since I've already landed patches for Chrome Driver using camel case (although still not exposed).

@annevk
Copy link
Member Author

annevk commented Mar 31, 2023

Ultimately WebDriver is outside of the web platform so as long we ensure that the names don't leak into the web platform it doesn't matter too much.

@stephenmcgruer
Copy link

It'd be interesting to know whether @stephenmcgruer would be willing to change the Secure Payment Confirmation spec to align with this proposal. As far as I know only Chrome implements such feature. From Chrome we have @domenic's opinion about the extension command added to the HTML spec, who slightly prefer to use dash-separated (following the Design Principles and change Secure Payment Confirmation spec.

Totally happy to change SPC to match whatever consensus we come to, as long as it is a consensus :). It's not too much pain to pay down I think (change the spec, change Chromedriver, change the WPT tests).

@javifernandez
Copy link

@jgraham It seems that nobody has strong opinions on this issue, so the simpler approach would be to keep the SPC implementation as it is (so use camelCase), change the HTML spec regarding the Custom Handlers automated testing and adapt my ongoing implementation for ChromeDriver accordingly,

However, in your analysis here it seems neither hyphenated or camelCase were among you options; your proposal would imply changes in both specs and their corresponding implementations (only for ChromeDriver for now, luckily). Could you please confirm your position ?

@jgraham
Copy link
Member

jgraham commented May 10, 2023

My proposal isn't a proposal as such, it's an observation about what would be most consistent going forward.

That said, one reasonable question is how widely deployed SPC and Custom Handlers automation are? If they're only really used in web-platform-tests, it seems like we should be able to change things without much bother. If they're also being exposed in webapp testing clients (e.g. Puppeteer) then it's too much risk to change things, and we can just accept it as another inconsistency in the way that values are handled in the protocol.

I think if you'd like clear guidance going forward the most helpful thing would be some proposed spec text setting out the conventions, so that extension spec authors can refer to it. I assume people with an opinion would be more likely to comment on a definite proposal than a general issue.

@javifernandez
Copy link

Custom Handlers automation is still not fully implemented, so the impact of doing changes in the spec and driver code is minimal. Regarding SPC, I'm not 100% sure, but it as commented that making changes is possible if we have a solid agreement.

I'd be willing to propose text for the WebDriver spec with some conventions, as you said. Unless anybody has strong opinions, we could go with what SPC has already implemented. The only argument given opposing to this would be to follow the Design Principles, but they were not strong positions, so any other opinion is more than welcome.

@stephenmcgruer
Copy link

Regarding SPC, I'm not 100% sure, but it as commented that making changes is possible if we have a solid agreement.

SPC is happy to change to whatever the consensus is, and I don't expect a web-compat risk for that. (I'm not aware of anyone using it in e.g. Puppeteer).

@css-meeting-bot

This comment was marked as off-topic.

@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Naming conventions for WebDriver / extension specs.

The full IRC log of that discussion <AutomatedTester> topic: Naming conventions for WebDriver / extension specs
<AutomatedTester> github: https://github.com//issues/1725
<AutomatedTester> jfernandez: I will be very brief. The issue is there are conflicts when enumerating driver extension commands
<AutomatedTester> ... I guess that it has never happened
<jgraham_> q+
<AutomatedTester> but now we have a conflict between secure payment and custom handlers
<AutomatedTester> ... so I would like to get an agreement for enumerations
<AutomatedTester> ... I don't think there is a strong agreement which way like : or - or so on
<AutomatedTester> ack next
<AutomatedTester> jgraham_ (IRC): I think this is purely about the naming conventions. I have been looking through the specs and we are not consistent
<AutomatedTester> ... in bidi we use all lower case
<AutomatedTester> ... error codes have spaces
<AutomatedTester> ... worker types are - separated
<AutomatedTester> ... action types are camel cased
<AutomatedTester> ... where for property names in the json protocol we consistently use camel case
<AutomatedTester> ... we never use snake case. My opinion is that we standardise on space separated like error codes as they are nicely human readable
<AutomatedTester> jfernandez: for context, when <person> at Google did this suggested we do camel case
<AutomatedTester> ... I am not sure the rational
<AutomatedTester> jgraham_ (IRC): the only problem with camel case is that it is a outlier for the work we already have
<AutomatedTester> ... but there is not clear consensus
<AutomatedTester> jfernandez: the SPC editors are happy to changes here
<AutomatedTester> jgraham_ (IRC): since there are no opinions that people comment on the issue
<jgraham_> ScribeNick: jgraham
<mathiasbynens> q+
<jgraham_> jgraham_: I don't mind, but the error codes seem like the most fundamental part of the protocol
<jgraham_> mathiasbynens: We could follow the design guidelines for WebIDL enums
<jgraham_> gsnedders: We could have a straw poll in the GH issue?
<mathiasbynens> jfernandez: can you point me (offline) to the ChromeDriver CL/discussion where camelcase was suggested? thanks
<jgraham_> jgraham: Yes, let's resolve to have a strawpoll on the GH issue.

@javifernandez
Copy link

javifernandez commented May 11, 2023

We have the following options, please, reply to the comment with the react emoji associated to each option:

  1. 🚀 : : -> camelCase (currently used by SPC)
  2. 👀 -> lower-case-dash-delimited (Design Principles)
  3. 😄 -> lower case space separated (used in WebDriver error codes)
  4. 🎉 : -> alllowercase (Bidi serialized type names)

@gsnedders
Copy link
Contributor

Wait, what?! I don't have anywhere near so many reacts!

Screenshot 2023-05-11 at 18 59 08

@javifernandez
Copy link

Wait, what?! I don't have anywhere near so many reacts!

I've simplified the reacts, hope this helps.

@jgraham
Copy link
Member

jgraham commented May 12, 2023

Camel case is spelled camelCase; CamelCase would actually be PascalCase (which would arguably be better than camelCase for emum varaiants since it's commonly used for type names and enum variant names in popular programming languages, but I don't want to introduce another option that's different to everything else at this stage).

@javifernandez
Copy link

Camel case is spelled camelCase; CamelCase would actually be PascalCase (which would arguably be better than camelCase for emum varaiants since it's commonly used for type names and enum variant names in popular programming languages, but I don't want to introduce another option that's different to everything else at this stage).

oh, indeed. I've fixed the typo in the options above, I hope this subtle change doesn't affect to the people that had already voted.

@gsnedders
Copy link
Contributor

To look slightly closer and give complete lists, beyond what @jgraham mentioned above:

  1. camelCase
<code>acceptInsecureCerts</code>
<code>activeElement</code>
<code>altKey</code>
<code>altitudeAngle</code>
<code>alwaysMatch</code>
<code>azimuthAngle</code>
<code>browserName</code>
<code>browserVersion</code>
<code>charCode</code>
<code>ctrlKey</code>
<code>deltaX</code>
<code>deltaY</code>
<code>firstMatch</code>
<code>ftpProxy</code>
<code>httpOnly</code>
<code>httpProxy</code>
<code>innerHTML</code>
<code>isComposing</code>
<code>keyCode</code>
<code>keyDown</code>
<code>keyPress</code>
<code>keyUp</code>
<code>metaKey</code>
<code>noProxy</code>
<code>outerHTML</code>
<code>pageHide</code>
<code>pageLoad</code>
<code>pageLoadStrategy</code>
<code>pageRanges</code>
<code>pageShow</code>
<code>platformName</code>
<code>pointerCancel</code>
<code>pointerDown</code>
<code>pointerMove</code>
<code>pointerType</code>
<code>pointerUp</code>
<code>proxyAutoconfigUrl</code>
<code>proxyType</code>
<code>sameSite</code>
<code>serializeToString</code>
<code>sessionId</code>
<code>setSelectionRange</code>
<code>setWindowRect</code>
<code>shiftKey</code>
<code>shrinkToFit</code>
<code>snapshotItem</code>
<code>snapshotLength</code>
<code>socksProxy</code>
<code>socksVersion</code>
<code>sslProxy</code>
<code>strictFileInteractability</code>
<code>tangentialPressure</code>
<code>tiltX</code>
<code>tiltY</code>
<code>toJSON</code>
<code>toString</code>
<code>unhandledPromptBehavior</code>
<code>visibilityState</code>

This roughly shows us that capabilities, actions, and timeouts use camel-case. Capabilities and timeouts are the keys of a map; only actions uses them as an enum.

  1. lower-case-dash-delimited
<code>cookie-string</code>
<code>expiry-time</code>
<code>http-only-flag</code>
<code>no-cache</code>
<code>pointer-events</code>
<code>secure-only-flag</code>

All of these are external references.

  1. lower case space separated
<code>accept and notify</code>
<code>composition event</code>
<code>css selector</code>
<code>current delta x</code>
<code>current delta y</code>
<code>detached shadow root</code>
<code>dismiss and notify</code>
<code>element click intercepted</code>
<code>element not interactable</code>
<code>insecure certificate</code>
<code>invalid argument</code>
<code>invalid cookie domain</code>
<code>invalid element state</code>
<code>invalid selector</code>
<code>invalid session id</code>
<code>javascript error</code>
<code>link text</code>
<code>move target out of bounds</code>
<code>no such alert</code>
<code>no such cookie</code>
<code>no such element</code>
<code>no such frame</code>
<code>no such shadow root</code>
<code>no such window</code>
<code>partial link text</code>
<code>script timeout</code>
<code>session not created</code>
<code>stale element reference</code>
<code>tag name</code>
<code>unable to capture screen</code>
<code>unable to set cookie</code>
<code>unexpected alert open</code>
<code>unknown command</code>
<code>unknown error</code>
<code>unknown method</code>
<code>unsupported operation</code>
<code>valid values</code>

We have errors (arguably an enum), user prompt handlers (definitely an enum), locator strategies (definitely an enum).

  1. alllowercase

I don't know how to find this easily!

@gsnedders
Copy link
Contributor

Also: whatever we resolve here, we should actually document this within the WebDriver spec to give other spec authors guidance.

@jgraham
Copy link
Member

jgraham commented May 12, 2023

I think the camel case examples are mostly of things that aren't enum values. Capabilities in particular are just JSON keys, and all the event properties like isComposing aren't part of the protocol. From scanning the list, none of the examples seem relevant.

@jgraham
Copy link
Member

jgraham commented May 12, 2023

I also think that at this point looking in BiDi is also helpful as precedent.

@jgraham
Copy link
Member

jgraham commented May 12, 2023

In WebDriver I see the following things that are actually enumish values:

  1. camelCase
  • Action names
  1. lower-case-dash-delimited
  • [none]
  1. lower case space separated
  • Error codes
  • Locator strategies
  1. alllowercase
  • proxyType capability (notably autodetect)
  1. Unclear (each entry is just one lowercase word)
  • Page load strategies
  • New window type hint
  • Input source types
  • Pointer types
  • Input origin
  • Print orientation

@jgraham
Copy link
Member

jgraham commented May 12, 2023

In WebDriver-bidi the list looks like the following (excluding things that direct ports from WebDriver):

  1. camelCase
  • Method names (These are intended to look like JS methods, not enums, but technically they are enum-like)
  • Event names (as above)
  1. lower-case-dash-delimited
  • Realm types
  1. lower case space separated
  • [No extra ones]
  1. alllowercase
  • Remote value types
  • Local values types
  • User prompt type
  1. Unclear
  • Readiness state
  • Browsing context type hint
  • Script result type
  • Shadow root mode
  • Result ownership
  • Network initator type (assuming preflight is all one word)
  • Log entry type
  • Log level
  1. Other
  • JS special numbers (NaN, -0, Infinity, -Infinity) - almost PascalCase

@jgraham
Copy link
Member

jgraham commented May 18, 2023

It occurs to me that one technical consideration is that alllowercase doesn't allow converting to other forms algorithmically, whereas all the others do allow that.

@javifernandez
Copy link

So far only four people have voted (including me) but there is a tie :( so I'm not sure what to do.

@javifernandez
Copy link

In our discussion at TPAC about this issue we have agreed to use camelCase. This matches what it's specified in the
SPC Trancsation Mode extension command, already implemented in chromedriver.

jgraham: sounds like we have agreement on the first part that we need to adopt camel case

Hence, I think we can close this issue as resolved.

@jgraham
Copy link
Member

jgraham commented Sep 28, 2023

Arguably we should keep the issue open until we have a PR documenting the naming conventions in the spec.

@javifernandez
Copy link

Arguably we should keep the issue open until we have a PR documenting the naming conventions in the spec.

Considering that the tittle of the issue is general enough, it makes sense to leave it open, as you said. We discussed the specific cases of the RPH and SPC extension commands conflicts, but it's reasonable to expand its scope as the tittle suggest.

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

No branches or pull requests

6 participants