Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
53 changes: 51 additions & 2 deletions test/e2e/specs/a11y.test.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,20 @@
/**
* Internal dependencies
*/
import { newPost, pressWithModifier } from '../support/utils';
import {
ACCESS_MODIFIER_KEYS,
newPost,
pressWithModifier,
} from '../support/utils';

function isCloseButtonFocused() {
return page.$eval( ':focus', ( focusedElement ) => {
return focusedElement.getAttribute( 'aria-label' ) === 'Close dialog';
} );
}

describe( 'a11y', () => {
beforeAll( async () => {
beforeEach( async () => {
await newPost();
} );

Expand All @@ -19,4 +29,43 @@ describe( 'a11y', () => {

expect( isFocusedToggle ).toBe( true );
} );

it( 'constrains focus to a modal when tabbing', async () => {
// Open help modal
await pressWithModifier( ACCESS_MODIFIER_KEYS, 'h' );

// Test that the Close button of the modal is focused when the
// latter is opened.
expect( await isCloseButtonFocused() ).toBe( true );

await page.keyboard.press( 'Tab' );

// Test that the Close button of the modal is focused when the
// latter is opened.
expect( await isCloseButtonFocused() ).toBe( true );
} );

it( 'returns focus to the first tabbable in a modal after blurring a tabbable', async () => {
await pressWithModifier( ACCESS_MODIFIER_KEYS, 'h' );

// Click to move focus to an element after the last tabbable within the
// modal.
await page.click( '.components-modal__content' );

await page.keyboard.press( 'Tab' );

expect( await isCloseButtonFocused() ).toBe( true );
} );

it( 'returns focus to the last tabbable in a modal after blurring a tabbable and tabbing in reverse direction', async () => {
await pressWithModifier( ACCESS_MODIFIER_KEYS, 'h' );

// Click to move focus to an element before the first tabbable within
// the modal.
await page.click( '.components-modal__header-heading' );

await pressWithModifier( 'Shift', 'Tab' );

expect( await isCloseButtonFocused() ).toBe( true );
} );
} );