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

feat: add a11y autocomplete-valid #8520

Merged
merged 5 commits into from
May 5, 2023

Conversation

cruessler
Copy link
Contributor

This PR adds validation to the autocomplete attribute for <input> elements, similar to the ESLint rule jsx-a11y/autocomplete-valid. It is related to #820.

The implementation generally follows the implementation in axe-core.

This PR currently only works for <input> elements. According to the spec, these rules also apply to <textarea> and <select> elements, but the corresponding ESLint rule only applies to <input> elements.

This PR is quite strict in validating the autocomplete value, because I tried to follow the spec as much as possible. axe-core which is what the ESLint rule uses under the hood, dropped support for checking for approriate types at some point. See dequelabs/axe-core#2912 and dequelabs/axe-core#2917. I saw that you recently relaxed checks on a different rule, and I was wondering whether my PR is too strict. I’m not deeply familiar with A11y, so any guidance is welcome!

I got most of the test cases from ESLint and axe-core. I was not sure whether it made sense to add more test cases. Let me know whether you want any changes in that regard!

@vercel
Copy link

vercel bot commented Apr 20, 2023

@cruessler is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@dummdidumm
Copy link
Member

Thanks for researching that the rule is deprecated in axe-core! Given that axe-core is quite strict in other places, I'd say eslint-jsx-a11y just hasn't caught up on that one yet. We therefore shouldn't implement this, so I'm closing this issue, sorry. Still congratulations on your first Svelte PR!

@dummdidumm dummdidumm closed this Apr 20, 2023
@cruessler
Copy link
Contributor Author

axe-core used to have two rules related to autocomplete: autocomplete-valid and autocomplete-appropriate. Only the latter has been removed. The former still exists as far as I can tell.

@dummdidumm
Copy link
Member

dummdidumm commented Apr 21, 2023

I'm sorry, I misunderstood then. I then suggest to remove the parts from the PR that are testing for autocomplete-appropriate and only keep the parts for autocomplete-valid.

Btw I saw that you added quite a few new tokens, it may be that they are already part of axobject-query or aria-query which are already dependencies of Svelte so you can use them.

@dummdidumm dummdidumm reopened this Apr 21, 2023
Copy link
Contributor Author

@cruessler cruessler left a comment

Choose a reason for hiding this comment

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

I added a few comments, hoping to provide a bit of context for the review.

'impp'
]);

export function is_valid_autocomplete(type: null | true | string, autocomplete: null | true | string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The types come from the inferred return type of get_static_value. I was unsure whether to add them, but wanted to be cautious in order not to miss edge cases.

Comment on lines 298 to 299
const input_wears_autofill_anchor_mantle = normalized_type === 'hidden';
const input_wears_autofill_expectation_mantle = !input_wears_autofill_anchor_mantle;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The language comes from the spec. Given that axe-core’s implementation does not implement this part of the spec, do you want me to remove it?

Copy link
Member

Choose a reason for hiding this comment

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

What exactly do you mean by that? If it's about simplifying this to always allow on and off, then yes, let's relax this

@@ -223,3 +224,111 @@ export function is_semantic_role_element(role: ARIARoleDefinitionKey, tag_name:
}
return false;
}

// https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#autofilling-form-controls:-the-autocomplete-attribute
const address_type_tokens = new Set(['shipping', 'billing']);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not able to find any of the following tokens in either axobject-query or aria-query.

<input type="text" autocomplete />
<input type="hidden" autocomplete="off" />
<input type="hidden" autocomplete="on" />
<input type="text" autocomplete="" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@dummdidumm dummdidumm Apr 27, 2023

Choose a reason for hiding this comment

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

I'd say let's relax this then to allow the empty and boolean value. We also need a test that checks if it succeeds if a synamic value is given, like autocomplete={foo}

<input type="text" autocomplete="section-blue shipping street-address" />

<!-- INVALID -->
<input type="text" autocomplete />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Thank you!

@dummdidumm dummdidumm merged commit 3f2f1e5 into sveltejs:master May 5, 2023
@Conduitry
Copy link
Member

This has been released in 3.59.0 - thank you!

@xxaier
Copy link

xxaier commented May 6, 2023

[Success] All components are bundled
▲ [WARNING] A11y: The value 'null' is not supported by the attribute 'autocomplete' on element <input type="text"> [plugin esbuild-svelte]

    ../pkg/user/ui/Auth.svelte:1:135:
      1 │ ..."{account}" autocomplete="{ up ? 'off' : 'username' }" re...
        ╵             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

                                                                                                                                          ^
2:
3: <script lang="coffee">function asyncGeneratorStep(gen, resolve, reject, _next, _throw, key, arg) { try { var info = gen[key](arg); var value = info.value; } catch (error) { reject(error);
 return; } if (info.done) { resolve(value); } else { Promise.resolve(value).then(_next, _throw); } }

  The plugin "esbuild-svelte" was triggered by this import

    src/index.js:9:7:
      9 │ import './user/Auth.svelte';
        ╵        ~~~~~~~~~~~~~~~~~~~~

why this error when autocomplete as value ?

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.

4 participants