bug(modal): the first press of shift + tab does not focus an element in the modal, focus is not trapped

[ghost]

https://stackblitz.com/angular/ygpjvdbxbre

1. Launch demo modal
2. Press Tab - first element in the modal container gets the focus - focus is trapped ✔
3. Keep pressing Tab - focus is looped ✔
4. Press Shift + Tab - focus is going to a previous element, focus is still trapped ✔

---

1. Launch demo modal
2. Press Shift + Tab - element outside of the modal container gets the focus - focus is not trapped ❌

P.S. it works fine in Datepicker in a popup.

1. Click on the calendar icon
2. Press Shift + Tab - focus is trapped and looped ✔

[benouat]

Trying to think about it, it does not sound like a bug to me ...

---

Here are my thoughts about it:

The library provides a way to open anything in a modal window, then it's up to people to implement what to be focused first when modal is open.

The fact that it is working with Tab is for me a nice side-effect but it is not meant to be a feature.

[ghost]

@benouat I don't understand. How come Tab is "a side-effect, not a feature", if focus trapping is intentional? (ngbFocusTrap is used for this)

---

by the way, when you call Window.confirm(), Window.prompt() or Window.alert() the focus is completely trapped inside the native modal dialog (at least in Google Chrome)

[benouat]

EDIT: the github @ suggestion fooled me

@maxokorokov @Maxkorz my point is that from an API stand point, user should always specify/manage themselves what to be focused when modal is opened. I mean it should be mandatory somehow. They should never open a modal without handling that.

The fact that if you do open a modal without specifying an element to be focused is working when first pressing Tab is the side effect to me

---

Ideally, the html5 autofocus attribute should be used here, expect that it is not supported in dynamically created content...

💡Maybe we should implement a kind of NgbAutofocus as a directive hosted on [autofocus] that will perform the focus imperatively on next tick.

[ghost]

@benouat

The fact that if you do open a modal without specifying an element to be focused is working when first pressing Tab is the side effect to me

I think you're wrong.
As I understand, the modal window element is explicitly focused in ngAfterViewInit https://github.com/ng-bootstrap/ng-bootstrap/blob/master/src/modal/modal-window.ts#L63
And there's a test that checks that when a modal window is opened, the 'ngb-modal-window' should get the focus https://github.com/ng-bootstrap/ng-bootstrap/blob/master/src/modal/modal.spec.ts#L663

user should always specify/manage themselves what to be focused when modal is opened. They should never open a modal without handling that.

as a user of ng-bootstrap, I would not prefer to do that. I have over 30 modals in my project and it would be such a pain to manage the focus for each modal.

Ideally, the html5 autofocus attribute should be used here
Maybe we should implement a kind of NgbAutofocus as a directive hosted on [autofocus] that will perform the focus imperatively on next tick.

I don't think that this is a good idea. According to https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#attr-autofocus

Only one element in a document can have the autofocus attribute. It cannot be applied if the type attribute is hidden, because hidden inputs cannot be focused.

Automatically focusing a form control can confuse visually-impaired people who using screen-reading technology. When autofocus is assigned, screen-readers "teleport" their user to the form control without warning them beforehand.

[benouat]

@Maxkorz visually-impaired or not, we do want our user to be "teleported" into the content of the modal.

I still think the directive is a good idea, BTW material implemented it for their accessible focus management

[ghost]

@benouat I just checked, ngbFocusTrap works fine in the datepicker:

https://stackblitz.com/angular/nvpvpagxmoxo

1. Click on the calendar icon
2. Press Shift + Tab - focus is trapped and looped ✔

(I updated my first message)

[benouat]

yes it is working in the datepicker because of what I described:

my point is that from an API stand point, user should always specify/manage themselves what to be focused when modal is opened

In this case, it is not the user, but the datepicker itself that will focus the selected date when opened. Something is then initially focused, ngbFocusTrap can work properly.

