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

Controls below the angular material dialog overlay still can be focused by keyboard #13054

Open
new-mikha opened this issue Sep 10, 2018 · 11 comments
Assignees
Labels
Accessibility This issue is related to accessibility (a11y) area: cdk/overlay area: material/dialog P4 A relatively minor issue that is not relevant to core functions

Comments

@new-mikha
Copy link

new-mikha commented Sep 10, 2018

Bug, feature request, or proposal:

Bug

What is the expected behavior?

The dialog should be completely modal.

What is the current behavior?

Controls that are below in z-order of the Angular Material dialog overlay still can be focused by keyboard.

What are the steps to reproduce?

  • Go to the stock Angular Material Dialog example: https://material.angular.io/components/dialog/examples
  • Close the Dev Tools if it been opened earlier (this will just allow less TABs to press later, see below)
  • Click on the Pick One button, the dialog pops up
  • Click the browser address tab (Chrome on Windows been used for test)
  • Start pressing TAB on the keyboard - you will notice that controls underneath the dialog overlay are getting selected, with all keyboard interaction enabled for the selected control. E.g. it's possible to enter new values into text boxes, press buttons etc.

Out of curiosity I also checked Bootstrap Modal - it also got similar problem, but for that one Shift-TAB needs to be pressed to reproduce.

As a suggestion - I understand that the dialog overlay is currently based on cdkTrapFocus directive. This one is suffering from the same problem, I presume it's exactly same problem, so it would be nice to fix it on the level of cdkTrapFocus - because the focus trap is quite useful thing.

image

@josephperrott josephperrott self-assigned this Sep 10, 2018
@josephperrott josephperrott added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Sep 10, 2018
@josephperrott josephperrott removed their assignment Sep 10, 2018
@josephperrott josephperrott added the Accessibility This issue is related to accessibility (a11y) label Sep 10, 2018
@jelbourn
Copy link
Member

The solution on this one is to change the way we do focus trapping. Instead of catching focus and redirecting, it needs to effectively disable everything on the page. This is something already on our backlog.

@jelbourn jelbourn added P4 A relatively minor issue that is not relevant to core functions and removed P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Oct 26, 2018
@swinkel17
Copy link

@jelbourn do you guys have any idea when this bug may be fixed? I'm using it for a project that needs to go live in a couple months and this is an accessibility concern

@jpzwarte
Copy link
Contributor

jpzwarte commented Feb 1, 2019

I cannot seem to reproduce this issue with MatDialog. I can however reproduce it with CdkDialog. The main difference afaics is the moment when the focusTrap is created. For CdkDialog, this is during class instantiation. For MatDialog, this is delayed until the enter animation is done. Perhaps this has something to do with the bug in CdkDialog? cc @crisbeto

@calebegg
Copy link
Member

I can still reproduce this, just following the steps in the original report.

I don't think you need to disable everything on the page; https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html doesn't do that, and is still effective at trapping focus. The trick seems to be catching the 'focus' event on document

@jelbourn
Copy link
Member

@calebegg Yeah, that would be a good improvement to the existing implementation for now.

@crisbeto what do you think? It should be pretty straightforward to add a focus listener to body while a focus trap is active and redirect back into the region

@crisbeto
Copy link
Member

@jelbourn How would we handle the case when there are multiple focus traps on the same page which would be competing for the focus?

@jelbourn
Copy link
Member

We'd probably want to do the most recently focused one.

