WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.57k stars 4.22k forks source link

Media library button opens a blank modal when MediaPlaceholder is inside a Popover #39282

Closed mbmjertan closed 4 months ago

mbmjertan commented 2 years ago

Description

When a MediaPlaceholder is nested inside a Popover component, the Media library button triggers a blank modal instead of the media library.

This does not happen when MediaPlaceholder is a direct child of PanelBody or a block itself.

Could be related to https://github.com/WordPress/gutenberg/issues/12830

Step-by-step reproduction instructions

I can reproduce by adding this to an InspectorControls panel:

<Popover noArrow={false} position='middle right'>
  <MediaPlaceholder
    accept={['.vtt', 'text/vtt']}
    onSelect={
      (track) => {
        console.log(track);
      }
    }
  >
    The Media Library button above will fail to render the Media Library modal, rendering a blank modal instead.
  </MediaPlaceholder>
</Popover>

Screenshots, screen recording, code snippet

A screen recording showing the Media Library button working when in PanelBody, and failing when in Popover:

https://user-images.githubusercontent.com/1742806/157219573-aa0cc2d1-1954-4614-a2f0-efa691887824.mov

Environment info

WP 5.9.1, Gutenberg build shipped with WP.

Reproducible in Firefox 97, Chrome 99, Safari 15.1 on macOS 12.0.1.

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

stokesman commented 2 years ago

I couldn't reproduce this on WordPress 6.0-alpha-52772 with or without latest Gutenberg from trunk. I'll test on an older install too. First, here's the snippet (can be pasted in devtools console) I used for testing:

(({
  blocks: { registerBlockType },
  blockEditor: { InspectorControls, MediaPlaceholder },
  components: { Popover },
  element: { createElement: el, Fragment },
}) => {
  registerBlockType( 'tester/issue-39282', {
    title: 'Test 39282',
    edit: () => {
      return el( Fragment, null,
        el( InspectorControls, {},
          el( Popover, { noArrow: false, position: 'middle right' },
            el( MediaPlaceholder, {
              accept: ['.vtt', 'text/vtt'], onSelect: function (track) {
                console.log(track);
              } }, "The Media Library button above will fail to render the Media Library modal, rendering a blank modal instead."
            )
          )
        ),
        el( 'p', {}, 'Inspect me!' ),
      );
    },
    save: () => null,
  } );
})(wp);
stokesman commented 2 years ago

I also couldn't reproduce on WordPress 5.8.3 or 5.9.1.

What I did see might be an issue though. The popover is above every other popover/modal. Here's a screen capture:

https://user-images.githubusercontent.com/9000376/157481702-0ba2740d-e7b4-4f3a-a45f-dedf1cc6e70a.mp4

@mbmjertan, would you test with the snippet I used to confirm it will reproduce the issue for you? It was a conversion of what you posted originally.

mbmjertan commented 2 years ago

So sorry, the example I provided isn't correct! I've removed the cause by accident 🙈

(I can confirm that my sample, as well as yours, don't reproduce this issue 🤦 )

I originally did this, which was failing.

// ...
const [trackEditOpen, setTrackEditOpen] = useState({});

return (
// ...
{trackEditOpen[index] === true &&

  <Popover onClose={() => setTrackEditOpen({...trackEditOpen, [index]: false})} noArrow={false} position='middle right'>

    <div className='es-popover-content'>
      {!videoSubtitleTracks[index].src &&
        <MediaPlaceholder
          accept={['.vtt', 'text/vtt']}
          labels = {{
            title: __('Track file', 'eightshift-frontend-libs'),
          }}
          onSelect={
            (track) => {
              const modifiedVideoSubtitleTracks = [...videoSubtitleTracks];
              modifiedVideoSubtitleTracks[index].src = track.url;
              setAttributes({ [getAttrKey('videoSubtitleTracks', attributes, manifest)]: modifiedVideoSubtitleTracks });
            }
          }
        >
          {__('Upload a VTT file containing captions, subtitles, descriptions or chapters for this video', 'eightshift-frontend-libs')}
        </MediaPlaceholder>

      }

      {videoSubtitleTracks[index].src &&
        <>
          <b>{sprintf(__('Track #%d', 'eightshift-frontend-libs'), index + 1)}</b>

        </>
      }
    </div>
  </Popover>
}
);

It's been possible to reproduce this for me using this code (see full commit in eightshift-frontend-libs)

I've tested this out, and the root cause could be that onClose gets called when the media library is opened, which unmounts Popover and MediaPlaceholder.

This could very well be expected behaviour, in which case feel free to close this.

However, in that case I don't see a way to use MediaPlaceholder (or otherwise trigger the media library modal) from a Popover that's dynamically rendered. Any ideas on how to support that would be useful.

stokesman commented 2 years ago

Thanks @mbmjertan, I've been able to reproduce this now.

Snippet used for reproduction: ```javascript (({ blocks: { registerBlockType }, blockEditor: { InspectorControls, MediaPlaceholder }, components: { Popover, Button }, element: { createElement: el, Fragment, useReducer }, }) => { registerBlockType( 'tester/issue-39282', { title: 'Test 39282', edit: () => { const [ isOpen, toggleIsOpen ] = useReducer( state => ! state, false ); return el( Fragment, null, el( InspectorControls, {}, el( Button, { onClick: toggleIsOpen }, 'Toggle Popover' ), isOpen && el( Popover, { onClose: toggleIsOpen, noArrow: false, position: 'middle right' }, el( MediaPlaceholder, { accept: ['.vtt', 'text/vtt'], onSelect: function (track) { console.log(track); } }, "The Media Library button above will fail to render the Media Library modal, rendering a blank modal instead." ) ) ), el( 'p', {}, 'Inspect me!' ), ); }, save: () => null, } ); })(wp); ```

…I don't see a way to use MediaPlaceholder (or otherwise trigger the media library modal) from a Popover that's dynamically rendered. Any ideas on how to support that would be useful

Dropdown has some logic to avoid closing when a dialog is opened from it. Using it instead of Popover directly gets around the blank modal part of this problem but then you'll run into the layering issue like in my screen capture. So I guess that's an issue of its own. For now, a bit of CSS can remedy.

.media-modal-backdrop {
  z-index: 1000001;
}
.media-modal {
  z-index: 1000002;
}
Mamaduka commented 2 years ago

I can still reproduce the issue, and below is the screenshot of an error I see in the console.

Recently @noisysocks implemented a similar UI in #42352. I would recommend checking the code for examples of how to use MediaUpload with the Dropdown component.

Screenshot

CleanShot 2022-09-20 at 18 29 54

yarovikov commented 1 year ago

Any news? I have the same bug using mediaUpload inside Modal

OlliPerikanta commented 1 year ago

I'm getting this error too when i'm trying to open Media libary inside Modal Component.

daonham commented 1 year ago

use quick fix:

<Modal
    title={__('Modal')}
    onRequestClose={(event) => {
        if (event.target.className.includes('notification-upload-media')) {
            return;
        }

        setOpen(!isOpen);
    }}
    >
<MediaUpload
 onSelect={(mediaUpload) => {
         setMedia({
        id: mediaUpload.id,
        url: mediaUpload.url,
    });
}}
value={''}
allowedTypes={['image']}
render={({ open }) => (
<Button
    className="notification-upload-media"
    variant="primary"
    onClick={open}
    style={{ height: 30, marginBottom: 8 }}
>
    {__('Upload')}
</Button>
)}
/>
wwdes commented 6 months ago

I have the same issue and made a workaround by implementing isMediaModalOpen state which is regulated when opening and closing the modal:

<MediaUploadCheck>
    <MediaUpload
        modalClass="media-modal"
        onSelect={(media) => setMediaValue(media)}
        onClose={() => {
            setIsMediaModalOpen(false)
        }}
        //value={mediaValue}
        render={({ open }) => (
            <Button
                onClick={() => {
                    setIsMediaModalOpen(true)
                    open()
                }}
                isLarge
            >
                {mediaValue ? "Change Image" : "Select Image"}
            </Button>
        )}
    />
</MediaUploadCheck>

then in the popover component, i close the modal only if isMediaModalOpen is false.

    <Popover
        className="mask-settings-popover"
        noArrow={false}
        placement="left"
        offset={32}
        onClose={() => !isMediaModalOpen && setActivePopover(null)}
    >

Now the media modal is working, however, one issue remains: The popover has a higher z-index then the modal (should it not be the other way around?). The easiest way to fix this was to give the popover a lower z-index value. I have not tested it thoroughly but it seems to work fine.

talldan commented 4 months ago

This is the same issue as #12830 so closing as a duplicate