Menu items with display: none or hidden erroneously receive focus when using arrow keys

[CetinSert]

See previous regression from 2022-08-02 which you had resolved favorably.

• <sl-menu> and <sl-menu-item> regression in 2.0.0-beta.79 caused by allowing focus on disabled items for improved accessibility #845
  The above link contrasts this, now repeated, regression against behaviors from various native (Windows Notepad, macOS Finder) and web apps (Visual Studio Code, Adobe Spectrum Web Components).

The reason I labeled this as a feature request rather than a bug report is because I feel your efforts for accessibility should be recognized, encouraged and earn you the license/freedom to improve upon the state of the art. That being said, we do need a simple way to restore behavior for users without disabilities matching learned experiences from all other platforms.

---

What issue are you having?

• 🚨 BREAKING: improved the accessibility of <sl-menu-item> so checked items are announced as such
  • Checkbox menu items must now have type="checkbox" before applying the checked attribute
  • Checkbox menu items will now toggle their checked state on their own when selected
  • Disabled menu items will now receive focus, but are still not selectable

This causes a regression when we use disabled with a not-displayed <sl-menu-item>.

1. Visit https://sl.rt.ht/1107/?io
   (shoelace.real-time.hypertext)
2. Press Ctrl+O to focus on the output panel
3. Press Tab to focus on the first <sl-menu-item>
4. Press ↓
   • ✔️ .87 focus jumps to Option 3
   • ❌ .88 focus is on hidden Option 2 😅
5. Press Ctrl+I to focus on the input panel
6. Press ↑ to change version .87 to .88
7. Repeat steps 2–4

Describe the solution you'd like

Provide a way to prevent <sl-menu-item>s from receiving focus while keeping them in DOM.

<sl-menu-item inert> does not work and probably it should be made to work (transferring inert down to its relevant shadow-dom descendant.)

Describe alternatives you've considered

Monkey-patching Shoelace, removing/adding <sl-menu-item>s from DOM.
Both of the alternatives require excessive work for something that should be simple.

---

It would also be great if the focus-related Shoelace keyboard event handlers considered whether the next <sl-menu-item> has CSS display: none without requiring a new inert attribute.

[claviska]

This change was made because, as it turns out, that allowing focus on disabled items is an accessibility best practice. If disabled items don't receive focus, here's what happens to visually impaired users:

• The screen reader announces "5 menu items"
• The user steps through each item, expecting to hear five items
• The user only hears four items

The user is left wondering what happened to the fifth item and has no way to identify where and what the disabled item even is. Contrast this to disabled items that do receive focus:

• The screen reader announces "5 menu items"
• The user steps through each item, expecting to hear five items
• The user hears all five items, but this time one is announced as "dimmed" or "disabled"

The visually impaired user is aware of where the item is, what it is, and the fact that it's currently disabled.

Here's another scenario for keyboard users who often rely on muscle memory to do things quickly. Consider the following menu when we don't skip disabled items:

<sl-menu>
  <sl-menu-item>New</sl-menu-item>
  <sl-menu-item>Open</sl-menu-item>
  <sl-menu-item>Save</sl-menu-item>
  <sl-menu-item>Delete</sl-menu-item>
</sl-menu>

1. The user focuses into the menu
2. The user presses down twice
3. The user presses enter to trigger "Save"

Now let's disable the "Save" option and try again:

<sl-menu>
  <sl-menu-item>New</sl-menu-item>
  <sl-menu-item>Open</sl-menu-item>
  <sl-menu-item disabled>Save</sl-menu-item>
  <sl-menu-item>Delete</sl-menu-item>
</sl-menu>

1. The user focuses into the menu
2. The user quickly presses down twice to select on "Save"
3. The user presses enter, but inadvertently selects "Delete" because "Save" was disabled and got skipped!

Had the "Save" been focusable, the user would not have selected "Delete." Sure, we can argue that it's the user's fault for not being more cautious — keyboard users do tend to work quickly — or we can make the UX better by ensuring it remains consistent regardless of which options are disabled.

These explanations are pretty straight-forward, but a common rebuttal is that disabled form controls and buttons don't receive focus. That's 100% true when tabbing! However, disabled form controls are still navigable with screen readers, so they'll be announced as dimmed/disabled, just not when tabbing. Here's a video showing how VoiceOver users hear disabled inputs when the virtual cursor lands on them.

CleanShot.2023-01-06.at.11.09.34.mp4

Note that the virtual cursor isn't moved exclusively with Tab, but also with modifier keys. In VoiceOver, for example, it's common to use Ctrl + Option + Right Arrow to move to the next element, even if it's not tabbable, as shown in the video above.

By the way, I finally found the W3 source for best practices on focusing disabled controls which explains things further:

By default, disabled HTML input elements are removed from the tab sequence. In most contexts, the normal expectation is that disabled interactive elements are not focusable. However, there are some contexts where it is common for disabled elements to be focusable, especially inside of composite widgets. For example, as demonstrated in the Menu or Menu bar pattern, disabled items are focusable when navigating through a menu with the arrow keys.

Removing focusability from disabled elements can offer users both advantages and disadvantages. Allowing keyboard users to skip disabled elements usually reduces the number of key presses required to complete a task. However, preventing focus from moving to disabled elements can hide their presence from screen reader users who "see" by moving the focus.

Authors are encouraged to adopt a consistent set of conventions for the focusability of disabled elements. The examples in this guide adopt the following conventions, which both reflect common practice and attempt to balance competing concerns.

For elements that are in the tab sequence when enabled, remove them from the tab sequence when disabled. For the following composite widget elements, keep them focusable when disabled:

• Options in a Listbox
• Menu items in a Menu or menu bar
• Tab elements in a set of Tabs
• Tree items in a Tree View

This logic doesn't apply to form controls, but certain "composite widget elements" such as options, menus, tabs, and tree items. As this appears to be the clearest, most definitive guideline available, Shoelace will continue to follow this pattern to improve the experience for as many users as possible.

[CetinSert]

Thank you for the detailed answer. So you have no interest in providing support for inert either? Because if you do, I would like to open a new feature request. @claviska

[claviska]

If inert works, it works. It’s a global attribute so, if it doesn’t, that’s either per spec or a browser bug. However, I’m not interested in making it easier for devs to make things harder and less accessible to use.

That being said, we do need a simple way to restore behavior for users without disabilities matching learned experiences from all other platforms.

How do you determine this? Do you ask your users if they have disabilities, or is “users without disabilities” a guise for personal preference? Don’t you think it’s easier to follow established best practices instead of expending all this energy reimplementing behaviors that are less accessible?

I’ll agree there are inconsistencies in the wild. For example, menus in macOS allow you to focus on disabled items whereas Windows does not. But this isn’t an operating system, it’s the Web and when the W3 puts out clear guidance on how to do something accessibly, it’s a good idea to follow that advice for the benefit of everyone.

I don’t get the persistence to undo this when it’s clearly documented. 🤷🏻‍♂️

[CetinSert]

@claviska

If inert works, it works. It’s a global attribute so, if it doesn’t, that’s either per spec or a browser bug.

o__O We cannot write this off to specs or browsers when we have Shoelace's very own getAllItems (and thus handleKeyDown, getCurrentItem, setCurrentItem) implemented in an inert-unaware fashion:
b330657#diff-5a985db40b62ab2036f2e38176aaeb51df0a0da1ceef433f81b698d0ba783b6cR55

if getAllItems were made aware of whether <sl-menu-item>s have CSS display: none, this would be a great middle ground satisfying accessibility and providing a way to declaratively change items of a menu based on arbitrary conditions without having to re-invent bookkeeping mechanisms or mess with DOM.

[CetinSert]

With Shoelace handlers explicitly disabled,

<link rel=stylesheet href=//cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.88/dist/themes/light.css>
<script type=module>
  import { SlMenu } from "//cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.88/dist/shoelace.js";
  SlMenu.prototype.setCurrentItem = i => { console.warn('ignore', i); };
  SlMenu.prototype.handleKeyDown = function(e) { console.warn(this, e.key); };
</script>
<script type=module src=//cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.88/dist/shoelace.js></script>

<sl-menu-item inert> just works™.

---

If inert works, it works. It’s a global attribute so, if it doesn’t, that’s either per spec or a browser bug.

Despite the misplaced confidence,
Shoelace handlers were actively breaking inert!
Nothing per spec or a bug in implementing browsers.

[CetinSert]

