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.

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

Don't disappear layout effects unnecessarily #25660

Merged

Conversation

sammy-SC
Copy link
Contributor

@sammy-SC sammy-SC commented Nov 10, 2022

Nested Offscreens can run into a case where outer Offscreen is revealed while inner one is hidden in a single commit. This is an edge case that was previously missed. We need to prevent call to disappear layout effects.

When we go from state:

<Offscreen mode={'hidden'}> // outer offscreen
  <Offscreen mode={'visible'}> // inner offscreen
    {children}
  </Offscreen>
</Offscreen>

To following. Notice that visibility of each offscreen flips.

<Offscreen mode={'visible'}> // outer offscreen
  <Offscreen mode={'hidden'}> // inner offscreen
    {children}
  </Offscreen>
</Offscreen>

Inner offscreen must not call recursivelyTraverseDisappearLayoutEffects.
Check unit tests for an example of this.

Nested Offscreens can run into a case where anscestor Offscreen
is revealed while inner one is hidden. This is an edge case that
was previously missed. We need to prevent call to disappear layout
effects. Check unit tests for an example of this.
@sammy-SC sammy-SC requested a review from acdlite November 10, 2022 13:43
@sammy-SC sammy-SC marked this pull request as ready for review November 10, 2022 13:43
@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Nov 10, 2022
@sizebot
Copy link

sizebot commented Nov 10, 2022

Comparing: 1e3e30d...d5adfe0

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 153.65 kB 153.65 kB = 48.90 kB 48.90 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 155.57 kB 155.57 kB = 49.51 kB 49.51 kB
facebook-www/ReactDOM-prod.classic.js = 530.46 kB 530.48 kB = 94.67 kB 94.68 kB
facebook-www/ReactDOM-prod.modern.js = 515.71 kB 515.74 kB = 92.49 kB 92.50 kB
facebook-www/ReactDOMForked-prod.classic.js = 530.46 kB 530.48 kB = 94.67 kB 94.68 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against d5adfe0

@sammy-SC sammy-SC merged commit d1e35c7 into facebook:main Nov 10, 2022
@sammy-SC sammy-SC deleted the offscreen-fix-incorrect-disappearLayoutEffects branch November 10, 2022 14:49
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Nov 10, 2022
Summary:
This sync includes the following changes:
- **[d1e35c703](facebook/react@d1e35c703 )**: Don't disappear layout effects unnecessarily ([#25660](facebook/react#25660)) //<Samuel Susla>//
- **[1e3e30dae](facebook/react@1e3e30dae )**: Fix useSyncExternalStore dropped update when state is dispatched in render phase ([#25578](facebook/react#25578)) //<Aurélien Chivot-Buhler>//

Changelog:
[General][Changed] - React Native sync for revisions 4bd245e...d1e35c7

jest_e2e[run_all_tests]

Reviewed By: GijsWeterings

Differential Revision: D41187776

fbshipit-source-id: 9c82b79458487191f4a26cf643321603d30cea5a
mofeiZ pushed a commit to mofeiZ/react that referenced this pull request Nov 17, 2022
Nested Offscreens can run into a case where outer Offscreen is revealed
while inner one is hidden in a single commit. This is an edge case that
was previously missed. We need to prevent call to disappear layout
effects.

When we go from state:
```jsx
<Offscreen mode={'hidden'}> // outer offscreen
  <Offscreen mode={'visible'}> // inner offscreen
    {children}
  </Offscreen>
</Offscreen>
```

To following. Notice that visibility of each offscreen flips.

```jsx
<Offscreen mode={'visible'}> // outer offscreen
  <Offscreen mode={'hidden'}> // inner offscreen
    {children}
  </Offscreen>
</Offscreen>
```

Inner offscreen must not call
`recursivelyTraverseDisappearLayoutEffects`.
Check unit tests for an example of this.
rickhanlonii pushed a commit that referenced this pull request Dec 3, 2022
Nested Offscreens can run into a case where outer Offscreen is revealed
while inner one is hidden in a single commit. This is an edge case that
was previously missed. We need to prevent call to disappear layout
effects.

When we go from state:
```jsx
<Offscreen mode={'hidden'}> // outer offscreen
  <Offscreen mode={'visible'}> // inner offscreen
    {children}
  </Offscreen>
</Offscreen>
```

To following. Notice that visibility of each offscreen flips.

```jsx
<Offscreen mode={'visible'}> // outer offscreen
  <Offscreen mode={'hidden'}> // inner offscreen
    {children}
  </Offscreen>
</Offscreen>
```

Inner offscreen must not call
`recursivelyTraverseDisappearLayoutEffects`.
Check unit tests for an example of this.
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This sync includes the following changes:
- **[d1e35c703](facebook/react@d1e35c703 )**: Don't disappear layout effects unnecessarily ([facebook#25660](facebook/react#25660)) //<Samuel Susla>//
- **[1e3e30dae](facebook/react@1e3e30dae )**: Fix useSyncExternalStore dropped update when state is dispatched in render phase ([facebook#25578](facebook/react#25578)) //<Aurélien Chivot-Buhler>//

Changelog:
[General][Changed] - React Native sync for revisions 4bd245e...d1e35c7

jest_e2e[run_all_tests]

Reviewed By: GijsWeterings

Differential Revision: D41187776

fbshipit-source-id: 9c82b79458487191f4a26cf643321603d30cea5a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants