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.

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 responder propagation handling #16657

Merged
merged 3 commits into from
Sep 4, 2019
Merged
Show file tree
Hide file tree
Changes from 2 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
8 changes: 3 additions & 5 deletions packages/legacy-events/ReactGenericBatching.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ import {
} from './ReactControlledComponent';
import {enableFlareAPI} from 'shared/ReactFeatureFlags';

import {invokeGuardedCallbackAndCatchFirstError} from 'shared/ReactErrorUtils';

// Used as a way to call batchedUpdates when we don't have a reference to
// the renderer. Such as when we're dispatching events or if third party
// libraries need to call batchedUpdates. Eventually, this API will go away when
Expand Down Expand Up @@ -77,12 +75,12 @@ export function batchedEventUpdates(fn, a, b) {
}
}

export function executeUserEventHandler(fn: any => void, value: any) {
// This is for the React Flare event system
export function executeUserEventHandler(fn: any => void, value: any): any {
const previouslyInEventHandler = isInsideEventHandler;
try {
isInsideEventHandler = true;
const type = typeof value === 'object' && value !== null ? value.type : '';
invokeGuardedCallbackAndCatchFirstError(type, fn, undefined, value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a follow-up to fix this? It seems important that event handler doesn't crash the rest of the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I plan on addressing it this week.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was already fixed in a follow-up right? This guard is still there on master

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LOL. I did, I’m clearly losing the plot.

return fn(value);
} finally {
isInsideEventHandler = previouslyInEventHandler;
}
Expand Down
29 changes: 21 additions & 8 deletions packages/react-dom/src/events/DOMEventResponderSystem.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,41 +75,44 @@ const rootEventTypesToEventResponderInstances: Map<
Set<ReactDOMEventResponderInstance>,
> = new Map();

type PropagationBehavior = 0 | 1;

const DoNotPropagateToNextResponder = 0;
const PropagateToNextResponder = 1;

let currentTimeStamp = 0;
let currentTimers = new Map();
let currentInstance: null | ReactDOMEventResponderInstance = null;
let currentTimerIDCounter = 0;
let currentDocument: null | Document = null;
let currentPropagationBehavior: PropagationBehavior = DoNotPropagateToNextResponder;

const eventResponderContext: ReactDOMResponderContext = {
dispatchEvent(
eventValue: any,
eventListener: any => void,
eventPriority: EventPriority,
): void {
): any {
validateResponderContext();
validateEventValue(eventValue);
switch (eventPriority) {
case DiscreteEvent: {
flushDiscreteUpdatesIfNeeded(currentTimeStamp);
discreteUpdates(() =>
return discreteUpdates(() =>
executeUserEventHandler(eventListener, eventValue),
);
break;
}
case UserBlockingEvent: {
if (enableUserBlockingEvents) {
runWithPriority(UserBlockingPriority, () =>
return runWithPriority(UserBlockingPriority, () =>
executeUserEventHandler(eventListener, eventValue),
);
} else {
executeUserEventHandler(eventListener, eventValue);
return executeUserEventHandler(eventListener, eventValue);
}
break;
}
case ContinuousEvent: {
executeUserEventHandler(eventListener, eventValue);
break;
return executeUserEventHandler(eventListener, eventValue);
}
}
},
Expand Down Expand Up @@ -259,6 +262,9 @@ const eventResponderContext: ReactDOMResponderContext = {
}
return false;
},
continuePropagation() {
currentPropagationBehavior = PropagateToNextResponder;
},
enqueueStateRestore,
};

Expand Down Expand Up @@ -462,6 +468,10 @@ function traverseAndHandleEventResponderInstances(
| Element
| Document);
onEvent(responderEvent, eventResponderContext, props, state);
if (currentPropagationBehavior === PropagateToNextResponder) {
visitedResponders.delete(responder);
currentPropagationBehavior = DoNotPropagateToNextResponder;
}
}
}
}
Expand Down Expand Up @@ -562,6 +572,8 @@ export function dispatchEventForResponderEventSystem(
const previousTimers = currentTimers;
const previousTimeStamp = currentTimeStamp;
const previousDocument = currentDocument;
const previousPropagationBehavior = currentPropagationBehavior;
currentPropagationBehavior = DoNotPropagateToNextResponder;
currentTimers = null;
// nodeType 9 is DOCUMENT_NODE
currentDocument =
Expand All @@ -585,6 +597,7 @@ export function dispatchEventForResponderEventSystem(
currentInstance = previousInstance;
currentTimeStamp = previousTimeStamp;
currentDocument = previousDocument;
currentPropagationBehavior = previousPropagationBehavior;
}
}
}
Expand Down
9 changes: 8 additions & 1 deletion packages/react-events/src/dom/Keyboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,14 @@ function dispatchKeyboardEvent(
target,
defaultPrevented,
);
context.dispatchEvent(syntheticEvent, listener, DiscreteEvent);
const shouldPropagate = context.dispatchEvent(
syntheticEvent,
listener,
DiscreteEvent,
);
if (shouldPropagate) {
context.continuePropagation();
}
}

const keyboardResponderImpl = {
Expand Down
84 changes: 84 additions & 0 deletions packages/react-events/src/dom/__tests__/Keyboard-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -251,4 +251,88 @@ describe('Keyboard event responder', () => {
);
});
});

describe('correctly handles responder propagation', () => {
describe('onKeyDown', () => {
let onKeyDownInner, onKeyDownOuter, ref;

function renderPropagationTest(propagates) {
onKeyDownInner = jest.fn(() => propagates);
onKeyDownOuter = jest.fn();
ref = React.createRef();
const Component = () => {
const listenerInner = useKeyboard({
onKeyDown: onKeyDownInner,
});
const listenerOuter = useKeyboard({
onKeyDown: onKeyDownOuter,
});
return (
<div listeners={listenerOuter}>
<div ref={ref} listeners={listenerInner} />
</div>
);
};
ReactDOM.render(<Component />, container);
}

it('propagates when cb returns true', () => {
renderPropagationTest(true);
const target = createEventTarget(ref.current);
target.keydown();
expect(onKeyDownInner).toBeCalled();
expect(onKeyDownOuter).toBeCalled();
});

it('does not propagate when cb returns false', () => {
renderPropagationTest(false);
const target = createEventTarget(ref.current);
target.keydown();
expect(onKeyDownInner).toBeCalled();
expect(onKeyDownOuter).not.toBeCalled();
});
});

describe('onKeyUp', () => {
let onKeyUpInner, onKeyUpOuter, ref;

function renderPropagationTest(propagates) {
onKeyUpInner = jest.fn(() => propagates);
onKeyUpOuter = jest.fn();
ref = React.createRef();
const Component = () => {
const listenerInner = useKeyboard({
onKeyUp: onKeyUpInner,
});
const listenerOuter = useKeyboard({
onKeyUp: onKeyUpOuter,
});
return (
<div listeners={listenerOuter}>
<div ref={ref} listeners={listenerInner} />
</div>
);
};
ReactDOM.render(<Component />, container);
}

it('propagates when cb returns true', () => {
renderPropagationTest(true);
const target = createEventTarget(ref.current);
target.keydown();
target.keyup();
expect(onKeyUpInner).toBeCalled();
expect(onKeyUpOuter).toBeCalled();
});

it('does not propagate when cb returns false', () => {
renderPropagationTest(false);
const target = createEventTarget(ref.current);
target.keydown();
target.keyup();
expect(onKeyUpInner).toBeCalled();
expect(onKeyUpOuter).not.toBeCalled();
});
});
});
});
1 change: 1 addition & 0 deletions packages/shared/ReactDOMTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ export type ReactDOMResponderContext = {
target: Element | Document,
elementType: string,
) => boolean,
continuePropagation(): void,
// Used for controller components
enqueueStateRestore(Element | Document): void,
};