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

Fix: Hide new/updated nodes in already hidden tree #21929

Merged
merged 1 commit into from
Jul 26, 2021
Merged

Fix: Hide new/updated nodes in already hidden tree #21929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 20 additions & 6 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 @@ -63,6 +63,7 @@ import {NoMode, ConcurrentMode, ProfileMode} from './ReactTypeOfMode';
import {
Ref,
RefStatic,
Placement,
Update,
Visibility,
NoFlags,
Expand Down Expand Up @@ -1369,13 +1370,26 @@ function completeWork(
}
}

// Don't bubble properties for hidden children.
if (
!nextIsHidden ||
includesSomeLane(subtreeRenderLanes, (OffscreenLane: Lane)) ||
(workInProgress.mode & ConcurrentMode) === NoMode
) {
if (!nextIsHidden || (workInProgress.mode & ConcurrentMode) === NoMode) {
bubbleProperties(workInProgress);
} else {
// Don't bubble properties for hidden children unless we're rendering
// at offscreen priority.
if (includesSomeLane(subtreeRenderLanes, (OffscreenLane: Lane))) {
bubbleProperties(workInProgress);
if (supportsMutation) {
// Check if there was an insertion or update in the hidden subtree.
// If so, we need to hide those nodes in the commit phase, so
// schedule a visibility effect.
if (
workInProgress.tag !== LegacyHiddenComponent &&
workInProgress.subtreeFlags & (Placement | Update) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure it's safe to check/rely on subtreeFlags here?

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 the static subtree flags that have caused regressions in the past, not the ones that are specific to a particular render, which we still rely on in a bunch of places (e.g. checking subtreeFlags & LayoutMask during the layout phase).

(Though also I believe I fixed the static flag issue in a previous PR, so we should try adding those checks back. Waiting for the Suspense effects feature flag to land first.)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the static subtree flags that have caused regressions in the past, not the ones that are specific to a particular render, which we still rely on in a bunch of places (e.g. checking subtreeFlags & LayoutMask during the layout phase).

Oh, good call.

(Though also I believe I fixed the static flag issue in a previous PR, so we should try adding those checks back. Waiting for the Suspense effects feature flag to land first.)

Oooh, that's exciting.

newProps.mode !== 'unstable-defer-without-hiding'
) {
workInProgress.flags |= Visibility;
}
}
}
}

if (enableCache) {
Expand Down
26 changes: 20 additions & 6 deletions packages/react-reconciler/src/ReactFiberCompleteWork.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ import {NoMode, ConcurrentMode, ProfileMode} from './ReactTypeOfMode';
import {
Ref,
RefStatic,
Placement,
Update,
Visibility,
NoFlags,
Expand Down Expand Up @@ -1369,13 +1370,26 @@ function completeWork(
}
}

// Don't bubble properties for hidden children.
if (
!nextIsHidden ||
includesSomeLane(subtreeRenderLanes, (OffscreenLane: Lane)) ||
(workInProgress.mode & ConcurrentMode) === NoMode
) {
if (!nextIsHidden || (workInProgress.mode & ConcurrentMode) === NoMode) {
bubbleProperties(workInProgress);
} else {
// Don't bubble properties for hidden children unless we're rendering
// at offscreen priority.
if (includesSomeLane(subtreeRenderLanes, (OffscreenLane: Lane))) {
bubbleProperties(workInProgress);
if (supportsMutation) {
// Check if there was an insertion or update in the hidden subtree.
// If so, we need to hide those nodes in the commit phase, so
// schedule a visibility effect.
if (
workInProgress.tag !== LegacyHiddenComponent &&
workInProgress.subtreeFlags & (Placement | Update) &&
newProps.mode !== 'unstable-defer-without-hiding'
) {
workInProgress.flags |= Visibility;
}
}
}
}

if (enableCache) {
Expand Down
112 changes: 80 additions & 32 deletions packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,14 +201,7 @@ describe('ReactOffscreen', () => {
});
// No layout effect.
expect(Scheduler).toHaveYielded(['Child']);
if (gate(flags => flags.persistent)) {
expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
} else {
// TODO: Offscreen does not yet hide/unhide children correctly in mutation
// mode. Until we do, it should only be used inside a host component
// wrapper whose visibility is toggled simultaneously.
expect(root).toMatchRenderedOutput(<span prop="Child" />);
}
expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);

// Unhide the tree. The layout effect is mounted.
await act(async () => {
Expand Down Expand Up @@ -255,14 +248,7 @@ describe('ReactOffscreen', () => {
);
});
expect(Scheduler).toHaveYielded(['Unmount layout', 'Child']);
if (gate(flags => flags.persistent)) {
expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
} else {
// TODO: Offscreen does not yet hide/unhide children correctly in mutation
// mode. Until we do, it should only be used inside a host component
// wrapper whose visibility is toggled simultaneously.
expect(root).toMatchRenderedOutput(<span prop="Child" />);
}
expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);

// Unhide the tree. The layout effect is re-mounted.
await act(async () => {
Expand Down Expand Up @@ -299,14 +285,7 @@ describe('ReactOffscreen', () => {
);
});
expect(Scheduler).toHaveYielded(['Child']);
if (gate(flags => flags.persistent)) {
expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
} else {
// TODO: Offscreen does not yet hide/unhide children correctly in mutation
// mode. Until we do, it should only be used inside a host component
// wrapper whose visibility is toggled simultaneously.
expect(root).toMatchRenderedOutput(<span prop="Child" />);
}
expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);

// Show the tree. The layout effect is mounted.
await act(async () => {
Expand All @@ -328,14 +307,7 @@ describe('ReactOffscreen', () => {
);
});
expect(Scheduler).toHaveYielded(['Unmount layout', 'Child']);
if (gate(flags => flags.persistent)) {
expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
} else {
// TODO: Offscreen does not yet hide/unhide children correctly in mutation
// mode. Until we do, it should only be used inside a host component
// wrapper whose visibility is toggled simultaneously.
expect(root).toMatchRenderedOutput(<span prop="Child" />);
}
expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
});

// @gate experimental || www
Expand Down Expand Up @@ -385,4 +357,80 @@ describe('ReactOffscreen', () => {
});
expect(Scheduler).toHaveYielded(['Unmount layout']);
});

