Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#48894 reviewing enhancement

Improve the small user sub window at the right corner on the admin bar a.k.a. the Howdy fly-out

Reported by: ixkaito's profile ixkaito Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Toolbar Keywords: has-patch has-screenshots
Focuses: ui, accessibility, javascript Cc:

Description

I always accidentally click the link in the small user sub window when I want to publish a post. Does anyone have any ideas to improve this motion? For example, disable the hover effect and display the small window after clicking the link at the right corner.

Attachments (19)

Kapture 2019-12-06 at 15.25.28.gif (7.8 MB) - added by ixkaito 4 years ago.
buddypress_usermenu_example.jpg (22.4 KB) - added by xkon 4 years ago.
Screen Shot 2020-02-19 at 6.32.03 PM.png (234.8 KB) - added by garrett-eclipse 4 years ago.
Fullscreen Mode - The Fullscreen Mode was designed to assist in removing this interface conflict
48894.diff (1.9 KB) - added by audrasjb 4 years ago.
Toolbar: Accessibility: Make the ”Howdy” menu open only when clicking
09e96cdc2b0656adf1fb3c482c8e1f26.gif (471.2 KB) - added by audrasjb 4 years ago.
Testing 48894.diff
48894.2.diff (2.6 KB) - added by afercia 4 years ago.
48894.3.diff (6.9 KB) - added by afercia 4 years ago.
48894.png (44.9 KB) - added by afercia 4 years ago.
WP-top-admin-bar-profile-image-left.jpg (19.7 KB) - added by paaljoachim 4 years ago.
Keeping consistency for all top admin bar drop down/action menus. The following moves the profile icon to the left. - Icon on the left. Text on the right.
48894.4.diff (4.2 KB) - added by isabel_brison 4 years ago.
Toggle all admin bar dropdowns on click or keypress.
Screen Shot 2020-07-03 at 2.51.32 pm.png (142.2 KB) - added by isabel_brison 4 years ago.
Admin bar with all dropdowns open
query-monitor-expanded.png (41.6 KB) - added by sabernhardt 4 years ago.
Query Monitor dropdown expanded
48894 all open.png (377.4 KB) - added by afercia 4 years ago.
48894.5.diff (5.5 KB) - added by isabel_brison 4 years ago.
Addressed feedback on previous patch.
48894.6.diff (6.3 KB) - added by isabel_brison 4 years ago.
Add close on click outside and fix scroll.
48894.7.diff (6.3 KB) - added by isabel_brison 4 years ago.
Close on focus outside.
Kapture 2020-07-07 at 20.58.19.gif (2.6 MB) - added by ixkaito 4 years ago.
48894.8.diff (6.0 KB) - added by isabel_brison 4 years ago.
Fix scrolling issue and remove unneeded handling of Enter key.
48894.patch (6.0 KB) - added by mukesh27 3 years ago.
Updated patch.

Change History (96)

#1 @SergeyBiryukov
4 years ago

  • Component changed from Administration to Toolbar

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 years ago

#3 @afercia
4 years ago

  • Keywords needs-design-feedback added

This ticket was discussed during today's accessibility bug-scrub: agreed that maybe it's time to reconsider the whole "My account" fly-out and get a new design. Adding the related keyword.

#4 @xkon
4 years ago

My reply, when discussed on the ticket scrubs, was that by default WordPress uses a dropdown menu at which everything points to just profile.php as well as logout.

There are plugins that already hook there ( see buddypress_usermenu_example.jpg ) so completely changing the functionality might not be optimal but we might want to reconsider the "default state" at least.

Maybe we could utilize a "default state" of having 2 simple links towards the profile and logout without the need of the dropdown and if any plugin adds more items on the menu then it could be "converted" to the way it currently works?

Not sure if that would be possible or a perfect solution but it would definitely avoid the menu from interfering with actions like the Publish mentioned or even the Screen Options / Help that are the usual suspects on the far right corner there.

Re-mentioning this here just to start a discussion on any possible improvements :)

