Modal itself receiving focus rather than first tabbable element

[getdave]

The standard Modal component defines a tabindex which forces it to be focused when mounted:

gutenberg/packages/components/src/modal/index.tsx

Line 232 in 8e0fdca

tabIndex={ -1 }

As this is non-configurable it appears to be a deliberate decision but one that may be questionable from an a11y perspective as noted by @andrewhayward (see below).

Raising this Issue to discuss whether this attribute needs to be reconsidered or made configurable.

What is the rationale behind it and what arguments are there for/against removing it?

---

For what it's worth, from an accessibility perspective at least, it's actually strongly advised that focus is moved to a control within the dialog, rather than keeping focus on the dialog itself. In this case, where the sole purpose of the dialog is to update the text value, the text field would be a good candidate to automatically receive focus here.

Originally posted by @andrewhayward in #53735 (comment)

[getdave]

Pinging @ciampo for input from a components maintenance perspective and also @alexstine from a a11y and UX standpoint.

[alexstine]

This is one of those highly subjective accessibility topics which has a lot of requirements.

1. If the modal has a title and description, you must communicate those with aria-label or aria-labelledby and aria-describedby. I think we've got this covered for the most part.
2. Placing focus on a button can be a worse experience because it could be Cancel, it could be Confirm, and this could lead to unexpected actions. Also, if the requirements above are not met, for example, the description text is not read, this could cause users to navigate backwards to get the full context they need.
3. Focus in a field is actually okay as long as number 1 is fully covered and the field contains a label.

While I think it is okay to allow developers to manage focus differently, I don't think it should be a default to focus a control inside the modal.

Thanks.

[getdave]

Ok so this comes down to best practices and developer education (myself included).

So in the specific example of #53735 we should focus the input as all the criteria are met. However in other examples this might not be desirable.

So shall we close this one out?

Actually I believe we need to update the Modal component so that we can selectively choose to remove the tabindex=-1 and allow focusing the first field instead. implement custom focusOnMount behaviour and choose which element to be focused. Currently passing firstElement will focus the Close button because it's first focusable in the DOM within the Modal.

[alexstine]

@getdave Correct.

1. Disable auto focus.
2. Allow a ref to be passed to the input field for children of the modal. Not sure how you do that in practice, but it makes sense to me.

[getdave]

Some ideas for changes to the focusOnMount API to allow developers to select elements other than the first tabbable element:

Pass an index

Pass an index which represents which of the found tabble focusable elements within the Modal node should be focused.

<Modal
    { ...otherProps }
    focusOnMount={ 1 } // select the 2nd element
>

Pass an function

Define a function which is passed all the focusable nodes allowing you to select one:

<Modal
    { ...otherProps }
    focusOnMount={ ( focusableNodes ) => {
        return focusableNodes[ 1 ];
    } }
>

[andrewhayward]

Sorry, yes, my recommendation should have come with the caveats @alexstine provided here!

I wonder whether, rather than try to fudge something into the <Modal> itself, we could just make use of the native autofocus attribute instead.

<Modal { ... }>
  <InputControl { ... } autoFocus />
  <Button { ... }>Close Modal</Button>
</Modal>

I tried it locally, and it seems to work. Initially at least – it messes with focus management when closing the dialog, but our energy might be better spent resolving that than reinventing this particular wheel. Just a thought.

Autofocus.breaking.tab.flow.on.dialog.close.mov

[alexstine]

I think autofocus should work fine. The tab index on the wrapper shouldn't prevent this from working.

[ciampo]

@getdave would the suggested autofocus prop suit your needs?

In case it doesn't, and you'd be still interested in making changes to useFocusOnMount, here's my thoughts:

• I'm not a fan of passing an index, since it would be a bit too convoluted to understand
• passing a callback function could work
• an alternative could also be to pass an id or a CSS selector — in that case, the hook could look for the first node matching that id or selector, and focus that node if found

What do you think?

[getdave]

Thanks everyone. That's all really helpful.

an alternative could also be to pass an id or a CSS selector — in that case, the hook could look for the first node matching that id or selector, and focus that node if found

What I'd like to avoid is people selecting non-tabbable/focusable elements. If we use the callback approach the callback would receive an array of tabbable elements meaning that the consumer would need to pick from a list that is pre-filtered for a11y compliance.

