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

feat(components): support automatic focus sentinel #5260

Merged
merged 8 commits into from
Feb 14, 2020
Merged

feat(components): support automatic focus sentinel #5260

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1bbe349
feat(components): support automatic focus sentinel
asudoh Feb 2, 2020
b09d3d8
test(Modal): fix tests
asudoh Feb 4, 2020
c0cf32d
Merge branch 'master' into auto-focus-sentinel
tw15egan Feb 4, 2020
bf7bdc7
Merge branch 'master' into auto-focus-sentinel
tw15egan Feb 4, 2020
fb8d81d
Update packages/react/src/components/Modal/Modal.js
asudoh Feb 11, 2020
00d0804
chore(devenv): remove unused story
asudoh Feb 13, 2020
ca1e9c3
chore(Modal): rename variables
asudoh Feb 13, 2020
3ccfd96
Merge branch 'master' into auto-focus-sentinel
asudoh Feb 14, 2020
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
Binary file added .yarn/offline-mirror/lodash.findlast-4.6.0.tgz
View file
Open in desktop
Binary file not shown.
5 changes: 0 additions & 5 deletions packages/react/.storybook/Container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,6 @@ function Container({ story }) {
}}>
{story()}
</div>
<input
aria-label="input-text-offleft"
type="text"
className={`${prefix}--visually-hidden`}
/>
</React.StrictMode>
);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/react/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@
"classnames": "2.2.6",
"downshift": "^1.31.14",
"flatpickr": "4.6.1",
"focus-trap-react": "^6.0.0",
"invariant": "^2.2.3",
"lodash.debounce": "^4.0.8",
"lodash.findlast": "^4.5.0",
"lodash.isequal": "^4.5.0",
"lodash.omit": "^4.5.0",
"react-is": "^16.8.6",
Expand Down
70 changes: 38 additions & 32 deletions packages/react/src/components/ComposedModal/ComposedModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { settings } from 'carbon-components';
import { Close20 } from '@carbon/icons-react';
import toggleClass from '../../tools/toggleClass';
import requiredIfGivenPropExists from '../../prop-types/requiredIfGivenPropExists';
import wrapFocus from '../../internal/wrapFocus';

const { prefix } = settings;

Expand All @@ -27,6 +28,8 @@ export default class ComposedModal extends Component {
outerModal = React.createRef();
innerModal = React.createRef();
button = React.createRef();
startSentinel = React.createRef();
endSentinel = React.createRef();

static propTypes = {
/**
Expand Down Expand Up @@ -78,19 +81,6 @@ export default class ComposedModal extends Component {
};
}

elementOrParentIsFloatingMenu = target => {
const {
selectorsFloatingMenus = [
`.${prefix}--overflow-menu-options`,
`.${prefix}--tooltip`,
'.flatpickr-calendar',
],
} = this.props;
if (target && typeof target.closest === 'function') {
return selectorsFloatingMenus.some(selector => target.closest(selector));
}
};

handleKeyDown = evt => {
// Esc key
if (evt.which === 27) {
Expand All @@ -109,22 +99,23 @@ export default class ComposedModal extends Component {
}
};

focusModal = () => {
if (this.outerModal.current) {
this.outerModal.current.focus();
}
};

handleBlur = evt => {
// Keyboard trap
if (
this.innerModal.current &&
this.props.open &&
evt.relatedTarget &&
!this.innerModal.current.contains(evt.relatedTarget) &&
!this.elementOrParentIsFloatingMenu(evt.relatedTarget)
) {
this.focusModal();
handleBlur = ({
target: oldActiveNode,
relatedTarget: currentActiveNode,
}) => {
const { open, selectorsFloatingMenus } = this.props;
if (open && currentActiveNode && oldActiveNode) {
const { current: modalNode } = this.innerModal;
const { current: startSentinelNode } = this.startSentinel;
const { current: endSentinelNode } = this.endSentinel;
wrapFocus({
modalNode,
startSentinelNode,
endSentinelNode,
currentActiveNode,
oldActiveNode,
selectorsFloatingMenus,
});
}
};

Expand Down Expand Up @@ -240,11 +231,26 @@ export default class ComposedModal extends Component {
onClick={this.handleClick}
onKeyDown={this.handleKeyDown}
onTransitionEnd={open ? this.handleTransitionEnd : undefined}
className={modalClass}
tabIndex={-1}>
<div ref={this.innerModal} className={containerClass}>
className={modalClass}>
{/* Non-translatable: Focus-wrap code makes this `<span>` not actually read by screen readers */}
<span
ref={this.startSentinel}
tabIndex="0"
role="link"
className={`${prefix}--visually-hidden`}>
Focus sentinel
</span>
<div ref={this.innerModal} className={containerClass} tabIndex={-1}>
{childrenWithProps}
</div>
{/* Non-translatable: Focus-wrap code makes this `<span>` not actually read by screen readers */}
<span
ref={this.endSentinel}
tabIndex="0"
role="link"
className={`${prefix}--visually-hidden`}>
Focus sentinel
</span>
</div>
);
}
Expand Down
16 changes: 15 additions & 1 deletion packages/react/src/components/ComposedModal/__snapshots__/ComposedModal-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,25 @@ exports[`<ComposedModal /> renders 1`] = `
onTransitionEnd={[Function]}
open={true}
role="presentation"
tabIndex={-1}
>
<span
className="bx--visually-hidden"
role="link"
tabIndex="0"
>
Focus sentinel
</span>
<div
className="bx--modal-container"
tabIndex={-1}
/>
<span
className="bx--visually-hidden"
role="link"
tabIndex="0"
>
Focus sentinel
</span>
</div>
</ComposedModal>
`;
2 changes: 1 addition & 1 deletion packages/react/src/components/Modal/Modal-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { settings } from 'carbon-components';
const { prefix } = settings;

// The modal is the 0th child inside the wrapper on account of focus-trap-react
const getModal = wrapper => wrapper.childAt(0);
const getModal = wrapper => wrapper.find('.bx--modal');

describe('Modal', () => {
describe('Renders as expected', () => {
Expand Down
110 changes: 54 additions & 56 deletions packages/react/src/components/Modal/Modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,13 @@ import React, { Component } from 'react';
import classNames from 'classnames';
import { settings } from 'carbon-components';
import { Close20 } from '@carbon/icons-react';
import FocusTrap from 'focus-trap-react';
import toggleClass from '../../tools/toggleClass';
import Button from '../Button';
import deprecate from '../../prop-types/deprecate';
import requiredIfGivenPropExists from '../../prop-types/requiredIfGivenPropExists';
import wrapFocus, {
elementOrParentIsFloatingMenu,
} from '../../internal/wrapFocus';
import setupGetInstanceId from '../../tools/setupGetInstanceId';

const { prefix } = settings;
Expand Down Expand Up @@ -138,10 +141,13 @@ export default class Modal extends Component {
size: PropTypes.oneOf(['xs', 'sm', 'lg']),

/**
* Specify whether the modal should use 3rd party `focus-trap-react` for the focus-wrap feature.
* NOTE: by default this is true.
* Deprecated; Used for advanced focust-wrapping feature using 3rd party library,
asudoh marked this conversation as resolved.
Show resolved Hide resolved
* but it's now achieved without a 3rd party library.
*/
focusTrap: PropTypes.bool,
focusTrap: deprecate(
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're deprecating this prop, are we not going to give the user the ability to choose whether or not they want the modal focus-wrapped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for asking @abbeyhrt - Focus-wrapping behavior runs regardless of focusTrap property, even before it was introduced or after it was introduced. Some time ago the codebase introduced a third-party focus-trap-react library for better focus-wrapping behavior, but it introduced lots of side effects as the PR description of this PR links to. focusTrap property was introduced to disable such third-party library to avoid the side effects. This PR removes the property because the third-party is no longer needed for focus-wrapping.

PropTypes.bool,
`\nThe prop \`focusTrap\` for Modal has been deprecated, as the feature of \`persistent\` runs by default.`
),

/**
* Specify whether the modal contains scrolling content
Expand All @@ -167,30 +173,18 @@ export default class Modal extends Component {
modalHeading: '',
modalLabel: '',
selectorPrimaryFocus: '[data-modal-primary-focus]',
focusTrap: true,
hasScrollingContent: false,
};

button = React.createRef();
outerModal = React.createRef();
innerModal = React.createRef();
startSentinel = React.createRef();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a clearer plain-language way we can name this as opposed to "sentinel". It's difficult jargon to even Google a simple definition of. I assume you're meaning like a sentinel node, but there's probably a friendlier way to describe that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dakahn Hope this link shed some light: https://developers.google.com/web/fundamentals/accessibility/focus/using-tabindex

Copy link
Contributor

@dakahn dakahn Feb 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I understand the concept of sentinel elements in a focus-trap, but my question was if we could name these in plainer and simpler to understand language? firstElement and lastElement spring to mind.

If not that's cool too, but maybe a quick comment explaining the concept of sentinels to somebody who -- like myself -- has implemented wrapping focus but was totally unfamiliar with this term.

in a focus trap we have "sentinel" elements that designate the first and last elements in the tab order and signify when we should wrap

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "start trap" and "end trap"?

endSentinel = React.createRef();
modalInstanceId = `modal-${getInstanceId()}`;
modalLabelId = `${prefix}--modal-header__label--${this.modalInstanceId}`;
modalHeadingId = `${prefix}--modal-header__heading--${this.modalInstanceId}`;

elementOrParentIsFloatingMenu = target => {
const {
selectorsFloatingMenus = [
`.${prefix}--overflow-menu-options`,
`.${prefix}--tooltip`,
'.flatpickr-calendar',
],
} = this.props;
if (target && typeof target.closest === 'function') {
return selectorsFloatingMenus.some(selector => target.closest(selector));
}
};

handleKeyDown = evt => {
if (this.props.open) {
if (evt.which === 27) {
Expand All @@ -206,28 +200,32 @@ export default class Modal extends Component {
if (
this.innerModal.current &&
!this.innerModal.current.contains(evt.target) &&
!this.elementOrParentIsFloatingMenu(evt.target)
!elementOrParentIsFloatingMenu(
evt.target,
this.props.selectorsFloatingMenus
)
) {
this.props.onRequestClose(evt);
}
};

focusModal = () => {
if (this.outerModal.current) {
this.outerModal.current.focus();
}
};

handleBlur = evt => {
// Keyboard trap
if (
this.innerModal.current &&
this.props.open &&
evt.relatedTarget &&
!this.innerModal.current.contains(evt.relatedTarget) &&
!this.elementOrParentIsFloatingMenu(evt.relatedTarget)
) {
this.focusModal();
handleBlur = ({
target: oldActiveNode,
relatedTarget: currentActiveNode,
}) => {
const { open, selectorsFloatingMenus } = this.props;
if (open && currentActiveNode && oldActiveNode) {
const { current: modalNode } = this.innerModal;
const { current: startSentinelNode } = this.startSentinel;
const { current: endSentinelNode } = this.endSentinel;
wrapFocus({
modalNode,
startSentinelNode,
endSentinelNode,
currentActiveNode,
oldActiveNode,
selectorsFloatingMenus,
});
}
};

Expand Down Expand Up @@ -277,9 +275,7 @@ export default class Modal extends Component {
if (!this.props.open) {
return;
}
if (!this.props.focusTrap) {
this.focusButton(this.innerModal.current);
}
this.focusButton(this.innerModal.current);
}

handleTransitionEnd = evt => {
Expand All @@ -290,9 +286,7 @@ export default class Modal extends Component {
this.outerModal.current.offsetHeight &&
this.beingOpen
) {
if (!this.props.focusTrap) {
this.focusButton(evt.currentTarget);
}
this.focusButton(evt.currentTarget);
this.beingOpen = false;
}
};
Expand All @@ -317,7 +311,6 @@ export default class Modal extends Component {
selectorsFloatingMenus, // eslint-disable-line
shouldSubmitOnEnter, // eslint-disable-line
size,
focusTrap,
hasScrollingContent,
...other
} = this.props;
Expand Down Expand Up @@ -379,7 +372,8 @@ export default class Modal extends Component {
role="dialog"
className={containerClasses}
aria-label={ariaLabel}
aria-modal="true">
aria-modal="true"
tabIndex="-1">
<div className={`${prefix}--modal-header`}>
{passiveModal && modalButton}
{modalLabel && (
Expand Down Expand Up @@ -422,30 +416,34 @@ export default class Modal extends Component {
</div>
);

const modal = (
return (
<div
{...other}
onKeyDown={this.handleKeyDown}
onMouseDown={this.handleMousedown}
onBlur={this.handleBlur}
className={modalClasses}
role="presentation"
tabIndex={-1}
onTransitionEnd={this.props.open ? this.handleTransitionEnd : undefined}
ref={this.outerModal}>
{/* Non-translatable: Focus-wrap code makes this `<span>` not actually read by screen readers */}
<span
ref={this.startSentinel}
tabIndex="0"
role="link"
className={`${prefix}--visually-hidden`}>
Focus sentinel
</span>
{modalBody}
{/* Non-translatable: Focus-wrap code makes this `<span>` not actually read by screen readers */}
<span
ref={this.endSentinel}
tabIndex="0"
role="link"
className={`${prefix}--visually-hidden`}>
Focus sentinel
</span>
</div>
);

return !focusTrap ? (
modal
) : (
// `<FocusTrap>` has `active: true` in its `defaultProps`
<FocusTrap
active={!!open}
focusTrapOptions={{ initialFocus: this.initialFocus }}>
{modal}
</FocusTrap>
);
}
}
Loading