@garrett-eclipse
4 years ago

Fullscreen Mode - The Fullscreen Mode was designed to assist in removing this interface conflict

#5 in reply to: ↑ description @garrett-eclipse
4 years ago

  • Keywords close added

Replying to ixkaito:

I always accidentally click the link in the small user sub window when I want to publish a post. Does anyone have any ideas to improve this motion? For example, disable the hover effect and display the small window after clicking the link at the right corner.

Hello @ixkaito from watching your demo I feel you'd benefit from using the 'Fullscreen mode' of the editor which is found under the triple dot icon ⋮ (More tools & options).
https://core.trac.wordpress.org/raw-attachment/ticket/48894/Screen%20Shot%202020-02-19%20at%206.32.03%20PM.png

This will suppress the admin bar from view when editing a post and remove the conflict of the Publish and User dropdown being in close vicinity.

@afercia / @xkon if the my-account flyout needs a redesign for accessibility improvements can we identify the current issues and pitfalls to help drive any design changes, was there any raised in the scrub to drive the need?

#6 @afercia
4 years ago

  • Keywords close removed

Will propose to add this ticket to the next accessibility team meeting agenda.

I wouldn't say this issue relates only to the block editor page. It is a long standing issue in all the admin pages, where the "Help" and "Screen Options" are close to the fly-out. Not sure this should be closed.

This ticket was mentioned in Slack in #accessibility by nrqsnchz. View the logs.


4 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 years ago

#9 @afercia
4 years ago

  • Focuses javascript css administration removed
  • Milestone changed from Awaiting Review to 5.5
  • Owner set to afercia
  • Status changed from new to assigned

This ticket was discussed during today's accessibility bug-scrub and last week accessibility meeting: agreed to create a small team dedicated to this issue. Moving to 5.5 for initial research.

This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


4 years ago

This ticket was mentioned in Slack in #accessibility by nrqsnchz. View the logs.


4 years ago

#13 @afercia
4 years ago

  • Summary changed from Improve the small user sub window at the right corner on the admin bar to Improve the small user sub window at the right corner on the admin bar a.k.a. the Howdy fly-out

This ticket was discussed during today's accessibility weekly meeting. Agreed to start with with a simpler step and make the "Howdy" menu open only when clicking. No hover.

@audrasjb
4 years ago

Toolbar: Accessibility: Make the ”Howdy” menu open only when clicking

@audrasjb
4 years ago

Testing 48894.diff

#14 @audrasjb
4 years ago

  • Keywords has-patch has-screenshots added

Hi,

48894.diff handles our first step here. It replaces hover event with click, only for the Howdy menu.

Works fine on my side, but please test.

Cheers,
Jb

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


4 years ago

#16 @garrett-eclipse
4 years ago

Has HoverIntent been explored for this reveal?

I understand the switch from hover to click is driven by the accidental interference on hover. But would hoverintent address those concerns without removing the hover functionality completely as it is currently engrained in the users cognitive memory experience.

A similar issue was resolved with hoverintent on wp.org glossary autolinking in make posts as seen in meta#4508

*Sidenote: No extra script needed as jQuery HoverIntent is available by default in WordPress using the hoverIntent enqueue handle.

#17 @afercia
4 years ago

Hoverintent is already in place for all the admin bar items, including the "Howdy" menu. That doesn't solve the issue.

Maybe the Howdy menu item needs to visually indicate it works differently. Also, it would be best to check what happens on touch screens, where the touchstart event adds one more click event on the <li> elements.

Aside: since [46872] the admin bar uses a no-jQuery version of hoverintent.

#18 @afercia
4 years ago

  • Keywords needs-refresh added

Testing 48894.diff:

  • use Chrome dev tools to test on a mobile device emulation
  • make sure the device type in the Chrome dev tools top bar (at the right of "Zoom" level) is set to "mobile"
  • click the Howdy menu
  • the menu doesn't open

Actually, this part of the patch:

