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.

Sign up for GitHub

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

[Bugfix] Don't hide/unhide unless visibility changes #21875

Merged
merged 2 commits into from
Jul 14, 2021
+223 −103
Merged

[Bugfix] Don't hide/unhide unless visibility changes #21875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2537142
Use Visibility flag to schedule a hide/show effect
acdlite Jul 14, 2021
ca72f59
Added failing test case
Jul 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 41 additions & 31 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

}
}
if (supportsPersistence) {
if (nextDidTimeout) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I read this, I wondered why we didn't also have the nextDidTimeout !== prevDidTimeout check here, then I noticed before we also had this comment that didn't get copied over:

// TODO: Only schedule updates if not prevDidTimeout.

I'm not really familiar with the intricacies of persistent mode. Was this TODO just not necessary? Or was this a copy-paste error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still relevant but I replaced it with the TODO comment below:

          // 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.

// 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