Skip to content

Split interactive supports focus into tabbable and focusable cases #236

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 30, 2017
Merged

Split interactive supports focus into tabbable and focusable cases #236

merged 2 commits into from
May 30, 2017

Conversation

jessebeach
Copy link
Collaborator

Updating the interactive-supports-focus rule to distinguish between elements that should have a tabindex of 0 (and thus be tabbable) and elements that should have a tabindex of -1 (and thus be focusable).

We don't want to encourage developers to add tabIndex="0" willy-nilly to their components. Too many tabindices can create a degraded experience.

The recommended set of roles that should be assigned a tabindex of 0 is given in the recommended settings:

'jsx-a11y/interactive-supports-focus': [
  'error',
  {
    tabbable: [
      'button',
      'checkbox',
      'link',
      'searchbox',
      'spinbutton',
      'switch',
      'textbox',
    ],
  },
]

@jessebeach jessebeach changed the title Finer casing interactive supports focus Split interactive supports focus into tabbable and focusable cases May 9, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 99.549% when pulling d71d1d0 on jessebeach:finer-casing-interactive-supports-focus into c16fa00 on evcohen:master.

@jessebeach jessebeach force-pushed the finer-casing-interactive-supports-focus branch from d71d1d0 to cae5919 Compare May 10, 2017 02:56
@jessebeach
Copy link
Collaborator Author

Tests should be passing now. I'd love a review!

@beefancohen
Copy link
Contributor

@jessebeach got you on a review tmrw!

@beefancohen beefancohen self-requested a review May 10, 2017 04:50
@coveralls
Copy link

coveralls commented May 10, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.002%) to 99.549% when pulling cae5919 on jessebeach:finer-casing-interactive-supports-focus into c16fa00 on evcohen:master.

@jessebeach jessebeach force-pushed the finer-casing-interactive-supports-focus branch from cae5919 to 4caf285 Compare May 16, 2017 02:32
@coveralls
Copy link

coveralls commented May 16, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.002%) to 99.549% when pulling 4caf285 on jessebeach:finer-casing-interactive-supports-focus into 9971241 on evcohen:master.

beefancohen
beefancohen reviewed May 16, 2017
View reviewed changes
docs/rules/interactive-supports-focus.md Outdated
@@ -4,7 +4,9 @@ Elements with an interactive role and interaction handlers (mouse or key press)

## How do I resolve this error?

### Case: This element is a stand-alone control like a button, a link or a form element
### Case: Elements with the '${role}' interactive role must be tabbable
Copy link
Contributor

Choose a reason for hiding this comment

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

'${role}' ?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh you're referencing the error message. i think this can be clearer:

#### Case: I got the error "Elements with the '${role}' interactive role must be tabbable." How can I fix this?

or something like that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great suggestion!

src/rules/interactive-supports-focus.js Outdated

const schema = generateObjSchema();
const schema = generateObjSchema({
tabbable: enumArraySchema([
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use aria-query for this? i.e. like ...roles.tabbable()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just the interactives. I'll get those from aria-query.

src/rules/interactive-supports-focus.js Outdated
JSXOpeningElement: (
node: JSXOpeningElement,
) => {
let tabbable = context.options
Copy link
Contributor

Choose a reason for hiding this comment

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

you should be able to make this a const with one line:

const tabbable = (context.options && context.options[0] && context.options[0].tabbable) || [];

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lint complains here when mixing multiple &&s and then finishing with ||. I originally had it like you suggested.

Copy link
Member

Choose a reason for hiding this comment

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

It complains even with the wrapping parens?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It complains even with the wrapping parens?

Indeed, it does not!! Updated.

@jessebeach jessebeach force-pushed the finer-casing-interactive-supports-focus branch 2 times, most recently from 7fc62e6 to d9418c4 Compare May 17, 2017 21:35
@jessebeach
Copy link
Collaborator Author

@evcohen I've made the requested changes.

@coveralls
Copy link

coveralls commented May 17, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.004%) to 99.551% when pulling d9418c4 on jessebeach:finer-casing-interactive-supports-focus into 0228998 on evcohen:master.

@jessebeach jessebeach force-pushed the finer-casing-interactive-supports-focus branch from d9418c4 to 3988e6b Compare May 17, 2017 22:32
@coveralls
Copy link

coveralls commented May 17, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.003%) to 99.55% when pulling 3988e6b on jessebeach:finer-casing-interactive-supports-focus into 0228998 on evcohen:master.

@jessebeach
Copy link
Collaborator Author

Shall we get this in?

@jessebeach jessebeach force-pushed the finer-casing-interactive-supports-focus branch from 3988e6b to a61c7e7 Compare May 22, 2017 22:59
@jessebeach
Copy link
Collaborator Author

Rebased on current master branch.

@coveralls
Copy link

coveralls commented May 22, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.003%) to 99.573% when pulling a61c7e7 on jessebeach:finer-casing-interactive-supports-focus into 83994a8 on evcohen:master.

@jessebeach
Copy link
Collaborator Author

Any objections to merging this?

beefancohen
beefancohen approved these changes May 30, 2017
View reviewed changes
Copy link
Contributor

@beefancohen beefancohen left a comment

Choose a reason for hiding this comment

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

LGTM!

@jessebeach jessebeach force-pushed the finer-casing-interactive-supports-focus branch from a61c7e7 to d856a7f Compare May 30, 2017 17:07
@coveralls
Copy link

coveralls commented May 30, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.003%) to 99.573% when pulling d856a7f on jessebeach:finer-casing-interactive-supports-focus into c0de387 on evcohen:master.

@jessebeach jessebeach merged commit 4f0052d into jsx-eslint:master May 30, 2017
@jessebeach jessebeach deleted the finer-casing-interactive-supports-focus branch May 30, 2017 18:17
@renovate renovate bot mentioned this pull request Jun 27, 2017
Merged
@ljharb ljharb mentioned this pull request Jul 3, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@beefancohen beefancohen beefancohen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jessebeach @coveralls @beefancohen @ljharb