w3c / webdriver

Remote control interface that enables introspection and control of user agents.
https://w3c.github.io/webdriver/
Other
679 stars 194 forks source link

Enum value naming and consistency with the web platform #1725

Open annevk opened 1 year ago

annevk commented 1 year ago

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., https://github.com/w3c/secure-payment-confirmation/issues/219.

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

cc @javifernandez @stephenmcgruer @dontcallmedom @foolip

jgraham commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

The Browser Testing and Tools Working Group just discussed Diverging driver behavior for "clear the modifier key state" in "Dispatch actions for a string".

The full IRC log of that discussion <AutomatedTester> Topic: Diverging driver behavior for "clear the modifier key state" in "Dispatch actions for a string"
<jgraham_> github: https://github.com/w3c/webdriver/issues/1725
css-meeting-bot commented 1 year ago

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/w3c/webdriver/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 commented 1 year ago

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

  1. :rocket: : : -> camelCase (currently used by SPC)
  2. :eyes: -> lower-case-dash-delimited (Design Principles)
  3. :smile: -> lower case space separated (used in WebDriver error codes)
  4. :tada: : -> alllowercase (Bidi serialized type names)
gsnedders commented 1 year ago

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

Screenshot 2023-05-11 at 18 59 08
javifernandez commented 1 year ago

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

I've simplified the reacts, hope this helps.

jgraham commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

jgraham commented 1 year ago

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 commented 1 year ago

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

jgraham commented 1 year ago

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

  1. camelCase
    • Action names
  2. lower-case-dash-delimited
    • [none]
  3. lower case space separated
    • Error codes
    • Locator strategies
  4. alllowercase
    • proxyType capability (notably autodetect)
  5. 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 commented 1 year ago

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)
  2. lower-case-dash-delimited
    • Realm types
  3. lower case space separated
    • [No extra ones]
  4. alllowercase
    • Remote value types
    • Local values types
    • User prompt type
  5. 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 commented 1 year ago

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 commented 1 year ago

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

javifernandez commented 1 year ago

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 commented 1 year ago

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

javifernandez commented 1 year ago

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.