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

Popover position is incorrect if used within a Modal component #8468

Closed
tn3rb opened this issue Aug 3, 2018 · 13 comments
Closed

Popover position is incorrect if used within a Modal component #8468

tn3rb opened this issue Aug 3, 2018 · 13 comments
Labels
[Feature] UI Components Impacts or related to the UI component system [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@tn3rb
Copy link

tn3rb commented Aug 3, 2018

Describe the bug
First off, my apologies if this has already been reported. I was unable to find an existing issue that describes what I've found.
Any component that internally uses a Popover for displaying content such as Tooltip or Dropdown, does not position the Popover correctly if used within a Modal component. I haven't explored the cause yet in any depth, but my initial thought is that it is due to Popover using window to calculate positions instead of basing things off of the parent container.

To Reproduce
Load the following code in the Post editor (post.php admin route) (plz note: it replaces #wpbody-content):
Or create your own Modal and add components that use Popover

import ReactDOM from 'react-dom';
import { Component } from 'react';
import { DropdownMenu, IconButton, Modal } from '@wordpress/components';

class PopoverTest extends Component {
	constructor( props ) {
		super( props );
		this.state = { modalOpen: false };
	}

	toggleModal = () => {
		this.setState( prevState => ( { modalOpen: ! prevState.modalOpen } ) );
	};

	directionMenu = () => {
		return (
			<DropdownMenu
				icon="move"
				label="Select a direction"
				controls={ [
					{
						title: 'Up',
						icon: 'arrow-up-alt',
					},
					{
						title: 'Right',
						icon: 'arrow-right-alt',
					},
					{
						title: 'Down',
						icon: 'arrow-down-alt',
					},
					{
						title: 'Left',
						icon: 'arrow-left-alt',
					},
				] }
			/>
		);
	};

	render() {
		return this.state.modalOpen ? (
			<Modal
				title={ 'Modal Test' }
				onRequestClose={ this.toggleModal }
				closeButtonLabel={ 'close' }
			>
				<p>Checkout the position of the Tooltip for the CLOSE button!</p>
				<p>hint: it&apos;s way over there &gt;&gt;&gt;</p>
				<p>Now click on the button below to open the DropdownMenu</p>
				{ this.directionMenu() }
			</Modal>
		) : (
			<div style={
				{
					display: 'block',
					lineHeight: '2em',
					margin: '10% auto',
					textAlign: 'center',
					width: '500px',
				}
			}>
				<h1>Modal Popover Position Test</h1>
				<p>Here is how a DropdownMenu functions outside of a Modal</p>
				<div style={ { display: 'inline-block' } }>{ this.directionMenu() }</div>
				<p>Notice how the Tooltips appear correctly when you hover over the button!</p>
				<h3>Now click this button to open a Modal</h3>
				<p>But again, pay attention to how the Tooltip appears correctly when you hover over it!</p>
				<div style={ { display: 'inline-block' } }>
					<IconButton
						icon={ 'admin-generic' }
						label={ 'test' }
						tooltip={ 'CLICK ME' }
						onClick={ this.toggleModal }
					/>
				</div>
			</div>
		);
	}
}

ReactDOM.render(
	<PopoverTest />,
	document.getElementById( 'wpbody-content' )
);

Expected behavior
Popovers should position correctly when used within a Modal component

Screenshots
modal popover position test

Desktop (please complete the following information):

  • OS: Windows 10
  • Browser Versions: Chrome 67.0.3396.99 and FF 62.0b13
@designsimply designsimply added [Type] Bug An existing feature does not function as intended [Feature] UI Components Impacts or related to the UI component system labels Aug 3, 2018
@designsimply
Copy link
Member

Thank you for the report! #7992 looks similar and describes a problem where the floating link toolbar (popover/popup) sometimes appears in the wrong place.

My default is to want to close all duplicates so we can consolidate issues and focus. @talldan do you think a fix for #7992 will cover the case outlined here in #8468 as well?

@talldan
Copy link
Contributor

talldan commented Aug 8, 2018

@designsimply Sorry for the late response. I'm not sure, there's quite a lot going on in how popovers are positioned so I'd need a bit of time to debug the issue.

I had a quick test by the way, and couldn't reproduce the issue above (granted I didn't use the exact code). I actually had a separate issue, that the dropdown menu appears behind the modal causing the options to be unclickable:
screen shot 2018-08-08 at 1 16 11 pm

@aduth
Copy link
Member

aduth commented Sep 18, 2018

The Popover.Slot component exists to instruct where a popover's slot should be rendered. In the post editor, this is toward the top of the layout.

A modal could be considered almost akin to a separate embedded document, in which case we might want to change where Popover.Slot is rendered.

Popover.Slot itself doesn't support being rendered twice, at least not within the same SlotFillProvider context. There's a possible avenue to explore where you render a separate SlotFillProvider and your own Popover.Slot, at which point all popovers will render into that new Slot location. However, rendering your own SlotFillProvider could have additional undesired consequences.

There might be other options to explore for making this better supported.

@mtias
Copy link
Member

mtias commented Oct 27, 2018

I am not sure if this is a design pattern that should be encouraged either, adding needs design feedback.

@mtias mtias added the Needs Design Feedback Needs general design feedback. label Oct 27, 2018
@karmatosed
Copy link
Member

I would second this really isn't something to encourage from a usability view. What is the use case being met here?

@tn3rb
Copy link
Author

tn3rb commented Nov 26, 2018

What's the use case for using any kind of element that supports tooltips within a modal?

I'm confused... are you saying that modals should not contain any kind of element that uses a tooltip?

If the issue is specifically with my using the DropdownMenu component within a Modal, that's just an example I provided using code from existing Gutenberg component documentation to illustrate the problem. I'm not intending to specifically use that combination of components but think it should at least be possible to use tooltips within a modal window.

Regardless of what you put inside a modal window, last I checked, the tooltip for the modal close button itself does not position properly within the modal, and suffers the same problem of appearing far off to the right and below where it should appear.

@karmatosed
Copy link
Member

@tn3rb the root both @mtias and myself are trying to get to is whether we should encourage this design pattern. That comes down to even allowing it.

Regardless of what you put inside a modal window, last I checked, the tooltip for the modal close button itself does not position properly within the modal, and suffers the same problem of appearing far off to the right and below where it should appear.

This sounds like the bug here to separate out. I agree tooltips should position correctly, however I also agree we should look at each element and see if something to encourage. Perhaps that is something for documentation right now.

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Nov 28, 2018
@tn3rb
Copy link
Author

tn3rb commented Dec 4, 2018

@karmatosed (or @mtias)

can you please clarify what you mean by "this design pattern"?

@youknowriad
Copy link
Contributor

There has been improvements to the Popover component to make it more flexible and adapts to changes. Is this fixed now?

@youknowriad youknowriad added the Needs Testing Needs further testing to be confirmed. label Jan 26, 2019
@spencerfinnell
Copy link

spencerfinnell commented Feb 18, 2019

The placement issues are resolved for me, but the z-index is incorrect when using with a Modal component. This prevents things like the MediaPlaceholder from working properly as the "Insert from URL" is hidden.

I can see the thought process about this pattern potentially being abused, but since tooltips and standard selects already function correctly over a modal the Popover component which provides similar functionality should as well.

https://cloudup.com/c6AC0KkW51k
https://cloudup.com/cvwhx6MWSUU

@karmatosed karmatosed removed the Needs Testing Needs further testing to be confirmed. label Jul 19, 2019
@Mamaduka
Copy link
Member

Hi,

I've just encountered this issue. Can reproduce it with WP 5.2 and the latest Gutenberg plugin (6.6.0).
modal-tooltip

Example code:

import { Button, Dashicon, IconButton, Modal, Tooltip } from '@wordpress/components';
import { useState } from '@wordpress/element';

const ToolTipModal = () => {
    const [ isOpen, setOpen ] = useState( false );
    return (
        <>
        <Button isDefault onClick={ () => setOpen( true ) }>Open Modal</Button>
        { isOpen && (
            <Modal
                title="Modal with Tooltip"
                onRequestClose={ () => setOpen( false ) }
                className="component-tooltip-modal"
            >
                <p>
                    Just a tooltip.
                    <Tooltip text="Hope this is the right place">
                        <Button>
                            <Dashicon icon="editor-help" />
                        </Button>
                    </Tooltip>
                </p>
                <p>
                    IconButton
                    <IconButton
                        icon="smiley"
                        tooltip="Hope this is the right place"
                        label="Icon Button"
                    />
                </p>
            </Modal>
        ) }
        </>
    );
};

@paaljoachim
Copy link
Contributor

paaljoachim commented Feb 20, 2021

What needs to be done here?

@youknowriad Riad @talldan Dan.

Riad is focusing on the modal component lately here: #28574 and in another PR as well.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Feb 10, 2022
@ndiego
Copy link
Member

ndiego commented Jul 5, 2022

I have just tested this using @Mamaduka code example above and this no longer appears to be an issue, see the screenshot below:

image

Given this successful test, I am closing this issue.

@ndiego ndiego closed this as completed Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.