// Toggle hover class on My Account menu.
adminBarMyAccount.addEventListener( 'click', function( event ) {
	event.preventDefault();
	adminBarMyAccount.classList.toggle( 'hover' );
} );

toggles the CSS class a second time so the hover class is added and immediately removed: the menu doesn't open.

This code should run only on non-touch devices.

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


4 years ago

#20 follow-up: @afercia
4 years ago

Reporting form today's accessibility bug-scrub. Regarding the link that opens the menu:

We could provide both maybe. There are the hide-if-js and hide-if-no-js CSS classes that can be used for this purpose.

  • link with hide-if-js
  • a separate button with hide-if-no-js

#21 in reply to: ↑ 20 @sabernhardt
4 years ago

from afercia:

We could provide both maybe.

"Both" refers to the suggestion of using an expand/collapse button (with JavaScript enabled, otherwise using the link).

I had envisioned that button without the link, featuring:

  1. "Howdy, [name]" as unclickable text outside the button,
  2. The profile image inside the button (so that still shows at mobile sizes),
  3. Button text—such as "Account"—visible at desktop screen sizes,
  4. A down arrow on the side (and maybe up when expanded), and
  5. False/true aria-expanded attributes.

https://live.staticflickr.com/65535/49807870273_58684d6d80_m.jpg

However, it may be better to add a simpler expandable button (perhaps showing only the arrow) next to the current link.

One way or another, adding an arrow would provide a visual indication that things have changed. Then it would be less of a surprise that hovering does nothing.

#23 @afercia
4 years ago

Worth noting [47600] changed both "Your Profile" and "My Profile" links in admin menu and toolbar to just "Profile" for consistency. I'd suggest to stay consistent and use "Profile" instead of "Account".

#24 @afercia
4 years ago

#48471 was marked as a duplicate.

#25 @afercia
4 years ago

Related: #48494 which was in turn split out from #48471.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 years ago

@afercia
4 years ago

#28 @afercia
4 years ago

  • Keywords needs-refresh removed

48894.2.diff tries to solve the issue on mobile by removing the click event listener for touch devices. Some testing would be nice (see comment:18).

Apart from that, I'd think we should consider a few more things:

  • the toggle link should have a button semantics and work also with the Space Bar when JS is on
  • this should be implemented via JavaScript with a new function, something like setupMyAccountToggle
  • see also the CSS class aria-button-if-js and what it does
  • as a user, I would expect the menu to close when I click outside of it

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 years ago

@afercia
4 years ago

#31 @afercia
4 years ago

48894.3.diff:

  • when JS is on, adds role=button and aria=expanded to the my account link
  • handle keyboard events to make the my account link work with the Spacebar key
  • this way, when JS is ON the link is perceived and behaves like a button
  • add chevron dashicons to visually distinguish this menu item and reflect the collapsed / expanded state
  • clean up a bit the JS

See screenshot below.

I think this is now ready for a final review.

@afercia
4 years ago

#32 @sabernhardt
4 years ago

Testing 48894.3.diff:
I did not have any problems using the link/button in four Windows browsers (Chrome, Firefox, the new Edge and IE11), with JavaScript and NVDA. I also checked in Chrome without JavaScript. The patch could be ready as it is.

I still would like to move the Howdy text outside the link/button so the button text describes the submenu better. Maybe I can come up with a variation to consider within the next week, now that the more difficult parts are done. Then we can determine whether or not my option improves the toolbar. Adding an aria-label of "Profile" is a simple change that could help non-sighted screen reader users, but there might be a better option for everybody.

#33 @sabernhardt
4 years ago

Appending "Profile" to the button text as screen-reader-text should be a better option than my aria-label idea.

	$howdy = sprintf( __( 'Howdy, %s' ), '<span class="display-name">' . $current_user->display_name . '</span>' ) . '<span class="screen-reader-text"> ' . __( 'Profile' ) . '</span>';

(Creating a separate variable from $howdy for the extra span could make the code cleaner, though.)