Again, issue for modal is that if you don't focus something when it is opened, ngbFocusTrap won't work properly.

[ghost]

@benouat I still think that the issue is in the ngbFocusTrap.
ngbFocusTrap is described as Function that enforces browser focus to be trapped inside a DOM element.. Which does not happen for modals.

Small fix in ngbFocusTrap fixes the issue

....
      .subscribe(([tabEvent, focusedElement]) => {
        const[first, last] = getFocusableBoundaryElements(element);

        // trap focus on the first press of Shift + Tab
        if (element === tabEvent.target && tabEvent.shiftKey) {
          last.focus();
          tabEvent.preventDefault();
        }
...

What do you think?

[benouat]

I am fine with this change, but if we decide to handle the case of elementon which ngbFocusTrap has been activated with Shift+Tab we must also handle the other one with just Tab.

I know that handling the other one with just Tab is already working (because this is the default browser behaviour), but for consistency, if we implement one, we have to implement the other one that will just override the default behaviour

Something a bit like that :

if ((focusedElement === first || focusedElement === element) && tabEvent.shiftKey) {
  last.focus();
  tabEvent.preventDefault();
}

if ((focusedElement === last || focusedElement === element) && !tabEvent.shiftKey) {
  first.focus();
  tabEvent.preventDefault();
}

---

Nonetheless, documentation should clearly state that when using modal, user should take care (in a kind of mandatory way) of focusing something inside the modal.

[ghost]

I know that handling the other one with just Tab is already working (because this is the default browser behaviour), but for consistency, if we implement one, we have to implement the other one that will just override the default behaviour

I agree 👍

---

when using modal, user should take care (in a kind of mandatory way) of focusing something inside the modal.

I strongly disagree 👎 😃

[pkozlowski-opensource]

Nonetheless, documentation should clearly state that when using modal, user should take care (in a kind of mandatory way) of focusing something inside the modal.

We could strongly suggest this but IMO modal should have sensible defaults (ex. first focusable element or modal window itself (as today).

[ghost]

@benouat can you open a PR with your solution?

[benouat]

@Maxkorz yes sure, i will !

[mendeza]

Hi Folks,
My team at Thomson Reuters has just retained an Accessibility testing expert who spent years working on Government projects. I asked him to look at this issue. His feedback:

Once inside the Modal, focus must be trapped; SHIFT-TAB immediately after entry would give focus to the last focusable item at the bottom/end of the Modal (wrap-around).

Once the Modal is spawned, focus must go to the first actionable item - presumably the close (X) button. FYI, arbitrarily focusing some element in the middle of content could be troublesome to exit if the Modal was entered by error.

A suggestion he made is to set the aria-describedby attribute on the close button and reference the ID of the modal title. I realize that's a problematic thing to enforce, though you could show examples in your demos. What makes this a bit better than the aria-label="Close" that you currently use, is it provides more context to users, so that they can be exactly sure of what they are closing when focus lands on the close button.

I hope this is helpful.

[CBly249013]

Something like

< div>< h1 id="M1">Modal title< /h1>
< span ARIA-label="close" ARIA-describedby="M1"> x< /span>
< /div>

Or if ARIA-describedby can't be used

< div>< h1>Modal title
< span ARIA-label="close modal title"> x< /span>
< /div>

[benouat]

Thanks for you input @mendeza ! Much appreciated here

Now after a few days step back, let me try to explain why I am reluctant to perform the change I described and we discussed previously in this issue using your comments:

Once inside the Modal, focus must be trapped;

We are fine here, NgbFocusTrap is doing that today. Important words here are "Once inside".

Once the Modal is spawned, focus must go to the first actionable item

This is what we thought would be enough by focusing the modal window (@pkozlowski-opensource we don't do first focusable). It is apparently not enough and should be changed (disclaimer, it is not a framework only task, ng-bootstrap user must also be involved)

1. This is why I have been repeating myself from the beginning of this issue, as a framework, we can't (or we could, but don't) know what the modal content contains, so to make sure focus is going somewhere (important words here "focus must go") users should take responsibility to focus themselves something (ie going inside).
   Then NgbFocusTrap will do its job, Shift+Tab will work as expected.

2. To help users doing that properly, this is why I suggested to introduce an NgbAutofocus directive that will do that: simply set the focus to the first actionable item, defined by the user using the directive, as soon as the modal is opened/shown

[ghost]

@benouat Lets say you add the NgbAutofocus directive. But you can't force a user to use it (I won't use it for sure). And the bug will not be fixed. Once the focus is trapped, it shouldn't be possible to focus an element outside of the trapped element. Once the modal is opened, you shouldn't be able to focus an element outside of the modal window. But you can do that with Shift + Tab. And that is a bug that can be fixed with 10 lines of code.

You saw the bug in the demo. You can focus an element "behind" the modal window. You can't blame a user for that.

[pkozlowski-opensource]

IMO we should do the following:

• add a NgbAutofocus directive so people can indicate an element inside modal to be auto-focused;
• if the NgbAutofocus directive is not used we should focus first focusable element.

"we should focus first focusable element" will be painful to do, though, as it will require lots of low-level DOM querying.

[maxokorokov]

As everybody is commenting this, I'll leave my 2 cents too :)

• add the NgbAutoFocus directive
• optionally focus first focusable element, if user desires to do so. I wouldn't try focusing first element by default, because I have absolutely no idea what the modal content might be.

Something like:

modalService.open(content, {focusFirst: true})

This issue is not a bug to me, because ONCE anything is focused in the popup, focus is trapped. There is nothing focused initially.

I prefer this to be closed as wontfix and open a separate feature request with clear desired API.

---

UPDATE: actually on reflection inversed version with {focusFirst: false} might be better

[maxokorokov]

Opened #2728

[peterblazejewicz]

Hi folks,
I'm under impression that SHIFT + TAB allows me to quickly start from the bottom most interactive control and ease navigation to the option of my choice, simply by going backward (I hit the MacOS about this mac and start backward navigation with no no fuss).
the problem is that implementation of the focus trap seems to expect users to start with Tab, while disregarding Tab + SHIFT option.
I'd like to see a code, like one discussed above being, added:
#2718 (comment)
thanks!

ref:
twbs/bootstrap#25277 (comment)
https://www.w3.org/TR/wai-aria-practices-1.1/#dialog_modal

React strap working impl:
https://github.com/reactstrap/reactstrap/blob/master/src/Modal.js#L218-L224

[benouat]

@peterblazejewicz here are a few highlight

the problem is that implementation of the focus trap seems to expect users to start with Tab, while disregarding Tab + SHIFT option

This is a wrong assumption. Our focus-trap implementation works great with both Tab and Shift+Tab once and only once the focus is locted anywhere inside (not inclusive) the element that host the focus trap.

---

@Maxkorz @peterblazejewicz You both seem to be against the usage of this [ngbAutofocus] feature we are about to introduce in #2728, so

Let me try one more time to explain and justify our choices regarding this entire story

To us, opening a dialog/modal (call it the name you want) is no different that open a new page/document inside a browser tab. When you initially navigate to a new page, browsers vendor won't take any assumption on what to be focused specifically, hence focus end up on document body. This is why HTML specs provide an [autofocus] attribute for the user to specify an element.

We decided to follow the exact same path here in ng-bootstrap. Opening a modal won't focus anything special by default, hence our initial implementation giving focus to the modal window. Then we introduce [ngbAutofocus] for people to let us know what to focus on open.

Then, lat point, regarding focus trap, open a page in a browser, and using Shift+Tab won't cycle trough the document. User will end up inside the browser chrome. Here also we follow the same path.

I know it might not suit everyone, but at least we are consistent.