Skip to content

Commit

Permalink
[Bugfix] Don't hide/unhide unless visibility changes (facebook#21875)
Browse files Browse the repository at this point in the history 
* Use Visibility flag to schedule a hide/show effect

Instead of the Update flag, which is also used for other side-effects,
like refs.

I originally added the Visibility flag for this purpose in facebook#20043 but
it got reverted last winter when we were bisecting the effects refactor.

* Added failing test case

Co-authored-by: Brian Vaughn <bvaughn@fb.com>
  • Loading branch information
@acdlite @zhengjitf
2 people authored and zhengjitf committed Apr 15, 2022
1 parent f7686f0 commit 5add14a
Show file tree
Hide file tree
Showing 6 changed files with 223 additions and 103 deletions.
72 changes: 41 additions & 31 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ import {
ChildDeletion,
Snapshot,
Update,
Callback,
Ref,
Hydrating,
HydratingAndUpdate,
Expand All @@ -75,6 +74,7 @@ import {
MutationMask,
LayoutMask,
PassiveMask,
Visibility,
} from './ReactFiberFlags';
import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -615,7 +615,7 @@ function commitLayoutEffectOnFiber(
finishedWork: Fiber,
committedLanes: Lanes,
): void {
if ((finishedWork.flags & (Update | Callback)) !== NoFlags) {
if ((finishedWork.flags & LayoutMask) !== NoFlags) {
switch (finishedWork.tag) {
case FunctionComponent:
case ForwardRef:
Expand Down Expand Up @@ -1776,7 +1776,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
return;
}
case SuspenseComponent: {
commitSuspenseComponent(finishedWork);
commitSuspenseCallback(finishedWork);
attachSuspenseRetryListeners(finishedWork);
return;
}
Expand Down Expand Up @@ -1899,7 +1899,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
return;
}
case SuspenseComponent: {
commitSuspenseComponent(finishedWork);
commitSuspenseCallback(finishedWork);
attachSuspenseRetryListeners(finishedWork);
return;
}
Expand All @@ -1918,13 +1918,6 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
}
break;
}
case OffscreenComponent:
case LegacyHiddenComponent: {
const newState: OffscreenState | null = finishedWork.memoizedState;
const isHidden = newState !== null;
hideOrUnhideAllChildren(finishedWork, isHidden);
return;
}
}
invariant(
false,
Expand All @@ -1933,27 +1926,9 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
);
}

