🇺🇸United States @bronzehedwick

+1

Also, it would be great if this MR didn't change any SVG file that doesn't really need to change to make it easier to isolate the changes.

Sorry about that @ckrina, those changes snuck in due to IDE automatically inserted newlines at the end of the file. I've pushed a commit to revert that, so those changes should no longer be in the diff.

@SKAUGHT aria-hidden="true" is applied to the .toolbar-button__abbreviation element. That should handle the use case you're mentioning.

@smustgrave I added an abbreviation test. It's my first time writing a Drupal test, so open to any feedback. Thanks!

bronzehedwick → changed the visibility of the branch 3424744-on-collapsed-sidebars to hidden.

bronzehedwick → changed the visibility of the branch 3424744-on-collapsed-sidebars to hidden.

lauriii → credited bronzehedwick → .

Fixed by https://www.drupal.org/project/navigation/issues/3436130 📌 Ensure keyboard navigation works with the drawer Active

Fixed by https://www.drupal.org/project/navigation/issues/3436130 📌 Ensure keyboard navigation works with the drawer Active

Fixed by https://www.drupal.org/project/navigation/issues/3436130 📌 Ensure keyboard navigation works with the drawer Active

Got it @SKAUGHT, I pushed a commit that hard codes 40 as the width and height.

moving width/height to var seems not needed [as this doesn't recenter, re-size the actual paths]

This will re-size/re-center the entire graphic, as intended.

All width/height sizing and any drawing code inside an SVG is relative to the viewBox. So the width and height on <rect fill="#347efe" width="32" height="32" rx="8"/> being set to 32 is the same as saying 100% of the width and height, since the viewBox is 32/32.

Increasing the width/height on the <svg> element scales everything inside it based on the ratio of the viewBox, aka the S in SVG ;)

also noticed viewbox is "0 0 32 32" -- i think should be 40

As per above, the SVG is displayed at 40/40, but it's scaling baseline is 32/32 (aka, the viewBox). Changing the viewBox from 32/32 will make the internal scaling math off, in this case, adding extra white space around the SVG, optically shrinking it.

You can test all this by changing the width and height values on the <svg> element.

I also think we need to rework current events system.

They should be better documented and all iterations click/hovers/keys should go in same manner.
Since only events cares globally about multiple things. EG

- Switch expand/collapse text
- Switch classes
- Controls sibling elements open/close

@finnsky would this be appropriate for a follow up ticket?

Ah that makes sense, thanks @SKAUGHT. I pushed a commit that adds back that condition.

I created a new MR with a new approach. Is this more inline with what you're expecting @ckrina?

@finnsky that last commit fixed the issue. Thanks!

This approach also resolves the follow up issues, listed below.

• Hover effect can cause problems with keyboard focus 📌 Hover effect can cause problems with keyboard focus Active
• Improv keyboard navigation focus inside drawer 📌 Improv keyboard navigation inside drawer Active
• Third level menu accordions are not navigable via the arrow keys 📌 Third level menu accordions are not navigable via the arrow keys Active

Moving to RTBC.

Thanks @finnsky! Your updates addressed all my feedback.

There is one small, new regression, I think from the 1.x rebase. There's extra padding and grey bar below each third level accordion.

This seems to be caused by the recently merged additional padding MR 🐛 Bottom padding needs to be increased along gray container box of grandchildren menu items Fixed running up against the new styles.

One solution would be to change line 38 of toolbar-menu.pcss.css from:

.toolbar-menu--level-2 {

to:

.toolbar-menu--level-2:not([inert]) {

I think this is a reasonable approach, removing the extra network request. We don't get the added styling benefits, but those don't have a plan to be taken advantage of right now, and this will remove refactoring. Marking RBTC!

I think this issue has been fixed by #3440228 🐛 Bottom padding needs to be increased along gray container box of grandchildren menu items Fixed . Is that right @jwitkowski79?

Looks good! I tested the MR locally, and found no regressions. I searched the code base for any references to user.jpg, and found none.

Hey @finnsky, thanks for this! Some things I noticed.

• When closing a popover with left arrow, focus moves up to the previous menu item, instead of moving back to the popover's button.
• Pressing right arrow inside an open popover, the the popover is dismissed and focus is moved to the next main menu item below the current item. I would expect nothing to happen.
• When inside a third level menu (accordion), there's no way to move out of it.
  • Pressing either arrow left or right dismiss the entire popover.
  • Pressing Space scrolls the page.

  I would expect pressing arrow left (and maybe arrow right?) or Space to collapse the accordion, moving focus back to the top level of the accordion.

• After initially opening a popover, pressing up arrow dismisses the popover. I would expect the focus to move to the last menu item in the popover.
• When focus is on a collapsed third level accordion, pressing left arrow does nothing. I would expect the popover to be dismissed.

bronzehedwick → created an issue.

Thanks for catching @ckrina! I've updated the MR.

bronzehedwick → made their first commit to this issue’s fork.

bronzehedwick → created an issue.

Moving to NR, given @ckrina's guidance, above. The follow up issues are below.

1. Improv keyboard navigation focus inside drawer 📌 Improv keyboard navigation inside drawer Active
2. Hover effect can cause problems with keyboard focus 📌 Hover effect can cause problems with keyboard focus Active

bronzehedwick → created an issue.

Pushed a fix for the inaccessible Shortcuts menu via arrow keys. Simple oversight, just needed to add tabindex.

Looks like the problem here is the hover. The hover effect is controlled in JavaScript, and has a delay on toggling (by design). The hover off is triggered when you hover over another menu item. That means that if you quickly hover over a few items, then move your mouse outside the menu drawer, the hovered items will stay hovered. This blocks arrow movement, unless you press arrow left to "dismiss" the (hidden) popover.

I haven't been able to reproduce the JS error @rkoller and @finnsky are seeing yet.

This is an architectural issue, and will probably require some larger refactoring to resolve.

I think this and the other remaining issues should be moved into the follow up ticket 📌 Improv keyboard navigation inside drawer Active , to reduce churn while the core MR is being made. Does that make sense with what you are thinking @ckrina?

in safari moving by the arrow keys through the top level items sometimes just stops working and i am unable to move up and down anymore (in edge and firefox i was unable to reproduce that)

I pushed a fix for this. I think the issue is pressing right arrow on menu items that don't have popovers, which pushes focus to a hidden element, preventing other arrow movement until left arrow is pressed.

bronzehedwick → created an issue.

Here's the follow up ticket https://www.drupal.org/project/navigation/issues/3440047 📌 Improv keyboard navigation inside drawer Active

bronzehedwick → created an issue.

Thanks for the review @rkoller. I pushed an update that should address the following.

1. Up and down arrow keys moving focus to both expandable menu items and links.
2. Up and down arrow keys moving focus as above inside popovers when opened.
3. Typing a character jumping to either expandable menus or links, as above.
4. Left and right arrows should open and close popovers consistently.

There's some issues left, below.

1. Focus immediately moving to the first popover item when opened. Currently the first down arrow press after the popover is opened will focus on the first popover menu item.
2. The shortcuts popover menu items are still inaccessible from the keyboard.
3. Mixing navigation with tab and arrow keys can result in unexpected jumps in focus.

Since this MR does improve the keyboard navigation, and fixing the issues above won't be insignificant, I propose we split the remaining items into a new ticket.

I added code to handle keyboard navigation, patterned off the W3C guidelines outlined in the ticket description.

One change from the table above I think should be made is reversing the Up/Down and Left/Right arrow functions. The W3C example is a horizontal menu, and this is a vertical menu.

Thanks for adding that @skaught! That table is what I'm going on.

Thanks for the suggestion @silviu-s. As mentioned previously in the comment thread, I am working on this, and will open an MR to review with everything listed. As a point of process, an issue should not transition to "Needs Review" until all aspects of the scope are addressed.

@ckrina I fixed the incorrect chevron direction for RTL, as well as rebasing on top of 1.x and resolving the conflicts.

@rkoller Agreed on Esc. I'm following the W3C guidelines, which also includes patterns for arrow key navigation, Home and End.

Regarding the items that are not keyboard navigable, thanks for spotting! I'll look into that.

Hi all, I pushed fixes for the issues raised. However, note that a real RTL language, like Arabic, must be installed and selected for a proper test. Switching the dir property via browser dev tools will result in incorrect behavior, as Drupal isn't aware of this, and will incorrectly set an offset variable the navigation relies on to display properly.

Issues fixed

1. When the drawer slides out, there is a subtle gradient that sweeps across the lower part of the navigation (see gif below)
2. At desktop, the first level chevrons should be pointing to the left, in the direction that the drawer now opens.
3. RTL uses the wrong offset variable for it's positioning math (the "left" variable instead of the "right" variable)

Thanks for the feedback all, taking a look.

https://www.drupal.org/project/navigation/issues/3427657 📌 Implement the new designs to the drawer Needs review is fixed :)

Pushed an MR to fix popovers in RTL languages.

bronzehedwick → made their first commit to this issue’s fork.

I think I've figured this one out, and have a solution in an MR. I agree it seems like an unintended side effect that both hover and click toggle the menu on large screens. This is caused by two issues.

1. Hovers are handled only on small screens, while clicks are handled on all screen sizes.
2. Keeping the menu visible is done on :focus, not :focus-visible, so is triggered by a click.

I'd love to hear if anyone has a different strategy or thoughts around this.

Thanks!

Hey @rkoller, I tested the VO/keyboard focus with the built-in macOS accessibility tools using Firefox, and wasn't able to replicate the issue.

vo-keyboard-focus-in-sync.mov →

Is there a step to reproducing I'm missing?

Great catch @rkoller! We can move the focus using JavaScript. I'll work on that now.

@ckrina I've corrected the issue: the top level caret now always points right, and the submenu arrows still animate up/down.

bronzehedwick → created an issue.

On mobile the behavior only toggles on click, so it's possible the click events are "leaking" into desktop.

bronzehedwick → created an issue.

No worries, thanks for checking @lauriii!

Maybe it's just me, but I'm not able to replicate this issue @ckrina. Could this have been discovered on a slightly older fix version?

Perfect, thanks @ckrina!

@ckrina does this ticket mean that we should refactor existing indirect variable names, like --admin-toolbar-color-focus, to a direct color variable, like --admin-toolbar-color-blue-100?

bronzehedwick → made their first commit to this issue’s fork.

@ckrina Woops, I forgot to fix dismissing the submenu on hover out. Pushed a fix for that!