-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Block Editor Settings: use v2 DropdownMenu #51400
Conversation
Size Change: +1.02 kB (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
Flaky tests detected in 601a07e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5289786548
|
5a5b8dc
to
3701a7b
Compare
I know this is still a WIP, but just thought I'd take it for a very quick spin. It looks like this is also going to resolve #50988 from early testing (CMD+click with another browser window no longer results in multiple block settings dropdown menus displaying), so I'm quite excited for this one! 😄 |
1bcc5a2
to
8811ea2
Compare
…wnmenu v2 components
… to keep the dropdown open
…onClick => onSelect
- stopping the propagation of the ESC keydown, so that only the dropdown closes and the toolbar stays open - restoring passing an onClose callback to slotfills, including the create reusable pattern modal
8811ea2
to
d7dcf02
Compare
Update: After working on this PR pretty much for the whole week, I've managed to iron out most wrinkles, but I've hit a bit of a wall: modals opened on top of the dropdown (for example, the one for creating reusable blocks, or for locking/unlocking a block) don't work properly. (as also highlighted by e2e test failures). This is a consequence of the A possibility that I briefly explored is to close the
I've also explored setting the
We always knew that there was a chance for this kind of issue when introducing a new component that doesn't rely on our internal |
4dfe304
to
601a07e
Compare
I'd suggest disabling the default modal behavior and implementing a custom modal logic that considers nested/third-party dialogs. Even though you can change all dialogs in the app to use Radix's Dialog, you can't control how third-party dialogs will render. This includes browser extensions users may have installed, like Google Translate, Grammarly, 1Password, etc., but also other persistent widgets that should be accessible in parallel (like floating chat widgets). Here's how we fixed this on Ariakit with a brief explanation: ariakit/ariakit#2339 |
I'm also curious to learn how this works. As far as I know, SlotFill won't automatically provide context to elements rendered outside the original tree. If Radix uses React Context to control its menu items, the context should be manually re-created in the fill tree, otherwise things like roving tabindex may not work as expected. That's what the Block Toolbar component does. Oh, I think I got it. The dropdown slots aren't using the I created this example using Ariakit Menu and the Some things to note:
|
Hey @diegohaz , thank you for chiming in!
I'll look into this and report back. As I stated above, removing modality will cause another issue: radix's (or other third-party libs) popover-based components won't work well with anchors in the iframed editor. In our first-party
My point regarding the The point that you make about internal context not being passed down correctly makes sense though, and would also apply to using the new menu items from the new
I'm not 100% sure if it is, but I assume that if we got Radix (or third-party libraries) to export their context, we could simply forward them via
Is the reason why it works because data is shared via ariakit stores instead of React Context? |
Where can I learn more about that? Is there a minimum reproduction of that behavior somewhere so I can play with?
Yes. But that means they will either have to expose their internals or design a public API around that. The first option will put a burden on consumers that will rely on a very unstable code. The latter puts a burden on the library maintainers as designing that kind of API may take a lot of time (it took me years).
For the SlotFill thing, yes. |
Last year the I had also opened a few issues on On our first party
Couldn't the |
Just following up on this; was there any movement since June? |
Hey @richtabor , I have been AFK for the past 2 months and therefore wasn't able to continue work on this. At the moment, it looks like the experimental Radix UI-based DropdownMenu is not meeting our requirements (see #51400 (comment) for more details). I even opened an issue on the Radix UI repo, but 3 months have passed and no one from the project's maintainers has even replied. In the upcoming weeks, I will try to test Ariakit's DropdownMenu component for this very same test case (the block editor settings dropdown), to see if hopefully it will meet our requirements. |
@ciampo should we drop this now that we're not planning to continue with Radix as an alternative for the |
Yes. I will still refer to this PR when applying the same changes with the ariakit-based dropdown menu, but in the meantime we can close this PR. |
Part of #50459
What?
This PR refactors the block toolbar's "settings" dropdown menu — the menu that is toggled by pressing the last button (labelled "Options" and associated to a vertical "..." icon.
The refactor involved migrating from the legacy
DropdownMenu
component to the new version (currently still experimental and only available as a locked private API of the@wordpress/components
package).Why?
The main advantage of migrating to the new version of the component is the support for nested menus.
This PR also represents the first "real world" usage of the experimental component (ie. not isolated in Storybook), which will expose a number of issues related to integrating the component with the existing set of components and constraints of the block editor.
Is this PR including breaking changes?
No, this PR does not include breaking changes. The dropdown menu and all its items have been refactored to use the new experimental version of the
DropdownMenu
component, without otherwise changing any public-facing APIs (including thefillProps
passed to the different slotfills).Even if a third-party developer was to add a legacy
MenuItem
to the block settings dropdown via theBlockSettingsMenuControls
slotfill, theMenuItem
would continue to render without any errors, and clicking on the item would still cause the expected results. The only difference may be visual, as the new dropdownmenu components have slightly different styles and spacing.How?
These are the changes being proposed in this PR. As the PR grows in scope, we may decide to split some of the proposed changes to separate, smaller PRs.
📝 Changes
Expand to show the detailed list of changes
General changes:
DropdownMenu
,MenuGroup
,MenuItem
) with the new equivalent experimental components. Main differences:toggleProps
prop, the component requires a trigger to be explicitly passed via thetrigger
prop. I've therefore passed aButton
component as the trigger and forwarded thetrigger
props to itmenuProps
prop, the component can accept certain props directly (likeonKeyDown
)disableOpenOnArrowDown
prop. I've implemented the same functionality by controlling theDropdownMenu
component with internal state and added a keyboard event listener on the trigger topreventDefault
when the arrow down key is pressed.DropdownMenuItem
component doesn't accept ashortcut
prop. Instead, I've created a customShortcut
component and passed it via thesuffix
propDropdownMenuItem
component doesn't accept anicon
prop. Instead, I've used theprefix
prop and passed the icon via theIcon
componentonClick
props toonSelect
for the newDropdownMenuItem
.DropdownMenu
component accepted, aschildren
, a render function. This function accepted an object containing theonClose
callback, which could be used to programmatically close the dropdown menu. The newDropdownMenu
component doesn't expose this functionality, but an equivalent callback has been created and passed to the children components for backward-compat reasons.DropdownMenuItem
component automatically closes the dropdown menu when clicked. This meant that a lot of the calls to theonClose
callback have been removed, and that explicitevent.preventDefault()
calls have been added where the menu needs to stay open after clicking the menu item.DropdownMenuGroup
component doesn't render a line to visually demarcate the group. Instead, I used the newDropdownMenuSeparator
component.useCopyToClipboard
hook, and instead implemented a custom solution using native web APIs instead (more details on this below)packages/block-editor/src/components/block-settings-menu/style.scss
were deleted, since they were targeting the legacy dropdown. As a consequence, I also removed all occurrences of theblock-editor-block-settings-menu
andblock-editor-block-settings-menu__popover
classes from the repository.useCopyToClipboard
hook has been updated to support ancontainer
argument, useful to circumvent the focus trap added by theDropdownMenu
componentChanges applied to the experimental
DropdownMenu
component:onKeyDown
prop on the rootDropdownMenu
component. This is to support the use-case of the block toolbar settings menu, which uses the prop to support keyboard shortcuts (source)z-index
for content wrapper elements as the legacyPopover
menu, to avoid stacking order bugs when other modals are opened.DropdownMenuItem
. This is to support the use-case of the "select parent block" menu item, which is supposed to highlight the parent block when hovering the menu itemhref
,rel
, andtarget
props onDropdownMenuItem
. This is to support the use-case of the "Manage reusable blocks" menu item, which is supposed to link away from the editor. When thehref
prop is not empty, theDropdownMenuItem
automatically renders as an<a />
tag instead of a<div />
. Some extra styles were also necessary to override the default Gutenberg styles associated with anchor tags.DropdownMenu
, likeid
,aria-labelledby
,className
. These attributes are often used by consumers of the component, and were necessary in ensuring that the trigger's id was passed correctly to the menu's context in this specific PR instanceMenuItem
components look betterChanges applied to the
Modal
component:pointer-events: all
to the modal wrapper. This is to override styles added by the Radix DropdownMenu, which setspointer-events: none
when opened in a modal way. Without the style override, the modal would not receive pointer events.DropdownMenu
behave as expected.🛑 Pending issues
The new DropdownMenu component interferes with Modal components opened on top of the dropdown
Due to how the new
DropdownMenu
component is implemented, Modals rendered on top of open dropdown menus don't react to pointer events and don't get keyboard focus.I was able to (potentially) solve the pointer events problem (by forcing
pointer-events: all
on the modal), although that solution would require more testing to make sure it's robust (for example, what happens if a modal DropdownMenu is opened inside a modal?).What I haven't been able to solve so far regards keyboard focus. The new
DropdownMenu
component, in fact, seems to trap keyboard focus on the dropdown. And therefore, when opening a modal on top of an opened dropdown menu, the keyboard focus stays on the dropdown menu (this can be verified because, when typing letters, the dropdown menu still highlights the matching menu item.It looks like we'd need to use the
Dialog
component from Radix to have modals work as expected: https://codesandbox.io/s/dropdownmenu-dialog-items-forked-9sy6j8?file=/src/App.js💬 Pending discussions
Where and how to implement the resize behavior when meeting the edge of the viewport
Radix's
DropdownMenu
offers this behavior by exposing the--radix-dropdown-menu-content-available-height
CSS variable, and letting the consumer of the component actually implement the resizing behaviour. This is what I've done so far in this PR, but we should ask ourselves a few questions:DropdownMenu
component? If so, we should think carefully about how to expose it and future-proof it against potential radix changesDropdownMenu
, would it be ok that consumers of the component use a CSS variable that clearly states '"radix" ? It would be good to keep radix as an implementation detail.Decide the best way to allow `DropdownMenuItem` to render as an anchor tag
Currently, the
DropdownMenuItem
exposes thehref
prop and internally decided to render an<a />
or a<div />
. This approach is simple and mimics the APIs of the legacyMenuItem
and theButton
component, but it lacks in scalability (what if we needed to render something else as a menu item?)Alternative approaches:
asChild
prop, allowing consumers of the component to pass whatever component they wantasChild
set totrue
in the internal implementation, requiring consumers of the component to always specify a "menu item wrapper" elementReflect on which props we should expose on `DropdownMenu` and `DropdownMenuItem`
Using the components in a real-world scenario highlighted the need for consumers of the components to pass certain HTML attributes, like
className
,onKeyDown
, andaria-labelledby
.So far, our strategy on these components has been to reduce the amount of props exposed as much as possible, but I wonder if we should reconsider that and allow a wider range of props (ie. standard HTML attributes?)
Testing Instructions
Make sure that:
Testing Instructions for Keyboard
Screenshots or screencast
trunk