Skip to content

Add more precise typing for autocomplete HTML attribute #1467

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

Merged

Conversation

yanndinendal
Copy link
Contributor

@yanndinendal yanndinendal commented Jan 9, 2023

This will allow to autocomplete… the autoComplete attribute in vscode for example.

autoComplete autocompletion in vscode

The list is quite long and it's not easy remembering the spelling of each one of them (first_name or given-name? password or current-password?), and it would improve the UX of sites if they were more appropriately used (for example, some people may think that the input's type and/or name attributes may be enough for fields like email or password, but knowing to use autoComplete="new-password" or one-time-code can be really useful).


I tried declaring a type AutoComplete to avoid repetition but didn't find how to do that:
microsoft/TypeScript#52169.

See updated proposal at #1467 (comment)

Fixes microsoft/TypeScript#52168.
See also #71.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2023

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

@yanndinendal
Copy link
Contributor Author

yanndinendal commented Jan 9, 2023

@microsoft-github-policy-service agree

@yanndinendal
Copy link
Contributor Author

@microsoft-github-policy-service agree

@saschanaz
Copy link
Contributor

First, please define an enum (which then generates a union type) and use that.

Second, are we sure every item here is well supported? Poorly supported items shouldn't be added.

@HolgerJeromin
Copy link
Contributor

Typescript team should be confident that they want (string & {}) in this repo which uses a side effect of current typescript intellisense code.
If yes, this should perhaps be done for all attributes instead of one (the data should be available afaik).

@orta
Copy link
Contributor

orta commented Jan 10, 2023

/cc @sandersn @weswigham

@yanndinendal
Copy link
Contributor Author

yanndinendal commented Jan 10, 2023

