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] Cannot focus iframe when open #17497

Closed
2 tasks done
PutziSan opened this issue Sep 20, 2019 · 17 comments · Fixed by #18643
Closed
2 tasks done

[Modal] Cannot focus iframe when open #17497

PutziSan opened this issue Sep 20, 2019 · 17 comments · Fixed by #18643
Labels
accessibility a11y component: FocusTrap The React component. component: modal This is the name of the generic UI component, not the React module!

Comments

@PutziSan
Copy link

PutziSan commented Sep 20, 2019

This bug was introduced by #16585

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When an dialog is open, it steals the focus.

mui-focus-bug

(At the end of the gif I can focus the editor again, but I think that's because of a hot-update of codesandbox, which interrupts the focus loop of the dialog.)

Expected Behavior 🤔

When the dialog is open, I can focus unrelated parts of the tab (e.g. in an iframe).

Steps to Reproduce 🕹

on sandbox (https://codesandbox.io/s/cllze)

  1. open simple dialog
  2. try to focus the editor, after 200ms the focus will lost, so you cannot inoput something in the editor

Context 🔦

I stumbled across it because I use a payment service provider that loads an iframe with an embedded modal where the customer can then make the payment. Unfortunately, this was no longer possible since release 4.3.0, as the input fields of the payment service provider immediately lost focus. Since there was no error message, it was only noticed when a customer reported the error.

Your Environment 🌎

Tech Version
Material-UI since v4.3.3 (#16585)
@eps1lon
Copy link
Member

eps1lon commented Sep 20, 2019

The dialog containing focus is intended following https://www.w3.org/TR/wai-aria-practices/#dialog_modal:

That is, users cannot interact with content outside an active dialog window.

So the codesandbox behavior is correct.

I guess in your use case you have an iframe within that dialog and the dialog does not allow the iframe to be focused? Could you create a codesandbox with that specific use case?

@eps1lon eps1lon added accessibility a11y component: dialog This is the name of the generic UI component, not the React module! discussion labels Sep 20, 2019
@PutziSan
Copy link
Author

@eps1lon Thank you very much for your answer.
I have created a sandbox, which simulates my case quite well: https://codesandbox.io/s/material-demo-jlbtn

before version 4.3.0 it worked like this, then unfortunately no more. So for me 4.3 was a bit like a hidden breaking change.

@oliviertassinari oliviertassinari added component: modal This is the name of the generic UI component, not the React module! and removed component: dialog This is the name of the generic UI component, not the React module! labels Sep 20, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 20, 2019

The concern seems very close to #15694 and #15450. You can find 3 props related to focus management on the Modal and Dialog components: disableEnforceFocus, disableAutoFocus and disableRestoreFocus.

before version 4.3.0 it worked like this, then unfortunately no more. So for me 4.3 was a bit like a hidden breaking change.

@PutziSan Or a bug fix, it depends on your perspective.


Regarding better supporting this use case, it's tricky as the logic comes from TrapFocus, a component we will likely defer to React itself in the future (FocusScope). In this context, I can only suggest you to disable the focus enforce feature with the disableEnforceFocus prop.

@eps1lon Should we close?

@eps1lon
Copy link
Member

eps1lon commented Sep 20, 2019

I think trapping the focus only within the current focus is a reasonable feature to have. Since this is security related it's even more useful.

I would keep this open since we have no idea if or when better react primitives land. It might be worth looking into a better solution in the meantime.

@neemah
Copy link

neemah commented Sep 25, 2019

As far as i understood there will be not fix, nor workaround as it depends on React internals, that is in progress, and react update cycle is quite... unpredictable.

What would you suggest right now?

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 25, 2019

@neemah

  • The current workaround is to disable one of the accessibility requirement with disableEnforceFocus.
  • We don't depend yet on React internals.
  • We are interested in providing a better solution. We haven't discovered it yet.

@neemah
Copy link

neemah commented Sep 25, 2019

Sorry i thought that there is something in React that is being refactored and will help to fix this issue :-(

Thanks for suggestion, we will try!!! 🙏

@eps1lon
Copy link
Member

eps1lon commented Sep 25, 2019

As far as i understood there will be not fix, nor workaround as it depends on React internals, that is in progress, and react update cycle is quite... unpredictable.

🤔

It might be worth looking into a better solution in the meantime.

I wanted to emphasize that any solution we do implement has to be tested thoroughly since the implementation itself might change very soon.

Focus isn't an easy problem which is illustrated again by this issue. Our implementation traps focus inside the window of the dialog container. We need to make sure that if we do allow other windows to get focus that these windows are part of the dialog. It is probably tricky to determine that given that the DOM tree isn't a subset of the react tree (because portals) and we need to know if that other window is part of the dialog. I'm not even sure we can know if focus moved to an iframe or if this fact is hidden for security reasons. At that point it will be impossible to contain the focus properly since it's indistinguishable from #16584

@oliviertassinari oliviertassinari changed the title Cannot focus other parts of the app when dialog is open [Modal] Cannot focus other parts of the app when dialog is open Nov 30, 2019
@oliviertassinari oliviertassinari changed the title [Modal] Cannot focus other parts of the app when dialog is open [Modal] Cannot focus other parts of the app when open Nov 30, 2019
@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. and removed discussion labels Nov 30, 2019
@oliviertassinari
Copy link
Member

Given more thoughts to this problem, I can't think of anything better than an update of the documentation:

diff --git a/docs/src/pages/components/modal/modal.md b/docs/src/pages/components/modal/modal.md
index f2c3b19c8..24966c8be 100644
--- a/docs/src/pages/components/modal/modal.md
+++ b/docs/src/pages/components/modal/modal.md
@@ -62,6 +62,14 @@ In order to display the modal, you need to disable the portal feature with the `

 {{"demo": "pages/components/modal/ServerModal.js"}}

+## Limitations
+
+### Focus trap
+
+The modal moves the focus back to the body of the component if the focus tries to escape it.
+
+This is done for accessibility purposes, however, it might create issues.
+In the event the users need to interact with another part of the page, e.g. with a chatbot window, you can disable the behavior:
+
+```jsx
+<Modal disableEnforceFocus />
+```
+
 ## Accessibility

 (WAI-ARIA: https://www.w3.org/TR/wai-aria-practices/#dialog_modal)

@PutziSan What do you think about it? Do you want to give it a pull request? :)

@PutziSan
Copy link
Author

PutziSan commented Dec 1, 2019

Yeah, I'm happy to do that, thanks for the chance.
Should I also add a link on the dialog documentation page similar to Dialogs#accessibility?

in docs/src/pages/components/dialogs/dialogs.md:

 {{"demo": "pages/components/dialogs/ScrollDialog.js"}}
 
+ ## Limitations
+ 
+ Follow the [Modal limitations section](/components/modal/#limitations).
+ 
 ## Accessibility
 
 Follow the [Modal accessibility section](/components/modal/#accessibility).

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 1, 2019

@PutziSan Possibly.

@chrisjhoughton
Copy link

chrisjhoughton commented Jan 28, 2020

For those of you who want to disable this globally so focus is not enforced on any of your dialogs or drawers, add these global prop override to your Theme file.

(Side note: you can't override global MuiModal props because of this bug)

  props: {
    MuiModal: {
      disableEnforceFocus: true,
    },
  },

Use case: we're using Intercom messenger for chat support. Enforcing focus in dialogs prevents the messenger being used when a modal is open.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 28, 2020

@chrisjhoughton Thanks for the feedback. The modal approach should work, I have updated your comment. Your use case seems common. I'm curious, did you struggle to figure out what was wrong? How much time did you waste?

I was considering reducing the interval frequency from 50ms to 500ms, to make it easier to identify. It also seems that the focus manager React is working on doesn't have this behavior in the first place, to double-check.

Also, intercom.com uses an iframe to render its chat, and so does drift.com (the low-cost alternative).
What if we were ignoring cases when the focus moves to an iframe that is not a child of the TrapFocus?
When using code sandbox, it's so frustrating not to be able to type anything in the code panel, when a modal is open.

@TrySound
Copy link
Contributor

TrySound commented Apr 1, 2020

This would be sweet. We have translations in context editor. Putting editor or app into iframe does not work for us. We workarounded this with disableEnforceFocus in theme for now.

@oliviertassinari oliviertassinari added the component: FocusTrap The React component. label Apr 22, 2020
@uc-asv

This comment has been minimized.

@joeyzhousherwin
Copy link

Hi, Just curious if there's any update on this, @oliviertassinari
I ran into a similar problem with dialog and intercom. we have a form in a dialog, however, we wanted to have intercom available to users if they ran into any questions at any point.

Just wondering if there are any plans with allow iframes to escape the focus trap?

@oliviertassinari oliviertassinari changed the title [Modal] Cannot focus other parts of the app when open [Modal] Cannot focus iframe when open Jun 10, 2021
@oliviertassinari oliviertassinari removed docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. labels Jun 10, 2021
@oliviertassinari
Copy link
Member

Just wondering if there are any plans with allow iframes to escape the focus trap?

@joeyzhousherwin I have updated the title of the issue as most of the feedback we got seemed related to handling iframes. There is no update other than we document it https://next.material-ui.com/components/modal/#focus-trap.

Feel free to upvote this issue if you think that the focus shouldn't steal from the iframes.

Reading https://www.w3.org/TR/wai-aria-practices/#dialog_modal more carefully, it does feel like an iframe chatbot could be considered as part of the modal displayed, even if not inside the same parent DOM node. But the current workaround might be enough.

Sometimes, I also wonder if a dedicated prop to consider extra components as part of the trap focus could make sense, like this option https://github.com/focus-trap/focus-trap-react#containerelements

Merkur39 added a commit to cozy/cozy-banks that referenced this issue Sep 1, 2023
When opening the Intent modal to connect a
konnector via the `TransactionModal` component,
already having a modal open prevents inputs from
focusing on the Intent modal, see:
mui/material-ui#17497

The trick here is to disable accessibility
(`disableEnforceFocus`) only when the Intent modal is open,
so as not to have regression.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: FocusTrap The React component. component: modal This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants