[WIP] Navigation Menu: dropdown activation mode #18445

retrofox · 2019-11-11T19:56:28Z

Description

This PR implements the ability to set the event which opens the dropdown menu in the front-end, adding a section in the inspector control.

Fixes #18395

Notes:

I'm not totally unconfident about the text defined on this block. Feedback is more than welcome!
~~onClick should be handled also with js since it isn't possible with just CSS.~~

How has this been tested?

Once these changes are applied, you should be able to see the Dropdown Menu Activation block in the inspector control:

It allows changing the behavior of how the dropdown menu shows in the front-end through of the selected event: hover or click.

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR. .

retrofox · 2019-11-12T13:45:40Z

Question: How are we going to deal when the menu has defined the activation mode with click, and the item has a link?

In some way, it's related to the possibility to render an item with a not-anchor element.

getdave

Nice work here. A tricky problem.

A few a11y concerns and minor CSS implementation points.

Also when testing hover mode with TwentyTwenty I saw this

getdave · 2019-11-13T09:19:09Z

packages/block-library/src/navigation-menu/edit.js

@@ -73,7 +74,7 @@ function NavigationMenu( {
 return null;
 }

- return pages.map( ( { title, type, link: url, id } ) => (
+ return pages.map( ( { title, type, link: url, id, menuEventActivation } ) => (


Wondering whether menuEventActivation would be clearer as dropdownActivationEvent. At the least swapping Event and Activation should happen to make the "thing" we are describing (ie: the event) is clearer (ie: menuActivationEvent).

getdave · 2019-11-13T09:21:10Z

packages/block-library/src/navigation-menu/index.php

+ (
+ ( $has_sub_items && ! $on_hover_mode )
+ ? (
+ '<input


We'll need some ARIA roles here such as aria-pressed and aria-expanded.

getdave · 2019-11-13T09:24:12Z

packages/block-library/src/navigation-menu/index.php

+ class="wp-block-navigation-menu-item__toggle-handler"
+ type="checkbox"
+ >' .
+ '<label


Perhaps take a look at Bootstrap's "Dropdown" implementation for a11y guidance here.

getdave · 2019-11-13T09:26:01Z

packages/block-library/src/navigation-menu/inspector-control-menu-activation.js

+ selected={ selected }
+ onChange={ onChange }
+ label={ __( 'Dropdown menu activation' ) }
+ help={ __( 'Select the way of how the dropdown menu will be activated.' ) }


Select the way of how the dropdown menu will be activated

Could be phrased as

Choose the way dropdown menus will be activated

getdave · 2019-11-13T09:26:35Z

packages/block-library/src/navigation-menu/style.scss

+ &:hover,
+ &:focus-within {
+ cursor: pointer;
+ z-index: 99999;


What's this super high value about? 😄

getdave · 2019-11-13T09:27:34Z

packages/block-library/src/navigation-menu/style.scss

+ // Submenu Display
+ &:hover > ul,
+ &:focus-within > ul,
+ & ul:hover,


Why are these needed? If the ul is within the li then the :hover on the li should sustain the hover effect.

getdave · 2019-11-13T09:30:07Z

packages/block-library/src/navigation-menu/style.scss

+
+ &.is-activated-by-click li {
+ input.wp-block-navigation-menu-item__toggle-handler {
+ display: none;


This will make the control impossible to access for assistive tech. Better to use the VisuallyHidden component or a CSS solution to hide visually but retain access for assistive tech:

See also https://wordpress.slack.com/archives/C02QB2JS7/p1573637389298100

getdave · 2019-11-13T09:30:56Z

packages/block-library/src/navigation-menu/style.scss

+ display: none;
+ }
+
+ label.wp-block-navigation-menu-item__toggle-for {


I'd avoid qualifying with label unless it is required for specificity. Otherwise we're increasing the specificity for no reason which I would tend to avoid at all costs.

Ditto for input. above.

karmatosed · 2020-01-20T21:35:41Z

I wanted to dig a little in why this is being added and how perhaps we could simplify. First, is this going to be seen by everyone or something you can extend? If everyone, I would suggest considering having a toggle and reducing a little to what is the default. Maybe by having a top settings section:

I do wonder though how this works with sub-menus as the parent is a link. Perhaps I am missing some background on this as it would be great to know.

mtias · 2020-03-10T12:12:33Z

What's the status on this one?

jasmussen · 2021-03-18T10:10:12Z

In #22824 (comment) we are discussing a hamburger menu button. For this to work and be accessible, we can't use the checkbox hack, we have to have some JS output. Potentially that same JS could be used to toggle open submenus on click.

getdave · 2021-04-07T13:38:00Z

@jasmussen Do we still want to pursue this? Having the option to toggle based on hover/focus vs click could be useful. How do you see this working with the hamburger you're discussing in relation to mobile?

@retrofox are you likely to be working on this anytime soon?

retrofox · 2021-04-07T14:39:07Z

@retrofox are you likely to be working on this anytime soon?

👋, Dave. I'm not sure whether I'll be able to look into this in the short term. I'm going to take a look when I have the chance. I think it depends on the answer to - Do we still want to pursue this?

jasmussen · 2021-04-08T06:17:15Z

Do we still want to pursue this? Having the option to toggle based on hover/focus vs click could be useful. How do you see this working with the hamburger you're discussing in relation to mobile?

I think we want to eventually revisit this. However I also think it's more complicated than it seems. As an exapmle, if the primary menu item links to /products, but also opens a dropdown of subpages, how do you get to /products, if clicking on the menu item just opens the dropdown? Options:

You can't open the dropdown by clicking the menu item, you have to click the chevron to the right of it. Consistent, but probably not obvious.
You can only visit /products if you click twice, once to open, another to visit. Probably also not discoverable, or accessible.
The main menu item becomes completely inert, and the /product link is duplicated inside. Could work well, likely needs some ability to customize the duplicate label, such as "See all", though potentially we could use the Menu Description field for this.
The menu item becomes impossible to visit entirely. Not a good solution.
A new menu item is created, "Dropdown menu", which serves only to open the dropdown on click.

It's solvable, we should solve it. But it's less obvious than a toggle, I would say.

aristath · 2021-04-08T06:39:57Z

As a sidenote, in the twentytwentyone theme we did something similar, allowing both hover & click events.
It was a bit tricky getting it right, but I feel it was the best of both worlds, as it allows most scenarios and is as accessible as a navigation can possibly be.
To accomplish that we inject a <button> (the caret) in menu items (see markup for the button here), and then there's a script that takes care of the rest: https://github.com/WordPress/twentytwentyone/blob/trunk/assets/js/primary-navigation.js
I assume we can do the same here, and it's not a binary option (click or hover), in order to be accessible we need to take into account all interaction paths.
On a personal note I love click and strongly dislike hover... but the above implementation proved to be satisfying to all.

jasmussen · 2021-04-08T06:48:22Z

Thanks for sharing that, @aristath, I wasn't aware. Is this the behavior to which you are referring?

I.e. as you say, both hover and click at the same time? If yes, that does seem to be a variant of the first option I suggested, a split button. I do appreciate that it can become a toggle like you suggest, but I don't personally find the behavior entirely intuitive or obvious.

That said, I'd be super happy to help polish a PR that landed that exact behavior as a baseline, and then we could always go from there.

aristath · 2021-04-08T07:01:15Z

Is this the behavior to which you are referring?

Yes. The implementation adds a split button, and initially we had submenus only expand on click. But there were a few complaints from people too used to the on-hover action, so the combo behavior was a compromise between the 2.
It works great with keyboard navigation though, which was a big priority.

Using a split button opens up a lot of possibilities... It can work on hover, on click or both, and styling-wise there's a lot we can do 👍

mrfoxtalbot · 2022-10-03T10:17:24Z

I believe this has already been addressed. Version 14.2.0 of the editor has this option for the Navigation Block:

I am going to close this but please feel free to reopen if necessary, @retrofox. Thank you!

retrofox added 5 commits November 11, 2019 16:07

navigation-menu: add menuEventActivation attr

ce26bee

navigation-menu: add MenuActivation insp control

92b346b

navigation-menu: add menu action to inspector

0fb2b72

navigation-menu: init value for menu activation

bad7457

navigation-menu: handling hover event

eade6e5

retrofox added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. [Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Block] Navigation Affects the Navigation Block labels Nov 11, 2019

retrofox requested review from getdave, mtias, obenland and frontdevde November 11, 2019 19:56

retrofox requested review from ajitbohra, jorgefilipecosta, Soean and talldan as code owners November 11, 2019 19:56

retrofox requested review from noisysocks and removed request for ajitbohra November 11, 2019 19:56

navigation-menu: experiment onClick hac

bc939b1

getdave changed the title ~~[WIP] Navigation Menu: activation mode~~ [WIP] Navigation Menu: dropdown activation mode Nov 13, 2019

getdave requested changes Nov 13, 2019

View reviewed changes

karmatosed removed the Needs Design Feedback Needs general design feedback. label Jan 22, 2020

talldan removed the [Feature] List View Menu item in the top toolbar to select blocks from a list of links. label Jun 11, 2020

Base automatically changed from master to trunk March 1, 2021 15:42

jasmussen mentioned this pull request Aug 5, 2021

Add a Submenu block for use in Navigation #33775

Merged

7 tasks

mrfoxtalbot closed this Oct 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Navigation Menu: dropdown activation mode #18445

[WIP] Navigation Menu: dropdown activation mode #18445

retrofox commented Nov 11, 2019 •

edited

Loading

retrofox commented Nov 12, 2019 •

edited

Loading

getdave left a comment •

edited

Loading

getdave Nov 13, 2019

getdave Nov 13, 2019

getdave Nov 13, 2019

getdave Nov 13, 2019

getdave Nov 13, 2019

getdave Nov 13, 2019

getdave Nov 13, 2019

getdave Nov 13, 2019

karmatosed commented Jan 20, 2020

mtias commented Mar 10, 2020

jasmussen commented Mar 18, 2021

getdave commented Apr 7, 2021

retrofox commented Apr 7, 2021

jasmussen commented Apr 8, 2021

aristath commented Apr 8, 2021 •

edited

Loading

jasmussen commented Apr 8, 2021

aristath commented Apr 8, 2021 •

edited

Loading

mrfoxtalbot commented Oct 3, 2022

[WIP] Navigation Menu: dropdown activation mode #18445

[WIP] Navigation Menu: dropdown activation mode #18445

Conversation

retrofox commented Nov 11, 2019 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

retrofox commented Nov 12, 2019 • edited Loading

getdave left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karmatosed commented Jan 20, 2020

mtias commented Mar 10, 2020

jasmussen commented Mar 18, 2021

getdave commented Apr 7, 2021

retrofox commented Apr 7, 2021

jasmussen commented Apr 8, 2021

aristath commented Apr 8, 2021 • edited Loading

jasmussen commented Apr 8, 2021

aristath commented Apr 8, 2021 • edited Loading

mrfoxtalbot commented Oct 3, 2022

retrofox commented Nov 11, 2019 •

edited

Loading

retrofox commented Nov 12, 2019 •

edited

Loading

getdave left a comment •

edited

Loading

aristath commented Apr 8, 2021 •

edited

Loading

aristath commented Apr 8, 2021 •

edited

Loading