[PopoverOverlay] Fix regression introduced by removal of CSSTransition

[loic-d]

WHY are these changes introduced?

This PR fixes a regression in PopoverOverlay introduced by #1756

The onClose prop was fired when clicking or touching the element activating the Popover when that element was not the activator received as a prop.

In this case, the Popover would close right away.

It feels like a weird use case but that's how the ImageEditor in web is using Popover. The activator prop is just an empty div. The real Popover triggers are rendered higher up in the DOM tree.

WHAT is this pull request doing?

This PR fixes the regression.

In handleClick, we prevent onClose to be called when the source of the click is a descendant of the Popover or the Popover is being opened (this one was not working as expected).

We are now properly setting the transition status to Entering instead of Entered when the Popover is opened. It prevents clicks that are not coming from descendants to fire onClose while the Popover is opening.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Test the Playground code with and without the changes.

Before: closePopover is fired when opening the Popover.
After: closePopover won't be fired when opening the Popover (fix)

Copy-paste this code in playground/Playground.tsx:

import React from 'react';
import {ActionList, Button, Popover} from '@shopify/polaris';

export default class PopoverContentExample extends React.Component {
  state = {
    active: false,
  };

  openPopover = () => {
    console.log('openPopover called');
    this.setState({active: true});
  };

  closePopover = () => {
    console.log('closePopover called');
    this.setState({active: false});
  };

  render() {
    return (
      <div>
        <Button onClick={this.openPopover}>Toggle `Popover`</Button>
        <div style={{height: '250px'}}>
          <Popover
            active={this.state.active}
            activator={<div />}
            onClose={this.closePopover}
          >
            Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nulla vitae
            orci tellus. Pellentesque non elementum ipsum, at feugiat libero.
            Nulla id mollis nisi. Nam ut ligula mollis, commodo felis ut,
            consequat orci. In in sagittis magna, vitae rutrum mi. Ut facilisis,
            quam eu pulvinar elementum, lorem mauris lobortis leo, a posuere
            erat tortor in ante. Nunc fermentum nulla posuere, tincidunt velit
            id, pharetra velit. Ut congue varius semper. Nullam cursus
            ullamcorper sem, cursus euismod diam mollis id. Nullam sodales nibh
            libero, id auctor lectus condimentum eu. Curabitur dui velit, ornare
            sed vestibulum at, egestas at ante. Proin feugiat eros metus.
            Integer lacinia urna a orci accumsan, eget dignissim turpis pretium.
            Cras feugiat, mi convallis facilisis venenatis, odio lectus
            hendrerit velit, in sollicitudin magna nisi quis lacus. Proin purus
            nunc, posuere eu leo ultrices, hendrerit interdum massa. In non
            felis id justo egestas cursus. Pellentesque euismod quis sem in
            luctus. Nullam ultrices consequat nibh ac cursus. Morbi blandit
            finibus velit, ac scelerisque diam viverra vitae. In a orci orci.
          </Popover>
        </div>
      </div>
    );
  }
}

🎩 In web:

• Start the web server
• Clone polaris-react and checkout popover-overlay-fix
• yarn then build a consumer for web yarn run build-consumer web (still in polaris-react)
• Open a Product details page (React) with an image (or upload one)
• Edit the image but do not save it
• Click the < (back) button in the left panel, the X of the Modal or the 'Undo all' button
• Make sure the leave confirmation is rendered and works as expected. You should also be able to close it.

Web issue for context: https://github.com/Shopify/web/issues/17477

🎩 checklist

• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility
• Updated the component's README.md with documentation changes
• Tophatted documentation changes in the style guide

[loic-d]

Setting Entering ensures onClose is not fired in handleClick

https://github.com/Shopify/polaris-react/pull/2000/files#diff-f2a2881feff25a3d1e6fad684176b150R221

[jgodson]

I think we’d need to ensure this timeout gets cleared if the component gets unmounted?

[tmlayton]

Yeah probably a good practice to do this

[loic-d]

Totally. I'll clear the newly added setTimeout (entering) and the existing one (exiting).

[amrocha]

This is a good way of addressing the issue, sorry for not catching it earlier.

Is this urgent? Before we merge I'd really like to have a test for this so that we can avoid having the same issue in the future

A test would be really simple, in Popover.test.tsx you just need to render the example you have, and use spies to make sure that openPopover gets called but closePopover doesn't

[tmlayton]

What about the mount lifecycle? While there is no bug, it has the same hole where the state will be Entered while it is technically still Entering.

[tmlayton]

Otherwise looks good! But as @amrocha mentioned needs a test 🤖

[loic-d]

Now clearing the transition timers when the component unmounts.

[loic-d]

Added a test replicating the bug we encountered.

[loic-d]

@tmlayton Regarding the mount lifecycle, I believe we should not update the transition status all together. The component can be mounted but not visible active = false. WDYT? @AndrewMusgrave might have more context on this one.

[loic-d]

@tmlayton @amrocha

I updated the PR:

• Cleared transition timer when the component un-mounts
• Added test to make sure the regression was fixed
• Improved release notes

If you guys can take another 👀 at it today that would be great!

[dleroux]

Looks good to me.

[amrocha]

[loic-d]

Fix has been released in v4rc6

[BPScott]

Fixed in c6d321b, which was merged to master in #2002