[wp-trac] [WordPress Trac] #46682: Site Health: fix (or remove) arrows navigation on the accordion

[WordPress Trac]

#46682: Site Health: fix (or remove) arrows navigation on the accordion
-------------------------------+-------------------------------------
 Reporter:  afercia            |      Owner:  (none)
     Type:  defect (bug)       |     Status:  new
 Priority:  normal             |  Milestone:  5.2
Component:  Administration     |    Version:  trunk
 Severity:  normal             |   Keywords:  needs-patch site-health
  Focuses:  ui, accessibility  |
-------------------------------+-------------------------------------
 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 [https://api.jquery.com/event.which/ 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 [https://www.w3.org/TR/wai-aria-practices-1.1/#accordion
 marked as "Optional" in the ARIA authoring practices]

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/46682>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform