maxjhandsome
/
react
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Bugfix: Infinite loop in beforeblur event (#19053) 

* Failing test: Infinite loop in beforeblur event

If the focused node is hidden by a Suspense boundary, we fire the
beforeblur event. Our check for whether a tree is being hidden isn't
specific enough. It should only fire when the tree is initially hidden,
but it's being fired for updates, too.

* Only fire beforeblur on visible -> hidden

Should only beforeblur fire if the node was previously visible. Not
during updates to an already hidden tree.

To optimize this, we should use a dedicated effect tag and mark it in
the render phase. I've left this for a follow-up, though. Maybe can
revisit after the planned refactor of the commit phase.

* Move logic to commit phase

isFiberSuspenseAndTimedOut is used elsewhere, so I inlined the commit
logic into the commit phase itself.
This commit is contained in:
Andrew Clark 2020-06-01 09:01:21 -07:00 committed by GitHub
parent 1d85bb3ce1
commit 4c7036e807
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 145 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

62
packages/react-dom/src/events/__tests__/DOMModernPluginEventSystem-test.internal.js
View File

@ -2489,6 +2489,68 @@ describe('DOMModernPluginEventSystem', () => {
document.body.removeChild(container2);
});
// @gate experimental
it('regression: does not fire beforeblur/afterblur if target is already hidden', () => {
const Suspense = React.Suspense;
let suspend = false;
const promise = Promise.resolve();
const beforeBlurHandle = ReactDOM.unstable_createEventHandle(
'beforeblur',
);
const innerRef = React.createRef();
function Child() {
if (suspend) {
throw promise;
}
return <input ref={innerRef} />;
}
const Component = () => {
const ref = React.useRef(null);
const [, setState] = React.useState(0);
React.useEffect(() => {
beforeBlurHandle.setListener(ref.current, () => {
// In the regression case, this would trigger an update, then
// the resulting render would trigger another blur event,
// which would trigger an update again, and on and on in an
// infinite loop.
setState(n => n + 1);
});
}, []);
return (
<div ref={ref}>
<Suspense fallback="Loading...">
<Child />
</Suspense>
</div>
);
};
const container2 = document.createElement('div');
document.body.appendChild(container2);
const root = ReactDOM.createRoot(container2);
ReactTestUtils.act(() => {
root.render(<Component />);
});
// Focus the input node
const inner = innerRef.current;
const target = createEventTarget(inner);
target.focus();
// Suspend. This hides the input node, causing it to lose focus.
suspend = true;
ReactTestUtils.act(() => {
root.render(<Component />);
});
document.body.removeChild(container2);
});
describe('Compatibility with Scopes API', () => {
beforeEach(() => {
jest.resetModules();

17
packages/react-reconciler/src/ReactFiberCommitWork.new.js
View File

@ -1752,6 +1752,23 @@ function attachSuspenseRetryListeners(finishedWork: Fiber) {
}
}
// This function detects when a Suspense boundary goes from visible to hidden.
// It returns false if the boundary is already hidden.
// TODO: Use an effect tag.
export function isSuspenseBoundaryBeingHidden(
current: Fiber | null,
finishedWork: Fiber,
): boolean {
if (current !== null) {
const oldState: SuspenseState | null = current.memoizedState;
if (oldState === null || oldState.dehydrated !== null) {
const newState: SuspenseState | null = finishedWork.memoizedState;
return newState !== null && newState.dehydrated === null;
}
}
return false;
}
function commitResetTextContent(current: Fiber) {
if (!supportsMutation) {
return;

17
packages/react-reconciler/src/ReactFiberCommitWork.old.js
View File

@ -1735,6 +1735,23 @@ function attachSuspenseRetryListeners(finishedWork: Fiber) {
}
}
// This function detects when a Suspense boundary goes from visible to hidden.
// It returns false if the boundary is already hidden.
// TODO: Use an effect tag.
export function isSuspenseBoundaryBeingHidden(
current: Fiber | null,
finishedWork: Fiber,
): boolean {
if (current !== null) {
const oldState: SuspenseState | null = current.memoizedState;
if (oldState === null || oldState.dehydrated !== null) {
const newState: SuspenseState | null = finishedWork.memoizedState;
return newState !== null && newState.dehydrated === null;
}
}
return false;
}
function commitResetTextContent(current: Fiber) {
if (!supportsMutation) {
return;

38
packages/react-reconciler/src/ReactFiberTreeReflection.js vendored
View File

@ -25,7 +25,7 @@ import {
FundamentalComponent,
SuspenseComponent,
} from './ReactWorkTags';
import {NoEffect, Placement, Hydrating, Deletion} from './ReactSideEffectTags';
import {NoEffect, Placement, Hydrating} from './ReactSideEffectTags';
import {enableFundamentalAPI} from 'shared/ReactFeatureFlags';
const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;
@ -342,7 +342,10 @@ export function isFiberSuspenseAndTimedOut(fiber: Fiber): boolean {
);
}
function doesFiberContain(parentFiber: Fiber, childFiber: Fiber): boolean {
export function doesFiberContain(
parentFiber: Fiber,
childFiber: Fiber,
): boolean {
let node = childFiber;
const parentFiberAlternate = parentFiber.alternate;
while (node !== null) {
@ -353,34 +356,3 @@ function doesFiberContain(parentFiber: Fiber, childFiber: Fiber): boolean {
}
return false;
}
function isFiberTimedOutSuspenseThatContainsTargetFiber(
fiber: Fiber,
targetFiber: Fiber,
): boolean {
const child = fiber.child;
return (
isFiberSuspenseAndTimedOut(fiber) &&
child !== null &&
doesFiberContain(child, targetFiber)
);
}
function isFiberDeletedAndContainsTargetFiber(
fiber: Fiber,
targetFiber: Fiber,
): boolean {
return (
(fiber.effectTag & Deletion) !== 0 && doesFiberContain(fiber, targetFiber)
);
}
export function isFiberHiddenOrDeletedAndContains(
parentFiber: Fiber,
childFiber: Fiber,
): boolean {
return (
isFiberDeletedAndContainsTargetFiber(parentFiber, childFiber) ||
isFiberTimedOutSuspenseThatContainsTargetFiber(parentFiber, childFiber)
);
}

31
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View File

@ -159,6 +159,7 @@ import {
commitAttachRef,
commitPassiveEffectDurations,
commitResetTextContent,
isSuspenseBoundaryBeingHidden,
} from './ReactFiberCommitWork.new';
import {enqueueUpdate} from './ReactUpdateQueue.new';
import {resetContextDependencies} from './ReactFiberNewContext.new';
@ -201,7 +202,7 @@ import {onCommitRoot as onCommitRootTestSelector} from './ReactTestSelectors';
// Used by `act`
import enqueueTask from 'shared/enqueueTask';
import {isFiberHiddenOrDeletedAndContains} from './ReactFiberTreeReflection';
import {doesFiberContain} from './ReactFiberTreeReflection';
const ceil = Math.ceil;
@ -2102,19 +2103,31 @@ function commitRootImpl(root, renderPriorityLevel) {
function commitBeforeMutationEffects() {
while (nextEffect !== null) {
if (
!shouldFireAfterActiveInstanceBlur &&
focusedInstanceHandle !== null &&
isFiberHiddenOrDeletedAndContains(nextEffect, focusedInstanceHandle)
) {
shouldFireAfterActiveInstanceBlur = true;
beforeActiveInstanceBlur();
const current = nextEffect.alternate;
if (!shouldFireAfterActiveInstanceBlur && focusedInstanceHandle !== null) {
if ((nextEffect.effectTag & Deletion) !== NoEffect) {
if (doesFiberContain(nextEffect, focusedInstanceHandle)) {
shouldFireAfterActiveInstanceBlur = true;
beforeActiveInstanceBlur();
}
} else {
// TODO: Move this out of the hot path using a dedicated effect tag.
if (
nextEffect.tag === SuspenseComponent &&
isSuspenseBoundaryBeingHidden(current, nextEffect) &&
doesFiberContain(nextEffect, focusedInstanceHandle)
) {
shouldFireAfterActiveInstanceBlur = true;
beforeActiveInstanceBlur();
}
}
}
const effectTag = nextEffect.effectTag;
if ((effectTag & Snapshot) !== NoEffect) {
setCurrentDebugFiberInDEV(nextEffect);
const current = nextEffect.alternate;
commitBeforeMutationEffectOnFiber(current, nextEffect);
resetCurrentDebugFiberInDEV();

31
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View File

@ -156,6 +156,7 @@ import {
commitAttachRef,
commitPassiveEffectDurations,
commitResetTextContent,
isSuspenseBoundaryBeingHidden,
} from './ReactFiberCommitWork.old';
import {enqueueUpdate} from './ReactUpdateQueue.old';
import {resetContextDependencies} from './ReactFiberNewContext.old';
@ -193,7 +194,7 @@ import {onCommitRoot as onCommitRootTestSelector} from './ReactTestSelectors';
// Used by `act`
import enqueueTask from 'shared/enqueueTask';
import {isFiberHiddenOrDeletedAndContains} from './ReactFiberTreeReflection';
import {doesFiberContain} from './ReactFiberTreeReflection';
const ceil = Math.ceil;
@ -2220,19 +2221,31 @@ function commitRootImpl(root, renderPriorityLevel) {
function commitBeforeMutationEffects() {
while (nextEffect !== null) {
if (
!shouldFireAfterActiveInstanceBlur &&
focusedInstanceHandle !== null &&
isFiberHiddenOrDeletedAndContains(nextEffect, focusedInstanceHandle)
) {
shouldFireAfterActiveInstanceBlur = true;
beforeActiveInstanceBlur();
const current = nextEffect.alternate;
if (!shouldFireAfterActiveInstanceBlur && focusedInstanceHandle !== null) {
if ((nextEffect.effectTag & Deletion) !== NoEffect) {
if (doesFiberContain(nextEffect, focusedInstanceHandle)) {
shouldFireAfterActiveInstanceBlur = true;
beforeActiveInstanceBlur();
}
} else {
// TODO: Move this out of the hot path using a dedicated effect tag.
if (
nextEffect.tag === SuspenseComponent &&
isSuspenseBoundaryBeingHidden(current, nextEffect) &&
doesFiberContain(nextEffect, focusedInstanceHandle)
) {
shouldFireAfterActiveInstanceBlur = true;
beforeActiveInstanceBlur();
}
}
}
const effectTag = nextEffect.effectTag;
if ((effectTag & Snapshot) !== NoEffect) {
setCurrentDebugFiberInDEV(nextEffect);
const current = nextEffect.alternate;
commitBeforeMutationEffectOnFiber(current, nextEffect);
resetCurrentDebugFiberInDEV();