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
51 changes: 36 additions & 15 deletions packages/react-interactions/accessibility/src/FocusList.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ type FocusListProps = {|
portrait: boolean,
wrap?: boolean,
tabScope?: ReactScope,
allowModifiers?: boolean,
|};

const {useRef} = React;
Expand Down Expand Up @@ -82,6 +83,13 @@ function getListProps(currentCell: ReactScopeMethods): Object {
return {};
}

function hasModifierKey(event: KeyboardEvent): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is already in the 'shared' module

Copy link
Contributor Author

@trueadm trueadm Oct 1, 2019

Choose a reason for hiding this comment

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

I originally used that thinking the exact same thing, but it's actually slightly different as it expects a different event object (that it reads nativeEvent from). I'm not too bothered about duplication here, we can tidy up once we're happy with the moving parts. :)

const {altKey, ctrlKey, metaKey, shiftKey} = event;
return (
altKey === true || ctrlKey === true || metaKey === true || shiftKey === true
);
}

export function createFocusList(scope: ReactScope): Array<React.Component> {
const TableScope = React.unstable_createScope(scope.fn);

Expand All @@ -90,14 +98,16 @@ export function createFocusList(scope: ReactScope): Array<React.Component> {
portrait,
wrap,
tabScope: TabScope,
allowModifiers,
}): FocusListProps {
const tabScopeRef = useRef(null);
return (
<TableScope
type="list"
portrait={portrait}
wrap={wrap}
tabScopeRef={tabScopeRef}>
tabScopeRef={tabScopeRef}
allowModifiers={allowModifiers}>
{TabScope ? (
<TabScope ref={tabScopeRef}>{children}</TabScope>
) : (
Expand All @@ -117,25 +127,36 @@ export function createFocusList(scope: ReactScope): Array<React.Component> {
const listProps = list && list.getProps();
if (list !== null && listProps.type === 'list') {
const portrait = listProps.portrait;
switch (event.key) {
case 'Tab': {
const tabScope = getListProps(currentItem).tabScopeRef.current;
if (tabScope) {
const activeNode = document.activeElement;
const nodes = tabScope.getScopedNodes();
for (let i = 0; i < nodes.length; i++) {
const node = nodes[i];
if (node !== activeNode) {
setElementCanTab(node, false);
} else {
setElementCanTab(node, true);
}
const key = event.key;

if (key === 'Tab') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that "shift" is a modifier, and it impacts "Tab" (reverses the direction) it seems odd that we check for key === 'Tab' before bailing out if modifiers aren't allowed. Is this a bug?

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 modifiers logic is only for the keyboard arrow keys, not for Tab.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. That wasn't super clear to me just looking at the code and going off of the name of the prop. Maybe some inline comment clarifying the intent could be helpful?

const tabScope = getListProps(currentItem).tabScopeRef.current;
if (tabScope) {
const activeNode = document.activeElement;
const nodes = tabScope.getScopedNodes();
for (let i = 0; i < nodes.length; i++) {
const node = nodes[i];
if (node !== activeNode) {
setElementCanTab(node, false);
} else {
setElementCanTab(node, true);
}
return;
}
return;
}
event.continuePropagation();
return;
}
// Using modifier keys with keyboard arrow events should be no-ops
// unless an explicit allowModifiers flag is set on the FocusList.
if (hasModifierKey(event)) {
const allowModifiers = getListProps(currentItem).allowModifiers;
if (!allowModifiers) {
event.continuePropagation();
return;
}
}
switch (key) {
case 'ArrowUp': {
if (portrait) {
const previousListItem = getPreviousListItem(
Expand Down
50 changes: 35 additions & 15 deletions packages/react-interactions/accessibility/src/FocusTable.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ type FocusTableProps = {|
) => void,
wrap?: boolean,
tabScope?: ReactScope,
allowModifiers?: boolean,
|};

const {useRef} = React;
Expand Down Expand Up @@ -152,6 +153,13 @@ function getTableProps(currentCell: ReactScopeMethods): Object {
return {};
}

function hasModifierKey(event: KeyboardEvent): boolean {
const {altKey, ctrlKey, metaKey, shiftKey} = event;
return (
altKey === true || ctrlKey === true || metaKey === true || shiftKey === true
);
}

export function createFocusTable(scope: ReactScope): Array<React.Component> {
const TableScope = React.unstable_createScope(scope.fn);

Expand All @@ -161,6 +169,7 @@ export function createFocusTable(scope: ReactScope): Array<React.Component> {
id,
wrap,
tabScope: TabScope,
allowModifiers,
}): FocusTableProps {
const tabScopeRef = useRef(null);
return (
Expand All @@ -169,7 +178,8 @@ export function createFocusTable(scope: ReactScope): Array<React.Component> {
onKeyboardOut={onKeyboardOut}
id={id}
wrap={wrap}
tabScopeRef={tabScopeRef}>
tabScopeRef={tabScopeRef}
allowModifiers={allowModifiers}>
{TabScope ? (
<TabScope ref={tabScopeRef}>{children}</TabScope>
) : (
Expand All @@ -192,25 +202,35 @@ export function createFocusTable(scope: ReactScope): Array<React.Component> {
event.continuePropagation();
return;
}
switch (event.key) {
case 'Tab': {
const tabScope = getTableProps(currentCell).tabScopeRef.current;
if (tabScope) {
const activeNode = document.activeElement;
const nodes = tabScope.getScopedNodes();
for (let i = 0; i < nodes.length; i++) {
const node = nodes[i];
if (node !== activeNode) {
setElementCanTab(node, false);
} else {
setElementCanTab(node, true);
}
const key = event.key;
if (key === 'Tab') {
const tabScope = getTableProps(currentCell).tabScopeRef.current;
if (tabScope) {
const activeNode = document.activeElement;
const nodes = tabScope.getScopedNodes();
for (let i = 0; i < nodes.length; i++) {
const node = nodes[i];
if (node !== activeNode) {
setElementCanTab(node, false);
} else {
setElementCanTab(node, true);
}
return;
}
return;
}
event.continuePropagation();
return;
}
// Using modifier keys with keyboard arrow events should be no-ops
// unless an explicit allowModifiers flag is set on the FocusTable.
if (hasModifierKey(event)) {
const allowModifiers = getTableProps(currentCell).allowModifiers;
if (!allowModifiers) {
event.continuePropagation();
return;
}
}
switch (key) {
case 'ArrowUp': {
const [cells, cellIndex] = getRowCells(currentCell);
if (cells !== null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,8 @@
* @flow
*/

import {
createEventTarget,
emulateBrowserTab,
} from 'react-interactions/events/src/dom/testing-library';
import {createEventTarget} from 'react-interactions/events/src/dom/testing-library';
import {emulateBrowserTab} from '../emulateBrowserTab';

let React;
let ReactFeatureFlags;
Expand Down Expand Up @@ -46,8 +44,11 @@ describe('FocusList', () => {
function createFocusListComponent() {
const [FocusList, FocusItem] = createFocusList(TabbableScope);

return ({portrait, wrap}) => (
<FocusList portrait={portrait} wrap={wrap}>
return ({portrait, wrap, allowModifiers}) => (
<FocusList
portrait={portrait}
wrap={wrap}
allowModifiers={allowModifiers}>
<ul>
<FocusItem>
<li tabIndex={0}>Item 1</li>
Expand Down Expand Up @@ -94,6 +95,12 @@ describe('FocusList', () => {
key: 'ArrowLeft',
});
expect(document.activeElement.textContent).toBe('Item 3');
// Should be a no-op due to modifier
thirdListItem.keydown({
key: 'ArrowUp',
altKey: true,
});
expect(document.activeElement.textContent).toBe('Item 3');
});

it('handles keyboard arrow operations (landscape)', () => {
Expand Down Expand Up @@ -160,6 +167,23 @@ describe('FocusList', () => {
expect(document.activeElement.textContent).toBe('Item 3');
});

it('handles keyboard arrow operations (portrait) with allowModifiers', () => {
const Test = createFocusListComponent();

ReactDOM.render(
<Test portrait={true} allowModifiers={true} />,
container,
);
const listItems = document.querySelectorAll('li');
let firstListItem = createEventTarget(listItems[0]);
firstListItem.focus();
firstListItem.keydown({
key: 'ArrowDown',
altKey: true,
});
expect(document.activeElement.textContent).toBe('Item 2');
});

it('handles keyboard arrow operations mixed with tabbing', () => {
const [FocusList, FocusItem] = createFocusList(TabbableScope);
const beforeRef = React.createRef();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,8 @@
* @flow
*/

import {
createEventTarget,
emulateBrowserTab,
} from 'react-interactions/events/src/dom/testing-library';
import {createEventTarget} from 'react-interactions/events/src/dom/testing-library';
import {emulateBrowserTab} from '../emulateBrowserTab';

let React;
let ReactFeatureFlags;
Expand Down Expand Up @@ -48,8 +46,12 @@ describe('FocusTable', () => {
TabbableScope,
);

return ({onKeyboardOut, id, wrap}) => (
<FocusTable onKeyboardOut={onKeyboardOut} id={id} wrap={wrap}>
return ({onKeyboardOut, id, wrap, allowModifiers}) => (
<FocusTable
onKeyboardOut={onKeyboardOut}
id={id}
wrap={wrap}
allowModifiers={allowModifiers}>
<table>
<tbody>
<FocusTableRow>
Expand Down Expand Up @@ -115,6 +117,20 @@ describe('FocusTable', () => {
);
}

it('handles keyboard arrow operations with allowModifiers', () => {
const Test = createFocusTableComponent();

ReactDOM.render(<Test allowModifiers={true} />, container);
const buttons = document.querySelectorAll('button');
const a1 = createEventTarget(buttons[0]);
a1.focus();
a1.keydown({
key: 'ArrowRight',
altKey: true,
});
expect(document.activeElement.textContent).toBe('A2');
});

it('handles keyboard arrow operations', () => {
const Test = createFocusTableComponent();

Expand All @@ -133,7 +149,7 @@ describe('FocusTable', () => {
});
expect(document.activeElement.textContent).toBe('B2');

const b2 = createEventTarget(document.activeElement);
let b2 = createEventTarget(document.activeElement);
b2.keydown({
key: 'ArrowLeft',
});
Expand All @@ -154,6 +170,14 @@ describe('FocusTable', () => {
key: 'ArrowUp',
});
expect(document.activeElement.textContent).toBe('B1');
// Should be a no-op due to modifier
b2 = createEventTarget(document.activeElement);
b2.keydown({
key: 'ArrowUp',
altKey: true,
});
b2 = createEventTarget(document.activeElement);
expect(document.activeElement.textContent).toBe('B1');
});

it('handles keyboard arrow operations between tables', () => {
Expand Down
40 changes: 40 additions & 0 deletions packages/react-interactions/accessibility/src/emulateBrowserTab.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
*/

'use strict';

import {createEventTarget} from 'react-interactions/events/src/dom/testing-library';

// This function is used by the a11y modules for testing
export function emulateBrowserTab(backwards) {
const activeElement = document.activeElement;
const focusedElem = createEventTarget(activeElement);
let defaultPrevented = false;
focusedElem.keydown({
key: 'Tab',
shiftKey: backwards,
preventDefault() {
defaultPrevented = true;
},
});
if (!defaultPrevented) {
// This is not a full spec compliant version, but should be suffice for this test
const focusableElems = Array.from(
document.querySelectorAll(
'input, button, select, textarea, a[href], [tabindex], [contenteditable], iframe, object, embed',
),
).filter(
elem => elem.tabIndex > -1 && !elem.disabled && !elem.contentEditable,
);
const idx = focusableElems.indexOf(activeElement);
if (idx !== -1) {
focusableElems[backwards ? idx - 1 : idx + 1].focus();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -158,33 +158,6 @@ function testWithPointerType(message, testFn) {
});
}

function emulateBrowserTab(backwards) {
const activeElement = document.activeElement;
const focusedElem = createEventTarget(activeElement);
let defaultPrevented = false;
focusedElem.keydown({
key: 'Tab',
shiftKey: backwards,
preventDefault() {
defaultPrevented = true;
},
});
if (!defaultPrevented) {
// This is not a full spec compliant version, but should be suffice for this test
const focusableElems = Array.from(
document.querySelectorAll(
'input, button, select, textarea, a[href], [tabindex], [contenteditable], iframe, object, embed',
),
).filter(
elem => elem.tabIndex > -1 && !elem.disabled && !elem.contentEditable,
);
const idx = focusableElems.indexOf(activeElement);
if (idx !== -1) {
focusableElems[backwards ? idx - 1 : idx + 1].focus();
}
}
}

export {
buttonsType,
createEventTarget,
Expand All @@ -193,5 +166,4 @@ export {
hasPointerEvent,
setPointerEvent,
testWithPointerType,
emulateBrowserTab,
};