// @gate experimental || www
it('hides new insertions into an already hidden tree', async () => {
const root = ReactNoop.createRoot();
await act(async () => {
root.render(
<Offscreen mode="hidden">
<span>Hi</span>
</Offscreen>,
);
});
expect(root).toMatchRenderedOutput(<span hidden={true}>Hi</span>);

// Insert a new node into the hidden tree
await act(async () => {
root.render(
<Offscreen mode="hidden">
<span>Hi</span>
<span>Something new</span>
</Offscreen>,
);
});
expect(root).toMatchRenderedOutput(
<>
<span hidden={true}>Hi</span>
{/* This new node should also be hidden */}
<span hidden={true}>Something new</span>
</>,
);
});

// @gate experimental || www
it('hides updated nodes inside an already hidden tree', async () => {
const root = ReactNoop.createRoot();
await act(async () => {
root.render(
<Offscreen mode="hidden">
<span>Hi</span>
</Offscreen>,
);
});
expect(root).toMatchRenderedOutput(<span hidden={true}>Hi</span>);

// Set the `hidden` prop to on an already hidden node
await act(async () => {
root.render(
<Offscreen mode="hidden">
<span hidden={false}>Hi</span>
</Offscreen>,
);
});
// It should still be hidden, because the Offscreen container overrides it
expect(root).toMatchRenderedOutput(<span hidden={true}>Hi</span>);

// Unhide the boundary
await act(async () => {
root.render(
<Offscreen mode="visible">
<span hidden={true}>Hi</span>
</Offscreen>,
);
});
// It should still be hidden, because of the prop
expect(root).toMatchRenderedOutput(<span hidden={true}>Hi</span>);

// Remove the `hidden` prop
await act(async () => {
root.render(
<Offscreen mode="visible">
<span>Hi</span>
</Offscreen>,
);
});
// Now it's visible
expect(root).toMatchRenderedOutput(<span>Hi</span>);
});
});