[Popover] Positioning delayed

[therahl]

Problem description

Popover component renders children before applying position styling. This results in a 'flash' of the popover in the top left corner (x: 0, y:0) before positioning correctly. This is caused by the setPlacement function being wrapped in setTimeout. Removing the setTimeout wrapper resolves the issue. The timeout was added for this issue: #7663.

Initial render 'flash':

Split second later, correctly positioned:

Versions

• Material-UI: 0.19.0
• React: 15.5
• Browser: Chrome 60.0.3112.90

[stevewillard]

I'm seeing this exact behavior in 1.0.0-beta.8. Happy to submit a PR if you think removing it is the correct change.

Edit: just looked more closely at the 1.x beta code, and it doesn't seem to have a setTimeout for the placement positioning.

[austinh]

I am also having this same problem. Removing the setTimeout indeed fixes the issue. Right now we are hotpatching Popover.prototype to remove the setTimeout.

[oliviertassinari]

Side note, the setTimeout was added by @lostpebble to support React Fiber.

[lostpebble]

Yes, I happened to have noticed this now on slower devices as well. It was a quick fix for making the pre-v1 version of material-ui compatible with React Fiber. It seems like it's showing its cracks...

Basically, the API has changed when working on lower level rendering order stuff. As far as I know the next version of material-ui has rewritten this part of the rendering process to be compatible with React Fiber without such hacks - using the newer API interface.

When using React v16 with the 0.19 versions of material-ui, without this setTimeout the popovers will always render fixed to a corner of the screen instead of relative to the container you called it from.

[austinh]

Any update with this one?

[atrauzzi]

Indeed, any planned release date for a fix?

[austinh]

This bug also happens for me with React v16, so I don't think the fix does what it intends to do in the first place. It happens consistently for me on React 15.6 and React 16

[lostpebble]

As mentioned earlier, there were some underlying changes that have happened with React v16 which is causing however the Popover stuff used to render, to not render at the right anchor point anymore.

This setTimeout fix prevents this happening:

Unfortunately it was not a complete solution, and I realise this problem of the flashing Popover needs to be fixed.

The whole reason I put the setTimeout in there as a temporary fix was because I don't know the deeper workings of material-ui or what's changed as far as React v16 is concerned in this specific rendering case. The Popover stuff specifically is quite a complicated bunch of code and my main goal was to get material-ui to function on top of React v16 - for which resolving this anchor-point issue and getting rid of react-touch-event-plugin was needed.

If you want to support React v16, although flawed, this setTimeout is far better than what the gif above shows - and in that case it does what it intends to do. I never noticed that flash until I ran into problems with a slow CPU or blocking the UI thread with overly heavy code. I realise it is not ideal, but it at least allows a start of using pre-v1 material-ui with React v16. If anyone would like to dive into it and try re-wire it to how React v16 requires, it would be much appreciated as I just don't have to capacity to do that at the moment.

I am told that the new v1 versions of material-ui do solve this problem, as they built it from the ground up considering the newer React changes.

[bradbirnbaum]

Any chance we can get a fix? This introduced a bad regression.

[oliviertassinari]

@lostpebble You have been doing an excellent job adding the support of react@16 with our legacy version!

@bradbirnbaum Given the impact of this issue, I could try to find some time to work on it, but no promise, I have a lot of other important issues I want to take care of on the v1 branch.

[atrauzzi]

Just want to say -- This is happening in react 15 as well, so it's not currently limited to 16 either.

[jtorhoff]

It also appears that there is no enter animation of the popover since the fix by @lostpebble

[atrauzzi]

Yeah, the open animation doesn't work. This definitely needs attention.

[Sharlaan]

@austinh Can you give more details about your hotpatching solution on Popover.prototype please ? since apparently this won't be fixed for M-UI < v1,x (?)

[austinh]

I have since moved on from the hotpatching solution b/c I’m trying to transition to mui v1, but if you look at the Popover.js class in this library and override the functions with Popover.prototype.componentDidMount/etc and change the setTimeouts to just be function that pass through, you might get it to still work. You can also try extending Popover with class Popover2 extends Popover syntax and just use Popover2 so that you don’t have to overwrite the prototype chain.

…

On Oct 26, 2017, at 6:05 AM, Raphaël Morineau ***@***.***> wrote: @austinh Can you give more details about your hotpatching solution on Popover.prototype please ? since apparently this won't be fixed for M-UI < v1,x (?) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[FlackMonkey]

Got same issue:
[email protected]
[email protected]

Popover blinks for some few visible frames (1000ms/24) on top-left corner of screen and then shows up in correct place without transition.

Popover closes with part of transition and then dissappears.

Found situtaion: if you update DOM with sth after (in my case
<IconButton><ANyMUITSVGIcon/></IconButton) transition and positionning works normally, which is not a solution.

Waiting for ETA and fix itself. Thanks!

[m2mathew]

I am trying to dig into this issue, but I am having trouble re-creating it. The Material-UI docs site has it working, even if I upgrade it to use React 15.6.2. (Upgrading to React 16.x is very involved...trying to hold off on that unless we must do it.)

Does anyone have concrete steps that made this appear?