Then the button text starts with the visible part, and the word "Profile" gives more context for screen reader users who do not see the button.

In English, NVDA would read the above example as "Howdy [pause] Name Profile menu button collapsed submenu clickable"

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


4 years ago

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


4 years ago

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


4 years ago

This ticket was mentioned in Slack in #design by ryokuhi. View the logs.


4 years ago

#38 @paaljoachim
4 years ago

The above design with clicking the drop down arrow (chevron) to open the submenu looks nice and clean.
Link to Slack: https://wordpress.slack.com/archives/C02S78ZAL/p1593530905037800

Last edited 4 years ago by paaljoachim (previous) (diff)

This ticket was mentioned in Slack in #design by mapk. View the logs.


4 years ago

#40 follow-ups: @mapk
4 years ago

As noted in one of the recent slack conversations, there are a few thoughts here.

  1. The dropdown icon is on the wrong side of the text. I know the icons are on the left side for the other topbar menu items, but those are considered icons in the sense that they aren't necessary interaction indicators. A dropdown icon is most commonly located to the right of the text. It serves a different purpose than the other icons.
  2. As noted by Joy, shouldn't all topbar menu items have the same interaction? If the ones on the left open on hover and the ones on the right open on click, this causes some interaction confusion. I haven't tested the patch, so maybe this has been worked out already? I agree that whichever direction we go should be applied to all menu items in the same location.

@paaljoachim
4 years ago

Keeping consistency for all top admin bar drop down/action menus. The following moves the profile icon to the left. - Icon on the left. Text on the right.

#41 in reply to: ↑ 40 @audrasjb
4 years ago

Replying to mapk:

  1. As noted by Joy, shouldn't all topbar menu items have the same interaction? If the ones on the left open on hover and the ones on the right open on click, this causes some interaction confusion. I haven't tested the patch, so maybe this has been worked out already? I agree that whichever direction we go should be applied to all menu items in the same location.

This ticket aims to fix a specific issue: there is some interferences between the "Howdy" fly-out menu and the things that are located on the upper-right side of the software. As discuted during previous accessibility team meetings, it would have been better to replace all the hover interactions with onclicks, but it's too complicated to get that fixed in a single release. As the issue with the "Howdy menu" is really annoying, the mid-point is to fix the issue with this menu first, then to iterate if this change is considered as the best solution for the whole interface.

#42 in reply to: ↑ 40 @afercia
4 years ago

Replying to mapk:

  1. The dropdown icon is on the wrong side of the text.

I'd agree. What to do with the avatar image though? Not sure having the avatar and the dropdown icon on the same side would look so nice. Personally, as mentioned during latest a11y bug-scrub, I'd just remove the avatar image which is definitely too small for today's screens. A 16 by 16 pixels avatar doesn't help that much. Not sure the avatar serves a valuable purpose in the first place. Also, in the responsive view the avatar is the only visible control for the menu. The dropdown icon would be more appropriate.
Please let us know what's the best option, design-wise. We have only a few days before Beta :)

  1. As noted by Joy, shouldn't all topbar menu items have the same interaction?

Yeah this is something that was considered. Some background first:

  • At firs, we considered to entirely remove this menu: the only valuable content in the menu is the "Log out" link, which could be placed elsewhere. Re: the edit profile links, there are other links to the profile in the UI. So we felt the menu itself would need to be re-considered but agreed this should happen in a broader discussion.
  • Turned out some plugins e.g. BuddyPress add stuff within this menu. As often happen in WordPress, there's no established API to add items within this menu. The WordPress UI can be "hacked" in many ways and plugins can often do what they want but that doesn't mean these "hacks" are guaranteed to be supported. So for the future, I'd still consider to completely rethink this menu and maybe remove it.
  • The other admin bar items have long-standing accessibility issues. See for example #34668. They would need some refactoring and some simplified interaction. Right now, the interaction happens in 3 different ways for desktop mouse, desktop keyboard, and responsive view touch. Far from ideal, although I do realize the original intent (years ago) was good. The necessary refactoring is definitely out of the scope of this ticket.
  • Anecdotally, I can't count the number of times I accidentally triggered the Howdy menu when I just wanted to publish a post or when moving the mouse in the top right area for other purposes. This is what we would like to fix now, in the simpler possible way :)
  • We also agreed on the need for future iterations but for now I'd think the dropdown icon is enough to distinguish this menu from the other ones to indicate it behaves differently.

I can quickly refresh the patch to move the dropdown icon, please just let us know where it should be moved and what to do with the avatar image :)

Last edited 4 years ago by afercia (previous) (diff)

This ticket was mentioned in Slack in #design by afercia. View the logs.


4 years ago

#44 follow-ups: @mapk
4 years ago

Thanks for the context, @afercia and @audrasjb! After reviewing, I'd like to add my personal feedback: The proposed mid-point solution will introduce inconsistencies and visual oddities that are too severe IMO.

Adding a dropdown arrow is visually challenging. It definitely should not go on the left side of the text and adding it to the right of the avatar is awkward. Dropping the avatar in exchange for the dropdown arrow may work, but this still leaves us with an inconsistent topbar interaction model.

Rather than moving forward on this, I opt for a more cohesive solution that includes adding onClicks to all topbar menu items. Leaving out any dropdown arrows would be equally fine once this interaction is implemented.

I understand this option will prevent it from getting into WP 5.5, but I think this solution opens more visual and interaction problems in an effort to solve one.

#45 in reply to: ↑ 44 @audrasjb
4 years ago

  • Keywords needs-refresh added; needs-design-feedback removed

Replying to mapk:

I understand this option will prevent it from getting into WP 5.5, but I think this solution opens more visual and interaction problems in an effort to solve one.

There is technically still time to make this happen before beta 1. My worry is more the available time of accessibility contributors… If we all share that adding onclick event to all items is the best approach, maybe we can try to ping the core-JS team to see if we can find some support. Otherwise, this 5.5 project will indeed be punted to 5.6.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


4 years ago

#47 in reply to: ↑ 44 @afercia
4 years ago

Replying to mapk:

I understand this option will prevent it from getting into WP 5.5

Fair enough. As said, the accessibility team is in favor of extending this behavior to all the admin bar menus. The real point is whether this should happen now or be postponed to a future iteration. We're just a few days from Beta 1.

In this regard, I'd like to point out an issue that I saw becoming more and more recurrent during the last release cycles: lack of timely feedback.

I'd like to note that:

  1. today is July 2, 2020
  2. this ticket was created on December 6, 2019
  3. the needs-design-feedback keyword was added on January 10, 2020
  4. the milestone was set to 5.5 on February 28, 2020

So, 3. and 4. happened respectively almost 6 months ago and 4 months ago. There was a lot of time to offer feedback, but that didn't happen.

Receiving feedback so late, at 4 days from Beta 1, turns to be just a blocker for any progress.

More importantly, it is extremely demotivating and unfair towards other teams' efforts.

I think this issue spans across various teams and it's getting to a point that I think it should be discussed in a broader context.

@isabel_brison
4 years ago

Toggle all admin bar dropdowns on click or keypress.

#48 @isabel_brison
4 years ago

@afercia, @audrasjb: I just added a patch to change behaviour for all the dropdowns, and remove the dropdown arrow from the account dropdown as per @mapk's comment above. Please test and let me know if anything further is needed!

@isabel_brison
4 years ago

Admin bar with all dropdowns open

#49 @afercia
4 years ago

@isabel_brison thanks for the patch! I see a few issues.

More importantly:

  1. When using the Spacebar, we need to emulate the native behavior: the Spacebar key activates buttons when it is released. That's why 48894.3.diff was using the keyup event instead of keydown. That part shouldn't be changed.
  2. Emulating a mobile device (do set "Mobile" in the Chrome dev tools) the menus don't open any longer. This is the reason why the previous patch removed the event listeners when ontouchstart is present. See comment:18