If we allow access to entire DOM then it could lead to folks introducing a11y issues.

@getdave would the suggested autofocus prop suit your needs?

I seem to recall this property doesn't fit the React paradigm that well. I'm going to see if I can dig up some concrete rationale for that now and report back here.

[getdave]

autofocus is easy to use but only works when the is initially rendered; since React intelligently only re-renders elements that have changed, the autofocus attribute isn't reliable in all cases.

This article seems to suggest that autofocus may not always be unreliable.

I found out that React actively manages the attribute to cover up some browser inconsistencies in how it's handled. It does this by calling .focus() manually on the node. Sometimes if the node is rendered but not in the DOM focus is still called but the focus won't be transfered as you'd expect.

In short most advice I could find seemed to point to actively managing via a ref as we do currently. Certainly trying to do both at the same time would not be a good plan.

[youknowriad]

I personally like the current focusOnMount props we have and their consistency between the two dialog components we have (Popover and Modal). I'd suggest that maybe the use-case here is that the Modal renders a "close" button and that close button seem to be a special case.

In other words maybe focusOnMount=firstElement for a Modal component doesn't really mean "focus close button" but more "focus the first element within the modal content" (by modal content I mean the "children" of the Modal).

[getdave]

Looking at this, it seems it will probably require moving the Close button outside of the element which receives the ref for focusOnMount on Modal. However would would need to keep it inside the element receiving the ref for the focus trap. So it's a delicate balance.

We could explore this route and leave existing APIs untouched though. What do you think @ciampo?

[ciampo]

Looking at this, it seems it will probably require moving the Close button outside of the element which receives the ref for focusOnMount on Modal. However would would need to keep it inside the element receiving the ref for the focus trap. So it's a delicate balance.

I'm not sure that's feasible, since the element receiving the focusOnMount ref is also the same element receiving the constrainedTabbing ref:

gutenberg/packages/components/src/modal/index.tsx

Lines 223 to 227 in ebae2d5

	ref={ useMergeRefs( [
	constrainedTabbingRef,
	focusReturnRef,
	focusOnMountRef,
	] ) }

I thought about moving the close button after the Modal's content (in terms of DOM order), but that would just partially resolve the problem, since any other tabbable element in the Modal's header would receive focus. Would that be an acceptable solution?

Otherwise, I think that the cleanest way to improve this is to augment the useFocusOnMount to get some sort of callback as suggested above.

[andrewhayward]

I don't know much about how focusOnMount works, but if autoFocus isn't appropriate, could we change focusOnMount to also accept an explicit ref? I don't think it does that currently, but I'm happy to be corrected!

[getdave]

@andrewhayward That's probably an option. Please also see these other suggested routes for enhancing focusOnMount.

I believe it's essential that we only allow consumers of the API to select nodes that are tabbable and exposing overly flexible APIs can lead to problems.

[andrewhayward]

This would require verification, but as far as I know calling focus on a non-focusable DOM node is a no-op, and doesn't change the active element. So it wouldn't be detrimental to the user experience particularly. Ultimately though I don't have any real preference, and this is very much just a suggestion.

That being said, as an API consumer, while preventing the API from receiving non-focusable nodes is definitely a good thing to aim for, I'd also want to see that balanced against the complexity of the API itself.

[getdave]

So it wouldn't be detrimental to the user experience particularly.

With respect I think it could. Let's say I'm "some developer" and I've had an a11y audit which says the modal must focus an element on mount. However, i'm not well versed in a11y so I use the API to attempt to place focus on a <p> paragraph text. All seems well. I don't bother to test and I move on. The issue now is that I've been allowed to make an inaccessible experience.

I'd also want to see that balanced against the complexity of the API itself.

I agree. Could you explain what part of the proposed callback API are feeling complex? How does that compare to an API which allows you to select any element?

To be clear, I'm really grateful for you input. I'm genuinely curious to get wider perspectives. I'm not sure I have the right answer here so I'm keen to stress test my assumptions. Your help is appreciated 🙇

[alexstine]

