[ScrollView] Don't always adjust content view padding for both scrollers #1132
Conversation
This file contains hidden or 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
|
Nit: "NSScroller may be allovated" -> allocated |
huwilkes
reviewed
Apr 22, 2022
huwilkes
approved these changes
Apr 22, 2022
harrieshin
reviewed
Apr 22, 2022
Saadnajmi
added a commit
to Saadnajmi/react-native-macos
that referenced
this pull request
Apr 23, 2022
…ers (microsoft#1132) * Only pass height or width if needed * fix typo * Update RCTScrollContentLocalData.h
Saadnajmi
added a commit
to Saadnajmi/react-native-macos
that referenced
this pull request
Apr 23, 2022
…ers (microsoft#1132) * Only pass height or width if needed * fix typo * Update RCTScrollContentLocalData.h
Saadnajmi
added a commit
to Saadnajmi/react-native-macos
that referenced
this pull request
Apr 23, 2022
…ers (microsoft#1132) * Only pass height or width if needed * fix typo * Update RCTScrollContentLocalData.h
Saadnajmi
added a commit
to Saadnajmi/react-native-macos
that referenced
this pull request
Apr 23, 2022
…ers (microsoft#1132) * Only pass height or width if needed * fix typo * Update RCTScrollContentLocalData.h
shwanton
pushed a commit
to shwanton/react-native-macos
that referenced
this pull request
Feb 13, 2023
…h scrollers (microsoft#1132) (microsoft#1133)" This reverts commit f5622ec. # Conflicts: # React/Views/ScrollView/RCTScrollContentView.m
shwanton
added a commit
to shwanton/react-native-macos
that referenced
this pull request
Feb 13, 2023
Summary: ## Sync OS changes for ScrollBar [[ScrollView] Don't always adjust content view padding for both scrollers microsoft#1132](microsoft#1132) [ScrollView - Respect the shows<Horizontal | Vertical>ScrollIndicator props on macOS microsoft#1135](microsoft#1135) [Remove horizontal scrollview indicator padding when empty microsoft#1635](microsoft#1635) [Add ScrollView Indicator Overlay style microsoft#1658](microsoft#1658) NOTE: This will override the "Always" user setting and force the scrollview to use the overlay style instead {F850963270} Test Plan: |ScrollView Overlay Style| |https://pxl.cl/2qp0D| |ScrollView works correctly| |https://pxl.cl/2qpd1| Reviewers: chpurrer, lefever Reviewed By: chpurrer Differential Revision: https://phabricator.intern.facebook.com/D42637723
shwanton
pushed a commit
to shwanton/react-native-macos
that referenced
this pull request
Mar 10, 2023
…h scrollers (microsoft#1132) (microsoft#1133)" This reverts commit f5622ec. # Conflicts: # React/Views/ScrollView/RCTScrollContentView.m
shwanton
added a commit
to shwanton/react-native-macos
that referenced
this pull request
Mar 10, 2023
Summary: ## Sync OS changes for ScrollBar [[ScrollView] Don't always adjust content view padding for both scrollers microsoft#1132](microsoft#1132) [ScrollView - Respect the shows<Horizontal | Vertical>ScrollIndicator props on macOS microsoft#1135](microsoft#1135) [Remove horizontal scrollview indicator padding when empty microsoft#1635](microsoft#1635) [Add ScrollView Indicator Overlay style microsoft#1658](microsoft#1658) NOTE: This will override the "Always" user setting and force the scrollview to use the overlay style instead {F850963270} Test Plan: |ScrollView Overlay Style| |https://pxl.cl/2qp0D| |ScrollView works correctly| |https://pxl.cl/2qpd1| Reviewers: chpurrer, lefever Reviewed By: chpurrer Subscribers: generatedunixname89002005327315 Differential Revision: https://phabricator.intern.facebook.com/D42637723
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Please select one of the following
Summary
Based on your system preferences, NSScrollView may or may not always show it's scrollbars. When the scrollbars are shown, the content view of the scroll view is smaller than the scroll view itself, depending on whether we show a horizontal scrollbar, a vertical one, or both. Yoga doesn't know about this shrinking of content view's size, so we get around this by adding padding to the content view if scroll bars are visible.
The issue was, we would add padding for both horizontal and vertical scrollbars, event if only one or the other was shown. We fix that here with this change. Previously we passed the entire NSScroller (both vertical and horizontal) to
RCTScollContentLocalDataso that we could read the height/width. According to Apple documentation, an NSScroller may be allocated even if it's not shown. So, instead of passing the whole scroller, let's just see if it's shown and pass the width/height instead.Changelog
[macOS] [Fixed] - ScrollView - Don't always adjust content view padding for both scrollers
Test Plan
Ran RN-Tester and scrollviews appeared as expected.