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

Jump to bottom

[ButtonBase] Use fork of focus-visible polyfill #15484

Merged
merged 9 commits into from
Apr 29, 2019
Merged

[ButtonBase] Use fork of focus-visible polyfill #15484

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2df809e
[ButtonBase] Use fork of focus-visible polyfill
eps1lon Apr 25, 2019
7e14cd4
Add listeners to owner document
eps1lon Apr 29, 2019
a19306f
leave imperative focus visible untouched
eps1lon Apr 29, 2019
7898a80
handleBlur -> handleBlurVisible
eps1lon Apr 29, 2019
6199ab1
Use :focus-visible selector if available
eps1lon Apr 29, 2019
e4a1928
Remove instanceof error checks
eps1lon Apr 29, 2019
33e07e2
Fix tests
eps1lon Apr 29, 2019
6037fe3
Fix shadowRoot test
eps1lon Apr 29, 2019
7758875
Merge branch 'next' into fix/focus-visible
eps1lon Apr 29, 2019
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
47 changes: 12 additions & 35 deletions packages/material-ui/src/ButtonBase/ButtonBase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@ import PropTypes from 'prop-types';
import ReactDOM from 'react-dom';
import clsx from 'clsx';
import { elementTypeAcceptingRef } from '@material-ui/utils';
import ownerWindow from '../utils/ownerWindow';
import withForwardedRef from '../utils/withForwardedRef';
import { setRef } from '../utils/reactHelpers';
import withStyles from '../styles/withStyles';
import NoSsr from '../NoSsr';
import { listenForFocusKeys, detectFocusVisible } from './focusVisible';
import { handleBlurVisible, isFocusVisible, prepare as prepareFocusVisible } from './focusVisible';
import TouchRipple from './TouchRipple';
import createRippleHandler from './createRippleHandler';

