fix(popup): calculate trigger position on portal mount for manual ope…

[sdrothco]

This is a fix for issue 1065

Hello,
I recently ran across this bug myself and noticed the 'good first contribution' tag on it, so I decided to dig in. I hope I have followed the correct process!

When the popup property 'open' is true on initial render, handleOpen() does not get called, so the trigger coordinates are not set prior to calling setPopupStyle(), which causes all positioning to be aborted. I have added code in handlePortalMount() to collect the trigger coordinates on initial render.

handlePortalMount() does not have an event parameter to access the currentTarget (as is done in handleOpen), so I have added a ref to the trigger in the render.

I used ReactDOM.findDOMNode() to handle the case where the trigger is a React Component.

[DuratarskeyK]

I've done fix like that myself, just didn't have time to finish and commit, but there's a problem, when popup is initially off-screen and it's state is open, when you scroll to it it's in the wrong position. Could you check if this happens here as well?

[sdrothco]

Apologies for the delay in responding; life got crazy for a bit. I am not seeing the behavior you describe?
Maybe you can tell me more about the particular case in question?

For my test, borrowing heavily from keeslinp's codepen from the bug report, I have this in the render:

<Container fluid>
  <Button onClick= {()=>this.setState({open:!this.state.open})}>Open popup</Button>
  <div style={{ height:'2000px'}}> </div>
  <div style={{marginTop:'25px'}}>
    Here is the anchor
    <Popup
      open={this.state.open}
      on='click'
      onOpen={()=>this.setState({open: true})}
      onClose={()=>this.setState({open: false})}
      trigger={<span><Icon name='settings'/></span>}
      >
      Here I am
    </Popup>
  </div>
</Container>

The popup consistently opens below the trigger and stays there when I scroll down to see it. If the trigger is a block level element, the popup appears on the right hand side of the bottom of the page since the trigger takes the full width.

I did, however, run across a different bug in my PR. If the trigger is a stateless component, you can't attach a ref so the popup is back up in the right hand top corner. That's why the icon trigger in my example is wrapped in a <span>. I'll update my PR when I come up with a fix for that.

[sdrothco]

To handle the stateless triggers, I have wrapped the trigger in a <span>, which will allow a ref. This did require an update to the unit test for hover, which needs to move to the parent span for simulating mouseenter.

There may be other ways to handle needing a ref on the trigger, and I'm open to suggestion.

[codecov-io]

Current coverage is 95.88% (diff: 100%)

Merging #1190 into master will decrease coverage by <.01%

@@             master      #1190   diff @@
==========================================
  Files           879        879          
  Lines          4888       4884     -4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           4687       4683     -4   
  Misses          201        201          
  Partials          0          0          

Powered by Codecov. Last update 9fe3fca...cab5297

[levithomason]

There may be other ways to handle needing a ref on the trigger, and I'm open to suggestion.

We have this problem in a few areas, not knowing when we can use a ref or not. Such as here #1012 (comment).

The issue is exacerbated by the fact that all components can augment (render another component in their place). If a component internally utilizes a ref to itself, then is augmented by a stateless component, the ref breaks because it is now rendering as a stateless component and not a class component. This was the cause of #659, using a ref and then rendering as a stateless component.

// class component, uses a ref
<Dropdown />

// WARNING: Syntax shown here is no longer supported! Use <Dropdown item />
// Renders as a stateless component, ref breaks
<Dropdown as={Menu.Item} />

In conclusion, we need to solve the issue of ensuring a ref is available for all cases where we are accepting an element, such as, as={ ... } and trigger={ ... }.

I've got an idea for a component that can return the DOM node for both classes and stateless components. I'll give it some development time today and see where I end up. If successful, we can use this instead of adding the span.

[lewisblackwood]

Hey guys 👋 is there anything we could do to move this one along? Would be great to see the positioning issue with controlled popups resolved.

[levithomason]

I simply haven't got the time to finish this, though I've prototyped the solution. The idea is anywhere we expect to be able to use a ref, we need to ensure the underlying component is a class component and not a functional component. Since, functional components do not support refs, using them returns null which causes bugs when you expect null to be a DOM node.

The solution is to create a util or HOC that returns the element if it a class component, otherwise, it wraps the element in a class component so that it can have a ref. Once we have this utility, we can wrap all usages of refs that expect DOM nodes.

Whoever has time to do this ^ has the time and ability to close this issue along with some others.

[levithomason]

I've moved this comment to a new issue that details the blocker and fix: #1321

[levithomason]

Fixed conflicts to keep this PR from getting stale.

[kasbah]

I tried this fix and it didn't seem to solve the issue for me. It still exhibits the problem illustrated by this Codepen: https://codepen.io/anon/pen/mWEJMN (you have to hover over the red square otherwise the popup won't appear in the right location).

Edit: Removed outdated details.

[raptoria]

any update?

[levithomason]

In my view, the blocker is still the same as I noted above, #1190 (comment). No meaningful progress has been made in this area, yet.

[ghost]

Any updates on this? I see that the popups on the react semantic website moves correctly with the triggers, but when I try to use it, the popup stays where it was open, even on scrolling. All my popups are controlled.

[levithomason]

The blocking issue now has a PR, #1879. Once merged, this PR can implement it.

[github-bdem]

+1 for this

[levithomason]

Fixed in #2775