Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modal: Elements with tabindex="-1" get focused #2884

Closed
Sixtisam opened this issue Nov 21, 2018 · 2 comments
Closed

Modal: Elements with tabindex="-1" get focused #2884

Sixtisam opened this issue Nov 21, 2018 · 2 comments

Comments

@Sixtisam
Copy link

Sixtisam commented Nov 21, 2018

When opening a modal, the documentation says the first focusable element is focused (if there is no ngbAutofocus marker). But when I assign tabindex="-1" to the first focusable element, it is still focused.

Link to StackBlitz reproduction: https://stackblitz.com/edit/angular-aoqoet
The expection here is, that instead of the close button, the date field gets focused. But if you check with document.activeElement, the close button is still focused

Versions of Angular, ng-bootstrap and Bootstrap:

Angular: 6.1.9

ng-bootstrap: 3.3.0, but is not yet fixed in master branch

Bootstrap: 4.1.3

I inspected the code and found the place, where the bug occurs:
File: src/util/focus-trap.ts
Line: 6-9

Today it is
const FOCUSABLE_ELEMENTS_SELECTOR = [ 'a[href]', 'button:not([disabled])', 'input:not([disabled]):not([type="hidden"])', 'select:not([disabled])', 'textarea:not([disabled])', '[contenteditable]', '[tabindex]:not([tabindex="-1"])' ].join(', ');

should be like this (note the :not([tabindex="-1"])) after each selector

var FOCUSABLE_ELEMENTS_SELECTOR = [ 'a[href]:not([tabindex="-1"])', 'button:not([disabled]):not([tabindex="-1"])', 'input:not([disabled]):not([type="hidden"]):not([tabindex="-1"])', 'select:not([disabled]):not([tabindex="-1"])', 'textarea:not([disabled]):not([tabindex="-1"])', '[contenteditable]:not([tabindex="-1"])', '[tabindex]:not([tabindex="-1"])' ].join(', ');

@pkozlowski-opensource
Copy link
Member

@benouat would you mind having a look at this one? I'm not sure if we can just add tabindex="-1" as suggested by @Sixtisam since it could be set dynamically (unless the tabindex property is reflected as an attribute, of course).

@benouat
Copy link
Member

benouat commented Nov 22, 2018

I will have a look asap. I am not in favour of hardcoding the tabindex="-1" everywhere. What I will do, or anyone willing to open a PR for that, is to perform a check right after executing the querySelectorAll to filter out the returned collection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants