[TrapFocus] fix: add onFocus handlers to sentinels

[PaulSavignano]

@oliviertassinari , In regards to our TrapFocus, I see two approaches to achieve the tabbable children behavior identified in #19651.  

Option 1, which is in my pr, uses the sentinels to refocus a tab onto the rootRefs children.  When the user tabs past the last tabbable child and lands on a sentinel, the sentinel's onFocus handler refocuses to the first tabbable element.  This approach removes the need for event listeners and only fires when the user lands on a sentinel.  Tabbing through the children is handled natively by the browser.

Option 2, we take full control of the tab events programmatically and focus an element by index.  This approaches requires additional considerations with radio buttons as the desired behavior with radios is to have only the group tabbable not each radio button.  Individual radio buttons should be navigated by using the arrow keys.

With either approach, It might provide value to abstract the logic into a hook.  This would allow uses to import the hook or the component.  Share your thoughts here as I would enjoy the opportunity to update the feature.  We could also roll it with typescript.

On the typescript topic, I'd like to see the component accept a tabbableElements prop so a user may provide custom els to query select.  I tried typing this prop in the index.d but received an error from the generateProptypes functions when I generated the files prop types.  Not sure why the type was { tabbableElements?: string[] } which I expect to be acceptable.   

[mui-pr-bot]

@material-ui/core: parsed: +0.22% , gzip: +0.41%

Details of bundle changes

Generated by 🚫 dangerJS against 8fa2de3

[oliviertassinari]

we take full control of the tab events programmatically and focus an element by index

@PaulSavignano I think that we should optimize for what happens in the worse case. Option 1 seems to minimize how bad it can go while staying light in the implementation.

With either approach, It might provide value to abstract the logic into a hook. This would allow uses to import the hook or the component.

I think that we can have this discussion for a follow-up pull request as it's not directly related to the problem we are going after. Regarding the value of a hook, what would a developer gain for this FocusTrap component as a hook?

On the typescript topic, I'd like to see the component accept a tabbableElements prop so a user may provide custom els to query select.

Oh yeah, interesting proposal, so that people have an escape hatch with our default query, for instance, https://github.com/davidtheclark/tabbable that might be more resilient but more bundle size demanding.

[oliviertassinari]

Another question, do we ever need to have the container tabbable?

[PaulSavignano]

what would a developer gain for this FocusTrap component as a hook?

A dev would gain the ability to use the logic on an existing component. Maybe the dev already has divs they would like to use and only require our logic. The hook could return the refs and onFocus fns as well as handle the effects. My motivation for this approach is more about an overall pattern than value specific to this component.

Regarding the tabbableElements prop, I see value in accepting this prop so the user may define their own selectors. In one of my first approaches, I found I missed the ListItem components, they were not in the tabbableEls array, so I added their attribute, role="button". There may be other attributes a user would like tabbable that we have not considered, and being able to pass them as a prop would make the component more extendable. I can look further into the generateProptypes fn if you'd prefer to see why it throws when consuming the optional string array type.

do we ever need to have the container tabbable?

Could you elaborate on the container? If you are referring to the parent the rootRef references, maybe we set a containing div on the TrapFocus itself to use the rootRef. Could a situation arise where a user is using the TrapFocus inside a parent that has other tabbable els? This scenario would result in the need to lift the TrapFocus or use a useTrapFocus hook to allow all the els to be tabbable.

[oliviertassinari]

A dev would gain the ability to use the logic on an existing component.

I think that we can resume this discussion once we have solved most of the problems with the component https://github.com/mui-org/material-ui/labels/component%3A%20TrapFocus. Take this change, we can drop the sentinels' div without making a breaking change, not true with a hook.

tabbableElements

+1

Could you elaborate on the container?

If you check the history of the component, I went with an implementation like Ant Design that uses a sentinel and a wrapping div that is tab focusable in order to minimize the bundle size of the implementation. #19651 is a legacy of this decision.

I wonder because of

But what if there is no focusable element? Which is probably why we have https://github.com/mui-org/material-ui/blob/732c45f3defe309c12ad012addacdfefdeed552b/packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js#L64

[eps1lon]

what would a developer gain for this FocusTrap component as a hook?

I tried this a couple of times to convert it to a hook. I don't like how we need to wrap a single div or rely on cloneElement.

[PaulSavignano]

Receiving a couple of errors I could use some direction on.

ci/circleci: test_browser-1:
I added @testing-library/user-event to help with simulating tab click events. This library helps move the tab focus upon tab click. It looks like a prop from this lib throwing in the ci/circleci: test_browser-1 run.

ci/circleci: test_static
This error is related to generating api docs. I added a prop, focusSelectors, to allow a user to define custom selectors for use in tab navigation. When I run yarn docs:api to update the markdown files I receive did not recognize object of type ChainExpression.

[mnajdova]

We should also consider elements that has tabindex=0 at least, shouldn't we? It may be harder with elements that have other positive values 🤔

[oliviertassinari]

What about we change the API to a getter? This would allow external users to rely on https://github.com/davidtheclark/tabbable.

[PaulSavignano]

Indeed. I like the approach of extension through tabIndex. I'll make the update.

[eps1lon]

Could you inline the code in test/utils from user-event that you used? Last time I checked the user-event package had a lot of flaws and simplifications. As a component library we need to be acutely aware of all the events fired.

[PaulSavignano]

Absolutely. Are you referring to importing userEvent from @testing-library/user-event rather than from test/utils for this file?

[eps1lon]

Just copy and paste the code into this repository that you need.

[oliviertassinari]

Which will solve the fail of the CI in the older browsers 👍

[eps1lon]

It's not obvious that we need 3 tabs. The 2 last tabs could be no-ops and the test would still pass. Maybe all 3 are. Having an assertion after each tab would improve readability.

Especially for focus behavior I'd rather have more assertions then absolutely necessary because you'll need them when working on the component later anyway.

[PaulSavignano]

True, I'll assert after each tab so we have visibility into each event.

[oliviertassinari]

It's not obvious that we need 3 tabs.

The tab() calls have no effect. I have tested without.

It seems that we need to provide the trap focus root: https://github.com/testing-library/user-event/blob/a3f751f0fdfa514ac4e6723a6dfd13c5c2f68c6d/src/tab.js#L22.

+    const focusTrap = screen.getByTestId('modal');
+    userEvent.tab({ focusTrap });
+    userEvent.tab({ focusTrap });
+    userEvent.tab({ focusTrap });
+    expect(screen.getByText('ok')).toHaveFocus();
+    userEvent.tab({ focusTrap });
+    expect(screen.getByText('x')).toHaveFocus();

[eps1lon]

This is overly simplified and I don't see how we could possible determine if an element is focusable by reading the DOM (in a performant way).

What is already missing:

• filter actually disabled elements
• filter expressly inert elements
• apply tabindex order
• filter elements that are not in sequential focus order according to the user-agent (this seems impossible)

This assumes that determining what element is tabbable is something we can do by reading the DOM. However, what elements are tabbable (in sequential focus order) can be determined by the user agent (https://html.spec.whatwg.org/multipage/interaction.html#sequentially-focusable). We can ask if an element is in sequential focus order by e.g. checking tag names or tabIndex. But returning "No" from that question does not imply that the element is not tabbable. Even if we answer yes then the user agent might still reject it (e.g. hidden elements).

[PaulSavignano]

Agreed. I'll add the check for disabled and hidden elements. I'll also review some other implementations to see if there are other attributes we should consider.

After the first, or last tabbable element is focused I am letting the browser control the sequence. I only coerce focus when the focus lands on a sentinel.

[oliviertassinari]

I don't think that we need this variable, what about?

diff --git a/packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js b/packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js
index 603eac26f..6ce1aba39 100644
--- a/packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js
+++ b/packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js
@@ -30,7 +30,6 @@ function Unstable_TrapFocus(props) {
     open,
   } = props;
   const ignoreNextEnforceFocus = React.useRef();
-  const lastEvent = React.useRef(null);
   const sentinelStart = React.useRef(null);
   const sentinelEnd = React.useRef(null);
   const nodeToRestore = React.useRef();
@@ -59,9 +58,8 @@ function Unstable_TrapFocus(props) {
     nodeToRestore.current = getDoc().activeElement;
   }

-  const onSentinelFocus = React.useCallback(() => {
+  const onSentinelFocus = React.useCallback((position) => () => {
     const focusRadios = rootRef.current.querySelectorAll('input:checked');
-    const isShiftTab = Boolean(lastEvent.current?.shiftKey && lastEvent.current?.keyCode === 9);
     const selectors = [...focusSelectorsRoot, ...focusSelectors].filter(Boolean);
     const focusChildren = rootRef.current.querySelectorAll(selectors.join(', '));
     const isFocusChildren = Boolean(focusChildren?.length);
@@ -71,7 +69,7 @@ function Unstable_TrapFocus(props) {
       focusChildren[focusChildren.length - 1].type === 'radio'
         ? focusRadios[0]
         : focusChildren[focusChildren.length - 1];
-    if (isShiftTab) {
+    if (position === 'start') {
       return focusEnd.focus();
     }
     return focusStart.focus();
@@ -142,7 +140,6 @@ function Unstable_TrapFocus(props) {
     };

     const loopFocus = (nativeEvent) => {
-      lastEvent.current = nativeEvent;
       // 9 = Tab
       if (disableEnforceFocus || !isEnabled() || nativeEvent.keyCode !== 9) {
         return;
@@ -209,9 +206,9 @@ function Unstable_TrapFocus(props) {

   return (
     <React.Fragment>
-      <div onFocus={onSentinelFocus} tabIndex={0} ref={sentinelStart} data-test="sentinelStart" />
+      <div onFocus={onSentinelFocus('start')} tabIndex={0} ref={sentinelStart} data-test="sentinelStart" />
       {React.cloneElement(children, { ref: handleRef, onFocus })}
-      <div onFocus={onSentinelFocus} tabIndex={0} ref={sentinelEnd} data-test="sentinelEnd" />
+      <div onFocus={onSentinelFocus('end')} tabIndex={0} ref={sentinelEnd} data-test="sentinelEnd" />
     </React.Fragment>
   );
 }

It fails some of the tests which surface how we could better write them :)

[PaulSavignano]

Nicely done! I'll make the update.

[gregnb]

@PaulSavignano are you still able to continue this effort? i have a project that would really depend on this fix. appreciate all this work you've done so far!

[oliviertassinari]

@gregnb We will need to reset the effort

[gregnb]

@oliviertassinari because this is too out of sync? or the approach?

[oliviertassinari]

You can read on the thread to learn more about the approaches explored, I think that you will find valuable information. I'm closing because it's unlikely that this pull request will move forward. Better be upfront with the community. It's up for grab.