function commitSuspenseComponent(finishedWork: Fiber) {
function commitSuspenseCallback(finishedWork: Fiber) {
// TODO: Move this to passive phase
const newState: SuspenseState | null = finishedWork.memoizedState;

if (newState !== null) {
markCommitTimeOfFallback();

if (supportsMutation) {
// Hide the Offscreen component that contains the primary children. TODO:
// Ideally, this effect would have been scheduled on the Offscreen fiber
// itself. That's how unhiding works: the Offscreen component schedules an
// effect on itself. However, in this case, the component didn't complete,
// so the fiber was never added to the effect list in the normal path. We
// could have appended it to the effect list in the Suspense component's
// second pass, but doing it this way is less complicated. This would be
// simpler if we got rid of the effect list and traversed the tree, like
// we're planning to do.
const primaryChildParent: Fiber = (finishedWork.child: any);
hideOrUnhideAllChildren(primaryChildParent, true);
}
}

if (enableSuspenseCallback && newState !== null) {
const suspenseCallback = finishedWork.memoizedProps.suspenseCallback;
if (typeof suspenseCallback === 'function') {
Expand Down Expand Up @@ -2127,6 +2102,10 @@ function commitMutationEffects_complete(root: FiberRoot) {
}

function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
// TODO: The factoring of this phase could probably be improved. Consider
// switching on the type of work before checking the flags. That's what
// we do in all the other phases. I think this one is only different
// because of the shared reconcilation logic below.
const flags = finishedWork.flags;

if (flags & ContentReset) {
Expand All @@ -2147,6 +2126,37 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
}
}

if (flags & Visibility) {
switch (finishedWork.tag) {
case SuspenseComponent: {
const newState: OffscreenState | null = finishedWork.memoizedState;
if (newState !== null) {
markCommitTimeOfFallback();
// Hide the Offscreen component that contains the primary children.
// TODO: Ideally, this effect would have been scheduled on the
// Offscreen fiber itself. That's how unhiding works: the Offscreen
// component schedules an effect on itself. However, in this case, the
// component didn't complete, so the fiber was never added to the
// effect list in the normal path. We could have appended it to the
// effect list in the Suspense component's second pass, but doing it
// this way is less complicated. This would be simpler if we got rid
// of the effect list and traversed the tree, like we're planning to
// do.
const primaryChildParent: Fiber = (finishedWork.child: any);
hideOrUnhideAllChildren(primaryChildParent, true);
}
break;
}
case OffscreenComponent:
case LegacyHiddenComponent: {
const newState: OffscreenState | null = finishedWork.memoizedState;
const isHidden = newState !== null;
hideOrUnhideAllChildren(finishedWork, isHidden);
break;
}
}
}

// The following switch statement is only concerned about placement,
// updates, and deletions. To avoid needing to add a case for every possible
// bitmap value, we remove the secondary effects from the effect tag and
Expand Down
72 changes: 41 additions & 31 deletions packages/react-reconciler/src/ReactFiberCommitWork.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ import {
ChildDeletion,
Snapshot,
Update,
Callback,
Ref,
Hydrating,
HydratingAndUpdate,
Expand All @@ -75,6 +74,7 @@ import {
MutationMask,
LayoutMask,
PassiveMask,
Visibility,
} from './ReactFiberFlags';
import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -615,7 +615,7 @@ function commitLayoutEffectOnFiber(
finishedWork: Fiber,
committedLanes: Lanes,
): void {
if ((finishedWork.flags & (Update | Callback)) !== NoFlags) {
if ((finishedWork.flags & LayoutMask) !== NoFlags) {
switch (finishedWork.tag) {
case FunctionComponent:
case ForwardRef:
Expand Down Expand Up @@ -1776,7 +1776,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
return;
}
case SuspenseComponent: {
commitSuspenseComponent(finishedWork);
commitSuspenseCallback(finishedWork);
attachSuspenseRetryListeners(finishedWork);
return;
}
Expand Down Expand Up @@ -1899,7 +1899,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
return;
}
case SuspenseComponent: {
commitSuspenseComponent(finishedWork);
commitSuspenseCallback(finishedWork);
attachSuspenseRetryListeners(finishedWork);
return;
}
Expand All @@ -1918,13 +1918,6 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
}
break;
}
case OffscreenComponent:
case LegacyHiddenComponent: {
const newState: OffscreenState | null = finishedWork.memoizedState;
const isHidden = newState !== null;
hideOrUnhideAllChildren(finishedWork, isHidden);
return;
}
}
invariant(
false,
Expand All @@ -1933,27 +1926,9 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
);
}

function commitSuspenseComponent(finishedWork: Fiber) {
function commitSuspenseCallback(finishedWork: Fiber) {
// TODO: Move this to passive phase
const newState: SuspenseState | null = finishedWork.memoizedState;

if (newState !== null) {
markCommitTimeOfFallback();

if (supportsMutation) {
// Hide the Offscreen component that contains the primary children. TODO:
// Ideally, this effect would have been scheduled on the Offscreen fiber
// itself. That's how unhiding works: the Offscreen component schedules an
// effect on itself. However, in this case, the component didn't complete,
// so the fiber was never added to the effect list in the normal path. We
// could have appended it to the effect list in the Suspense component's
// second pass, but doing it this way is less complicated. This would be
// simpler if we got rid of the effect list and traversed the tree, like
// we're planning to do.
const primaryChildParent: Fiber = (finishedWork.child: any);
hideOrUnhideAllChildren(primaryChildParent, true);
}
}

if (enableSuspenseCallback && newState !== null) {
const suspenseCallback = finishedWork.memoizedProps.suspenseCallback;
if (typeof suspenseCallback === 'function') {
Expand Down Expand Up @@ -2127,6 +2102,10 @@ function commitMutationEffects_complete(root: FiberRoot) {
}

function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
// TODO: The factoring of this phase could probably be improved. Consider
// switching on the type of work before checking the flags. That's what
// we do in all the other phases. I think this one is only different
// because of the shared reconcilation logic below.
const flags = finishedWork.flags;

if (flags & ContentReset) {
Expand All @@ -2147,6 +2126,37 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
}
}

if (flags & Visibility) {
switch (finishedWork.tag) {
case SuspenseComponent: {
const newState: OffscreenState | null = finishedWork.memoizedState;
if (newState !== null) {
markCommitTimeOfFallback();
// Hide the Offscreen component that contains the primary children.
// TODO: Ideally, this effect would have been scheduled on the
// Offscreen fiber itself. That's how unhiding works: the Offscreen
// component schedules an effect on itself. However, in this case, the
// component didn't complete, so the fiber was never added to the
// effect list in the normal path. We could have appended it to the
// effect list in the Suspense component's second pass, but doing it
// this way is less complicated. This would be simpler if we got rid
// of the effect list and traversed the tree, like we're planning to
// do.
const primaryChildParent: Fiber = (finishedWork.child: any);
hideOrUnhideAllChildren(primaryChildParent, true);
}
break;
}
case OffscreenComponent:
case LegacyHiddenComponent: {
const newState: OffscreenState | null = finishedWork.memoizedState;
const isHidden = newState !== null;
hideOrUnhideAllChildren(finishedWork, isHidden);
break;
}
}
}

// The following switch statement is only concerned about placement,
// updates, and deletions. To avoid needing to add a case for every possible
// bitmap value, we remove the secondary effects from the effect tag and
Expand Down
53 changes: 33 additions & 20 deletions packages/react-reconciler/src/ReactFiberCompleteWork.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@

import type {Fiber} from './ReactInternalTypes';
import type {Lanes, Lane} from './ReactFiberLane.new';
import type {ReactScopeInstance, ReactContext} from 'shared/ReactTypes';
import type {
ReactScopeInstance,
ReactContext,
Wakeable,
} from 'shared/ReactTypes';
import type {FiberRoot} from './ReactInternalTypes';
import type {
Instance,
Expand Down Expand Up @@ -60,6 +64,7 @@ import {
Ref,
RefStatic,
Update,
Visibility,
NoFlags,
DidCapture,
Snapshot,
Expand Down Expand Up @@ -320,7 +325,7 @@ if (supportsMutation) {
// down its children. Instead, we'll get insertions from each child in
// the portal directly.
} else if (node.tag === SuspenseComponent) {
if ((node.flags & Update) !== NoFlags) {
if ((node.flags & Visibility) !== NoFlags) {
// Need to toggle the visibility of the primary children.
const newIsHidden = node.memoizedState !== null;
if (newIsHidden) {
Expand Down Expand Up @@ -405,7 +410,7 @@ if (supportsMutation) {
// down its children. Instead, we'll get insertions from each child in
// the portal directly.
} else if (node.tag === SuspenseComponent) {
if ((node.flags & Update) !== NoFlags) {
if ((node.flags & Visibility) !== NoFlags) {
// Need to toggle the visibility of the primary children.
const newIsHidden = node.memoizedState !== null;
if (newIsHidden) {
Expand Down Expand Up @@ -1084,32 +1089,40 @@ function completeWork(
}
}

if (supportsPersistence) {
// TODO: Only schedule updates if not prevDidTimeout.
if (nextDidTimeout) {
// If this boundary just timed out, schedule an effect to attach a
// retry listener to the promise. This flag is also used to hide the
// primary children.
workInProgress.flags |= Update;
}
const wakeables: Set<Wakeable> | null = (workInProgress.updateQueue: any);
if (wakeables !== null) {
// Schedule an effect to attach a retry listener to the promise.
// TODO: Move to passive phase
workInProgress.flags |= Update;
}

if (supportsMutation) {
// TODO: Only schedule updates if these values are non equal, i.e. it changed.
if (nextDidTimeout || prevDidTimeout) {
// If this boundary just timed out, schedule an effect to attach a
// retry listener to the promise. This flag is also used to hide the
// primary children. In mutation mode, we also need the flag to
// *unhide* children that were previously hidden, so check if this
// is currently timed out, too.
workInProgress.flags |= Update;
if (nextDidTimeout !== prevDidTimeout) {
// In mutation mode, visibility is toggled by mutating the nearest
// host nodes whenever they switch from hidden -> visible or vice
// versa. We don't need to switch when the boundary updates but its
// visibility hasn't changed.
workInProgress.flags |= Visibility;
}
}
if (supportsPersistence) {
if (nextDidTimeout) {
// In persistent mode, visibility is toggled by cloning the nearest
// host nodes in the complete phase whenever the boundary is hidden.
// TODO: The plan is to add a transparent host wrapper (no layout)
// around the primary children and hide that node. Then we don't need
// to do the funky cloning business.
workInProgress.flags |= Visibility;
}
}

if (
enableSuspenseCallback &&
workInProgress.updateQueue !== null &&
workInProgress.memoizedProps.suspenseCallback != null
) {
// Always notify the callback
// TODO: Move to passive phase
workInProgress.flags |= Update;
}
bubbleProperties(workInProgress);
Expand Down Expand Up @@ -1396,7 +1409,7 @@ function completeWork(
prevIsHidden !== nextIsHidden &&
newProps.mode !== 'unstable-defer-without-hiding'
) {
workInProgress.flags |= Update;
workInProgress.flags |= Visibility;
}
}

Expand Down
Loading

0 comments on commit 5add14a

Please sign in to comment.