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

fix: autocomplete-appropriate node type resolution #1318

Merged
merged 6 commits into from
Jan 18, 2019

Conversation

jeeyyy
Copy link
Contributor

@jeeyyy jeeyyy commented Jan 16, 2019

Firefox returns node.type as text for certain scenarios like - <input autocomplete="bday-month" type="month">, hence using getAttribute to resolve the value of type solves for this edge case.

Due to this, the number of violations returned across different browsers were different as described in the bug below.

Closes issue:

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: << Name here >>

@jeeyyy jeeyyy requested a review from a team as a code owner January 16, 2019 15:49
* Firefox returns `node.type` as `text` for `type='month'`
* Hence using `node.getAttribute('type')` over `node.type`
*/
const nodeType = node.hasAttribute('type') ? node.getAttribute('type') : 'text';
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we need to use the attribute here rather than the DOM property, but you'll have to normalise it. I don't know exactly what that involves, you can look that up in the spec. It's probably case insensitive. It also needs to default to "text" when there's an invalid value here, not just when type isn't set. Otherwise an element with type="foobar" wouldn't be allowed to have autocomplete allowed for text.

* @instance
* @return {Array<Sting>}
*/
axe.utils.validInputTypes = function validInputTypes() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be assigned to axe.commons.utils, not to axe.utils. Also, I don't think this should be a function. It's inconsistent with how other thing in axe behave.

@@ -0,0 +1,38 @@
/* global axe */
Copy link
Contributor

Choose a reason for hiding this comment

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

If you create a new commons, please provide tests.

@WilcoFiers WilcoFiers merged commit 2fc3eeb into develop Jan 18, 2019
@WilcoFiers WilcoFiers deleted the fix-autocomplete-node-type branch January 18, 2019 16:32
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.

2 participants