Agree with @getdave . Developers can and have tried to use focus on nodes that can't receive focus. It has to be checked. Might want to check some of the utility functions in wordpress/dom. For example, focus does not always mean tabbable so the functions are split. You can focus an element with tabindex="-1" but you can't tab to it.

[draganescu]

I'm not a fan of passing an index, since it would be a bit too convoluted to understand

@ciampo what is convoluted in allowing to select an index from the list of tabbable elements? It seems consistent with the way browsers treat this whole focus business.

[ciampo]

what is convoluted in allowing to select an index from the list of tabbable elements? It seems consistent with the way browsers treat this whole focus business.

I think that something like

<Modal
  focusOnMount={ 4 }
  { ...props }
>
  { /* content */ }
</Modal>

Can be really mysterious and difficult to comprehend to any developer. To know what element is being focused, you'd need to inspect the modal's content, know exactly what are the focusable elements, and try your luck.
Furthermore, if the modal's internal DOM structure changes (or even the modal's content), there's a high chance that it will introduce a regression on which element gets focused.

Finally, despite the fact that the tabindex attribute supports any arbitrary int number, it is generally discouraged to use any value different from -1 and 0 (this is also explained on the MDN page linked above).

Instead, I think that passing a ref (as suggested by @andrewhayward ) or a callback function (as suggested by @getdave ) is a much more robust and easy-to-read option:

{ /* With a ref */ }
<Modal
  focusOnMount={ buttonToFocusOnMountRef }
  { ...props }
>
  { /* content */ }
  <button ref={ buttonToFocusOnMountRef }
</Modal>

{ /* With a callback */ }
<Modal
  focusOnMount={ ( tabbables ) => {
    // Literally any logic which returns either a focusable element, or undefined
    return tabbables.filter( (t) => t.id === 'button-to-focus' )
  } }
  { ...props }
>
  { /* content */ }
</Modal>

[andrewhayward]

So it wouldn't be detrimental to the user experience particularly.

With respect I think it could. Let's say I'm "some developer" and I've had an a11y audit which says the modal must focus an element on mount. However, i'm not well versed in a11y so I use the API to attempt to place focus on a <p> paragraph text. All seems well. I don't bother to test and I move on. The issue now is that I've been allowed to make an inaccessible experience.

Sorry, wasn't clear. I don't think that we should necessarily take Some Developer's word for what is or is not accessible. For example, if they provide a ref to a node that can't be focused, we could just continue to focus the modal itself, as we do now, and issue a warning.

I'd also want to see that balanced against the complexity of the API itself.

I agree. Could you explain what part of the proposed callback API are feeling complex? How does that compare to an API which allows you to select any element?

I don't have any strong feelings on the matter – just seems easier and more React-y to stick a ref on something and pass it along, than to iterate through an array 🤷

[draganescu]

I think @ciampo 's explanation on preffered API is perfec and I support the callback approach since it gives a developer the chance to build more maintainable code.

if the modal's internal DOM structure changes (or even the modal's content), there's a high chance that it will introduce a regression on which element gets focused.

This part seems unavoidable if we have an api that allows focus to be controlled from outside the modal itself. A test would take care of keeping this in check.

[getdave]

Here is a PR to make Modals ignore the Close button.

[afercia]

Reopening for reconsideration.

it's actually strongly advised that focus is moved to a control within the dialog, rather than keeping focus on the dialog itself.

The whatwg / html specification for the native <dialog> element is going in a different direction. For more details, please see this comment: #54590 (comment)

[ciampo]

As explained in #54590 (comment), this issue was about adding one additional choice for consumers of the component regarding focus handling on mount. The component's default behaviour is still to focus the div[role="dialog"] on mount, which is what the spec that you refer to recommends.

Given the above, I think that we can close this issue.

[alexstine]

Yes, the default will always be to focus the opened dialog. This simply allows us to place focus if developers feel there is a need to change it. I disagree with the official spec if this is now considered invalid. It makes little sense to not focus an input for example if you already have aria-labelledby and aria-describedby properly representing the content a user would pass on the way to the input field. In this situation, screen reader would read the modal title, description of the modal, and then read the label for the input field focus is now in. I do not recommend this pattern for every modal which is why we've made this developers choice.

[draganescu]

Fixed by #54590