@claviska (@tonivj5) – I have added support for inert internally in https://sl.rt.ht/1107/fix/?io 👈🏻 (click/tap for demo)

<link rel=stylesheet href=//cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.88/dist/themes/light.css>
<script type=module scope=sl-fix>
  import { SlMenu } from "//cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.88/dist/shoelace.js";
  const j = { // jump immediate/delayed, skip inert
    i: (t, f, s = x => x?.inert) => { const x = t[f]; return s(x) ? j.i(t[f], f) : x; },
    d: (t, f, s = x => x?.inert) => { const x = t;    return s(x) ? j.d(t[f], f) : x; },
  }; 
  SlMenu.prototype.setCurrentItem = i => { console.warn('ignore', i); };  
  SlMenu.prototype.handleKeyDown = function(e) {
    const                                t =        e.target;
    const                                a = j.d(this.firstElementChild,     'nextElementSibling');
    const                                z = j.d(this. lastElementChild, 'previousElementSibling');
    switch (e.key) {
    case 'Home':                         a                            ?.focus?.(); break;
    case 'End':                          z                            ?.focus?.(); break;
    case 'ArrowUp':   (t === a ? z : j.i(t, 'previousElementSibling'))?.focus?.(); break;
    case 'ArrowDown': (t === z ? a : j.i(t,     'nextElementSibling'))?.focus?.(); break;
    case 'Enter':                        t                             .click  (); break;
    }
  };
</script>
<script type=module src=//cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.88/dist/shoelace.js></script>

---

This approach does not require manually juggling around tabindex values 🚀.
They stay tabindex=0 at all times, not breaking inert in implementing browsers.

[CetinSert]

Now that we have left the technical aspect of this behind.

@CetinSert – That being said, we do need a simple way to restore behavior for users without disabilities matching learned experiences from all other platforms.

@claviska How do you determine this? Do you ask your users if they have disabilities, or is “users without disabilities” a guise for personal preference? Don’t you think it’s easier to follow established best practices instead of expending all this energy reimplementing behaviors that are less accessible?

---

How do you determine this? Do you ask your users if they have disabilities

User agents can act on behalf of their users and indicate their accessibility requirements to web developers.

I am not trying to make disabled <sl-menu-item>s from being accessible to screen readers but looking for a way to prevent them from receiving focus when they are also not displayed.

---

or is “users without disabilities” a guise for personal preference?

This registers as anywhere between "uncalled for" to "borderline rude".
I hope you can refrain from making unnecessary assumptions suggesting such false, fabricated need for a guise.

I was very clearly not looking for reverting disabled to a less accessible behavior a second time
but a new property/attribute to prevent an <sl-menu-item> from receiving focus (even only when display:none-d).

---

Don’t you think it’s easier to follow established best practices instead of expending all this energy reimplementing behaviors that are less accessible?

I do believe in your choices in building Shoelace. That is why from the very first paragraph of this issue, I recognized the need to improve upon the state of the art and support you making the better choices. Please note that I have not asked for reverting to the old behavior this time but suggested a need for supporting inert instead.

In my use case, I just want to set a single <sl-menu-item> property/attribute and have the browser with declarative CSS deal with hiding contextually irrelevant menu items from view and focus. This used to work with disabled. Now, thanks to the specs / accessibility goals you firmly follow, we have made it work with inert instead. With the new inert support, I can keep using Shoelace without having to use multiple <sl-menu> instances or otherwise having to bookkeep when to detach/re-attach menu items to DOM, say for signed-in vs anonymous users.

The PoC in my previous comment suggests a solution that expends less