Minor:

  • I see a missing @param notation on a function docblock.
  • Running grunt jshint, it tells me there are 7 errors in the admin-bar.js file.

#50 follow-up: @sabernhardt
4 years ago

  • Focuses javascript added

This also seems to require a special case for when the expandable menu link navigates to an anchor/section ID.

When I clicked the Query Monitor plugin's main link, the dropdown expanded and the details section at the bottom expanded as well. Clicking again on the admin bar link did not collapse the dropdown. Instead, I needed to navigate focus back to that element and use either the space or Enter key to collapse the menu.

@sabernhardt
4 years ago

Query Monitor dropdown expanded

#51 @afercia
4 years ago

/Cc @johnbillion :)

#52 follow-up: @isabel_brison
4 years ago

@afercia I made it a keydown handler because the Enter key won't work with keyup. If using keydown with Space key causes problems on any platform or assistive technology, we'll need two separate handlers. But if it works regardless, it would be better to have a single handler for both keys.

Let me know what you think and I can make another patch with the remaining changes.

#53 in reply to: ↑ 52 @afercia
4 years ago

Replying to isabel_brison:

If using keydown with Space key causes problems on any platform or assistive technology, we'll need two separate handlers.

That's what the previous patch did, for good reasons.

But if it works regardless, it would be better to have a single handler for both keys.

It "works" but it's not the expected interaction for the Spacebar. This is a problem for accessibility. Please follow the previous implementation and make the Spacebar work when it's released.

#54 @johnbillion
4 years ago

Yeah the admin bar menu for Query Monitor has never worked very well on touch devices because of its anchor link and the fact the user stays on the current page after clicking a menu item. It would be good to improve that.

#55 in reply to: ↑ 50 @afercia
4 years ago

Replying to sabernhardt:

This also seems to require a special case for when the expandable menu link navigates to an anchor/section ID.

When I clicked the Query Monitor plugin's main link, the dropdown expanded and the details section at the bottom expanded as well. Clicking again on the admin bar link did not collapse the dropdown. Instead, I needed to navigate focus back to that element and use either the space or Enter key to collapse the menu.

Good catch! This seems something specific to Query Monitor. Ideally, focus shouldn't be moved and should stay on the clicked top level menu item.

I tested a couple other plugins that add their own menus to the admin bar and they work correctly. Tested "Yoast SEO" and "View Admin As".

There's one important thing though:
Clicking a menu should close all the other ones :) Tis wasn't a problem in the previous patch, as only the "Howdy" menu was changed. Now that they open on click, they stay open when clicking other menus, leading to a far from ideal situation. See attached screenshot.

#56 @isabel_brison
4 years ago

This also seems to require a special case for when the expandable menu link navigates to an anchor/section ID.

@sabernhardt, @johnbillion: I'm not sure if it's related to the link being internal - in the click event handler, I'm checking the classlist for the <li> wrapper and it's not returning the hover class (even though the class is present). Does the plugin have any handlers on that element that might be conflicting with this one?

Clicking a menu should close all the other ones :)

@afercia this is an existing issue: currently, if you open the a menu with Enter, you can tab through and open the next menu and the first one stays open. I Agree we should fix it, as it's expected behaviour with the click interaction. I'm not sure I'll be able to do it before Tuesday though :(

@isabel_brison
4 years ago

Addressed feedback on previous patch.

#57 @isabel_brison
4 years ago

@afercia I've fixed the issues you mentioned in comment:49.

Regarding all the menus being open at the same time, I won't be able to address that before Tuesday, so if we want to get it in before the beta perhaps it would be quicker for someone else to look at it.

To clarify the desired interaction: I would expect an open menu to close when a click anywhere outside of it is detected, and not only when another menu is clicked.

We should also consider the keyboard interaction: to be consistent, it would make sense for an open menu to close when focus is transferred outside of it, which would potentially solve the problem of menu items transferring focus to a section of the current page, as in the Query Monitor example.

It would be good to have the thoughts of the design team on this - cc. @mapk.