vanessanschmitt added a commit to vanessanschmitt/material2-vschmitt that referenced this issue Dec 2, 2019
Note that much of this demo page is known to not be working with the
current focus trap implementation (ex. elements in the focus trap with
tabindex > 0 send focus out of the trap, elements in iframes/shadow DOM
get skipped when focus wraps, all focus traps can be escaped by clicking
on the URL and then pressing tab). The demo page will be helpful for
research into FocusTrap improvements (see
angular#13054).
vanessanschmitt added a commit to vanessanschmitt/material2-vschmitt that referenced this issue Dec 2, 2019
Note that much of this demo page is known to not be working with the
current focus trap implementation (ex. elements in the focus trap with
tabindex > 0 send focus out of the trap, elements in iframes/shadow DOM
get skipped when focus wraps, all focus traps can be escaped by clicking
on the URL and then pressing tab). The demo page will be helpful for
research into FocusTrap improvements (see
angular#13054).
vanessanschmitt added a commit to vanessanschmitt/material2-vschmitt that referenced this issue Dec 2, 2019
Note that much of this demo page is known to not be working with the
current focus trap implementation (ex. elements in the focus trap with
tabindex > 0 send focus out of the trap, elements in iframes/shadow DOM
get skipped when focus wraps, all focus traps can be escaped by clicking
on the URL and then pressing tab). The demo page will be helpful for
research into FocusTrap improvements (see
angular#13054).
vanessanschmitt added a commit to vanessanschmitt/material2-vschmitt that referenced this issue Dec 2, 2019
Note that much of this demo page is known to not be working with the
current focus trap implementation (ex. elements in the focus trap with
tabindex > 0 send focus out of the trap, elements in iframes/shadow DOM
get skipped when focus wraps, all focus traps can be escaped by clicking
on the URL and then pressing tab). The demo page will be helpful for
research into FocusTrap improvements (see
angular#13054).
vanessanschmitt added a commit to vanessanschmitt/material2-vschmitt that referenced this issue Dec 3, 2019
Note that much of this demo page is known to not be working with the
current focus trap implementation (ex. elements in the focus trap with
tabindex > 0 send focus out of the trap, elements in iframes/shadow DOM
get skipped when focus wraps, all focus traps can be escaped by clicking
on the URL and then pressing tab). The demo page will be helpful for
research into FocusTrap improvements (see
angular#13054).
vanessanschmitt added a commit to vanessanschmitt/material2-vschmitt that referenced this issue Dec 3, 2019
Note that much of this demo page is known to not be working with the
current focus trap implementation (ex. elements in the focus trap with
tabindex > 0 send focus out of the trap, elements in iframes/shadow DOM
get skipped when focus wraps, all focus traps can be escaped by clicking
on the URL and then pressing tab). The demo page will be helpful for
research into FocusTrap improvements (see
angular#13054).
vanessanschmitt added a commit to vanessanschmitt/material2-vschmitt that referenced this issue Dec 3, 2019
Note that much of this demo page is known to not be working with the
current focus trap implementation (ex. elements in the focus trap with
tabindex > 0 send focus out of the trap, elements in iframes/shadow DOM
get skipped when focus wraps, all focus traps can be escaped by clicking
on the URL and then pressing tab). The demo page will be helpful for
research into FocusTrap improvements (see
angular#13054).
jelbourn pushed a commit that referenced this issue Dec 4, 2019
Note that much of this demo page is known to not be working with the
current focus trap implementation (ex. elements in the focus trap with
tabindex > 0 send focus out of the trap, elements in iframes/shadow DOM
get skipped when focus wraps, all focus traps can be escaped by clicking
on the URL and then pressing tab). The demo page will be helpful for
research into FocusTrap improvements (see
#13054).
@BobobUnicorn
Copy link
Collaborator

Is this considered fixed now?

@jelbourn
Copy link
Member

@annieyw can you confirm if this is fixed or not? I don't quite remember if the most recent focus trap changes from @vanessanschmitt fixed this.

@annieyw
Copy link
Contributor

annieyw commented Nov 30, 2020

Just tried it on the v11 doc site with Chrome and the issue is still persists

@Splaktar
Copy link
Member

Splaktar commented Mar 7, 2021

Given #18201 (comment):

This commit does not enable ConfigurableFocusTrap anywhere.

It's not unexpected that this isn't fixed since MatDialog does not use the new ConfigurableFocusTrap:

private _focusTrap: FocusTrap;

@vanessanschmitt mentioned in #18201 (comment) that an internal CL for this was in progress. Did something end up blocking that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility This issue is related to accessibility (a11y) area: cdk/overlay area: material/dialog P4 A relatively minor issue that is not relevant to core functions
Projects
None yet
Development

No branches or pull requests