[gschjetne]

I have the problem using React 15.6.1 with the tap even plugin (2.0.1) and Material-UI 0.19.3.

I'll see if I can have someone build a minimal test project that re-creates the problem next week.

[romanzenka]

I see this issue with React 15.6.2, tap event plugin 2.0.1 and and Material-UI 0.19.4

[ViacheslavMylostyvyi]

There is reproducing https://github.com/leanmarine/material-ui-debug

[mattcarlotta]

Just to add to the above, I'm also getting this same pop over bug:

[email protected]
[email protected]
[email protected]

[oliviertassinari]

@ErreMalote I can't tell. I have personally stopped maintaining v0.x for quite some time.

[bstumpexb]

@LeeKevin where can i add this code to my project?
I tried adding to a file and 'Popover is not defined, popoveranimationdefault is not defained"
Should I add a simple .js file to the root solution directory somewhere?

[LeeKevin]

@bstumpexb You need to import the components first:

import React from 'react'
import { Paper, Popover } from 'material-ui'
import PopoverAnimationDefault from 'material-ui/Popover/PopoverAnimationDefault'

Make sure the prototype modifications are made before you render your React app.

[bstumpexb]

Thanks, worked like a charm. I added it to a separate file called 'preRender.js' and imported that file in the main app's 'index.js' file.

edit: don't know how to make code formatting work (pressed insert code button and then pasted in code...)

`
import React from 'react'
import { Paper, Popover } from 'material-ui'
import PopoverAnimationDefault from 'material-ui/Popover/PopoverAnimationDefault'

Popover.prototype.componentWillMount = function () {
this.renderLayer = () => {
const {
animated,
animation,
anchorEl, // eslint-disable-line no-unused-vars
anchorOrigin, // eslint-disable-line no-unused-vars
autoCloseWhenOffScreen, // eslint-disable-line no-unused-vars
canAutoPosition, // eslint-disable-line no-unused-vars
children,
onRequestClose, // eslint-disable-line no-unused-vars
style,
targetOrigin,
useLayerForClickAway, // eslint-disable-line no-unused-vars
scrollableContainer, // eslint-disable-line no-unused-vars
...other,
} = this.props

    let styleRoot = {
        ...style,
        opacity: this.state.setPlacement ? 1 : 0,  // MADE EDIT HERE
    }

    if (!animated) {
        styleRoot = {
            position: 'fixed',
            zIndex: this.context.muiTheme.zIndex.popover,
        }

        if (!this.state.open) {
            return null
        }

        return (
            <Paper style={Object.assign(styleRoot, style)} {...other}>
                {children}
            </Paper>
        )
    }

    const Animation = animation || PopoverAnimationDefault

    return (
        <Animation
            targetOrigin={targetOrigin}
            style={styleRoot}
            {...other}
            open={this.state.open && !this.state.closing}
        >
            {children}
        </Animation>
    )
}

}

Popover.prototype.componentWillReceiveProps = function (nextProps) {
if (nextProps.open === this.props.open) {
return
}

if (nextProps.open) {
    clearTimeout(this.timeout)
    this.timeout = null
    this.anchorEl = nextProps.anchorEl || this.props.anchorEl
    this.setState({
        open: true,
        closing: false,
        setPlacement: false,
    }, () => {
        // MADE EDIT HERE
        setTimeout(() => {
            this.setState({
                setPlacement: true,
            })
        })
    })
} else {
    if (nextProps.animated) {
        if (this.timeout !== null) return
        this.setState({ closing: true })
        this.timeout = setTimeout(() => {
            this.setState({
                open: false,
            }, () => {
                this.timeout = null
            })
        }, 500)
    } else {
        this.setState({
            open: false,
        })
    }
}

}
`

[pavanshinde47]

@oliviertassinari Is there is proper solution to this issue?

[bstumpexb]

Add this file into your /src directory.
Then import it from your index.js (or your main app) when react loads.

import './preRender'

This will overwrite all the popver animations to start with opacity 0. Also fixes inline calendar having the same problem

import React from 'react'
import { Paper, Popover } from 'material-ui'
import PopoverAnimationDefault from 'material-ui/Popover/PopoverAnimationDefault'

Popover.prototype.componentWillMount = function () {
this.renderLayer = () => {
const {
animated,
animation,
anchorEl, // eslint-disable-line no-unused-vars
anchorOrigin, // eslint-disable-line no-unused-vars
autoCloseWhenOffScreen, // eslint-disable-line no-unused-vars
canAutoPosition, // eslint-disable-line no-unused-vars
children,
onRequestClose, // eslint-disable-line no-unused-vars
style,
targetOrigin,
useLayerForClickAway, // eslint-disable-line no-unused-vars
scrollableContainer, // eslint-disable-line no-unused-vars
...other,
} = this.props

    let styleRoot = {
        ...style,
        opacity: this.state.setPlacement ? 1 : 0,  // MADE EDIT HERE
    }

    if (!animated) {
        styleRoot = {
            position: 'fixed',
            zIndex: this.context.muiTheme.zIndex.popover,
        }

        if (!this.state.open) {
            return null
        }

        return (
            <Paper style={Object.assign(styleRoot, style)} {...other}>
                {children}
            </Paper>
        )
    }

    const Animation = animation || PopoverAnimationDefault

    return (
        <Animation
            targetOrigin={targetOrigin}
            style={styleRoot}
            {...other}
            open={this.state.open && !this.state.closing}
        >
            {children}
        </Animation>
    )
}
}