#58 @mapk
4 years ago

To clarify the desired interaction: I would expect an open menu to close when a click anywhere outside of it is detected, and not only when another menu is clicked.

We should also consider the keyboard interaction: to be consistent, it would make sense for an open menu to close when focus is transferred outside of it

I agree here completely. These both feel necessary before we can merge this into Core. Thank you for all the work you've done with this, @isabel_brison. Do you have another person in mind who might have time to complete this before Tuesday?

#59 @isabel_brison
4 years ago

@mapk I don't have anyone in particular; will ask in #core-js.

This ticket was mentioned in Slack in #core-js by isabelbrison. View the logs.


4 years ago

#61 @afercia
4 years ago

Thanks @isabel_brison I do realise the effort put into this isn't trivial and appreciate it.

I noticed a few things:

  • The page still scrolls down when pressing Spacebar.
  • This is because there are 2 keydown events, one of them doesn't prevent the default action
  • This is the reason why 48894.3.diff was reusing the existing keydown event for the "My account" item. I think it's best to attach just one event and use the previous approach for all items.
  • There's the need to remove the hoverintent-js dependency.
Last edited 4 years ago by afercia (previous) (diff)

#62 @afercia
4 years ago

Looking better at it: we can't entirely remove hoverintent-js. Whether we like it or not, plugins do all sort of things with their menus. They do add their own sub-menus and without hoverintent-js they won't open any longer. Some plugins also attach an action to the sub-item that opens a sub-menu so that pressing Enter or Spacebar will likely trigger that action :(

For example:

  • install and activate the "View Admin As" plugin
  • on trunk: see that hovering a sub-item opens its sub-menu
  • with the patch applied: the sub-menu doesn't open any longer
  • pressing Enter will now trigger the action associated with the sub-item

This way, there's the risk this change will break several plugins.

As with many other things in WordPress that don't have an established API, plugins are allowed to do whatever they want. For this reason, this change should be as minimal as possible. At the very least:

  • we should restrict the change only to the top-level items
  • better: we should avoid a so large change and reconsider the initial proposal to change only the "Howdy" menu

That said, given the very late design feedback at a few days from Beta 1, I'm afraid we have only two options:

  • change only the "Howdy" menu with some different design: feedback from the design team should be given in the next few hours
  • postpone this change to the next release and later discuss publicly how to improve the process to make sure feedback from other teams happens timely and doesn't block the progress

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


4 years ago

#64 follow-up: @isabel_brison
4 years ago

This is the reason why 48894.3.diff​ was reusing the existing keydown event for the "My account" item. I think it's best to attach just one event and use the previous approach for all items.

@afercia by previous approach, do you mean handling both Space and Enter with keydown? An alternative would be to detect Space in the keydown handler and prevent default for it there. It's a bit messy though.

install and activate the "View Admin As" plugin
on trunk: see that hovering a sub-item opens its sub-menu
with the patch applied: the sub-menu doesn't open any longer
pressing Enter will now trigger the action associated with the sub-item

I'm checking the "View Admin As" dropdown on trunk and it's not keyboard accessible. I can open it on hover, but can't even get focus to the top-level item to open with Enter. Not sure why though: it's using divs instead of links but they have tabindex="0" set.
The event handlers I added should work for any li.menupop > .ab-item, but the element has to be focusable to register the keyboard event.

@isabel_brison
4 years ago

Add close on click outside and fix scroll.

@isabel_brison
4 years ago

Close on focus outside.

#65 @isabel_brison
4 years ago

My latest patch addresses closing the dropdowns when a click or keypress is captured anywhere outside them. I've also addressed the issue of the space key scrolling the page by preventing its default behaviour in the keydown event.

I don't believe we need hoverIntent anymore for the admin bar, as the keyboard event listeners should be able to handle any submenus as long as the markup is correctly structured, but it might be good to test with a few more plugins.

#66 in reply to: ↑ 64 @afercia
4 years ago

Replying to isabel_brison:

This is the reason why 48894.3.diff​ was reusing the existing keydown event for the "My account" item. I think it's best to attach just one event and use the previous approach for all items.

@afercia by previous approach, do you mean handling both Space and Enter with keydown? An alternative would be to detect Space in the keydown handler and prevent default for it there. It's a bit messy though.

  • the Enter key just needs the click event :)
  • the Spacebar a keyup event, to emulate native behavior on buttons

