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

Jump to bottom

Fix: LegacyHidden should not toggle effects #21928

Merged
merged 1 commit into from
Jul 21, 2021
Merged

Fix: LegacyHidden should not toggle effects #21928

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
36 changes: 17 additions & 19 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 @@ -2131,8 +2131,7 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
}
break;
}
case OffscreenComponent:
case LegacyHiddenComponent: {
case OffscreenComponent: {
const newState: OffscreenState | null = finishedWork.memoizedState;
const isHidden = newState !== null;
const current = finishedWork.alternate;
Expand All @@ -2145,27 +2144,26 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
hideOrUnhideAllChildren(offscreenBoundary, isHidden);
}

if (isHidden) {
if (!wasHidden) {
if (
enableSuspenseLayoutEffectSemantics &&
(offscreenBoundary.mode & ConcurrentMode) !== NoMode
) {
nextEffect = offscreenBoundary;
let offscreenChild = offscreenBoundary.child;
while (offscreenChild !== null) {
nextEffect = offscreenChild;
disappearLayoutEffects_begin(offscreenChild);
offscreenChild = offscreenChild.sibling;
if (enableSuspenseLayoutEffectSemantics) {
if (isHidden) {
if (!wasHidden) {
if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
nextEffect = offscreenBoundary;
let offscreenChild = offscreenBoundary.child;
while (offscreenChild !== null) {
nextEffect = offscreenChild;
disappearLayoutEffects_begin(offscreenChild);
offscreenChild = offscreenChild.sibling;
}
}
}
} else {
if (wasHidden) {
// TODO: Move re-appear call here for symmetry?
}
}
} else {
if (wasHidden) {
// TODO: Move re-appear call here for symmetry?
}
break;
}
break;
}
}
}
Expand Down
36 changes: 17 additions & 19 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 @@ -2131,8 +2131,7 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
}
break;
}
case OffscreenComponent:
case LegacyHiddenComponent: {
case OffscreenComponent: {
const newState: OffscreenState | null = finishedWork.memoizedState;
const isHidden = newState !== null;
const current = finishedWork.alternate;
Expand All @@ -2145,27 +2144,26 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
hideOrUnhideAllChildren(offscreenBoundary, isHidden);
}

if (isHidden) {
if (!wasHidden) {
if (
enableSuspenseLayoutEffectSemantics &&
(offscreenBoundary.mode & ConcurrentMode) !== NoMode
) {
nextEffect = offscreenBoundary;
let offscreenChild = offscreenBoundary.child;
while (offscreenChild !== null) {
nextEffect = offscreenChild;
disappearLayoutEffects_begin(offscreenChild);
offscreenChild = offscreenChild.sibling;
if (enableSuspenseLayoutEffectSemantics) {
if (isHidden) {
if (!wasHidden) {
if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
nextEffect = offscreenBoundary;
let offscreenChild = offscreenBoundary.child;
while (offscreenChild !== null) {
nextEffect = offscreenChild;
disappearLayoutEffects_begin(offscreenChild);
offscreenChild = offscreenChild.sibling;
}
}
}
} else {
if (wasHidden) {
// TODO: Move re-appear call here for symmetry?
}
}
} else {
if (wasHidden) {
// TODO: Move re-appear call here for symmetry?
}
break;
}
break;
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion packages/react-reconciler/src/ReactFiberCompleteWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1361,7 +1361,9 @@ function completeWork(
const prevIsHidden = prevState !== null;
if (
prevIsHidden !== nextIsHidden &&
newProps.mode !== 'unstable-defer-without-hiding'
newProps.mode !== 'unstable-defer-without-hiding' &&
// LegacyHidden doesn't do any hiding — it only pre-renders.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused. Legacy hidden does toggle visibility, right? It's just not supposed to run layout effects when visibility changes.

Wouldn't removing this flag also prevent it from calling hideOrUnhideAllChildren later?

Copy link
Collaborator Author

@acdlite acdlite Jul 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It used to hide stuff but I removed that behavior, because some existing code in FB was relying on the ability to override the style of <div hidden={true} /> with a CSS class by setting display: block. Which doesn't work if React sets an inline style, which is what hideOrUnhideAllChildren does.

So instead in www we always render LegacyHidden via a userspace abstraction that wraps it in a DOM node, and sets hidden=true on that DOM node. This also worked around the insertion problem fixed by this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The way the hidden attribute works is browser's have a built-in stylesheet with this rule:

[hidden] {
  display: none;
}

That has pretty low precedence in CSS, so a user stylesheet can override it by setting display to something else.

But the way Offscreen/Suspense works is by setting the display property directly on the node with an inline style. We also use !important. So it can't be overridden.

We don't actually want to support overriding the style like this, but some code was already relying on it, so I added the LegacyHidden type to prevent a regression and unblock the Lanes refactor.

Copy link
Contributor

Choose a reason for hiding this comment

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

It used to hide stuff but I removed that behavior

I see. Looking at how the Visibility flag is used, it's what ensures hideOrUnhideAllChildren is called which in turn is what calls hideInstance or unhideInstance in the host config, so this change seemed like it would break that.

So instead in www we always render LegacyHidden via a userspace abstraction that wraps it in a DOM node, and sets hidden=true on that DOM node.

Ah, so basically FB is the only ones that will ever use this component, and we'll eventually clean it up there too? (So this API doesn't need to work outside of the FB context?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, so basically FB is the only ones that will ever use this component, and we'll eventually clean it up there too? (So this API doesn't need to work outside of the FB context?)

Yeah exactly

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 used to hide stuff but I removed that behavior

To be clear I mean I removed this behavior like last year :D

workInProgress.tag !== LegacyHiddenComponent
) {
workInProgress.flags |= Visibility;
}
Expand Down
4 changes: 3 additions & 1 deletion packages/react-reconciler/src/ReactFiberCompleteWork.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1361,7 +1361,9 @@ function completeWork(
const prevIsHidden = prevState !== null;
if (
prevIsHidden !== nextIsHidden &&
newProps.mode !== 'unstable-defer-without-hiding'
newProps.mode !== 'unstable-defer-without-hiding' &&
// LegacyHidden doesn't do any hiding — it only pre-renders.
workInProgress.tag !== LegacyHiddenComponent
) {
workInProgress.flags |= Visibility;
}
Expand Down
48 changes: 48 additions & 0 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 @@ -337,4 +337,52 @@ describe('ReactOffscreen', () => {
expect(root).toMatchRenderedOutput(<span prop="Child" />);
}
});

// @gate experimental || www
it('does not toggle effects for LegacyHidden component', async () => {
// LegacyHidden is meant to be the same as offscreen except it doesn't
// do anything to effects. Only used by www, as a temporary migration step.
function Child({text}) {
useLayoutEffect(() => {
Scheduler.unstable_yieldValue('Mount layout');
return () => {
Scheduler.unstable_yieldValue('Unmount layout');
};
}, []);
return <Text text="Child" />;
}

const root = ReactNoop.createRoot();
await act(async () => {
root.render(
<LegacyHidden mode="visible">
<Child />
</LegacyHidden>,
);
});
expect(Scheduler).toHaveYielded(['Child', 'Mount layout']);

await act(async () => {
root.render(
<LegacyHidden mode="hidden">
<Child />
</LegacyHidden>,
);
});
expect(Scheduler).toHaveYielded(['Child']);

await act(async () => {
root.render(
<LegacyHidden mode="visible">
<Child />
</LegacyHidden>,
);
});
expect(Scheduler).toHaveYielded(['Child']);

await act(async () => {
root.render(null);
});
expect(Scheduler).toHaveYielded(['Unmount layout']);
});
});