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

WIP - Fabric ScrollContentView 0_73 #10

Draft
wants to merge 4 commits into
base: 0.73-stable
Choose a base branch
from

Conversation

shwanton
Copy link
Owner

DONOTMERGE - Fabric ScrollContentView 0_73

shwanton and others added 4 commits April 28, 2024 06:47
Summary:
**Context**
- On Fabric, the ScrollView content Fabric component was never added for macOS.
- We need this content view to manage the `inverted` flag on our scrollviews. Mobile does not have this requirement.

**Change**
- Add Scroll Content Fabric component view
- Fix the js spec w/ codegen support & Paper fallback
- Remove ScrollContentView from the RCTLegacyViewManagerInteropComponent fallback list

Test Plan:
|Paper||
|{F1102578530}| https://pxl.cl/3sqgW|

|Fabric||
| {F1102600561} | https://pxl.cl/3sqn5 |

Reviewers: lefever, #rn-desktop

Subscribers: ramanpreet

Differential Revision: https://phabricator.intern.facebook.com/D49645029

Tags: uikit-diff
…llView

Summary:
**Context**

- D49645029 Added a Fabric ScrollContentView
- ScrollView.js adds this `contentContainer` view from RN
https://www.internalfb.com/code/fbsource/[381a6f37779b]/third-party/microsoft-fork-of-react-native/react-native/Libraries/Components/ScrollView/ScrollView.js?lines=1742-1759
- When the view is mounted, it was being inserted as subview instead of being added as the `documentView` which is required by AppKit.
https://www.internalfb.com/code/fbsource/[381a6f37779b]/third-party/microsoft-fork-of-react-native/react-native/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm?lines=372-376
- This was inserting a `RCTUIView` in the view hierarchy and the ScrollContentView was not working correctly
 {F1119521762}

**Change**
- ScrollContentView is added as `documentView` on mount
- Document view is reset to initial value (Empty RCTUIView) on unmount

Test Plan:
|Fabric|
| {F1119532976} |

Reviewers: chpurrer, lefever, #rn-desktop

Reviewed By: chpurrer

Differential Revision: https://phabricator.intern.facebook.com/D50276834
Summary:
This diff adds a custom shadow node for the ScrollContentView component, allowing to propagate the view flipping state to the shadow tree.

The ScrollContentView was using a generated shadow node. The generation of the descriptor and shadow node was disabled in the spec and a custom implementation was added for the component.

Test Plan: Tested later in this stack.

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Subscribers: ramanpreet

Differential Revision: https://phabricator.intern.facebook.com/D54931340

Tasks: T182039265, T182037720
…w shadow nodes

Summary:
To enable the correct layout metrics transformation with flipped views, we override the `LayoutableShadowNode` function `getIsVerticalAxisFlipped` to return the current configuration of the component.

This enables the shadow tree to correctly convert coordinates to the right axis when going through these component instances that have their axis flipped.

Test Plan:
- Run Zeratul with Fabric enabled
- Hover over a reaction
  - Check that the reaction summary popup is positioned above the reaction bubble
- Hover over a message bubble
  - Check that the timestamp popup is positioned level with the center of the message bubble
- Test this out at the top/bottom of the scroll view clipping area.

| Before | After |
|--|
|  https://pxl.cl/4vKvG  |  https://pxl.cl/4vKvg  |

Reviewers: shawndempsey, #rn-desktop

Reviewed By: shawndempsey

Differential Revision: https://phabricator.intern.facebook.com/D54931342

Tasks: T182039265, T182037720
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant