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.

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

Handle edge cases in with-constrained-tabbing. #10174

Merged
merged 2 commits into from
Oct 3, 2018
Merged

Handle edge cases in with-constrained-tabbing. #10174

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d21ead
Handle edge cases in with-constrained-tabbing.
afercia Sep 25, 2018
2ee25cc
E2E Tests: a11y: Test focus retention in modals
mcsf Oct 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/components/src/higher-order/with-constrained-tabbing/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@ const withConstrainedTabbing = createHigherOrderComponent(
} else if ( ! event.shiftKey && event.target === lastTabbable ) {
event.preventDefault();
firstTabbable.focus();
/*
* When pressing Tab and none of the tabbables has focus, the keydown
* event happens on the wrapper div: move focus on the first tabbable.
*/
} else if ( ! tabbables.includes( event.target ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be enough to test event.target === this.focusContainRef.current ? More optimized, more explicit to what we are testing.

Or are there other cases where the target of the tab press would itself not be tabbable? I guess might account for elements with tabIndex=-1 within the content? If so, should be update the comment to reflect this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep it would be nicer and simpler. Unfortunately, it doesn't work on IE11 where the event.target seems to be the clicked children element in the content. See below, in order:

  • event.target
  • event.currentTarget
  • this.focusContainRef.current

screenshot 94

Copy link
Member

Choose a reason for hiding this comment

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

Ah! This is fine then.

event.preventDefault();
firstTabbable.focus();
}
}

Expand All @@ -44,6 +51,7 @@ const withConstrainedTabbing = createHigherOrderComponent(
<div
onKeyDown={ this.handleTabBehaviour }
ref={ this.focusContainRef }
tabIndex="-1"
>
<WrappedComponent { ...this.props } />
</div>
Expand Down