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

Don't emulate bubbling of the scroll event #19464

Merged
merged 2 commits into from Jul 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
114 changes: 105 additions & 9 deletions packages/react-dom/src/__tests__/ReactDOMEventListener-test.js
Expand Up @@ -539,7 +539,6 @@ 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();
const onToggle = jest.fn();
Expand All @@ -548,14 +547,12 @@ describe('ReactDOMEventListener', () => {
ReactDOM.render(
<div
onPlay={onPlay}
onScroll={onScroll}
onCancel={onCancel}
onClose={onClose}
onToggle={onToggle}>
<div
ref={ref}
onPlay={onPlay}
onScroll={onScroll}
onCancel={onCancel}
onClose={onClose}
onToggle={onToggle}
Expand All @@ -568,11 +565,6 @@ describe('ReactDOMEventListener', () => {
bubbles: false,
}),
);
ref.current.dispatchEvent(
new Event('scroll', {
bubbles: false,
}),
);
ref.current.dispatchEvent(
new Event('cancel', {
bubbles: false,
Expand All @@ -591,7 +583,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);
expect(onToggle).toHaveBeenCalledTimes(2);
Expand Down Expand Up @@ -643,4 +634,109 @@ 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,
}),
);
if (gate(flags => flags.disableOnScrollBubbling)) {
expect(log).toEqual([
['capture', 'grand'],
['capture', 'parent'],
['capture', 'child'],
['bubble', 'child'],
]);
} else {
expect(log).toEqual([
['capture', 'grand'],
['capture', 'parent'],
['capture', 'child'],
['bubble', 'child'],
['bubble', 'parent'],
['bubble', 'grand'],
]);
}
} 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);
}
});
});
24 changes: 17 additions & 7 deletions packages/react-dom/src/events/plugins/SimpleEventPlugin.js
Expand Up @@ -42,7 +42,10 @@ import {IS_EVENT_HANDLE_NON_MANAGED_NODE} from '../EventSystemFlags';
import getEventCharCode from '../getEventCharCode';
import {IS_CAPTURE_PHASE} from '../EventSystemFlags';

import {enableCreateEventHandleAPI} from 'shared/ReactFeatureFlags';
import {
enableCreateEventHandleAPI,
disableOnScrollBubbling,
} from 'shared/ReactFeatureFlags';

function extractEvents(
dispatchQueue: DispatchQueue,
Expand Down Expand Up @@ -165,13 +168,20 @@ 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.
let accumulateTargetOnly = false;
if (disableOnScrollBubbling) {
accumulateTargetOnly =
!inCapturePhase &&
// TODO: ideally, we'd eventually add all events from
// nonDelegatedEvents list in DOMPluginEventSystem.
// Then we can remove this special list.
topLevelType === DOMTopLevelEventTypes.TOP_SCROLL;
}

// We traverse only capture or bubble phase listeners
accumulateSinglePhaseListeners(
targetInst,
dispatchQueue,
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/ReactFeatureFlags.js
Expand Up @@ -128,3 +128,5 @@ export const deferRenderPhaseUpdateToNextBatch = true;

// Replacement for runWithPriority in React internals.
export const decoupleUpdatePriorityFromScheduler = false;

export const disableOnScrollBubbling = false;
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
Expand Up @@ -45,6 +45,7 @@ export const warnAboutSpreadingKeyToJSX = false;
export const enableComponentStackLocations = false;
export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
export const disableOnScrollBubbling = false;

export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = true;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
Expand Up @@ -44,6 +44,7 @@ export const warnAboutSpreadingKeyToJSX = false;
export const enableComponentStackLocations = false;
export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
export const disableOnScrollBubbling = false;

export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = true;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
Expand Up @@ -44,6 +44,7 @@ export const warnAboutSpreadingKeyToJSX = false;
export const enableComponentStackLocations = true;
export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
export const disableOnScrollBubbling = false;

export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = true;
Expand Down
Expand Up @@ -44,6 +44,7 @@ export const warnAboutSpreadingKeyToJSX = false;
export const enableComponentStackLocations = true;
export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
export const disableOnScrollBubbling = false;

export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = true;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.testing.js
Expand Up @@ -44,6 +44,7 @@ export const warnAboutSpreadingKeyToJSX = false;
export const enableComponentStackLocations = true;
export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
export const disableOnScrollBubbling = false;

export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = true;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.testing.www.js
Expand Up @@ -44,6 +44,7 @@ export const warnAboutSpreadingKeyToJSX = false;
export const enableComponentStackLocations = true;
export const enableLegacyFBSupport = !__EXPERIMENTAL__;
export const enableFilterEmptyStringAttributesDOM = false;
export const disableOnScrollBubbling = false;

export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = true;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www-dynamic.js
Expand Up @@ -18,6 +18,7 @@ export const disableInputAttributeSyncing = __VARIANT__;
export const enableFilterEmptyStringAttributesDOM = __VARIANT__;
export const enableLegacyFBSupport = __VARIANT__;
export const decoupleUpdatePriorityFromScheduler = __VARIANT__;
export const disableOnScrollBubbling = __VARIANT__;

// Enable this flag to help with concurrent mode debugging.
// It logs information to the console about React scheduling, rendering, and commit phases.
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www.js
Expand Up @@ -27,6 +27,7 @@ export const {
decoupleUpdatePriorityFromScheduler,
enableDebugTracing,
enableSchedulingProfilerComponentStacks,
disableOnScrollBubbling,
} = dynamicFeatureFlags;

// On WWW, __EXPERIMENTAL__ is used for a new modern build.
Expand Down