@saschanaz: thanks, fixed (I didn't know how to do it with these json files, at first).

I've re-read the spec and declared an AutoFill enum that matches the spec, and I've tested most of them and confirmed they seem indeed supported in at least Chrome or Firefox.
I couldn't find definite info on Can I Use or MDN except for some of them like new-passport which is largely supported:
https://caniuse.com/mdn-html_global_attributes_autocomplete_new-password

I've also fixed the possible values for autocomplete on a form element, which can't act as an autofill element, and confirmed it's supported on textareas and selects:


I added webauthn, which is in the spec (linked in the comments) and can be used in combination with username or current-password values (already available in stable releases of Chrome):
https://web.dev/passkey-form-autofill/


Unfortunately, I don't know how to add typescript auto-completion for a list of strings (space-separated tokens, like "username webauthn" or "shipping street-address"), but it should already cover most use-cases (and more advanced ones are handled with (string & {})).
@HolgerJeromin: So it's necessary to keep it instead of a strict list to follow the spec (it also prevents this to be a breaking change).

Also note that this syntax/workaround is already used and tested in the TypeScript repo,
such as with tests/baselines/reference/specialIntersectionsInMappedTypes.types's
type Alignment = (string & {}) | "left" | "center" | "right";
or tests/lib/react18/react18.d.ts's AriaRole.

@saschanaz
Copy link
Contributor

What does (string & {}) do, why not just string?

It's also currently broken, please use AutoFill | string instead for overrideType.

@yanndinendal
Copy link
Contributor Author

yanndinendal commented Jan 10, 2023

@saschanaz:

With | (string & {}):

autoComplete autocompletion in vscode

With | string:

no editor auto-completion at all.


See microsoft/TypeScript#33471, and microsoft/TypeScript#29729 for an explanation of the issue.

I think I originally stumbled on this method in React's typedefs.

@yanndinendal
Copy link
Contributor Author

It's also currently broken, please use AutoFill | string instead for overrideType.

@saschanaz: Sorry I don't understand that last part, the tests pass and it seems to work.
By broken, do you mean that it should generate something else? Or that some cases (not handled by the tests) will not work?

I can't use | string or we will lose autocompletion (it would have type string only).

@saschanaz
Copy link
Contributor

Look at the output: type AutoFill = "(string & {})". You don't want this 😛

@yanndinendal
Copy link
Contributor Author

yanndinendal commented Jan 10, 2023

(wrong obsolete comment)

Do you see the right commit? I see the full union:

image

@yanndinendal
Copy link
Contributor Author

yanndinendal commented Jan 10, 2023

Sorry I got it, the rest of the union is ok but "(string & {})" should not be quoted :o

I don't know how to do that :/ fixed

@RyanCavanaugh
Copy link
Member

Do not add string & {} to this file.

If the set of values is truly unconstrained, string is the appropriate type, and is what should be written here.

Copy link
Member

@RyanCavanaugh RyanCavanaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per prior comment

@saschanaz
Copy link
Contributor

Yup, that's also what other properties do.

@yanndinendal
Copy link
Contributor Author

yanndinendal commented Jan 11, 2023

If the set of values is truly unconstrained, string is the appropriate type, and is what should be written here.

It's not unconstrained. Writing string or any union | string wold lose any auto-completion, so we might as well close this PR if that's not something you want. Can I ask why?

I could try writing it as a string template literal type instead, based on this table from the spec, to allow 1, 2, 3 or 4 tokens from the union, in the correct order, but it would take me some time to do it right so we may want to first discuss if this could be investigated/accepted, or if it wouldn't be accepted either.

@yanndinendal
Copy link
Contributor Author

yanndinendal commented Jan 11, 2023

Typescript team should be confident that they want (string & {}) in this repo which uses a side effect of current typescript intellisense code.

just to clarify that this is to work around a bug in typescript; I would be happy to use | string instead once it's fixed, but in the meanwhile it would be no better than closing this PR and losing type suggestions.

See issues linked in #issuecomment-1377304118 for explanations.

@yanndinendal
Copy link
Contributor Author

Re #issuecomment-1378404272

I could try writing it as a string template literal type instead, based on this table from the spec, to allow 1, 2, 3 or 4 tokens from the union, in the correct order

Here are the full constraints, in the right order, for the set of space-separated tokens:

- `section-${string}`
- "shipping" | "billing"
- AutoFill // (the union I added to this PR, excluding "webauthn")
- "webauthn"

@yanndinendal
Copy link
Contributor Author

yanndinendal commented Jan 11, 2023

@saschanaz, @RyanCavanaugh: Ok, here is how it would look like, it should be restricted to all authorized permutations (because for Autofill detail tokens (all except on/off), the autofill field names union is the only mandatory token of the list).
I will need to declare these types in this repo's json files but I made it work locally with a standard .d.ts file and it's quite impressive what it can suggest!

  type AutoFillSection = `section-${string}`;
  type AutoFillAddressKind = 'shipping' | 'billing';
  type AutoFillNormalField =
    | 'name'
    | 'honorific-prefix'
    | 'given-name'
    | 'additional-name'
    | 'family-name'
    | 'honorific-suffix'
    | 'nickname'
    | 'username'
    | 'new-password'
    | 'current-password'
    | 'one-time-code'
    | 'organization-title'
    | 'organization'
    | 'street-address'
    | 'address-line1'
    | 'address-line2'
    | 'address-line3'
    | 'address-level4'
    | 'address-level3'
    | 'address-level2'
    | 'address-level1'
    | 'country'
    | 'country-name'
    | 'postal-code'
    | 'cc-name'
    | 'cc-given-name'
    | 'cc-additional-name'
    | 'cc-family-name'
    | 'cc-number'
    | 'cc-exp'
    | 'cc-exp-month'
    | 'cc-exp-year'
    | 'cc-csc'
    | 'cc-type'
    | 'transaction-currency'
    | 'transaction-amount'
    | 'language'
    | 'bday'
    | 'bday-day'
    | 'bday-month'
    | 'bday-year'
    | 'sex'
    | 'url'
    | 'photo';
  type AutoFillContactKind = 'home' | 'work' | 'mobile' | 'fax' | 'pager';
  type AutoFillContactField =
    | 'tel'
    | 'tel-country-code'
    | 'tel-national'
    | 'tel-area-code'
    | 'tel-local'
    | 'tel-local-prefix'
    | 'tel-local-suffix'
    | 'tel-extension'
    | 'email'
    | 'impp';
  type AutoFillField =
    | AutoFillNormalField
    | AutoFillContactField
    | `${AutoFillContactKind} ${AutoFillContactField}`;
  type AutoFillCredentialField = 'webauthn';

  type AutoFill =
    | 'off'
    | 'on'
    | AutoFillField
    | `${AutoFillSection} ${AutoFillField}`
    | `${AutoFillAddressKind} ${AutoFillField}`
    | `${AutoFillSection} ${AutoFillAddressKind} ${AutoFillField}`
    | `${AutoFillSection} ${AutoFillAddressKind} ${AutoFillField} ${AutoFillCredentialField}`
    | `${AutoFillAddressKind} ${AutoFillField} ${AutoFillCredentialField}`
    | `${AutoFillSection} ${AutoFillField} ${AutoFillCredentialField}`
    | `${AutoFillField} ${AutoFillCredentialField}`;

Here are example suggestions and valid strings:

  • even the space-separated "contact kind + contact field" template string are suggested, with all possible permutations:
    contact kind + contact field template string type autocompletion
  • only section- isn't auto-completed (but is typed/validated):
    section non-autocompletion
  • typed section-*:
    valid string with section-* and valid space-separated unions
  • invalid string with missing mandatory AutoFillField:
    invalid string example
  • other autocompleted valid strings with several categories:
    webauthn-autocompletion
    valid string with webauthn
  • still works for trivial values:
    one-word example
    on/off values
  • invalid example:
    off + other token

@HolgerJeromin
Copy link
Contributor

Also note that this syntax/workaround is already used and tested in the TypeScript repo,
such as with tests/baselines/reference/specialIntersectionsInMappedTypes.types's
type Alignment = (string & {}) | "left" | "center" | "right";
or tests/lib/react18/react18.d.ts's AriaRole.

Internal tests are different from code which is shipped to many developers and shapes the way they work.

@yanndinendal
Copy link
Contributor Author

yanndinendal commented Jan 11, 2023

@HolgerJeromin: Right. Does the above proposition sound better than the first one? With template string types?

@yanndinendal yanndinendal force-pushed the add_autocomplete_html_attribute branch 2 times, most recently from 2f40faa to ca386d5 Compare January 21, 2023 00:39
@yanndinendal
Copy link
Contributor Author

Hi @RyanCavanaugh, @saschanaz: I pushed a new version without | (string & {}) that is now fully spec-compliant. I would appreciate any feedback. :)

See #1467 (comment) for screenshots and context.

@yanndinendal yanndinendal marked this pull request as draft January 24, 2023 08:43
@sandersn
Copy link
Member

I turned the types into values and got 424:

var OptionalPostfixToken = _ => 2
var OptionalPrefixToken = _ => 2
var AutoFillSection = 1
var AutoFillAddressKind = 2
var AutoFillBase = 2
var AutoFillContactField = 9
var AutoFillContactKind = 3
var AutoFillCredentialField = 1
var AutoFillNormalField = 36
var AutoFillField = AutoFillNormalField + OptionalPrefixToken(AutoFillContactKind) * AutoFillContactField;
var AutoFill = AutoFillBase + OptionalPrefixToken(AutoFillSection)*OptionalPrefixToken(AutoFillAddressKind)*AutoFillField*OptionalPostfixToken(AutoFillCredentialField)
console.log(AutoFill)

I think that's reasonable; any sizeable addition to the DOM adds that many types.

Replaces: microsoft/TypeScript#52169

This will allow to autocomplete… the `autoComplete` attribute in vscode for example.

![autoComplete autocomplete in vscode](https://user-images.githubusercontent.com/58247/211378145-c01c665b-9f4b-4918-83cc-16532f555935.png)

The list is quite long and it's not easy remembering the spelling of each one of them (first_name or given-name? password or current-password?), and it would improve the UX of sites if they were more appropriately used (for example, some people may think that the input's `type` and/or `name` attributes may be enough for fields like email or password, but knowing to use `autoComplete="new-password"` or `one-time-code` can be really useful).
Add missing value according to spec (`webauthn`).
@yanndinendal yanndinendal force-pushed the add_autocomplete_html_attribute branch from d3cb416 to b3466ed Compare June 19, 2023 14:02
@yanndinendal
Copy link
Contributor Author

Edit: WPT wpt.fyi/results/html/semantics/forms/the-form-element/form-autocomplete.html?label=master&label=experimental&aligned&q=autocomplete

@saschanaz So looks like everything is good on that page, right?

The only tests that fail on two browsers are with one-time-code, but it's clearly supported by iOS Safari: https://developer.apple.com/documentation/security/password_autofill/enabling_password_autofill_on_an_html_input_element
and webauthn with a combination of everything else (section, mode, contact, field, and credential), which is probably an unexpected limitation (webauthn alone is supported; other combinations are supported), so there is no reason to add arbitrary restrictions to type definitions for this.

I've rebased, is it ready to merge? :)

Copy link
Contributor

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long delay, and thank you for maintaining this so far!

Looks good with some nits. It seems a bit more fields are supported now than this was written, but asking that change here would be very unfair, that can be done separately.

},
{
"name": "AutoFillSection",
"overrideType": "`section-${string}`"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a TODO comment about the over-match of section-{string}?

@saschanaz
Copy link
Contributor

I'm assuming that having the Optional*Token helper types here is okay, as @sandersn hasn't complained about that.

@yanndinendal
Copy link
Contributor Author

@saschanaz: pushed, thanks! :)

@saschanaz
Copy link
Contributor

Let's see I can merge this even if there's a change request, because that's solved now.

LGTM

@github-actions github-actions bot merged commit dbf56b1 into microsoft:main Jun 19, 2023
@github-actions
Copy link
Contributor

Merging because @saschanaz is a code-owner of all the changes - thanks!

@yanndinendal yanndinendal deleted the add_autocomplete_html_attribute branch June 19, 2023 18:14
@yanndinendal
Copy link
Contributor Author

@sandersn, @saschanaz: Hi, do you when it will be included in https://github.com/microsoft/TypeScript/blob/main/src/lib/dom.generated.d.ts and released? 😄

@saschanaz
Copy link
Contributor

(I'm not really allowed to know such info, but the roadmap implies the new release is coming soonish, as the release happens roughly every three months and the last release was three months ago.) https://github.com/microsoft/TypeScript/wiki/Roadmap)

@saschanaz
Copy link
Contributor

saschanaz commented Jun 20, 2023

If you need it today, consider using https://www.npmjs.com/package/@types/web.

... except it looks like the release step is broken since 0.0.99. (Edit: Filed #1580)

@sandersn
Copy link
Member

sandersn commented Jun 20, 2023

I'm assuming that having the Optional*Token helper types here is okay, as @sandersn hasn't complained about that.
Basically yes. I'll check the performance numbers when I copy the types into TS 5.2 beta tomorrow but I think it'll be OK.

I'll check out the publication failure and try to fix it.

Edit: Oh, looks you have a potential fix already. I'll merge that.

@yanndinendal
Copy link
Contributor Author

yanndinendal commented Oct 9, 2023

Unfortunately, this does not get picked up by @types/react / @types/react-dom JSX autoComplete prop. ¯\_(ツ)_/¯

Asking for instructions here: DefinitelyTyped/DefinitelyTyped#66982

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

Successfully merging this pull request may close these issues.

Improve HTML autocomplete attribute
7 participants