• machine energy than

  shoelace/src/components/menu/menu.ts

  Line 33 in e632b51

  return [...this.defaultSlot.assignedElements({ flatten: true })].filter((el: HTMLElement) => {

• developer energy than
  • either replicating <sl-menu> instances for every possible UI state
  • or having to bookkeep and detach/re-attach <sl-menu-item>s based on UI states

---

I don’t get the persistence to undo this when it’s clearly documented. 🤷🏻‍♂️

I don't get the eagerness to assume

• intent (to undo and make/keep sth. less accessible)
• ignorance

when it's Shoelace clearly mishandling things. 🤷🏻‍♂️😎

[CetinSert]

Fixed playground links in

• Menu items with display: none or hidden erroneously receive focus when using arrow keys #1107 (comment)
• Menu items with display: none or hidden erroneously receive focus when using arrow keys #1107 (comment)

[claviska]

I went through this issue from scratch because clearly there was a miscommunication somewhere. I overlooked the display: none portion of the initial issue, so it wasn't clear that you were specifically talking about hidden menu items. In my defense, this seemed like a reopening of your previous issue specifically because of phrases like:

See previous regression from 2022-08-02 which you had resolved favorably.

and

That being said, we do need a simple way to restore behavior for users without disabilities matching learned experiences from all other platforms.

The two words that made all the difference were buried below (emphasis mine):

This causes a regression when we use disabled with a not-displayed <sl-menu-item>.

My assumption was based on 1) the fact that this behavior changed just the other day; 2) the issue is titled “provide a way to prevent <sl-menu-item>s from receiving focus”, which was exactly what I intentionally changed; and 3) the first line of the issue linking to a “previous regression […] which you resolved favorable” to serve as justification for why said change should be reverted or reversible.

Everything in this issue except those two words led me to believe you were simply unhappy with the change and wanted an escape hatch for it. This type of request is unfortunately very common in open source, so you can understand why it might be frustrating.

Anyways, I'm renaming this issue to clarify the problem and I'll address it soon.

[CetinSert]

Thank you! I am really glad we could resolve this productively. I am sorry for my part in causing a miscommunication.

I will keep treating you and other developers making open-source contributions with utmost respect for their time and choices.

[CetinSert]

Anyways, I'm renaming this issue to clarify the problem and I'll address it soon.

Internally, extending the jump skip conditions in j.i, j.d helped with display: none, hidden and none-

Original #1107 (comment).

  const j = { // jump immediate/delayed, skip inert
    i: (t, f, s = x => x?.inert) => { const x = t[f]; return s(x) ? j.i(t[f], f) : x; },
    d: (t, f, s = x => x?.inert) => { const x = t;    return s(x) ? j.d(t[f], f) : x; },
  }; 

Extended (exclude ¬<sl-menu-item> ∨ .hidden ∨ .inert):

<link rel=stylesheet href=//cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.88/dist/themes/light.css>
<script type=module scope=sl-fix>
  import { SlMenu } from "//cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.88/dist/shoelace.js";
  const j = { // jump immediate/delayed, skip inert
    i: (t, f, s = x => x?.tagName !== 'SL-MENU-ITEM' || x?.hidden || x?.inert) => { const x = t[f]; return s(x) ? j.i(t[f], f) : x; },
    d: (t, f, s = x => x?.tagName !== 'SL-MENU-ITEM' || x?.hidden || x?.inert) => { const x = t;    return s(x) ? j.d(t[f], f) : x; },
  }; 
  SlMenu.prototype.setCurrentItem = i => { console.warn('ignore', i); };  
  SlMenu.prototype.handleKeyDown = function(e) {
    const                                t =        e.target;
    const                                a = j.d(this.firstElementChild,     'nextElementSibling');
    const                                z = j.d(this. lastElementChild, 'previousElementSibling');
    switch (e.key) {
    case 'Home':                         a                            ?.focus?.(); break;
    case 'End':                          z                            ?.focus?.(); break;
    case 'ArrowUp':   (t === a ? z : j.i(t, 'previousElementSibling'))?.focus?.(); break;
    case 'ArrowDown': (t === z ? a : j.i(t,     'nextElementSibling'))?.focus?.(); break;
    case 'Enter':                        t                             .click  (); break;
    }
  };
</script>
<script type=module src=//cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.88/dist/shoelace.js></script>

(haven't looked into detecting display: none (perhaps via getComputedStyle?, not sure about performance implications, need for caching though))

[claviska]

I don't really agree with hiding menu items as most menus shouldn't change dynamically, but inert will work to hide menu items now. Detecting visibility is a can of worms that requires accessing multiple reflow properties (because ancestors can be hidden, for example) and that's going to be performance nightmare. Those who want this will need to use inert.

I wasn't able to add all the tests I'd like due to Firefox hanging as soon as inert was present on a menu item (that and some combination of either sendKeys() or expect — not really sure why). This could have something to do with it being unsupported, but I don't have time to dive any deeper into that today.