onOuterAction triggers erroniously when click is on an element changed by react

[oliversong]

When you click on something that toggles its own existence based on react state, the event.target isn't in the DOM anymore and it triggers onOuterAction. A common case: a div inside the popover that changes to another div when you click on it, based on some underlying state.

The relevant react-popover code:

  checkForOuterAction: function checkForOuterAction(event) {
    var isOuterAction = !this.containerEl.contains(event.target) && !this.targetEl.contains(event.target);
    if (isOuterAction) this.props.onOuterAction();
  },

When checkForOuterAction is called, event.target returns the div, but the div is no longer in the DOM because it was rerendered to be the other div. Therefore isOuterAction returns true.

I don't think this.containerEl.contains(event.target) is a robust solution. Unless there's a good way to make this work with react's rerendering system, it seems like onOuterAction needs to be implemented with an invisible page wide overlay div underneath the popover.

[jasonkuhrt]

Thanks for raising this issue. I did not try to reproduce yet but what you are saying makes sense.

I don't think this.containerEl.contains(event.target) is a robust solution.

For now I agree, it seems that way.

it seems like onOuterAction needs to be implemented with an invisible page wide overlay div underneath the popover.

Let me clarify and see if I understand you correctly.

A page wide overlay _under_ the popover would make it possible to capture any event outside its bounds given that any interactions over the popover would never reach said overlay and so therefore we have a robust separation between the inner/outer, is that what you are suggesting?

If we did that doesn't that mean we delay page interaction by one event. For example if the user goes to tap some external button it will only disable the popover (+ overlay) after which they have to tap again. Thoughts?

[oliversong]

@jasonkuhrt yeah- that's exactly correct. That's good enough for my use case, but I can see how that could be problematic for some cases. I guess if you want to manipulate two things in the UI at once (one in the popover, one outside of it), it wouldn't work out.

[clayne11]

I think having a second click would be fine personally. I have some issues with the popover closing in certain situations when I'd prefer it didn't.

Example: Popover is open with a text input, user enters something into the input and submits. Upon submit a login modal opens. When the user interacts with the login modal, the popover closes. I would prefer to be able to layer an overlay on top of the popover that would absorb the clicks and not hit "onOuterAction".

[jasonkuhrt]

@oliversong Do you have a minimal repro case? I have this so far but its not trigger the reported issue:

const Main = React.createClass({
  getInitialState () {
    return {
      isOpen: false,
      isFirstBody: true
    }
  },
  secondBody () {
    this.setState({
      isFirstBody: false,
    })
  },
  closePopover () {
    this.setState({ isOpen: false })
  },
  openPopover () {
    this.setState({ isOpen: true })
  },
  render () {
    return (
      <div id='app'>
        <Popover
        isOpen={this.state.isOpen}
        onOuterAction={this.closePopover}
        body={this.renderBody()}
        >
          <p
          className='.Target'
          onClick={this.openPopover}
          >
            Target
          </p>
        </Popover>
      </div>
    )
  },
  renderBody () {
    return (
      this.state.isFirstBody
        ? this.renderShowing()
        : <span>Respawned!</span>
    )
  },
  renderShowing () {
    return (
      <p onClick={this.secondBody}>
        Hit me
      </p>
    )
  }
})

[jasonkuhrt]

No feedback so closing for now.