Skip to content

Commit

Permalink
Don't emulate bubbling of the scroll event (facebook#19464)
Browse files Browse the repository at this point in the history 
* Don't emulate bubbling of the scroll event

* Put behind a flag
  • Loading branch information
@gaearon
gaearon committed Jul 27, 2020
1 parent 217ecf5 commit 06d104e
Show file tree
Hide file tree
Showing 11 changed files with 132 additions and 16 deletions.
114 changes: 105 additions & 9 deletions packages/react-dom/src/__tests__/ReactDOMEventListener-test.js
View file
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
View file
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
View file
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
View file
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
View file
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
View file
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.test-renderer.www.js
View file
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
View file
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
View file
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
View file
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
View file
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

0 comments on commit 06d104e

Please sign in to comment.