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

[react-events] Keyboard support for virtual clicks #16780

Merged
merged 1 commit into from
Sep 16, 2019
Merged

[react-events] Keyboard support for virtual clicks #16780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
52 changes: 34 additions & 18 deletions packages/react-events/src/dom/Keyboard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,20 @@ import type {
ReactDOMResponderEvent,
ReactDOMResponderContext,
} from 'shared/ReactDOMTypes';
import type {ReactEventResponderListener} from 'shared/ReactTypes';

import React from 'react';
import {DiscreteEvent} from 'shared/ReactTypes';
import type {ReactEventResponderListener} from 'shared/ReactTypes';
import {isVirtualClick} from './shared';

type KeyboardEventType = 'keyboard:keydown' | 'keyboard:keyup';
type KeyboardEventType =
| 'keyboard:click'
| 'keyboard:keydown'
| 'keyboard:keyup';

type KeyboardProps = {|
disabled?: boolean,
onClick?: (e: KeyboardEvent) => ?boolean,
onKeyDown?: (e: KeyboardEvent) => ?boolean,
onKeyUp?: (e: KeyboardEvent) => ?boolean,
preventKeys?: PreventKeysArray,
Expand All @@ -34,8 +39,8 @@ export type KeyboardEvent = {|
altKey: boolean,
ctrlKey: boolean,
defaultPrevented: boolean,
isComposing: boolean,
key: string,
isComposing?: boolean,
key?: string,
metaKey: boolean,
pointerType: 'keyboard',
shiftKey: boolean,
Expand Down Expand Up @@ -120,10 +125,6 @@ const translateToKey = {
'224': 'Meta',
};

function isFunction(obj): boolean {
return typeof obj === 'function';
}

function getEventKey(nativeEvent: Object): string {
const nativeKey = nativeEvent.key;
if (nativeKey) {
Expand All @@ -147,21 +148,24 @@ function createKeyboardEvent(
defaultPrevented: boolean,
): KeyboardEvent {
const nativeEvent = (event: any).nativeEvent;
const {altKey, ctrlKey, isComposing, metaKey, shiftKey} = nativeEvent;

return {
const {altKey, ctrlKey, metaKey, shiftKey} = nativeEvent;
let keyboardEvent = {
altKey,
ctrlKey,
defaultPrevented,
isComposing,
key: getEventKey(nativeEvent),
metaKey,
pointerType: 'keyboard',
shiftKey,
target: event.target,
timeStamp: context.getTimeStamp(),
type,
};
if (type !== 'keyboard:click') {
const key = getEventKey(nativeEvent);
const isComposing = nativeEvent.isComposing;
keyboardEvent = context.objectAssign({isComposing, key}, keyboardEvent);
}
return keyboardEvent;
}

function dispatchKeyboardEvent(
Expand Down Expand Up @@ -242,7 +246,7 @@ const keyboardResponderImpl = {
}
state.isActive = true;
const onKeyDown = props.onKeyDown;
if (isFunction(onKeyDown)) {
if (onKeyDown != null) {
dispatchKeyboardEvent(
event,
((onKeyDown: any): (e: KeyboardEvent) => ?boolean),
Expand All @@ -251,13 +255,25 @@ const keyboardResponderImpl = {
state.defaultPrevented,
);
}
} else if (type === 'click' && state.isActive && state.defaultPrevented) {
// 'click' occurs before 'keyup' and may need native behavior prevented
nativeEvent.preventDefault();
} else if (type === 'click' && isVirtualClick(event)) {
const onClick = props.onClick;
if (onClick != null) {
dispatchKeyboardEvent(
event,
onClick,
context,
'keyboard:click',
state.defaultPrevented,
);
}
if (state.defaultPrevented && !nativeEvent.defaultPrevented) {
// 'click' occurs before 'keyup' and may need native behavior prevented
nativeEvent.preventDefault();
}
} else if (type === 'keyup') {
state.isActive = false;
const onKeyUp = props.onKeyUp;
if (isFunction(onKeyUp)) {
if (onKeyUp != null) {
dispatchKeyboardEvent(
event,
((onKeyUp: any): (e: KeyboardEvent) => ?boolean),
Expand Down
98 changes: 89 additions & 9 deletions packages/react-events/src/dom/__tests__/Keyboard-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,21 @@ describe('Keyboard responder', () => {
});

function renderPropagationTest(propagates) {
const onClickInner = jest.fn(() => propagates);
const onKeyDownInner = jest.fn(() => propagates);
const onKeyDownOuter = jest.fn();
const onKeyUpInner = jest.fn(() => propagates);
const onClickOuter = jest.fn();
const onKeyDownOuter = jest.fn();
const onKeyUpOuter = jest.fn();
const ref = React.createRef();
const Component = () => {
const listenerInner = useKeyboard({
onClick: onClickInner,
onKeyDown: onKeyDownInner,
onKeyUp: onKeyUpInner,
});
const listenerOuter = useKeyboard({
onClick: onClickOuter,
onKeyDown: onKeyDownOuter,
onKeyUp: onKeyUpOuter,
});
Expand All @@ -63,19 +67,23 @@ describe('Keyboard responder', () => {
};
ReactDOM.render(<Component />, container);
return {
onClickInner,
onKeyDownInner,
onKeyDownOuter,
onKeyUpInner,
onClickOuter,
onKeyDownOuter,
onKeyUpOuter,
ref,
};
}

test('propagates event when a callback returns true', () => {
test('propagates key event when a callback returns true', () => {
const {
onClickInner,
onKeyDownInner,
onKeyDownOuter,
onKeyUpInner,
onClickOuter,
onKeyDownOuter,
onKeyUpOuter,
ref,
} = renderPropagationTest(true);
Expand All @@ -86,13 +94,18 @@ describe('Keyboard responder', () => {
target.keyup();
expect(onKeyUpInner).toBeCalled();
expect(onKeyUpOuter).toBeCalled();
target.virtualclick();
expect(onClickInner).toBeCalled();
expect(onClickOuter).toBeCalled();
});

test('does not propagate event when a callback returns false', () => {
test('does not propagate key event when a callback returns false', () => {
const {
onClickInner,
onKeyDownInner,
onKeyDownOuter,
onKeyUpInner,
onClickOuter,
onKeyDownOuter,
onKeyUpOuter,
ref,
} = renderPropagationTest(false);
Expand All @@ -103,6 +116,9 @@ describe('Keyboard responder', () => {
target.keyup();
expect(onKeyUpInner).toBeCalled();
expect(onKeyUpOuter).not.toBeCalled();
target.virtualclick();
expect(onClickInner).toBeCalled();
expect(onClickOuter).not.toBeCalled();
});

describe('disabled', () => {
Expand All @@ -128,6 +144,64 @@ describe('Keyboard responder', () => {
});
});

describe('onClick', () => {
let onClick, ref;

beforeEach(() => {
onClick = jest.fn();
ref = React.createRef();
const Component = () => {
const listener = useKeyboard({onClick});
return <div ref={ref} listeners={listener} />;
};
ReactDOM.render(<Component />, container);
});

// e.g, "Enter" on link
test('keyboard click is between key events', () => {
const target = createEventTarget(ref.current);
target.keydown({key: 'Enter'});
target.keyup({key: 'Enter'});
target.virtualclick();
expect(onClick).toHaveBeenCalledTimes(1);
expect(onClick).toHaveBeenCalledWith(
expect.objectContaining({
altKey: false,
ctrlKey: false,
defaultPrevented: false,
metaKey: false,
pointerType: 'keyboard',
shiftKey: false,
target: target.node,
timeStamp: expect.any(Number),
type: 'keyboard:click',
}),
);
});

// e.g., "Spacebar" on button
test('keyboard click is after key events', () => {
const target = createEventTarget(ref.current);
target.keydown({key: 'Enter'});
target.keyup({key: 'Enter'});
target.virtualclick();
expect(onClick).toHaveBeenCalledTimes(1);
expect(onClick).toHaveBeenCalledWith(
expect.objectContaining({
altKey: false,
ctrlKey: false,
defaultPrevented: false,
metaKey: false,
pointerType: 'keyboard',
shiftKey: false,
target: target.node,
timeStamp: expect.any(Number),
type: 'keyboard:click',
}),
);
});
});

describe('onKeyDown', () => {
let onKeyDown, ref;

Expand Down Expand Up @@ -271,7 +345,7 @@ describe('Keyboard responder', () => {

const target = createEventTarget(ref.current);
target.keydown({key: 'Tab', preventDefault});
target.click({preventDefault: preventDefaultClick});
target.virtualclick({preventDefault: preventDefaultClick});

expect(onKeyDown).toHaveBeenCalledTimes(1);
expect(preventDefault).toBeCalled();
Expand All @@ -293,7 +367,10 @@ describe('Keyboard responder', () => {

const target = createEventTarget(ref.current);
target.keydown({key: 'Tab', preventDefault, shiftKey: true});
target.click({preventDefault: preventDefaultClick, shiftKey: true});
target.virtualclick({
preventDefault: preventDefaultClick,
shiftKey: true,
});

expect(onKeyDown).toHaveBeenCalledTimes(1);
expect(preventDefault).toBeCalled();
Expand All @@ -316,7 +393,10 @@ describe('Keyboard responder', () => {

const target = createEventTarget(ref.current);
target.keydown({key: 'Tab', preventDefault, shiftKey: false});
target.click({preventDefault: preventDefaultClick, shiftKey: false});
target.virtualclick({
preventDefault: preventDefaultClick,
shiftKey: false,
});

expect(onKeyDown).toHaveBeenCalledTimes(1);
expect(preventDefault).not.toBeCalled();
Expand Down
14 changes: 14 additions & 0 deletions packages/react-events/src/dom/shared/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,17 @@ export function hasModifierKey(event: ReactDOMResponderEvent): boolean {
altKey === true || ctrlKey === true || metaKey === true || shiftKey === true
);
}

// Keyboards, Assitive Technologies, and element.click() all produce "virtual"
// clicks that do not include coordinates and "detail" is always 0 (where
// pointer clicks are > 0).
export function isVirtualClick(event: ReactDOMResponderEvent): boolean {
const nativeEvent: any = event.nativeEvent;
return (
nativeEvent.detail === 0 &&
nativeEvent.screenX === 0 &&
nativeEvent.screenY === 0 &&
nativeEvent.clientX === 0 &&
nativeEvent.clientY === 0
);
}
Copy link

@craigkovatch craigkovatch Sep 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

(Not sure what the etiquette on a merged PR is -- please let me know if these comments would be more appropriate in e.g. a new Issue. cc @necolas @trueadm )

I don't believe this works cross-browser.

  1. in IE(11), detail is always 0, even for mouse-initiated click events. Can check for !nativeEvent.pointerType to work around this -- all click events in IE11 are PointerEvent, but only real pointer events have pointerType.

  2. in Safari, the screen and client coordinates are present even for a keyboard-initiated click. The client coordinates seem to default to the "center" of the element.

What I've used internally at Tableau for this that works cross-browser is return (nativeEvent.detail === 0 && !nativeEvent.pointerType);

Copy link
Contributor

Choose a reason for hiding this comment

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

This is really awesome feedback. Thank you cc @necolas seems like a valid thing we should be doing too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does (nativeEvent.detail === 0 && !nativeEvent.pointerType) work if that evaluates as true for mouse events as well as virtual clicks?

Copy link

@craigkovatch craigkovatch Sep 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

(nativeEvent.detail === 0 && !nativeEvent.pointerType) evaluates to true for "keyboard" clicks, not for mouse events.

My investigation showed that nativeEvent.detail === 0 is sufficient for all evergreen browsers; adding !nativeEvent.pointerEvent covers the IE11 case. Is that more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still don't follow. The click event is of type MouseEvent which has no pointerType property. Are you saying IE11's click event is of type PointerEvent?

Copy link

@craigkovatch craigkovatch Sep 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Are you saying IE11's click event is of type PointerEvent?

Yes, exactly. !nativeEvent.pointerType will be true for all other browsers in all scenarios, and true for IE11 when it's a keyboard-initiated click.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for confirming; we didn't know about that quirk. We'll be sure to incorporate your suggested fix. I like what Safari does here. I'm also curious to hear what you use this check for at Tableau

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#16915

Copy link

Choose a reason for hiding this comment

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

I'm also curious to hear what you use this check for at Tableau

It's a long, annoying story -- the short version is there an event normalizer (parallel to React) sitting on top of touch events that interferes with native keyboard click events on button elements. I needed this to differentiate the event sources so it could ignore those from the keyboard.

Copy link

@craigkovatch craigkovatch Nov 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@necolas little bit of unfortunate news -- it appears detail === 0 also for click events which are dispatched to an <input> when clicked on a wrapping <label>, even when clicking with a mouse. Not sure if this is a problem for your purposes, but wanted to give you a heads-up.