WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 6 months ago

Last modified 3 months ago

#46682 closed defect (bug) (fixed)

Site Health: fix (or remove) arrows navigation on the accordion

Reported by: afercia Owned by: afercia
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: site-health has-patch commit
Focuses: ui, accessibility Cc:

Description

Splitting this out from https://core.trac.wordpress.org/ticket/46573#comment:43

The arrows navigation on the accordion is a bit buggy:

https://core.trac.wordpress.org/browser/trunk/src/js/_enqueues/admin/site-health.js?rev=44986&marks=43-49#L42

1
The page scrolls natively when pressing the up/down arrow keys; the function uses keyup so the scrolling can't be prevented. To actually see this behavior the content needs to be long. You can reduce the browser's window height to reproduce.
Either there's the need of a keydown event with the sole purpose of preventing the default (which is meh) or the function should use keydown and prevent the default.

2
Pressing the down arrow moves focus to the last accordion item. Expected: should move focus to the next item. worth noting this jQuery bit:

$( '.health-check-accordion-trigger', $( this ).closest( 'dt' ).nextAll( 'dt' ) ).focus();

actually means:

get all the .health-check-accordion-trigger in the context of all the next dt elements so .focus() runs on the last returned item. There's the need of a .first() after .nextAll()

Instead, the Up arrow works as expected because in this bit of jQuery:

$( '.health-check-accordion-trigger', $( this ).closest( 'dt' ).prevAll( 'dt' ) ).focus();

the last returned element happens to be the one we need.

3
Not sure why toString() is used in e.keyCode.toString(). Worth also reminding when using jQuery which should be used instead of keyCode.

Possible options:

  • point 2 can be fixed filtering the collection with first()
  • preventing the scrolling is a bit more annoying as it would require an additional keydown event used only to prevent the default action
  • point 3: toString() should be removed, unless I'm missing something
  • alternatively, the whole arrows navigation could be removed, as it's non-essential and marked as "Optional" in the ARIA authoring practices

Attachments (2)

46682.diff (1.0 KB) - added by afercia 6 months ago.
46682.2.diff (748 bytes) - added by afercia 6 months ago.

Download all attachments as: .zip

Change History (12)

#1 @xkon
6 months ago

In my opinion they keyup/keydown should be left for scrolling purposes (so default browser behaviour always).

There's no other page within the admin as far as I know that has this functionality plus it's a little bit confusing as when you don't have an accordion item focused you can freelly scroll up/down with keys but that behaviour changes and it isn't expected when selecting one.

#2 @afercia
6 months ago

When testing, I was surprised to see arrows navigation was implemented. It's an optional requirement in the ARIA Authoring Practices but somehow unexpected. Its advantages are not immediately clear:

  • when the accordion panels are all closed, arrows navigation adds little value as it just replicates native tabbing
  • when the panels are open, it allows to jump directly through the accordion headers, skipping any focusable content within the panels: this is somehow useful but I wonder how many users will discover this feature

However, the combined effect of navigating through the accordion header and the page scrolling at the same time is confusing.

Let me try a quick patch to fix it so we can better test and evaluate if it's worth keeping it or can be removed.

@afercia
6 months ago

#3 @afercia
6 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

46682.diff is a quick patch to fix the expected behavior of arrows navigation on the accordion headers.

Please test :) No objections to entirely remove arrows navigation if it gets determined it adds little value. In my personal opinion, it does add some (little) value. /Cc @clorith

#4 @Clorith
6 months ago

I wasn't aware of the bug here with the arrows (I think that's me accidentally introducing it, thinking I'd not changed it, oops).

For some quick context, it was originally added solely based off a W3-WAI article, but it's not a requirement (we just wanted to do our best in providing the best possible experience), so if it adds unnecessary complexity, there's nothing wrong with removing it.

#5 @xkon
6 months ago

Thank you for the detailed explanation @afercia ! The patch works fine on my end as well.

Now that I could see how it should be normally working and along with your explanation I can understand the usability behind this as well and I can definitely change my original reply and thinking to this (haha):

On the Info page we have 1 accordion so it's super easy to go from one element to the other via the arrows.

On the Status page we have 3 split accordions.

While testing I was a bit confused at the start for when I would have to "tab" to get to the next accordion or back to the previous one (and yes I'm totally new to this, I saw it because of this ticket so thank you for that as well, learning new things everyday). I was actually expecting to arrow my way up and down seamlessly between all accordions (except the hidden one obviously but still when that was in view I thought that I could again just arrow my way to the previous .

I might not be the best candidate to test this or have a solid opinion on this, but seeing it for the first time and trying to use it in my opinion if we could somehow manage to also jump between "all" accordions on Status page would be even better (if that's possible). Would this make sense?

#6 @afercia
6 months ago

Yeah I get your point. Technically, I think it's doable. However, the expected interaction described in the ARIA authoring practices is on a per-accordion basis.

Regardless, I'm not sure how many users, including assistive technologies users, would really "expect" this kind of interaction. Overall, I'd tend to think keeping it simple is best and I'd just remove it. I'd leave the final call to @Clorith as this is his pet project :)

#7 @Clorith
6 months ago

Let's remove the arrow navigation.

The intention was to provide the best possible usability, but it sounds like it may be an unexpected interaction for users, and instead work against the intended purpose.

@afercia
6 months ago

#8 @afercia
6 months ago

  • Keywords commit added; needs-testing removed
  • Owner set to afercia
  • Status changed from new to assigned

46682.2.diff removes arrows navigation from the accordions.

#9 @afercia
6 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 45069:

Accessibility: Remove arrows navigation from the Site Health accordions.

Arrows navigation on accordions is an optional keyboard interaction feature mentioned in the WAI-ARIA Authoring Practices. While it can add some value in some specific cases, it's not so discoverable and it's unlikely users, including assistive technologies users, would really "expect" this kind of interaction.

See #46573.
Fixes #46682.

#10 @spacedmonkey
3 months ago

  • Component changed from Administration to Site Health
Note: See TracTickets for help on using tickets.