buttons/inputs with the aria-disabled attribute should still be considered focusable

[tannerlyons]

Hey @theKashey, thanks for all your hard work on the focus-lock family!

We recently upgraded to [email protected] (uses [email protected]) and see the following issue with focus-lock:

Background

This commit introduced some code that seems to assume buttons and inputs with the aria-disabled attribute are hidden.

export const notHiddenInput = (node: Element): boolean =>
-  !((isHTMLInputElement(node) || isHTMLInputElement(node)) && (node.type === 'hidden' || node.disabled));
+  !((isHTMLInputElement(node) || isHTMLButtonElement(node)) && (node.type === 'hidden' || node.disabled)) &&
+  // @ts-ignore
+  !node.ariaDisabled;

Note: node.ariaDisabled => undefined | "true" | "false"

Problem

In our case, we have modals somewhat like this:

<ReactFocusLock>
    <div role="dialog">
        <p>Some message</p>
        <button aria-disabled="false">Cancel</button>
        <button aria-disabled="false">OK</button>
    </div>
</ReactFocusLock>

Even though aria-disabled="false", focus-lock considers them "hidden" and react-focus-lock won't keep focus within the modal. Even if aria-disabled is set to true, they are still focusable and should be not be considered "hidden".

aria-disabled vs disabled

tldr: as you probably know, the disabled attribute hides the element from assistive devices and disallows focus. The ARIA spec intentionally allows focus on elements with aria-disabled set to true OR false.

Sources

focus-lock's behavior seems to be incorrect per the ARIA spec. Below in this snippet from the spec, "perceivable" means focusable for the purposes of a user using the keyboard to navigate the web page:

[aria-disabled] indicates that the element is perceivable but disabled.

MDN does a pretty good job of summarizing the pitfalls of the disabled attribute and why the aria-disabled attribute should be used in most cases to allow focus:

To semantically disable an element without removing it from focus order or from the accessibility tree, set aria-disabled="true" on the element.

and later:

The disabled Boolean attribute provides CSS styles and semantics and removes the ability to click or focus while not disabling hover. By removing the ability to focus and removing it from the accessibility tree, it makes it invisible to assistive technology users. For good user experience, you want to make sure everyone can access all the visible content, no matter how they access the web. It is important to be aware that using the disabled attribute can harm usability.

Bug reproduction

The following link shows that [email protected] won't stick focus into an element containing

https://codesandbox.io/s/focus-lock-aria-disabled-bug-repro-cgt0ux?file=/src/index.js

Proposed solution

I am happy to open a PR to show the fix, but in my mind this is just undoing the commit listed above. I looked around here and in react-focus-lock to see if I could figure out why you added the node.ariaDisabled check but I couldn't find any context on it, sorry!

[tannerlyons]

Proposed solution:

I renamed notHiddenInput to isHiddenInput to match the semantics in is.ts. If you'd like, I can constrain the fix to is.ts and not rename the function.

This diff is based off v0.10.2, not master.

tlyons:~/git/focus-lock ((v0.10.2) *)$ git diff src/
diff --git a/src/utils/DOMutils.ts b/src/utils/DOMutils.ts
index 6cd7960..b234a6b 100644
--- a/src/utils/DOMutils.ts
+++ b/src/utils/DOMutils.ts
@@ -1,12 +1,12 @@
 import { toArray } from './array';
-import { isVisibleCached, notHiddenInput, VisibilityCache } from './is';
+import { isVisibleCached, isHiddenInput, VisibilityCache } from './is';
 import { NodeIndex, orderByTabIndex } from './tabOrder';
 import { getFocusables, getParentAutofocusables } from './tabUtils';
 
 export const filterFocusable = (nodes: HTMLElement[], visibilityCache: VisibilityCache): HTMLElement[] =>
   toArray(nodes)
     .filter((node) => isVisibleCached(visibilityCache, node))
-    .filter((node) => notHiddenInput(node));
+    .filter((node) => !isHiddenInput(node));
 
 /**
  * only tabbable ones
diff --git a/src/utils/is.ts b/src/utils/is.ts
index 2422822..fd45be1 100644
--- a/src/utils/is.ts
+++ b/src/utils/is.ts
@@ -57,10 +57,8 @@ export const isHTMLInputElement = (node: Element): node is HTMLInputElement => n
 export const isRadioElement = (node: Element): node is HTMLInputElement =>
   isHTMLInputElement(node) && node.type === 'radio';
 
-export const notHiddenInput = (node: Element): boolean =>
-  !((isHTMLInputElement(node) || isHTMLButtonElement(node)) && (node.type === 'hidden' || node.disabled)) &&
-  // @ts-ignore
-  !node.ariaDisabled;
+export const isHiddenInput = (node: Element): boolean =>
+  (isHTMLInputElement(node) || isHTMLButtonElement(node)) && (node.type === 'hidden' || node.disabled);
 export const isGuard = (node: Element | undefined): boolean => Boolean(node && getDataset(node)?.focusGuard);
 export const isNotAGuard = (node: Element | undefined): boolean => !isGuard(node);

[theKashey]

🙄 I do remember writing this code, and thinking about supporting role=button explicitly, but I don't remember why I decided to prevent aria-hidden elements from being autofocusable.
We all sometimes doing something without second thought. However direct commit to master sounds like I was pretty sure on what I am doing...

🤷‍♂️ I will revert the change. It was working perfectly without it :)

[tannerlyons]

We've all been there! I consistently wish I could ask past Tanner what he was up to.

Thanks for looking at this so quickly! Let me know if I can help!

[theKashey]

Fixed at [email protected]