palantir / blueprint

A React-based UI toolkit for the web
https://blueprintjs.com/
Apache License 2.0
20.7k stars 2.17k forks source link

Portal popover not positioned on mount (and jumpy on resize) #2352

Closed icd2k3 closed 6 years ago

icd2k3 commented 6 years ago

Bug report

Actual behavior

Portal popover positioning is not set until a window resize event is triggered, and even then it is jumpy (see gif below). On open, the popover does not have any positioning set, then on window resize it continuously toggles between positioned and not positioned.

portal

Expected behavior

Portal popover positioning should be correct and not jump around when scrolling or resizing window.

Code

Pretty straightforward...

<Popover>
  <button>test</button>
  <div>
    tqwefqwef<br/>
    tqewfqwef<br/>
    tqwefqwef<br/>
  </div>
 </Popover>

Has anyone else run into this behavior?

giladgray commented 6 years ago

@icd2k3 it looks like your container is simply too small for your popover, and so popper.js is getting too smart about positioning. try disabling the preventOverflow popper modifier: modifiers={{ preventOverflow: { enabled: false}, hide: { enabled: false} }}

icd2k3 commented 6 years ago

Thanks @giladgray for the quick reply! Sorry, that gif is a bit misleading... there's actually plenty of space around the button... here's a better example:

image

So in this case body and the container div around the button are both width/height 100%

Edit: here's what the DOM looks like if that helps:

image

^ note that no transform is set there which should be expected?

giladgray commented 6 years ago

huh, i have not encountered such a messed up state before! not sure what else to offer here.

icd2k3 commented 6 years ago

@giladgray I can confirm the above example works fine in @blueprintjs/core@1.35.7 so this looks like a real bug in 2.0... possibly related to the switch from react-addons-css-transition-group to react-transition-group? I know I ran into some positioning problems in my own app when making that switch.

giladgray commented 6 years ago

oh no it's certainly due to switching the Popover positioning engine from Tether to Popper.js. you can try using Popover2 in 1.x to see if you have the same results (i'm fairly certain you will).

shadowmouse commented 6 years ago

I can also confirm some additional weirdness with the positioning when NOT using the Portal.

Example: <Popover usePortal={false} position={Position.RIGHT_TOP} .../>

Image: image

icd2k3 commented 6 years ago

@giladgray you are absolutely right! Even in version 1.35.7 using Portal2 produces the exact same result as the image above

nicholas-ochoa commented 6 years ago

I'm seeing a similar problem - it seems that the Popover is being positioned at top: 0 left: 0 - here is my code:

import '@blueprintjs/core/lib/css/blueprint.css';
import '@blueprintjs/icons/lib/css/blueprint-icons.css';
import './global.css';
import React from 'react';
import ReactDOM from 'react-dom';
import { Popover, Button, ButtonGroup, Position, Menu, MenuDivider, MenuItem } from '@blueprintjs/core';
import { FileMenu } from './components/menu1.js';

const container = document.getElementById('root');

ReactDOM.render((
  <div>
    <ButtonGroup vertical={true}>
      <Popover modifiers={{ preventOverflow: { enabled: false}, hide: { enabled: false} }}>
        <Button icon='refresh' intent='primary'></Button>
        <Menu>
          <MenuItem text="New" icon="document" />
          <MenuItem text="Open" icon="folder-shared" />
          <MenuItem text="Close" icon="add-to-folder" />
          <MenuDivider />
          <MenuItem text="Save" icon="floppy-disk" />
          <MenuItem text="Save as..." icon="floppy-disk" />
          <MenuDivider />
          <MenuItem text="Exit" icon="cross" />
        </Menu>
      </Popover>
      <Popover content={<FileMenu />}>
        <Button icon='zoom-in' intent='primary'></Button>
      </Popover>
    </ButtonGroup>
    <ButtonGroup vertical={true} className='buttonGroupRight'>
      <Button icon='refresh' intent='primary'></Button>
      <Button icon='zoom-in' intent='primary'></Button>
    </ButtonGroup>
  </div>
), container);
shadowmouse commented 6 years ago

It looks like this may be an issue with the underlying react-popper element. Using react-popper in the following setup has a similar issue.

Code:

import { Manager, Target, Popper, Arrow } from 'react-popper'

<Manager>
  <Target>
    <button className="pt-button" type="submit"> Select Download Fields </button>
  </Target>
  <Popper placement={"right"}>
    <div style={{padding: 10, backgroundColor: "white", border: "black 1px solid"}}>
      Popper Test
    </div>
  </Popper>
</Manager>

Result: image

tyscorp commented 6 years ago

This is caused by https://github.com/FezVrasta/popper.js/pull/596 which was released in popper.js@1.14.2. Commenting out those lines or downgrading to v1.14.1 fixes the issue.

icd2k3 commented 6 years ago

Similarly to @shadowmouse I tried using only react-popper with a portal component wrapper (which is not officially supported by react-popper, by the way) and I can replicate this same positioning issue.

ex:

import { Manager, Target, Popper, Arrow } from 'react-popper';

class Portal extends React.Component {
  render() {
    const { children, target = document.body } = this.props;
    return ReactDOM.createPortal(children, target);
  }
}

const PopperThing = () => (
  <Manager>
    <Target style={{ width: 120, height: 120, background: '#b4da55' }}>
      Target Box
    </Target>
    <Portal>
      <Popper placement="left">
        <div>Popover Example</div>
      </Popper>
    </Portal>
  </Manager>
);

^ this above code produces the same positioning problems noted above

icd2k3 commented 6 years ago

@tyscorp is correct. Using that example above with popper.js v1.14.1 fixes the issue

EDIT: for those using yarn that want to be unblocked while this is being resolved, just add this to your package.json and re-install blueprint

"resolutions": {
  "popper.js": "1.14.1"
},
giladgray commented 6 years ago

@tyscorp are you saying that there's a bug in the latest popper.js release? is there an issue on that repo to track fixing it?

shadowmouse commented 6 years ago

Popper.js has been updated to 1.14.3 and corrects the issue. An npm update fixed this for me.

Addendum : Or not. The positioning is kind of fixed in that its rendering properly. It's just relative to the wrong element. It's rendering relative to the wrapper rather than the popover target.

image

Confirmed, I can "correct" the issue by wrapping the Popover Element in a inline-block'd span with the desired width.

shadowmouse commented 6 years ago

Same Behavior confirmed in react-popper element. I'll just take the bug report over to either react-popper or popper.js directly.

giladgray commented 6 years ago

@shadowmouse the popover position is expected given the DOM state your screenshot shows. adjusting CSS display is the correct approach.

seems like this was an issue in popper.js and has been resolved in the latest release.

giladgray commented 6 years ago

resolved by popper.js @ 1.14.3