Components: Clicking on iframe in the content close the modal

[Tropicalista]

Description

I'm trying to load a tinymce editor in a gutenberg modal. I'm using the oficial component: https://github.com/tinymce/tinymce-react#readme

The component load correctly the editor, however clicking or typing something will close the modal.

Step-by-step reproduction instructions

1. Create a custom component as the one presented here: https://www.tiny.cloud/docs/integrations/react/
2. Create a simple basic modal and put the component there
3. Try to type some texts

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[Tropicalista]

The problem is partially solved by adding

shouldCloseOnClickOutside={ false }

However some glitch still remaining because cotextual menu need some css because the element are positioned on a lower z-index.

[t-hamano]

Hi @Tropicalista,

I've written test code and was able to reproduce this problem.

I think the cause may be that onRequestClose is called when the iframe in the modal gets focus.

Here is the test code.
I used @tinymce/tinymce-react and dummy iframe content.

See sample code

import { useBlockProps } from '@wordpress/block-editor';
import { Modal, Button } from '@wordpress/components';
import { useState } from '@wordpress/element';
import { registerBlockType } from '@wordpress/blocks';
import { Editor } from '@tinymce/tinymce-react';

registerBlockType( 'create-block/modal-editor', {
  edit: () => {
    const [ isOpen, setIsOpen ] = useState( false );
    return (
      <div { ...useBlockProps() }>
        <Button
          onClick={ () => setIsOpen( ! isOpen ) }
          variant="primary"
        >
          Toggle Modal Editor
        </Button>
        { isOpen && (
            <Modal
              onRequestClose={ () => {
                setIsOpen( false );
                console.log( 'modal closed.' );
              }}
            >
              <h2>React Tiny MCE</h2>
              <Editor />
              <h2>Iframe contents</h2>
              <iframe height="250" src="https://github.com/WordPress/gutenberg"></iframe>
            </Modal>
          )
        }
      </div>
    );
  },
  save: () => null,
} );

b4940356ef8e64c108a199fcc0590c93.mp4

This might be a problem that should be corrected on the Modal component or useFocusOutside hook.

Does anyone have a good idea?

[mrfoxtalbot]

Thank you for reporting this @Tropicalista. Do you have any suggestions on how this could be fixed so we can discuss them and move the issue forward? Thank you!

[Tropicalista]

At the moment I use shouldCloseOnClickOutside={ false } and it's working. You can see it in my Formello plugin.

[t-hamano]

Thank you for the reply, @Tropicalista.
If so, is it ok to close this issue?

[mrfoxtalbot]

If this is still reproducible, we should consider submitting a PR rather than closing the issue, so that this is fixed for everyone. I would love to do this myself but the issue is beyond my technical expertise.

@Tropicalista, @t-hamano, would you consider submitting a PR to fix this? Thank you!

[github-actions]

Hi,
This issue has gone 30 days without any activity. This means it is time for a check-in to make sure it is still relevant. If you are still experiencing this issue with the latest versions, you can help the project by responding to confirm the problem and by providing any updated reproduction steps.
Thanks for helping out.

[t-hamano]

I've thought about it again, and it may be strange to set shouldCloseOnClickOutside to false to prevent the iframe in the modal content from being closed when clicked, because the iframe is inside the content, not outside it.

Ideally, I would like to not call onRequestClose when an iframe element in the content is clicked, but I am wondering what implementation would be smart.

cc @ciampo @mirka

[ciampo]

Best way to move forward would be to open a PR containing the code to reproduce the issue — that way, we could look at debugging the problem and understand why the modal is closing.

[Tropicalista]

I suppose it's a z-index problem

[ciampo]

I suppose it's a z-index problem

Possibly — but to be sure, I'd have to be able to inspect the code reproducing the issue. Would you be able to open a PR? You could add a temporary Storybook example to Modal to replicate the problem

[t-hamano]

I have added the test code to the modal component Storybook. The branch is modal-click-iframe.
It is indeed for reproduction.

ccac29ff9d1975427ddf0e5e01ef1bd9.mp4

[ciampo]

Thank you @t-hamano !

I looked into the example, and I think that the issue is caused by this check in useFocusOutside:

gutenberg/packages/compose/src/hooks/use-focus-outside/index.js

Lines 162 to 169 in a4af9b3

	// If document is not focused then focus should remain
	// inside the wrapped component and therefore we cancel
	// this blur event thereby leaving focus in place.
	// https://developer.mozilla.org/en-US/docs/Web/API/Document/hasFocus.
	if ( ! document.hasFocus() ) {
	event.preventDefault();
	return;
	}

Basically, clicking on the iframe causes the modal to lose focus, firing the blur event listener. But the document.hasFocus() check still returns true, and therefore the blur event is not default prevented, and the modal closes.

I will open a PR with a tentative fix.

[ciampo]

I will open a PR with a tentative fix.

Spoke too soon — I've spent some time trying to come up with a good fix, but it's more complicated than I expected. Detecting reliably when a click causing the wrapper to blur is actually caused by an iframe or its contents is not easy.

I unfortunately don't have much capacity to work on it. Feel free to try your own attempt (I will still try to push some improvements to the hook that I made while working on this)

[ciampo]

Update — opened #45317 with a bunch of code quality improvements to useFocusOutside (no fix , unfortunately)

Moved the WIP to another PR — #45349. Feel free to branch off from that PR and try to find a robust solution!

[stokesman]

I think this wasn't meant to be linked and closed by #45317 so I'll reopen it.

Detecting reliably when a click causing the wrapper to blur is actually caused by an iframe or its contents is not easy.

👆💯

And even when you do that's only half the battle. I think I've been able to find a pretty robust solution but it relies on polling 🥴.

As an alternative, a simple fix for Modal is putting a pointerdown handler on its overlay element to trigger the close. Because of that overlay element and the fact that it uses useConstrainedTabbing it doesn't need useFocusOutside. That still leaves useFocusOutside > useDialog > consumers of either open to this issue though.

[ciampo]

Curious to take a look if/when the PR(s) are ready for a review!

[ciampo]

While working on other tasks, I came across the useFocusableIframe hook, which could actually be useful here — @stokesman could this help you ?

I may try to see if I can use it in #45349 as well

[stokesman]

Thanks Marco! I didn't see how that hook could be used directly since it would be consumers that need to put it on their iframes. I did see how I could steal its concept though to avoid the polling that I'd mentioned before. I've tested it out bit but I'm calling it day. Tomorrow I can make a PR.

[ciampo]

I was mainly thinking of asking consumers in need of using an iframe to also use that hook (adding a paragraph in both useFocusOutside and Modal's docs, and maybe even a storybook example for Modal).

But first, I would have tried it in my tests to see what else would be needed for the whole thing to keep working as expected. Sorry if I didn't explain myself clearly in the previous message!

[ivdimova]

I am working on a something similar to @Tropicalista and wondering if this problem was ever fixed? Even if I use useFocusableIframe the onFocus works fine in Chrome, but not in FF.

[ciampo]

Hey @ivdimova , the problem was not fixed. I had tried an approach with #45349 but (as you can read in the PR description) I didn't make much progress.

Feel free to work on a solution for this issue and I'll be happy to help with review & advice as necessary

[ciampo]

Tomorrow I can make a PR.

@stokesman , out of curiosity did you even get to work on this ?

[stokesman]

Yes, I did work on it. Now, the best I can remember is the approach I was trying (fixing it inside useFocusOutside) had some little edge case that kept me from being satisfied with it.

If @ivdimova is also using the Modal component like in the OP by @Tropicalista, and we want to fix this in the library, it could be worth trying the alternative I'd mentioned above #40912 (comment). To restate it, remove useFocusOutside from Modal and instead utilize its overlay element to catch any outside pointer interaction to trigger the close.

If Ivelina is making a custom dialog/modal (or wants to try it to avoid waiting on a fix) the same approach should work. I.e. use an overlay element to trigger closing and don't rely on useDialog or useFocusOutside. useConstrainedTabbing must be employed so focus can't otherwise escape.

[ciampo]

It could be worth trying the alternative I'd mentioned above #40912 (comment). To restate it, remove useFocusOutside from Modal and instead utilize its overlay element to catch any outside pointer interaction to trigger the close.

I don't currently have the capacity to work on this firsthand, but I'd happy to collaborate and help folks with advice and PR reviews.

If Ivelina is making a custom dialog/modal (or wants to try it to avoid waiting on a fix) the same approach should work. I.e. use an overlay element to trigger closing and don't rely on useDialog or useFocusOutside. useConstrainedTabbing must be employed so focus can't otherwise escape.

Would it be worth, at this point, revisiting useDialog and embed potentially revised functionality directly in it?

[afercia]

I was not aware of this issue and created a similar one at #50436

Worth mentioning this problem is now even more important because the Classic block now opens in a modal dialog. The TinyMCE editable area within the modal dialog is in an iframe. The specific problem for this block was worked around by not passing the onRequestClose prop (which expects a callback to handle the close behavior). That disables the 'close on click outside' behavior but unfortunately disables also the 'close on press the Escape key` behavior. As such, it's not a real solution to the problem and alters the expected behavior of the modal dialog.

[ciampo]

Thank you for the comment, @afercia !

As you can read above, we've been discussing possible solutions, but we haven't landed on one (or haven't found the time to work on it) just yet — feel free to open a PR in case you have an approach in mind, I'll be more than happy to collaborate on it!

[t-hamano]

I have created a sample using react-modal to explore the ideal solution to this problem.

Code Sandbox: https://codesandbox.io/s/vigorous-carson-6grq7f?file=/src/index.js

This library has onRequestClose prop, similar to Gutenberg's Modal component. However, clicking on an iframe in a modal does not close the modal. I have not investigated this in detail yet, but this implementation might be a solution.

03d60681174928d73ebeebe26b54148b.mp4

[stokesman]

@t-hamano From a brief look at that library and your sample, the reason it works is because it utilizes a click handler on the overlay element. Gutenberg’s Modal also has any overlay element and so the same could be done with it (as I've mentioned previously).

I've made a PR with that approach and linked this issue to it.

[ciampo]

Hey all, back from some extended AFK.

I'd like to assess the status of the issue. From what I can see:

• Modal: Fix loss of focus when clicking outside #52653 and Modal: Nuance outside interactions #52994 got merged, although I'm not sure if it affects this issue at all
• @stokesman opened Modal: fix closing when contained iframe is focused #51602 as a potential fix to the issue, where he removed useFocusOutside from the Modal component and added an event handler instead (although it's currently stuck because of a conflict with command center)
• @ciampo opened Try/use focus outside iframe #52040 as another potential fix, amending the useFocusOutside to support the iframe use case (although the solution is a bit hacky and requires thorough testing, in case there are more edge cases)

What are your thoughts? (cc @t-hamano @afercia )

[stokesman]

I'm pretty sure this is still present. Neither of the Modal PRs I merged were meant to affect it. Hope you had a great time AFK Marco!

[afercia]

@ciampo thanks for the ping. I commented on #51602 (comment)