Skip to content

Conversation

Saadnajmi
Copy link
Collaborator

Platforms Impacted

  • iOS
  • macOS
  • win32 (Office)
  • windows
  • android

Description of changes

Note: This PR requires microsoft/react-native-macos#1135 to land and get back ported as a prerequisite

On macOS, there are 2 types of scrollbars that you can switch between in user preferences:

  • Legacy: Scrollbars that appear at the trailing end of the content view in a dedicated space.
  • Overlay: iOS style scrollbars that are overlaid on top of the content view.

The former legacy style scrollbars add some margins to the scroll views' shadow view in order to account for the scroll bars. There is a race condition during layout of our ContextualMenu where the margins may show up even if we don't have a need for the scrollbars because all the content is visible. This was rather awkward, and a pretty big visual bug in ContextualMenu.

Screen.Recording.2022-04-25.at.1.43.47.AM.mov

We can fix this by telling ContextualMenu to only show scroll bars when we need them, AKA, when the user specified a maxHeight for vertical scrollbars, or maxWidth for horizontal scrollbars.

Verification

I tested a few different menus on our ContextualMenu test page, including one that wants a scrollbar and one that doesn't. Apart from that very last submenu, things seem to be laying out correctly.

Screen.Recording.2022-04-25.at.3.32.25.AM.mov

Pull request checklist

This PR has considered (when applicable):

  • Automated Tests
  • Documentation and examples
  • Keyboard Accessibility
  • Voiceover
  • Internationalization and Right-to-left Layouts

@Saadnajmi Saadnajmi requested a review from a team as a code owner April 25, 2022 11:36
@PPatBoyd
Copy link
Contributor

@Saadnajmi and if the context menu takes up the entire height of the screen, e.g. in a min-resolution max-text scale scenario?

@Saadnajmi
Copy link
Collaborator Author

@Saadnajmi and if the context menu takes up the entire height of the screen, e.g. in a min-resolution max-text scale scenario?

That's why we added support for a scroll view. It was added because our partners have that scenario, and adding a scroll view + max height prop to limit the size and make sure it never gets too big.

@Saadnajmi
Copy link
Collaborator Author

@PPatBoyd I tested it locally. I made a long list with 36 menu items (I just copy-pasted the original 12 items over twice), set showsVerticalIndicator=true, and removed the maxHeight/maxWidth. On win32, it seems that the Callout will just clip, and not show a scrollbar. It seems that you do need to have the max height set, which is probably why @lenahong added it to begin with. So I think this change is still valid on win32.

image

@Saadnajmi Saadnajmi merged commit 0f39d21 into microsoft:master Apr 26, 2022
@Saadnajmi Saadnajmi deleted the scrollview-margins-fix branch April 26, 2022 06:20
@Saadnajmi Saadnajmi mentioned this pull request Dec 21, 2022
10 tasks
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.

3 participants