install and activate the "View Admin As" plugin
on trunk: see that hovering a sub-item opens its sub-menu
with the patch applied: the sub-menu doesn't open any longer
pressing Enter will now trigger the action associated with the sub-item

I'm checking the "View Admin As" dropdown on trunk and it's not keyboard accessible. I can open it on hover, but can't even get focus to the top-level item to open with Enter. Not sure why though: it's using divs instead of links but they have tabindex="0" set.

I know it's not accessible but, by removing the hoverintent, it's not possible to open the sub-items any longer. Also, pressing Enter on them activates the attached action, which is unexpected.

#67 @ixkaito
4 years ago

Tesing 48894.7.diff with some plugins e.g. View Admin As, Debug Bar and Toolbar Extras both on desktop and mobile. It works fine for me. The interaction of Debug Bar was a little tricky, but maybe this is an issue of the plugin.

#68 @afercia
4 years ago

48894.7.diff isn't ready:

  • I still see two keydown events, thus the page scrolls down when pressing the Spacebar key on a top level item
  • the View Admin As sub-items don't work any longer:
    • open the menu
    • hover the mouse pointer, for example on "Editor" under "Roles"
    • the sub-item doesn't open
    • there's no other way to open the sub-menu: press Enter on "Editor" triggers the attached action

#69 @whyisjake
4 years ago

  • Milestone changed from 5.5 to 5.6

Looks like this is close, but not going to make the window for 5.5. I'm punting to the 5.6 milestone.

@isabel_brison
4 years ago

Fix scrolling issue and remove unneeded handling of Enter key.

#70 @isabel_brison
4 years ago

I still see two keydown events, thus the page scrolls down when pressing the Spacebar key on a top level item

There's a keyup event that handles toggling menus with Space, and a keydown event that I've now edited to only prevent default for the Space key, since it's not necessary to handle Enter. The reason it wasn't working before was I'd forgotten to add the parentheses on event.preventDefault :)
I didn't move the preventDefault to the Escape key handler because that one's attached to all menu items, so it would require extra logic in order to not break the Space key on input fields (such as the one in View As > Capabilities > filter). I guess we could do it that way, but the code would be much messier.

the View Admin As sub-items don't work any longer:

They do work: they now open on click instead of hover. They don't open on Enter because they're not focusable, but that's an existing issue with the plugin; it didn't open on Enter before these changes either.

#71 @noisysocks
4 years ago

The code in 48894.8.diff looks good to me and the change seems to work well when I test locally. Some very minor notes:

  • Code style: The addEventListener calls on line 71 and 72 need spaces within the ()s.
  • It would be nice to tell the reader using an inline comment what key has key code 32.

I'll defer to @mapk and @afercia whether this is ready to be committed from a design and accessibility perspective. At any rate we'll have to wait until trunk re-opens for 5.6.

Last edited 4 years ago by noisysocks (previous) (diff)

#72 @sjnbham
4 years ago

#48494 was marked as a duplicate.

@mukesh27
3 years ago

Updated patch.

#73 @mukesh27
3 years ago

  • Keywords needs-refresh removed

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


3 years ago

#75 @afercia
3 years ago

  • Milestone changed from 5.6 to Future Release

This ticket was discussed during today's accessibility bug-scrub: the team agreed to punt this ticket to future release as it seems there’s not a clear, agreed, direction for this issue and the last patches to refactor the interaction with the admin bar menus doesn't fully solvee the original issue.

#76 @afercia
3 years ago

  • Owner afercia deleted
  • Status changed from assigned to reviewing

This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.