Add a Submenu block for use in Navigation #33775
Conversation
tellthemachines
added
[Block] Navigation
|
Size Change: +3.66 kB (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
tellthemachines
force-pushed
the
add/dropdown-block
branch
from
70fb0dd to
3350e66
Compare
|
It's nice having the dropdown block in the inserter. For me it feels like it would be a clearer way to create a submenu compared to the current flow (particularly if the navigation block's quick inserter could be cleaned up to prioritise particular blocks). This makes it clear from the start what kind of an item I'm going to create, whereas adding a link and then converting to a submenu always felt like it took a bit of discovery and wasn't quite so direct. I'd imagine navigation links could still retain the toolbar button for creating a submenu, but it'd result in a transform from the navigation link to a dropdown. Not the most important part of this PR right now, but the name of this block is an interesting one. Dropdown implies a very specific style type of submenu. In the future, the navigation block might support other types of submenu (like an accordion), and I'm not sure whether that'd be the same block with a different style, or variation, or a different block entirely. |
|
Coming back to this after a bit of AFK, thanks so much for the PR. Here's what I see:
Some of the rough edges aside (it's a draft PR), there are some interesting revelations in trying this, and forgive me for mostly echoing Dan above:
As far as next steps, what are the smallest tweaks we need to make in order to find out if this is the best path forward? |
|
Thanks for the feedback, folks!
Oooh nice! That would be in addition to the transform option in the block name dropdown, right?
Good question, I think that depends on whether we can use the same markup for both patterns, in which case it would make sense to have the same block with two styling options or variations, and call it Submenu. Or perhaps the block should be context-aware, as an accordion only makes sense within a certain layout. You'd expect it to be a slideout menu, or at least vertically oriented. It wouldn't make sense to have an accordion sitting next to a dropdown in a horizontal nav. I think I went with Dropdown because at first I was considering if we'd want to use it outside the Nav block, but there's so much nav-specific behaviour in here that it's probably best to forget that 😅
@jasmussen could you elaborate a bit more on this? I'm thinking the Navigation Link block could be just a menu item with no additional functionality apart from perhaps the ability to be transformed into a Dropdown.
Absolutely, and I'm now wondering if we should tackle that before moving on with this work. I'm happy to do a PR but we'll probably want to agree on the appropriate classnames beforehand 😄
I'm working on the open on click functionality, as that was a general requirement for submenus from the a11y feedback on the Navigation Heading PR. Then there's a bunch of styling issues that need to be fixed, but perhaps we should get #33048 done beforehand, as it will avoid some code duplication in this PR. Really, the choice is between this PR and #33351 in terms of how we allow users to create submenus. I'm inclining towards this PR from a code perspective, and I like how obvious it is to have Dropdown as an option in the block inserter, but welcome more opinions on both fronts! |
I'm missing a beat. In my testing this branch and trunk, I don't see any transform options for any menu items at the moment. As I wrote the initial suggestion, I was thinking of the need for us to still support the existing use case of all the existing dropdown menus that only open on hover, and where the main opening item also links to its own page. But I'm now reminded of a conversation on the initial issue, #18395 (comment) about potentially avoiding this "mix and match". My comment there assumes that a JS-based click-only menu is ideally a progressive enhancement on the existing dropdown structure, but I'm happy to be corrected there.
Currently when you insert a menu item, but don't choose a destination, it looks like this:
That state serves to indicate both that the menu item is in a draft state (and won't show up on the front-end), but also to indicate, just like grammar underlines, that you can click it to correct it:
The behavior exists also in part to accommodate highly opinionated patterns. That means unless the menu item has both a label and a URL, it will show up in this draft state. So if it needs to be able to transform into a destination-less click-only dropdown, the flow will probably not feel great as is.
The current classes targeted in the Navigation block structural CSS are these, as copied from #31879:
The submenu indicators and submenu containers are specific to the Page List block and therefore mostly a separate issue that's less important. But both the menu item container and link containers are both used and targetted by all menu items. So, how about:
There's also a little conversation about it in #32677 (comment).
Thanks for that summary, makes a ton of sense. When approaching it from that angle, there's the added angle of "do we allow mix and match", and what does that mean for any JS we might need to output to ensure click-only behaviors? To be fair, I have seen a number of mega-menu sites that do mix and match, and have click or even hover-only menu items where the opener itself doesn't link anywhere. But at the same time I could also see it being useful to have a single navigation-block-level toggle to choose whether dropdowns opened on click or on hover. There's some mostly out of date work in #18445 that explores the latter. |
I haven't added them yet 😅 but am assuming we'd want to add them in the default location for block transforms.
The accessibility feedback is that we should avoid having the dropdowns open on focus, and should instead open them on click when using the keyboard (and this solution also avoids having to tab through endless submenu items to reach the next top-level item). It's fine to have them open on both click and hover, like Twenty Twenty One does. Potentially we could flip the question in #18395 and allow hover to be the configurable behaviour: have open on click be the default, and an option to also open on hover. That would mean the JS for open on click would always be output. I've just added that functionality in my latest commit, btw, and I left the open on hover working too, so we can have a play with this and see how it feels. |
|
Cool, click is great. But it does come with some design challenges that are perhaps best explained in this post I can highly recommend, so we kind of have to decide where we land on that.
That makes a lot of sense to me, considering we probably have to keep the hover mechanism around, even just for cases where the JS fails or is blocked from loading, or as back compat for existing hover only themes. We can even add a yellow warning notice like we do for low contrast, when you choose the hover only option. The challenge is, TwentyTwentyOne is designed around handling both hover and click, with a little "plus" icon being output for the click-only behavior. While I find it works well for that theme because it's designed around it, I'm skeptical it's a pattern we can roll out for every menu out there. Menus where the primary item links to its destination but also opens a submenu is like having the cake and eating it too — and it's not as obvious as the primary item performing only the single duty of opening the submenu. This is the part that needs the most flow-thought, I think. Picture this structure: If the entire menu, or just the menu item, was toggled to be click only, how would we handle the
The other alternative, outside of the split menu, would be the Dropdown item approach, where the main item doesn't link anywhere: While a fine approach, it would allow the potentially confusing mix and match of hover-only menus and click-only menus, and it would also require the menu to be built in this way. |
I'm thinking Twenty Twenty One is the best pattern to implement here, because it works just like a classic hover menu, and we only notice the open-on-click interaction when navigating with the keyboard. And when using the keyboard, it's frankly a relief not to have the submenus open as soon as we focus the parent element. The only obvious thing that we'd have to change in the Navigation block in order to implement that pattern is the submenu indicator could no longer be optional, as the click behaviour would need its own button whenever the parent element is a link, and we'd need the submenu indicator to signal the button's presence. But perhaps the indicator itself could be configurable, if a theme wanted to use a different icon. What other issues do you anticipate with using the Twenty Twenty One pattern, @jasmussen ?
I meant having hover as optional in addition to click, not instead of it, otherwise the menu will be inaccessible. Unless the option is to have hover + open on focus instead of open on click only. But the accessibility folks seemed pretty keen on not using focus to open submenus at all. |
I've just made #33918 to address this issue. I'm thinking we'd better also add a classname for submenu containers, and perhaps for the indicators as well? That would help reduce (or even eliminate) the custom styles in this PR. |
Here's what I see in TT1:
Moreover, the dropdown menu doesn't act as a modal in that it captures tab and closes on Escape, so it feels like it only adds limited value over the keyboard interaction compared to how the hover-only menu works with the keyboard:
Click is great, though, but click and hover I'm less sure about: it's an extra tab-stop, and it requires a very delicate design in order for the visual order to match the tab-order. I'm also worried about what this means for the existing WordPress themes that are designed assuming a hover-only design; if we were to enforce click by default, we'd have to output a split-button indicator like a plus or a chevron, I don't think we can be sure existing themes are ready for that. Approaching this fresh, on the other hand, I think we have a great opportunity to make an excellent click-only experience by default, one which works out of the box, is carefully designed, and perhaps uses Micromodal like the burger menu for keyboard interactions (which could be extra useful in mega-menu cases). If combined with leaving the existing hover/basic tabbing behavior as an optional fallback, it might thread the needle. Because this feels important to get right, I would hope we could get some wider input from some of our themers on this: @MaggieCabrera @melchoyce @critterverse @javierarce @kellychoffman — if you have time, thank you! |
👏 A unification of all those classes sounds useful, absolutely, especially if it isn't too much work. One option might be to just let the existing classes for navigation items be intact ( |
I agree the experience isn't great when clicking with a mouse, because we get a weird mix of click and hover behaviours, but both hover-only and click-only (using keyboard) is pretty nice:
Compared to the sub-optimal experience of opening on focus when there are large or multiple submenus: So I wouldn't give up straightaway on a hover/click combination, but we can definitely work to improve the points you mentioned. Perhaps we could try:
I was thinking that if we allow interaction on hover, the button icon could be positioned closer to the top-level element text, so that they don't seem to be separate elements? But that kind of makes open on click functionality less obvious for mouse users. I don't know, it's a complicated problem 😅 I'm not completely sold on the idea of moving the top-level link into the submenu, mainly because I suspect users may take exception to not being allowed to make that decision themselves, but generally I agree that having top-level items just be buttons that open submenus, instead of links, is nicer. But WP has had the top-level item be a link for such a long time that springing a completely new pattern on our users might be quite jarring. On the other hand, the Navigation block is completely new (and it's not released yet! So we still have room to change stuff around) so it might be a good place to make that change. |
|
👋
Oof. That seems like something that should be fixed in TT1. In case you're curious, this is the thread where we discussed the current TT1 implementation: WordPress/twentytwentyone#263 The separate, click-to-expand submenu was put in place for two reasons: for more intuitive keyboard navigation, and to reinforce the "hover is not necessarily a reflection of intent" concept. We added hover back in after testing, since folks (like me 😄) really expect it these days.
This suggestion from @tellthemachines seems reasonable to me, and I think is worth a try. |
|
Ideally, the nav menu would be able to support all three options from the w3 docs (https://www.w3.org/WAI/tutorials/menus/flyout/). I can then see the hover+click pattern being the default, as it is the most accessible. Another option to to have the nav block support only one of the w3 options, but is extensible in a way that a plugin could easily provide those options. https://codepen.io/okia/pen/jXWrYL I think that codepen above is a great example of how it should work. It combines keyboard navigation with hover in a really nice way. You can still have the parent item trigger a link but also use the spacebar or down arrow to navigate to it's children. Mouse interaction works on hover and feel natural. |
|
Thanks for the added context. To be clear, my primary concern is with backwards compatibility with existing menus that rely on hover: the "plus" as a click affordance might mess with their layouts. While the hover-only menu only provides basic accessibility, it is accessible, so I don't think there's any harm in existing menus retaining this behavior. Furthermore it's an opportunity to educate and warn in the interface, by providing a strong new default and warnings when choosing the legacy interface. Something like this, perhaps:
|
There's nothing wrong with having two ways of accomplishing something. This is a very similar situation to the Image to Cover transform: Cover is available as a block in the inserter, and it's possible to transform Image into Cover via the usual transforms menu, but the transform is additionally exposed in the Image toolbar as "Add text over image". There doesn't have to be a single best way; if having two or more ways to do something makes it easier for users to accomplish what they want, I can't see any downsides 🙂 Thinking specifically of this case, due to the differences between hover and click, different flows are better suited to each option: transforming from a Link is more intuitive for the hover menu, but adding the Submenu directly makes more sense for the click menu. Ultimately, some usability testing on this would help us confirm what is best. The option not showing in the inserter would only be an issue when using the inline inserter; for the sidebar one the social links etc. are under a different section, so won't interfere with the visibility of the Submenu block. I'm not sure which inserter gets most use, but I doubt it would be a huge issue even for the inline one.
Thanks, fixed!
Hmm, this also happens on trunk so it's either intended behaviour, or a bug that can be fixed separately 😅 Hover intent is a great idea! Again, it's not on trunk so we can do it separately. |
|
Did some testing and I noticed in TT1 Blocks empty submenus are transparent (theme styles disabled): The block behind also seems to be selectable so maybe it's more of a z-index thing. The same happens in standard Twenty Twenty One, but is harder to reproduce because there are bigger gaps between blocks, so it's harder to get a dropdown over the top of a trailing block. Also, side note, #34281 should be ready for review now. |
|
The way these options jump around doesn't feel good. It makes it really easy to click the wrong one. Kapture.2021-09-08.at.13.40.42.mp4 |
It was; fixed now.
I've moved them around so the disappearing one is at the bottom. In any case, there are plans to change some of these to buttons soon. |
| @@ -326,11 +327,16 @@ export default function NavigationLinkEdit( { | |||
| .length; | |||
|
|
|||
| return { | |||
| innerBlocks: getBlocks( clientId ), | |||
| isAtMaxNesting: | |||
| getBlockParentsByBlockName( clientId, name ).length >= | |||
There was a problem hiding this comment.
Should name be expanded to take into account the submenu block the same way isTopLevelLink does?
There was a problem hiding this comment.
Yes! Thanks, totally missed that 😅
| // Temporary fix until navigation link submenus are properly deprecated. | ||
| isTopLevelLink: | ||
| getBlockParentsByBlockName( clientId, name ).length === 0, | ||
| getBlockParentsByBlockName( clientId, [ | ||
| name, | ||
| 'core/navigation-submenu', | ||
| ] ).length === 0, |
There was a problem hiding this comment.
I don't know that it's possible to deprecate from one block type to another. 🤔
It might also be more flexible to make this check if the parent is a core/navigation block. something like getBlockName( getRootClientId( clientId ) ) === 'core/navigation'.
| } | ||
|
|
||
| // Show submenus on click. | ||
| .wp-block-navigation-submenu__toggle[aria-expanded="true"] + .wp-block-navigation__submenu-container { |
There was a problem hiding this comment.
Something I noticed in usage. I think the button could use cursor: pointer
| // Add the navigation block. | ||
| await insertBlock( 'Navigation' ); | ||
|
|
||
| await selectDropDownOption( 'Test Menu 2' ); |
There was a problem hiding this comment.
Not added here, but my OCD was triggered by 'DropDown' 😆
There was a problem hiding this comment.
Ugh yeah, would be good to rename that if possible.
| * @param array $attributes Block attributes. | ||
| * @return array Colors CSS classes and inline styles. | ||
| */ | ||
| function block_core_navigation_submenu_build_css_colors( $context, $attributes ) { |
There was a problem hiding this comment.
Maybe it's fine for it live in the nav block's PHP file, given this is using the colors from that block.
|
Thanks for the reviews, folks! I've fixed the pending issues. In the meantime, there's the navigation editor update PR, #34281, that I'm going to review and test in a bit. Should that one be merged together with this one, or is it not a blocker for merging this? There's also #34675 to update Page List block, but I think that one can be merged later. We haven't formally reached an agreement on whether to show Submenu in the inserter or not 😄 but I'm inclined to show it, and then gather data on how it's being used. The remaining thing is to investigate how/if we can deprecate submenus in Navigation Link. I'll look at that separately. |
|
@tellthemachines All still looks good. I say go ahead and merge, and then it'd be good also merge #34281 straight after. I've addressed the feedback on that one. |
annezazu
added
the
[Focus] Accessibility (a11y)
Description
Follow-up from/alternative to #33351; fixes #18866. Might simplify #23745, at least on the UI level.
Adds a Submenu block to replace submenu functionality in Navigation Link blocks.
What's the difference between Submenu block and the current Navigation Link submenus?
How are we supporting the current workflow of creating a submenu from a Navigation Link?
How are we ensuring Submenus are accessible?
How has this been tested?
Add a Navigation block to a post or template; add a Dropdown block inside it. Try to reproduce current functionality of Navigation Link submenus. Try creating a dropdown with a link or with a button as toggles, check both work as expected.
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).