Skip to content

Commit

Permalink
Fix: Hide new/updated nodes in already hidden tree (facebook#21929)
Browse files Browse the repository at this point in the history 
When a new node is added to an already hidden Offscreen tree, we need
to schedule a visibility effect to hide it. Previously we would only
hide when the boundary initially switches from visible to hidden, which
meant that newly inserted nodes would be visible.

We need to do the same thing for nodes that are updated, because the
update might affect the DOM node's `style.display` property.

The implementation is to check the `subtreeFlags` for an Insertion or
Update effect.

This only affects Offscreen, not Suspense, because Suspense boundaries
cannot be updated while in their fallback (hidden) state.

And it only affects mutation mode, because in persistent mode we
implement hiding by cloning the host tree during the complete phase,
which already happens on every update.
  • Loading branch information
@acdlite @zhengjitf
acdlite authored and zhengjitf committed Apr 15, 2022
1 parent 84d11d7 commit bce1e9e
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 44 deletions.
26 changes: 20 additions & 6 deletions packages/react-reconciler/src/ReactFiberCompleteWork.new.js
View file
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
26 changes: 20 additions & 6 deletions packages/react-reconciler/src/ReactFiberCompleteWork.old.js
View file
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
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>);
});
});

0 comments on commit bce1e9e

Please sign in to comment.