Expand Down Expand Up @@ -60,18 +59,7 @@ class ButtonBase extends React.Component {

buttonRef = React.createRef();

keyDown = false; // Used to help track keyboard activation keyDown

focusVisibleCheckTime = 50;

focusVisibleMaxCheckTimes = 5;

handleMouseDown = createRippleHandler(this, 'MouseDown', 'start', () => {
clearTimeout(this.focusVisibleTimeout);
if (this.state.focusVisible) {
this.setState({ focusVisible: false });
}
});
handleMouseDown = createRippleHandler(this, 'MouseDown', 'start');

handleMouseUp = createRippleHandler(this, 'MouseUp', 'stop');

Expand All @@ -89,16 +77,15 @@ class ButtonBase extends React.Component {

handleContextMenu = createRippleHandler(this, 'ContextMenu', 'stop');

handleBlur = createRippleHandler(this, 'Blur', 'stop', () => {
clearTimeout(this.focusVisibleTimeout);
handleBlur = createRippleHandler(this, 'Blur', 'stop', event => {
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
if (this.state.focusVisible) {
handleBlurVisible(event);
this.setState({ focusVisible: false });
}
});

componentDidMount() {
listenForFocusKeys(ownerWindow(this.getButtonNode()));

prepareFocusVisible(this.getButtonNode().ownerDocument);
Copy link
Contributor

Choose a reason for hiding this comment

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

@eps1lon It does look like this is going to add a set of document listeners for each ButtonBase mount. Should be able to short-circuit within focusVisible.js prepare function by tracking in a variable whether or not the listeners have already been added to the document.

Copy link
Member

Choose a reason for hiding this comment

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

Should we handle it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does look like this is going to add a set of document listeners for each ButtonBase mount.

addEventListener does not add duplicate listeners. Unless the focusVisible.js module is recreated for every ButtonBase instance they shouldn't be duplicated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the education.

if (this.props.action) {
this.props.action({
focusVisible: () => {
Expand All @@ -120,10 +107,6 @@ class ButtonBase extends React.Component {
}
}

componentWillUnmount() {
clearTimeout(this.focusVisibleTimeout);
}

getButtonNode() {
return ReactDOM.findDOMNode(this.buttonRef.current);
}
Expand All @@ -132,15 +115,6 @@ class ButtonBase extends React.Component {
this.ripple = node;
};

onFocusVisibleHandler = event => {
this.keyDown = false;
this.setState({ focusVisible: true });

if (this.props.onFocusVisible) {
this.props.onFocusVisible(event);
}
};

static getDerivedStateFromProps(nextProps, prevState) {
if (typeof prevState.focusVisible === 'undefined') {
return {
Expand Down Expand Up @@ -224,10 +198,13 @@ class ButtonBase extends React.Component {
this.buttonRef.current = event.currentTarget;
}

event.persist();
detectFocusVisible(this, this.getButtonNode(), () => {
this.onFocusVisibleHandler(event);
});
if (isFocusVisible(event)) {
this.setState({ focusVisible: true });

if (this.props.onFocusVisible) {
this.props.onFocusVisible(event);
}
}

if (this.props.onFocus) {
this.props.onFocus(event);
Expand Down
105 changes: 43 additions & 62 deletions packages/material-ui/src/ButtonBase/ButtonBase.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,17 @@ import ButtonBase from './ButtonBase';

const ButtonBaseNaked = unwrap(ButtonBase);

function focusVisible(element) {
element.ownerDocument.dispatchEvent(new window.Event('keydown'));
element.focus();
}

function simulatePointerDevice() {
// first focus on a page triggers focus visible until a pointer event
// has been dispatched
document.dispatchEvent(new window.Event('pointerdown'));
}

describe('<ButtonBase />', () => {
let mount;
let shallow;
Expand Down Expand Up @@ -270,55 +281,36 @@ describe('<ButtonBase />', () => {
return;
}

let wrapper;
let instance;
let button;
let clock;
let rootElement;

beforeEach(() => {
clock = useFakeTimers();
rootElement = document.createElement('div');
rootElement.tabIndex = 0;
document.body.appendChild(rootElement);
rootElement.attachShadow({ mode: 'open' });
wrapper = mount(<ButtonBase id="test-button">Hello</ButtonBase>, {
attachTo: rootElement.shadowRoot,
});
instance = wrapper.find('ButtonBase').instance();
button = rootElement.shadowRoot.getElementById('test-button');
if (!button) {
throw new Error('missing button');
}

button.focus();

if (document.activeElement !== rootElement) {
// Mock activeElement value and simulate host-retargeting in shadow root for
// jsdom@12.0.0 (https://github.com/jsdom/jsdom/issues/2343)
rootElement.focus();
rootElement.shadowRoot.activeElement = button;
wrapper.simulate('focus');
}

const event = new window.Event('keyup');
event.keyCode = 9; // Tab
window.dispatchEvent(event);
});

afterEach(() => {
clock.restore();
ReactDOM.unmountComponentAtNode(rootElement.shadowRoot);
document.body.removeChild(rootElement);
});

it('should set focus state for shadowRoot children', () => {
assert.strictEqual(wrapper.find(`.${classes.focusVisible}`).exists(), false);
mount(<ButtonBase id="test-button">Hello</ButtonBase>, {
attachTo: rootElement.shadowRoot,
});
simulatePointerDevice();

clock.tick(instance.focusVisibleCheckTime * instance.focusVisibleMaxCheckTimes);
wrapper.update();
const button = rootElement.shadowRoot.getElementById('test-button');
if (!button) {
throw new Error('missing button');
}

assert.strictEqual(button.classList.contains(classes.focusVisible), false);

focusVisible(button);

assert.strictEqual(wrapper.find(`.${classes.focusVisible}`).exists(), true);
assert.strictEqual(button.classList.contains(classes.focusVisible), true);
});
});

Expand All @@ -338,17 +330,20 @@ describe('<ButtonBase />', () => {

beforeEach(() => {
clock = useFakeTimers();
wrapper = mount(<ButtonBase>Hello</ButtonBase>);
wrapper = mount(
<ButtonBase
ref={element => {
button = element;
}}
>
Hello
</ButtonBase>,
);
simulatePointerDevice();
instance = wrapper.find('ButtonBase').instance();
button = wrapper.find('button').getDOMNode();
if (!button) {
throw new Error('missing button');
}
button.focus();

const event = new window.Event('keyup');
event.keyCode = 9; // Tab
window.dispatchEvent(event);
});

afterEach(() => {
Expand All @@ -357,25 +352,9 @@ describe('<ButtonBase />', () => {

it('should detect the keyboard', () => {
assert.strictEqual(getState().focusVisible, false);
clock.tick(instance.focusVisibleCheckTime * instance.focusVisibleMaxCheckTimes);
focusVisible(button);
assert.strictEqual(getState().focusVisible, true);
});

it('should ignore the keyboard after 1s', () => {
Copy link
Member

@oliviertassinari oliviertassinari Apr 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We changed the 1s duration at some point, entropy 👋

Copy link
Member Author

Choose a reason for hiding this comment

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

There's alot of behavior that isn't tested right now in our codebase. But this is a fork so it is actually tested somewhere. Just need to make sure we watch for releases of the base.

clock.tick(instance.focusVisibleCheckTime * instance.focusVisibleMaxCheckTimes);
assert.strictEqual(getState().focusVisible, true);
button.blur();
assert.strictEqual(getState().focusVisible, false);
button.focus();
clock.tick(instance.focusVisibleCheckTime * instance.focusVisibleMaxCheckTimes);
assert.strictEqual(getState().focusVisible, true);
clock.tick(1e3);
button.blur();
assert.strictEqual(getState().focusVisible, false);
button.focus();
clock.tick(instance.focusVisibleCheckTime * instance.focusVisibleMaxCheckTimes);
assert.strictEqual(getState().focusVisible, false);
});
});

describe('prop: disabled', () => {
Expand Down Expand Up @@ -450,17 +429,19 @@ describe('<ButtonBase />', () => {
});

it('onFocusVisibleHandler() should propagate call to onFocusVisible prop', () => {
const eventMock = 'woofButtonBase';
const onFocusVisibleSpy = spy();
const wrapper = mount(
<ButtonBase component="span" onFocusVisible={onFocusVisibleSpy}>
const buttonRef = React.createRef();
mount(
<ButtonBase component="span" onFocusVisible={onFocusVisibleSpy} ref={buttonRef}>
Hello
</ButtonBase>,
);
const instance = wrapper.find('ButtonBase').instance();
instance.onFocusVisibleHandler(eventMock);
simulatePointerDevice();

focusVisible(buttonRef.current);

assert.strictEqual(onFocusVisibleSpy.callCount, 1);
assert.strictEqual(onFocusVisibleSpy.calledWith(eventMock), true);
assert.strictEqual(onFocusVisibleSpy.firstCall.args.length, 1);
});

it('should work with a functional component', () => {
Expand Down
Loading