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

[Select] Create new Select component #541

Merged
merged 206 commits into from
Nov 14, 2024
Merged

[Select] Create new Select component #541

merged 206 commits into from
Nov 14, 2024

Conversation

atomiks
Copy link
Contributor

@atomiks atomiks commented Aug 15, 2024

Preview: https://deploy-preview-541--base-ui.netlify.app/components/react-select/

TODO:

  • Integrate with useField
  • Renamed alignMethod prop to alignOptionToTrigger

Olivier's edit: the v2 of mui/material-ui#8023 😄

@atomiks atomiks added the component: select This is the name of the generic UI component, not the React module! label Aug 15, 2024
@atomiks atomiks marked this pull request as draft August 15, 2024 02:37
@mui-bot
Copy link

mui-bot commented Aug 15, 2024

Netlify deploy preview

https://deploy-preview-541--base-ui.netlify.app/

Generated by 🚫 dangerJS against 18ef2e8

@atomiks atomiks force-pushed the feat/Select branch 2 times, most recently from 2384edd to c32adfc Compare August 23, 2024 06:17
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 2, 2024
ownerState,
propGetter: (externalProps) => getTriggerProps(getRootTriggerProps(externalProps)),
customStyleHookMapping: commonStyleHooks,
extraProps: otherProps,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if the Trigger had a CSS variable with the width of the popup, so it can match it (as in native OS controls)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the inverse? --anchor-width for popup is available. The trigger can't know the width of the popup when it's not mounted

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I didn't mean the inverse. Native OS selects adapt their width to the longest option. Perhaps this could be an option when keepMounted=true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, but I'm hesitant to add something that doesn't work by default. Furthermore, it's likely very performance intensive with longer lists to calculate as you'll need to measure every option with getBoundingClientRect to calculate the largest one. It also doesn't work during SSR, so will cause layout shift. It's likely something that should be hardcoded by the user if necessary.

packages/mui-base/src/Select/Root/SelectRoot.tsx Outdated Show resolved Hide resolved
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Sep 5, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 6, 2024
@michaldudak
Copy link
Member

Could you please review the list of reported Select bugs and set this PR to close the ones that will be fixed by it?

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Sep 11, 2024
@flaviendelangle
Copy link
Member

When you open the select, the page scrollbar disappears causing a layout shift on the right navbar in the doc (https://deploy-preview-541--base-ui.netlify.app/components/react-select/)

The @mui/material select also removes the scrollbar but there is no layout shift (https://mui.com/material-ui/react-select/)

@atomiks
Copy link
Contributor Author

atomiks commented Sep 11, 2024

@flaviendelangle interesting. The padding-right: 15px for the scrollbar width offset once it disappears prevents the main content from shifting but not the right navbar content (and end of the top navbar)

@michaldudak
Copy link
Member

michaldudak commented Sep 18, 2024

I'm wondering if we should prevent scrolling by default at all. I find it annoying when I open a select and something else changes appearance (=the scrollbar disappears). Native implementations differ in this matter - iOS prevents scrolling but doesn't hide the scrollbar, while Windows closes the popup when the page scrolls.
IMO locking the scroll is more important in modal windows.

I found an issue that's likely related to scroll locking: https://codesandbox.io/p/sandbox/q7qfjg - when you open the select, there's a small layout shift and you're able to scroll the page horizontally until the whole trigger disappears.

@atomiks
Copy link
Contributor Author

atomiks commented Sep 18, 2024

I'm wondering if we should prevent scrolling by default at all. I find it annoying when I open a select and something else changes appearance (=the scrollbar disappears).

The scroll locking changes we made in #604 prevents layout shift issues from disappearing scrollbars. The scroll lock is necessary with the item align anchoring because you can wheel the popup to reveal more items, but this can cause the page to scroll. It's not locked for the trigger align method.

I found an issue that's likely related to scroll locking: https://codesandbox.io/p/sandbox/q7qfjg - when you open the select, there's a small layout shift and you're able to scroll the page horizontally until the whole trigger disappears.

This looks like a bug with the .SelectPopup min-width style that doesn't limit itself by the --available-width.

This solves it:

  min-width: min(var(--available-width), calc(var(--anchor-width) + 20px));

There's a thread discussing default "functional styles" to apply to these elements and an API to override them in a thread in the base-ui channel. This could help prevent certain issues like this, including:

  • Making the popup scrollable by default
  • Limiting the popup's size to the available space (but the above calculation requires the user to use their own custom padding value)

@michaldudak
Copy link
Member

This looks like a bug with the .SelectPopup min-width style that doesn't limit itself by the --available-width.

This solves it:

The repro was taken directly from the demos, so they'll need to have the styles updated.

docs/data/components/select/select.mdx Outdated Show resolved Hide resolved
docs/src/styles/reset.css Outdated Show resolved Hide resolved
packages/mui-base/src/Field/Root/FieldRoot.test.tsx Outdated Show resolved Hide resolved
packages/mui-base/src/Select/Icon/SelectIcon.tsx Outdated Show resolved Hide resolved
packages/mui-base/src/Select/Root/useSelectRoot.tsx Outdated Show resolved Hide resolved
packages/mui-base/src/Select/Root/useSelectRoot.tsx Outdated Show resolved Hide resolved
packages/mui-base/src/Select/Separator/SelectSeparator.tsx Outdated Show resolved Hide resolved
packages/mui-base/src/Select/Separator/SelectSeparator.tsx Outdated Show resolved Hide resolved
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 24, 2024
@mnajdova
Copy link
Member

mnajdova commented Nov 12, 2024

  1. What's the heuristic about giving up on the align option to trigger when close to the edge? I am asking because we have kind of a different behavior based on the options, e.g.
Screenshot 2024-11-12 at 10 31 32 Screenshot 2024-11-12 at 10 31 42

I would assume these two to behave the same way (we have space for the scrolling arrow in both scenarios).

  1. Can we explain in this section why the change of the aligning affects whether the scroll is blocked or not. It could be surprising that just because you want things to be aligned differently it affects whether the scroll on the page is available or not.

  2. Based on https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/, we shouldn't support circular navigation when using Up/Down Arrow => we should set loop: false in the useListNavigation.

Screenshot 2024-11-12 at 10 35 25
  1. Based on https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/ we should open the listbox when user types printable characters with setting active descendant on the right item:

If the combobox is collapsed and the user types printable characters, the listbox is displayed and receives accessibility focus via aria-activedescendant. This enables users to perceive the presence of the options, and enables assistive technology users to comprehend the size of the list of options.

  1. Are we handling Home and End keys?

@atomiks
Copy link
Contributor Author

atomiks commented Nov 12, 2024

  1. What's the heuristic about giving up on the align option to trigger when close to the edge? I am asking because we have kind of a different behavior based on the options, e.g.

If the min-height is 100px or less (can be configured in their CSS), or if the reference is within 20px of the viewport edge, it falls back to standard anchoring. I felt the UX was much better in this case, matching macOS' fallback behavior.

Fair point!

Can change this, not sure if we should provide an option if so.

This seems to be a new pattern or suggestion that I haven't seen before, and the native select doesn't appear to do this. It doesn't seem critical to implement for now.

Yes, Home/End are handled


const listItem = useCompositeListItem({ label });
const { activeIndex, selectedIndex, setActiveIndex } = useSelectIndexContext();
const { getItemProps, setOpen, setValue, open, selectionRef, typingRef, valuesRef, popupRef } =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be moved to InnerSelectOption? It seems that these values don't change on each option highlight (except maybe getItemProps?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consuming the root context inside the memoized component is risky as any time the root context changes it will cause every option to re-render. Even if they don't change on each option highlight, they can change on other unrelated state changes. Minor refactorings could introduce issues unknowingly as well.

);

return (
<SelectOptionContext.Provider value={contextValue}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this provider can be rendered by InnerSelectOption

Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The performance of large lists is still not quite good. I played a bit with the profiler and identified that quite a lot of time is spent in FloatingFocusManager's handleMutation (https://github.com/floating-ui/floating-ui/blob/master/packages/react/src/components/FloatingFocusManager.tsx#L651). I initially thought that the slowdown may be caused by focusing each item on hover, but Radix does is as well and yet their Select feels snappier. IMO we can merge this PR so that this branch doesn't live for too long, but we should then investigate what can be done to make it even faster.

packages/mui-base/src/utils/constants.ts Outdated Show resolved Hide resolved
@atomiks atomiks merged commit d9ba2fe into mui:master Nov 14, 2024
20 of 21 checks passed
@atomiks atomiks deleted the feat/Select branch November 14, 2024 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment