WIP - Fabric ScrollView fixes #11
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… view Summary: Implement the native vertical axis flipping through the `isFlipped` method of the native macOS scroll view. Test Plan: Tested later in this stack Reviewers: shawndempsey, chpurrer, #rn-desktop Reviewed By: chpurrer Differential Revision: https://phabricator.intern.facebook.com/D49066276 Tasks: T154617099
Summary: Add the `inverted` property to the scroll view component properties in Fabric. Test Plan: Tested later in this stack Reviewers: shawndempsey, chpurrer, #rn-desktop Reviewed By: chpurrer Differential Revision: https://phabricator.intern.facebook.com/D49066275 Tasks: T154617099
Summary: The scroll view component uses an internal scroll view to which the `inverted` property needs to be propagated. Test Plan: Tested later in this stack Reviewers: shawndempsey, chpurrer, #rn-desktop Reviewed By: chpurrer Differential Revision: https://phabricator.intern.facebook.com/D49066273 Tasks: T154617099
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 registers component to the native scroll view content view bounds change notification which triggers when scrolling. This is used to dispatch the scroll notification to the delegate method implemented for the UIKit scroll views. This enables the dispatch of onScroll callbacks to the JS side. Test Plan: Run Zeratul with Fabric enabled and scroll a message thread. With the change the messages are being loaded when scrolling up in the history. | Before | After | |--| | https://pxl.cl/3MwMD | https://pxl.cl/3MwMl | Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D51165366
…nentView Summary: This diff enables the scrollbars on init for scroll views in Fabric together with autohiding. With the support of show/hide scroll bars properties, this adds full support for scroll bar visibility settings to scroll views in Fabric. Test Plan: Run Zeratul with Fabric enabled and scroll a message thread. With the change the scrollbars are showing up as expected. | Before | After | |--| | https://pxl.cl/3MwMl | https://pxl.cl/3MwNJ | Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D51165365
Summary: This diff implements the `contentInset` prop setting for the native scroll view used by `RCTScrollViewComponentView` in Fabric. The diff clips the scrollers and set the content inset on the native NSScrollView to match the functionality in UIKit. Test Plan: Run Zeratul with Fabric enabled. Scroll to the start of a message thread. With the change, the scroll content is correctly clipped to the top of the visible region of the message thread. | Before | After | |--| | https://pxl.cl/3MwQ2 | https://pxl.cl/3MwQf | Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D51165364
…scrollTo and scrollToEnd commands Summary: This diff adds the setContentOffset method to the `RCTEnhancedScrollView` used by `RCTScrollViewComponentView` in Fabric. This method is used by the `scrollTo` and `scrollToEnd` commands to programmatically scroll the view from the JS side. Test Plan: Run Zeratul with Fabric enabled and click on the arrow in the message thread to scroll to the end of the thread. | Before | After | |--| | https://pxl.cl/3MVQn | https://pxl.cl/3MVQs | Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D51222018 Tasks: T157893014
Summary: This diff adds the zoomToRect method to the `RCTEnhancedScrollView` used by `RCTScrollViewComponentView` in Fabric. This method is used by the `zoomToRect` command provided to the ScrollView on the JS side. Test Plan: Run Zeratul with Fabric enabled, open an image and use the zoom function in the media viewer. | Before | After | |--| | https://pxl.cl/3MVSl | https://pxl.cl/3MVSd | Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D51222020 Tasks: T169758894
Summary: This diff adds the flashScrollIndicators method to the `RCTEnhancedScrollView` used by `RCTScrollViewComponentView` in Fabric. This method is used by the command with the same name to enable the flashing of the scroll bars from the JS side. Test Plan: Run Zeratul with Fabric enabled and with a custom VirtualizedList.js implementation that adds the following code to `_onContentSizeChange`: ``` this.getScrollResponder().flashScrollIndicators(); ``` This should flash the scroll bars every time the content size of the scroll view changes. Meaning adding a message to a thread should flash the scroll bars using the `flashScrollIndicators` command. https://pxl.cl/3MVZ1 Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D51222019
…ll view Summary: - Override contentSize method to return documentView frame size on macOS - Update contentOffset setter to use clip view bounds as scroll view bounds - Add macOS scroll offset bias methods for content centering - Refresh content centering on content size changes Test Plan: Run Zeratul with Fabric and check that scrolling and content size changes still work as expected. https://pxl.cl/3NXWC Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D51292588
…xis flipping use Summary: To correctly transform shadow node layout metrics to ancestor coordinate spaces on macOS, we need to take view flipping into account to correctly flip the vertical axis during the transform. This diff adds a virtual function to `LayoutableShadowNode` that can be overridden to indicate if the view linked to the shadow node will layout its children with a flipped vertical axis (y = 0 is a the bottom of the view, the y axis is pointed upwards). By default the method returns false, which is the common case for native views in RN. On macOS, all views extending `RCTUIView` will configure the vertical axis to match the default on iOS. The origin is located at the top of the view and the y axis is pointed downwards. Test Plan: Tested later in this stack. Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D54931344 Tasks: T182039265, T182037720 Tags: uikit-diff
Summary: This diff updates the implementation of `LayoutableShadowNode::computeRelativeLayoutMetrics` which is used by the React `measure` api to return the layout metrics of a shadow node relative to an ancestor node. If a shadow node is a child of shadow node that lays out its children with a flipped vertical axis, then all transform calculations based on that node have to happen with coordinates within that same axis. The diff compares the flipped view state between the current node and the parent node to convert the current origin being transformed to the right axis orientation. A conversion happens if: - we move from a standard to a flipped axis - we move from a flipped axis to a standard one Test Plan: Tested later in this stack. Reviewers: shawndempsey, #rn-desktop Reviewed By: shawndempsey Differential Revision: https://phabricator.intern.facebook.com/D54931339 Tasks: T182039265, T182037720
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
Summary: React View components take the assumption that content clipping to the view's bounds is disabled by default. This diff fixes the propagation of the clipping setting to the `RCTUIView` instance and the backing core animation layer, by keeping both settings in sync with the component props. Core Animation on macOS also enabled clipping when a corner radius (border radius) is set on the layer. This would result in the random toggling of the clipping (overflow style) on the native view and make it out of sync with the component properties. This is being fixed by restoring the current `clipsToBounds` setting after setting the layer's corner radius property. Test Plan: - Run Cosmo Studio - Open the UI Reference (Developer > Show UI Reference) - Open the 'Inputs' example - Switch between other examples and back to 'Inputs' to verify that the clipping stays unchanged. | Before | After | |--| | https://pxl.cl/4xDWc | https://pxl.cl/4xDWt | Reviewers: shawndempsey, bedeoverend, #rn-desktop, #cosmo Reviewed By: bedeoverend Subscribers: generatedunixname499725568 Differential Revision: https://phabricator.intern.facebook.com/D55213691 Tasks: T182033885 Tags: uikit-diff # Conflicts: # packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WIP - Fabric ScrollView fixes