CustomSelectControl: Update to use a Popover component for rendering the internals #37272
Conversation
andrewserong
added
[Type] Bug
andrewserong
added
the
[Status] In Progress
andrewserong
changed the title
|
Size Change: +1.37 kB (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
@youknowriad and @ciampo, if you have time, could you please give a general 👍 or 👎 on the approach taken here before we continue to refine the results as per the PR description todo list? |
| import { Button, VisuallyHidden } from '../'; | ||
| import { Button, Popover, VisuallyHidden } from '../'; | ||
|
|
||
| const OptionList = ( { isOpen, children } ) => { |
There was a problem hiding this comment.
Instead of a custom implementation like that, maybe DropdownMenu is a component suited for this use-case (with all the focus management...)
|
Thanks for the suggestion @youknowriad — I think Here's a screengrab of where I got up to by the end of the day:
I'll keep chipping away at this, next up, I'll look into how we can set focus to the currently selected item when we open the dropdown menu. |
| style: item.style, | ||
| } } | ||
| onClick={ () => { | ||
| onSelectedItemChange( { |
There was a problem hiding this comment.
The readme for this component states the onChange prop is optional so onSelectedItemChange here might not be a function. We hit this error when testing the recent changes in the Storybook as well.
There was a problem hiding this comment.
Thanks for continuing to refine this @andrewserong 👌
I took it for another premature test drive. It's working well so far for me in the block editor.
In addition to the focus management items you've already mentioned, there were a few small issues that I believe will be ironed out as this evolves. In case it helps they were:
- The storybook example is currently broken - see earlier comment
- Styling, size, and position of the dropdown menu is quite different to before this update
Before / After screenshots
| Before | After |
|---|---|
 |
 |
 |
 |
There was a problem hiding this comment.
Hey @andrewserong , thank you for exploring a solution to avoid the CustomSelectControl getting cut off.
The main piece of feedback that I have is that, with the new approach in this PR, the semantics of the component are changing from being a listbox + option (like a native select) to being a menu + group + menuitem.
Given the fact that this component is a custom select component, I would argue that we should keep the same semantics — I'm not sure that DropdownMenu is the better choice in this regard, but I would like to hear also @diegohaz and @mirka 's opinion
|
Thanks for the feedback @ciampo!
That's a great point. I've had a go at updating this in commits (3e7af65 and 48b92c1) to see if we can get closer to the
In terms of the rendered markup, it looks like this gets us the Listbox
Option
I haven't (yet) addressed @aaronrobertshaw's styling feedback or the other issues, as I wanted to confirm whether or not If |
|
Note that we also have the more generic |
|
@diegohaz any thoughts on this? |
|
Thanks for the feedback @youknowriad @ciampo — I've explored a couple of options today and ran into a couple of issues with both the
The good news, is that it looks like we weren't too far off with the original addition of the
Please let me know if there are any objections with this approach — the PR's still a work in progress (I only just managed to get the styling looking pretty good before wrapping up for the day). For example, the Storybook demo styling is still pretty broken due to the centered position, but I'll fix up the positioning tomorrow:
I'm fairly sure that the addition of the Popover and preserving the rest of the existing component's behaviour is our best bet forward, but as always, very happy to hear other ideas 🙂 |
This also sounds like the most sensible (and least disruptive) approach. Feel free to ping again once you feel like the code is a good place for a review :) |
|
I'm fine with the plan but I do think it's a missed opportunity to consolidate our components and composability. The same behavior of CustomSelectControl might also be needed for ComboboxControl and potentially FormTokenField. why these components have different ways to handle popovers and keyboard navigation? They can all benefit from Dropdown and potentially hooks to handle the navigation. |
|
Semantically speaking, the select element is more similar to a combobox than to a dropdown menu. The dropdown menu trigger is a button. A button can only have a label. A select needs both a label and a value, and this is only possible with a few elements, like inputs, or elements with the A custom select is also one of the most complicated widgets to implement (correctly). This is the best article I know about this topic: https://sarahmhigley.com/writing/select-your-poison/ For now, I agree with the plan. if we aren't going to use a library for that, like downshift or Ariakit (soon), I'd also consider using the native select whenever possible. |
|
Thanks for confirming the approach, folks!
Agreed, initially I hoped we could consolidate at this stage. However, as Diego points out, there's a lot more complexity to implementing an accessible custom select component than I initially anticipated. After digging around in the keyboard navigation yesterday, I realised that I'm still keen for us to explore better composability / consolidation, but perhaps in the new year once the 5.9 work settles down (there's a good chunk of work for us to do on consolidating label spacing in components that we'd like to get to, too). I'll ping again when this PR is ready for a review. Thanks everyone! |
There was a problem hiding this comment.
Alrighty, I believe this is now ready for a proper review (CC: @ciampo). I've updated it to calculate the width for the menu via the width of the container, which I believe gets us pretty close to maintaining the same positioning / width as on trunk. If the animation of the popover feels like too much, we can switch it off with an animate={ false }. Here's a GIF of the current state within the editor (testing with the typography on the paragraph block is the most prominent place to test this):
A quick link to the Storybook demo after running npm run storybook:dev is: http://localhost:50240/?path=/story/components-customselectcontrol--long-labels
Let me know if you'd like to see any changes!
andrewserong
removed
the
[Status] In Progress
andrewserong
changed the title
andrewserong
force-pushed
the
try/popover-in-custom-select-control
branch
from
e734e6b to
a556efe
Compare
|
Rebased to fix conflicts with changelog. |
There was a problem hiding this comment.
Hey @andrewserong , thank you for iterating!
I think we're definitely moving in the right direction! Unfortunately, I won't be able to review again before the first week of January — hope that's ok!
| { width: containerWidth }, | ||
| ] = useResizeObserver(); | ||
| const widthMinusBorder = | ||
| containerWidth >= 2 ? containerWidth - 2 : containerWidth; |
There was a problem hiding this comment.
I wonder if there's a way to get the border-width of the Popover instead of hardcoding it. Maybe we could get a ref of the Popover and get the computedStyles ?
There was a problem hiding this comment.
Or maybe we could use a different value of box-sizing so that we don't need to take the border width into account at all?
There was a problem hiding this comment.
This one was slightly tricky because the border is set on .components-popover__content which is a child of the Popover container, so a ref doesn't quite get us there. Also, because .components-popover__content is a parent of the element we're attaching menuProps to, box-sizing doesn't help us either — the border is set on the parent so the width of the child cannot take it into account. Also, I wanted to avoid overriding Popover classes directly where we can.
I think the neatest thing to do here is to actually skip the JS calculation that added in 2px and instead set a negative horizontal margin that references $border-width so we're still matching a variable instead of using a magic number (I've seen this in a couple of other places in the repo, too). I've added that in this commit: 27c0cc2
Happy to continue to iterate there if you can think of a better way to handle it, but 🤞 this makes things a bit neater.
| <ul { ...menuProps }> | ||
| { isOpen && |
There was a problem hiding this comment.
I wonder if the ul should instead be wrapped around OptionList. That would also avoid the isOpen && check
i.e
<ul { ...menuProps }>
<OptionList isOpen={ isOpen } anchorRef={ anchorRef }>
{ items.map( ... ) }
</OptionList>
</ul>There was a problem hiding this comment.
The isOpen check is slightly odd, but unfortunately I don't think we can move OptionList to be a child of the ul, as then the Popover divs would be direct descendants of the ul element (where only li / script tags are allowed).
|
Thanks for the review @ciampo!
No worries, I'll be AFK for a couple of weeks from the end of this week, so if this PR hasn't been merged by then, feel free to merge it if it looks okay, otherwise I can continue on with it when I get back. |
…re using approximating select/option
…ting correct width of dropdown
…low calculating popover position based on the size of the button
andrewserong
force-pushed
the
try/popover-in-custom-select-control
branch
from
27c0cc2 to
d46ae30
Compare
There was a problem hiding this comment.
Thanks for iterating on this one @andrewserong 🙇
I've tested again after the latest changes and I think it is getting close now.
✅ Storybook: Popover displays and functions as expected
✅ Site editor: Popover displays and is not cut-off in Global Styles panel
✅ Block editor: Popover renders
✅ Keyboard navigation: Works well in both Site and Block editors
❓ Styling: There are some issues with scrollbars showing when they shouldn't
Everywhere I tested the updated CustomSelectControl displayed extraneous scrollbars. I believe this is in part due to a small issue with the negation of the $border-width Sass variable. Quickly hacking around in dev tools, setting the popover menu's margin to 0 sorted out the scrollbars. I'm lacking some of the history behind the attempt to account for border-width so I'm probably missing something.
I've left an inline comment and suggestion regarding the Sass variable negation.
Screenshots
| overflow: auto; | ||
| padding: 0; | ||
| position: absolute; | ||
| margin: 0 -$border-width; // Allow width value to include the Popover border width. |
There was a problem hiding this comment.
| margin: 0 -$border-width; // Allow width value to include the Popover border width. | |
| margin: 0 (-$border-width); // Allow width value to include the Popover border width. |
I believe the Sass compiler treats -$border-width like a subtraction operation.
So in the original code margin: 0 -$border-width; results in margin: -1px; not margin: 0 -1px. Given the comment only mentions the width I assume this is unintended.
There was a problem hiding this comment.
Great catch, thanks Aaron! Yes, the negative margin is only intended for the horizontal margin. It's to reduce the width of the menu by 2px to account for the border of the parent, so that the popover lines up perfectly with the toggle button. I'll see if I can squeeze in a bit of time today to tweak it.
|
Thanks again for reviewing @aaronrobertshaw! I thought I'd come up with a simple fix for the scrollbar issue by switching off overflow for the Popover (pushed in f261d3b) that kind of works. An additional problem for us with the scrollbars is with how the Menu area behaves when the vertical room is limited and how the Popover behaves. At the moment, both expect to scroll the content, and I ran out of time to tease apart which should "really" scroll the content, as choosing either one introduced subtle bugs. The way I've left it at the moment, is that they're switched off on the Popover, which superficially looks better: Screenshots
However, if you shorten your browser window, and then use the keyboard to scroll up and down between items, the selected item will scroll off the edge of the screen. Not quite what we want! I think what we want is for the actual height of the popover to be limited to the vertical edge of the screen, without scrolling at all, and for the Menu to be the true source of the scrollbar, but I didn't quite figure it out before wrapping up for the year. If anyone would like to have a go at fixing it, feel free to! Otherwise, I'll take another look at it with fresh 👀 when I'm back in a couple of weeks 🙂 |
There was a problem hiding this comment.
Not sure if this solves the problem at the root, but while playing around with Storybook, I noticed that the scrollbar (at least in the Storybook example) is caused by the "huge" item — in particular because its font size is quite large (200%), while the line-height is fixed (28px). This causes the item to text to overflow its item's height, and the scrollbar to appear:
Disabling the line-height rule allows the li item (and therefore the ul parent) to resize to the correct height, and thus avoids the scrollbar
I wonder if we should rewrite some of the CSS so that:
- the
line-heightis expressed in unitless values, becoming proportional to the font size - the
liitem has a min-height, and it otherwise grows as needed by its contents
|
Thanks for all the earlier feedback on this now very old PR. Since this is stale, and it looks like it'll be superseded by #41726 which refactors the component, I'll close this one out. |
Description
Raised in #36545 (comment), the
CustomSelectControlcomponent gets cut off when used in certain areas, such as in the global styles sidebar.This PR explores using a Popover component for rendering the drop down items. The goal is to ensure that the dropdown items do not get cut off, and that we preserve accessibility and backwards compatibility of the component.
Kudos @aaronrobertshaw for the approach in this PR.
How has this been tested?
Open up the site editor and go to the global styles tab. Select block styles, and go to a block that opts in to Typography controls (e.g. the Heading block). Select from the font size dropdown and before this PR, you should see that the dropdown gets cut off visually. After this PR, the Popover component should try to ensure that the dropdown options are always visible, or at least can be scrolled to.
Check to ensure that keyboard navigation is unaffected.
You can also test in Storybook by running
npm run storybook:devand then navigate to: http://localhost:50240/?path=/story/components-customselectcontrol--long-labelsTo-do
Try using the(Reverted attempt to useDropdownMenucomponent instead of thePopovercomponentDropdownMenucomponent due to accessibility issues)__experimentalPopover) so that we don't break backwards compatibility? Update: since we've managed to pretty well preserve the styling of the menu, I'm not sure this is worth it. Let me know if anyone thinks it's necessary, though!Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).