Skip to content

Add ref to Offscreen component #25254

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

Merged
merged 6 commits into from
Sep 23, 2022
Merged

Add ref to Offscreen component #25254

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a86f80d
Expose ref to Offscreen if mode is manual
sammy-SC Sep 13, 2022
d8611ac
Prepend private fields on OffscreenInstance with underscore
sammy-SC Sep 13, 2022
a8f7a96
Schedule Ref effect unconditionally on Offscreen
sammy-SC Sep 21, 2022
1608017
Make sure Offscreen's ref is detached when unmounted
sammy-SC Sep 21, 2022
4320341
Make sure ref is mounted/unmounted in all scenarious
sammy-SC Sep 21, 2022
c7e962f
Nit: pendingProps -> memoizedProps
acdlite Sep 23, 2022
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
Prev Previous commit
Next Next commit
Make sure Offscreen's ref is detached when unmounted
  • Loading branch information
@sammy-SC
sammy-SC committed Sep 21, 2022
commit 1608017e77274b378a421db9c071295fbe65fbae
7 changes: 6 additions & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,7 +1121,7 @@ function commitLayoutEffectOnFiber(
if (flags & Ref) {
safelyAttachRef(finishedWork, finishedWork.return);
}
} else if (finishedWork.pendingProps.mode !== undefined) {
} else {
safelyDetachRef(finishedWork, finishedWork.return);
Copy link
Collaborator

@acdlite acdlite Sep 14, 2022

Choose a reason for hiding this comment

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

This also needs to be wrapped in a Ref effect. Otherwise you might forget to schedule a Ref effect in the begin phase, but it sometimes works anyway accidentally because you happen to have another effect flag scheduled during the same commit.

Copy link
Collaborator

@acdlite acdlite Sep 14, 2022

Choose a reason for hiding this comment

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

Also I think this should be a regular else block. Not else if. Anything that's not manual should have its ref detached.

Copy link
Contributor Author

@sammy-SC sammy-SC Sep 21, 2022

Choose a reason for hiding this comment

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

This also needs to be wrapped in a Ref effect. Otherwise you might forget to schedule a Ref effect in the begin phase, but it sometimes works anyway accidentally because you happen to have another effect flag scheduled during the same commit.

I don't follow this. What do you mean it needs to be wrapped in a Ref effect?

}
} else {
Expand Down Expand Up @@ -2148,6 +2148,8 @@ function commitDeletionEffectsOnFiber(
offscreenSubtreeWasHidden =
prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;

safelyDetachRef(deletedFiber, nearestMountedAncestor);

recursivelyTraverseDeletionEffects(
finishedRoot,
nearestMountedAncestor,
Expand Down Expand Up @@ -2833,6 +2835,7 @@ export function disappearLayoutEffects(finishedWork: Fiber) {
// Nested Offscreen tree is already hidden. Don't disappear
// its effects.
} else {
safelyDetachRef(finishedWork, finishedWork.return);
Copy link
Contributor Author

@sammy-SC sammy-SC Sep 21, 2022

Choose a reason for hiding this comment

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

This should be called even if Offscreen is hidden.

recursivelyTraverseDisappearLayoutEffects(finishedWork);
}
break;
Expand Down Expand Up @@ -2974,6 +2977,8 @@ export function reappearLayoutEffects(
finishedWork,
includeWorkInProgressEffects,
);

safelyAttachRef(finishedWork, finishedWork.return);
}
break;
}
Expand Down
7 changes: 6 additions & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,7 +1121,7 @@ function commitLayoutEffectOnFiber(
if (flags & Ref) {
safelyAttachRef(finishedWork, finishedWork.return);
}
} else if (finishedWork.pendingProps.mode !== undefined) {
} else {
safelyDetachRef(finishedWork, finishedWork.return);
}
} else {
Expand Down Expand Up @@ -2148,6 +2148,8 @@ function commitDeletionEffectsOnFiber(
offscreenSubtreeWasHidden =
prevOffscreenSubtreeWasHidden || deletedFiber.memoizedState !== null;

safelyDetachRef(deletedFiber, nearestMountedAncestor);

recursivelyTraverseDeletionEffects(
finishedRoot,
nearestMountedAncestor,
Expand Down Expand Up @@ -2833,6 +2835,7 @@ export function disappearLayoutEffects(finishedWork: Fiber) {
// Nested Offscreen tree is already hidden. Don't disappear
// its effects.
} else {
safelyDetachRef(finishedWork, finishedWork.return);
Copy link
Collaborator

@acdlite acdlite Sep 21, 2022

Choose a reason for hiding this comment

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

Move this outside the condition

recursivelyTraverseDisappearLayoutEffects(finishedWork);
}
break;
Expand Down Expand Up @@ -2974,6 +2977,8 @@ export function reappearLayoutEffects(
finishedWork,
includeWorkInProgressEffects,
);

safelyAttachRef(finishedWork, finishedWork.return);
}
break;
}
Expand Down
80 changes: 80 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 @@ -1307,4 +1307,84 @@ describe('ReactOffscreen', () => {
expect(offscreenRef.current).not.toBeNull();
});
});

// @gate enableOffscreen
it('should detach ref if Offscreen is unmounted', async () => {
let offscreenRef;

function App({showOffscreen}) {
offscreenRef = useRef(null);
return showOffscreen ? (
<Offscreen
mode={'manual'}
ref={ref => {
offscreenRef.current = ref;
}}>
<div />
</Offscreen>
) : null;
}

const root = ReactNoop.createRoot();

await act(async () => {
root.render(<App showOffscreen={true} />);
});

expect(offscreenRef.current).not.toBeNull();

await act(async () => {
root.render(<App showOffscreen={false} />);
});

expect(offscreenRef.current).toBeNull();

await act(async () => {
root.render(<App showOffscreen={true} />);
});

expect(offscreenRef.current).not.toBeNull();
});

// @gate enableOffscreen
it('should detach ref if Offscreen is unmounted', async () => {
Copy link
Collaborator

@acdlite acdlite Sep 21, 2022

Choose a reason for hiding this comment

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

Suggested change
it('should detach ref if Offscreen is unmounted', async () => {
it('should detach ref if Offscreen is hidden by parent', async () => {

let offscreenRef;
let divRef;

function App({mode}) {
offscreenRef = useRef(null);
divRef = useRef(null);
return (
<Offscreen mode={mode}>
<Offscreen mode={'manual'} ref={offscreenRef}>
<div ref={divRef} />
</Offscreen>
</Offscreen>
);
}

const root = ReactNoop.createRoot();

await act(async () => {
root.render(<App mode={'hidden'} />);
});

expect(offscreenRef.current).toBeNull();
expect(divRef.current).toBeNull();

await act(async () => {
root.render(<App mode={'visible'} />);
});

expect(offscreenRef.current).not.toBeNull();
expect(divRef.current).not.toBeNull();

console.log('Becoming Hidden');
Copy link
Collaborator

@acdlite acdlite Sep 21, 2022

Choose a reason for hiding this comment

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

oops

await act(async () => {
root.render(<App mode={'hidden'} />);
});

expect(offscreenRef.current).toBeNull();
expect(divRef.current).toBeNull();
});
});