mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.82k stars 32.26k forks source link

[Modal] Cannot focus iframe when open #17497

Closed PutziSan closed 4 years ago

PutziSan commented 5 years ago

This bug was introduced by https://github.com/mui-org/material-ui/pull/16585

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 (https://github.com/mui-org/material-ui/pull/16585)
eps1lon commented 5 years ago

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?

PutziSan commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@neemah

neemah commented 5 years ago

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 commented 5 years ago

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.

:thinking:

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@PutziSan Possibly.

chrisjhoughton commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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.

uc-asv commented 4 years ago

I am also facing this issue whenever any dialog is opened on another dialog which is made by material UI then I am unable to type on the input of new dialog open. Is there anything I can do to fix it?

joeyzhousherwin commented 3 years ago

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 commented 3 years ago

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