Skip to content

Commit

Permalink
Don't emulate bubbling of the scroll event
Browse files Browse the repository at this point in the history
  • Loading branch information
gaearon committed Jul 27, 2020
1 parent 6bb86fd commit 21813de
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 19 deletions.
109 changes: 96 additions & 13 deletions packages/react-dom/src/__tests__/ReactDOMEventListener-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -539,21 +539,15 @@ describe('ReactDOMEventListener', () => {
const container = document.createElement('div');
const ref = React.createRef();
const onPlay = jest.fn();
const onScroll = jest.fn();
const onCancel = jest.fn();
const onClose = jest.fn();
document.body.appendChild(container);
try {
ReactDOM.render(
<div
onPlay={onPlay}
onScroll={onScroll}
onCancel={onCancel}
onClose={onClose}>
<div onPlay={onPlay} onCancel={onCancel} onClose={onClose}>
<div
ref={ref}
onPlay={onPlay}
onScroll={onScroll}
onCancel={onCancel}
onClose={onClose}
/>
Expand All @@ -565,11 +559,6 @@ describe('ReactDOMEventListener', () => {
bubbles: false,
}),
);
ref.current.dispatchEvent(
new Event('scroll', {
bubbles: false,
}),
);
ref.current.dispatchEvent(
new Event('cancel', {
bubbles: false,
Expand All @@ -583,7 +572,6 @@ describe('ReactDOMEventListener', () => {
// Regression test: ensure we still emulate bubbling with non-bubbling
// media
expect(onPlay).toHaveBeenCalledTimes(2);
expect(onScroll).toHaveBeenCalledTimes(2);
expect(onCancel).toHaveBeenCalledTimes(2);
expect(onClose).toHaveBeenCalledTimes(2);
} finally {
Expand Down Expand Up @@ -634,4 +622,99 @@ describe('ReactDOMEventListener', () => {
document.body.removeChild(container);
}
});

// We're moving towards aligning more closely with the browser.
// Currently we emulate bubbling for all non-bubbling events except scroll.
// We may expand this list in the future, removing emulated bubbling altogether.
it('should not emulate bubbling of scroll events', () => {
const container = document.createElement('div');
const ref = React.createRef();
const log = [];
const onScroll = jest.fn(e =>
log.push(['bubble', e.currentTarget.className]),
);
const onScrollCapture = jest.fn(e =>
log.push(['capture', e.currentTarget.className]),
);
document.body.appendChild(container);
try {
ReactDOM.render(
<div
className="grand"
onScroll={onScroll}
onScrollCapture={onScrollCapture}>
<div
className="parent"
onScroll={onScroll}
onScrollCapture={onScrollCapture}>
<div
className="child"
onScroll={onScroll}
onScrollCapture={onScrollCapture}
ref={ref}
/>
</div>
</div>,
container,
);
ref.current.dispatchEvent(
new Event('scroll', {
bubbles: false,
}),
);
expect(log).toEqual([
['capture', 'grand'],
['capture', 'parent'],
['capture', 'child'],
['bubble', 'child'],
]);
} finally {
document.body.removeChild(container);
}
});

// We're moving towards aligning more closely with the browser.
// Currently we emulate bubbling for all non-bubbling events except scroll.
// We may expand this list in the future, removing emulated bubbling altogether.
it('should not emulate bubbling of scroll events (no own handler)', () => {
const container = document.createElement('div');
const ref = React.createRef();
const log = [];
const onScroll = jest.fn(e =>
log.push(['bubble', e.currentTarget.className]),
);
const onScrollCapture = jest.fn(e =>
log.push(['capture', e.currentTarget.className]),
);
document.body.appendChild(container);
try {
ReactDOM.render(
<div
className="grand"
onScroll={onScroll}
onScrollCapture={onScrollCapture}>
<div
className="parent"
onScroll={onScroll}
onScrollCapture={onScrollCapture}>
{/* Intentionally no handler on the child: */}
<div className="child" ref={ref} />
</div>
</div>,
container,
);
ref.current.dispatchEvent(
new Event('scroll', {
bubbles: false,
}),
);
//
expect(log).toEqual([
['capture', 'grand'],
['capture', 'parent'],
]);
} finally {
document.body.removeChild(container);
}
});
});
16 changes: 10 additions & 6 deletions packages/react-dom/src/events/plugins/SimpleEventPlugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,13 +165,17 @@ function extractEvents(
inCapturePhase,
);
} else {
// TODO: We may also want to re-use the accumulateTargetOnly flag to
// special case bubbling for onScroll/media events at a later point.
// In which case we will want to make this flag boolean and ensure
// we change the targetInst to be of the container instance. Like:
const accumulateTargetOnly = false;
// Some events don't bubble in the browser.
// In the past, React has always bubbled them, but this can be surprising.
// We're going to try aligning closer to the browser behavior by not bubbling
// them in React either. We'll start by not bubbling onScroll, and then expand.
const accumulateTargetOnly =
!inCapturePhase &&
// TODO: ideally, we'd eventually add all events from
// nonDelegatedEvents list in ReactDOMEventListener.
// Then we can remove this special list.
topLevelType === DOMTopLevelEventTypes.TOP_SCROLL;

// We traverse only capture or bubble phase listeners
accumulateSinglePhaseListeners(
targetInst,
dispatchQueue,
Expand Down

0 comments on commit 21813de

Please sign in to comment.