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

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

Closed
mbmjertan opened this issue Mar 8, 2022 · 10 comments
Labels
[Feature] Media Anything that impacts the experience of managing media [Type] Bug An existing feature does not function as intended

Comments

@mbmjertan
Copy link

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 #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:

Screen.Recording.2022-03-08.at.11.27.18.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

@Mamaduka Mamaduka added [Feature] Media Anything that impacts the experience of managing media Needs Testing Needs further testing to be confirmed. labels Mar 8, 2022
@stokesman
Copy link
Contributor

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
Copy link
Contributor

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:

test-39282.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
Copy link
Author

mbmjertan commented Mar 9, 2022

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
Copy link
Contributor

stokesman commented Mar 10, 2022

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

Snippet used for reproduction:
(({
  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;
}

@github-actions github-actions bot added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Aug 25, 2022
@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended and removed Needs Testing Needs further testing to be confirmed. [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. labels Sep 20, 2022
@Mamaduka
Copy link
Member

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
Copy link

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

@OlliPerikanta
Copy link

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

@daonham
Copy link

daonham commented Jul 21, 2023

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
Copy link
Contributor

wwdes commented May 24, 2024

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
Copy link
Contributor

talldan commented Jul 24, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media Anything that impacts the experience of managing media [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

8 participants