Popover.prototype.componentWillReceiveProps = function (nextProps) {
if (nextProps.open === this.props.open) {
return
}

if (nextProps.open) {
    clearTimeout(this.timeout)
    this.timeout = null
    this.anchorEl = nextProps.anchorEl || this.props.anchorEl
    this.setState({
        open: true,
        closing: false,
        setPlacement: false,
    }, () => {
        // MADE EDIT HERE
        setTimeout(() => {
            this.setState({
                setPlacement: true,
            })
        })
    })
} else {
    if (nextProps.animated) {
        if (this.timeout !== null) return
        this.setState({ closing: true })
        this.timeout = setTimeout(() => {
            this.setState({
                open: false,
            }, () => {
                this.timeout = null
            })
        }, 500)
    } else {
        this.setState({
            open: false,
        })
    }
}
}

[deepaktomar47]

Thank you @bstumpexb 👍

[ashishsahu]

Wow its working Thanks @bstumpexb 💯

[pavanshinde47]

@bstumpexb Its a workaround so it can still cause issues. I just wanted to know from the team if they are working on a solution.

[oliviertassinari]

We do no longer plan any work on v0.x. I'm closing the issue. We are definitely not proud of this issue, but we have limited resources. Things have been taken care of on v1. Thanks for the report.

[Templaris]

Add this file into your /src directory.
Then import it from your index.js (or your main app) when react loads.

import './preRender'

This will overwrite all the popver animations to start with opacity 0. Also fixes inline calendar having the same problem

import React from 'react'
import { Paper, Popover } from 'material-ui'
import PopoverAnimationDefault from 'material-ui/Popover/PopoverAnimationDefault'

Popover.prototype.componentWillMount = function () {
this.renderLayer = () => {
const {
animated,
animation,
anchorEl, // eslint-disable-line no-unused-vars
anchorOrigin, // eslint-disable-line no-unused-vars
autoCloseWhenOffScreen, // eslint-disable-line no-unused-vars
canAutoPosition, // eslint-disable-line no-unused-vars
children,
onRequestClose, // eslint-disable-line no-unused-vars
style,
targetOrigin,
useLayerForClickAway, // eslint-disable-line no-unused-vars
scrollableContainer, // eslint-disable-line no-unused-vars
...other,
} = this.props

    let styleRoot = {
        ...style,
        opacity: this.state.setPlacement ? 1 : 0,  // MADE EDIT HERE
    }

    if (!animated) {
        styleRoot = {
            position: 'fixed',
            zIndex: this.context.muiTheme.zIndex.popover,
        }

        if (!this.state.open) {
            return null
        }

        return (
            <Paper style={Object.assign(styleRoot, style)} {...other}>
                {children}
            </Paper>
        )
    }

    const Animation = animation || PopoverAnimationDefault

    return (
        <Animation
            targetOrigin={targetOrigin}
            style={styleRoot}
            {...other}
            open={this.state.open && !this.state.closing}
        >
            {children}
        </Animation>
    )
}
}

Popover.prototype.componentWillReceiveProps = function (nextProps) {
if (nextProps.open === this.props.open) {
return
}

if (nextProps.open) {
    clearTimeout(this.timeout)
    this.timeout = null
    this.anchorEl = nextProps.anchorEl || this.props.anchorEl
    this.setState({
        open: true,
        closing: false,
        setPlacement: false,
    }, () => {
        // MADE EDIT HERE
        setTimeout(() => {
            this.setState({
                setPlacement: true,
            })
        })
    })
} else {
    if (nextProps.animated) {
        if (this.timeout !== null) return
        this.setState({ closing: true })
        this.timeout = setTimeout(() => {
            this.setState({
                open: false,
            }, () => {
                this.timeout = null
            })
        }, 500)
    } else {
        this.setState({
            open: false,
        })
    }
}
}

It works for me too when I had SelectField with MenuItems inside. When I added onFocus function to refresh items this flashing started to appear. After I add this hack solution everything works fine. Thanks man.

[m2mathew]

In our Call-Em-All app, we still have this happening in a few places that are using v0.x. We have been surgically going through and upgrading the annoying portions (like this popover issue) to v1.x with much success. We have a massive project underway to switch all components to v1.x, but it definitely takes a while with hundreds of components in a large app.

However, by using the v1.x Popover (or Popper or even Menu) in a few choice spots, it might be possible to avoid the modifications to the prototype of v0.x Popover that people seem to favor in this thread.

[riteshjagga]

Update: I was able to minimize the initial re-position animation by wrapping whatever is inside the Popover component with a Menu component, then it seems to open/close properly (it'll still show a flash from time to time, but significantly less than the above):

Other than Popovers, issue exists for SelectField, DropdownMenu components too. And wrapping DropdownMenu's content inside Menu doesn't show the selected value.