Skip to content

Commit

Permalink
Do not unmount layout effects on initial Offscreen mount (#25592)
Browse files Browse the repository at this point in the history
`wasHidden` is evaluted to false if `current` is null. This means
Offscreen has never been shown but this code assumes it is going from
'visible' to 'hidden' and unmounts layout effects.
To fix this, only unmount layout effects if `current` is not null.

I'm not able to repro this problem or write unit test for it. I see this
crash bug in production test.
The problem with repro is that if Offscreen starts as hidden, it's
render is deferred and current is no longer null.
  • Loading branch information
sammy-SC authored and rickhanlonii committed Dec 3, 2022
1 parent 76647a4 commit 7b536df
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 2 deletions.
3 changes: 2 additions & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -2880,7 +2880,8 @@ function commitMutationEffectsOnFiber(
}

if (isHidden) {
if (!wasHidden) {
// Check if this is an update, and the tree was previously visible.
if (current !== null && !wasHidden) {
if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
// Disappear the layout effects of all the children
recursivelyTraverseDisappearLayoutEffects(offscreenBoundary);
Expand Down
3 changes: 2 additions & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -2880,7 +2880,8 @@ function commitMutationEffectsOnFiber(
}

if (isHidden) {
if (!wasHidden) {
// Check if this is an update, and the tree was previously visible.
if (current !== null && !wasHidden) {
if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
// Disappear the layout effects of all the children
recursivelyTraverseDisappearLayoutEffects(offscreenBoundary);
Expand Down
38 changes: 38 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,44 @@ describe('ReactOffscreen', () => {
expect(root).toMatchRenderedOutput(<span prop="Child" />);
});

// @gate enableOffscreen
it('nested offscreen does not call componentWillUnmount when hidden', async () => {
// This is a bug that appeared during production test of <unstable_Offscreen />.
// It is a very specific scenario with nested Offscreens. The inner offscreen
// goes from visible to hidden in synchronous update.
class ClassComponent extends React.Component {
render() {
return <Text text="Child" />;
}

componentWillUnmount() {
Scheduler.unstable_yieldValue('componentWillUnmount');
}
}

function App() {
const [isVisible, setIsVisible] = React.useState(true);

if (isVisible === true) {
setIsVisible(false);
}

return (
<Offscreen mode="hidden">
<Offscreen mode={isVisible ? 'visible' : 'hidden'}>
<ClassComponent />
</Offscreen>
</Offscreen>
);
}

const root = ReactNoop.createRoot();
await act(async () => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded(['Child']);
});

// @gate enableOffscreen
it('mounts/unmounts layout effects when visibility changes (starting hidden)', async () => {
function Child({text}) {
Expand Down

0 comments on commit 7b536df

Please sign in to comment.