Semantic-ui-react: Popup in Fixed Menu positioning

[cytey]

Steps

1. Have a fixed menu
2. Have a popup for a menu item inside the fixed menu
3. Have a long page with scrollbar
4. Open the popup for the menu item
5. Scroll the page

Expected Result

Popup should not move as the page scrolls

Actual Result

Popup move as the page scrolls

Version

0.63.5

Testcase

http://codepen.io/anon/pen/ygyrze?editors=0010#0

[levithomason]

Thanks for the superb report.

Indeed, the Popup assumes its position to be relative to the window page offset. As the page scrolls, the window.pageYOffset value changes and the Popup changes position vertically.

The Popup's offset should instead be relative to the trigger element's position. In your example, that would be the icon. If its position were relative to the trigger (the icon) position, then it would be properly fixed during scroll.

Workaround

Not ideal, but to prevent the bug for now, you can enable hideOnScroll. This will close the Popup on scroll rather than display it in the incorrect position.

[qoalu]

Somehow related concern about the hideOnScroll.
I noticed that when you have a full-page app, meaning the body does not scroll, but instead one of the container divs has the scrollbar then hideOnScroll does not work. I guess that function binds to scroll event on the body. How about binding on mouse scroll event (and arrow keys, I guess) instead?

Edit: or better yet supply the target element to watch the scroll events on.
Edit 2: I made a pull request for some changes to allow this. It's my first ever pull request, please be gentle.

[levithomason]

If I understand correctly, binding to the parent node's scroll event is the way to go. The same solution offered above should be able to solve this hideOnScroll bug. If the parent container is used for the offset and the scroll listener then the Popup position and behavior will be based on the parent it is rendered in.

I can see an issue where the immediate parent doesn't scroll but some ancestor does, which would circumvent the solution. In that case, we may need to calculate scroll as "a change in page offset of my immediate parent". This way, if the element containing the Popup moves on the page, the Popup closes.

These are just loose ideas that I might explore first if I were attacking this. Whoever picks this up may find they do not work.

[peonmodel]

hi, currently i encountered the same problem using sidebars and popup

my current solution is mainly setting the useCapture flag for the addEventListener

handlePortalMount = (e) => {
    debug('handlePortalMount()')
    if (this.props.hideOnScroll) {
      window.addEventListener('scroll', this.hideOnScroll, true) // useCapture = true
    }

    const { onMount } = this.props
    if (onMount) onMount(e, this.props)
  }

hideOnScroll = (e) => {
    // check that event.target is an ancestor element that is scrolling
    const element = ReactDOM.findDOMNode(this.refs.element);
    if (event.target.contains(element)) {
        this.setState({ closed: true })
        window.removeEventListener('scroll', this.hideOnScroll, true)
        setTimeout(() => this.setState({ closed: false }), 50)
    }
  }

[siarheipashkevich]

I have also faced this problem.
Could you please resolve this problem?

[s-devaney]

Me too

[rijk]

No time to do a full PR, but I use this override to fix the problem. It looks at the mountNodes top and scroll position, and takes that into account when calculating the position of the tooltip.

This assumes that the mountNodes position is set to relative (but it does not have to be the element that is overflowing per se, it can be a child).

class MyPopup extends Popup {
  handleOpen = (e) => {
    let { top, bottom, left, right, width, height } = e.currentTarget.getBoundingClientRect()
    const { mountNode } = this.props

    if ( mountNode ) {
      const mountNodeTop = mountNode.getBoundingClientRect().top
      const mountNodeOffset = -mountNodeTop + mountNode.scrollTop

      top += mountNodeOffset
      bottom += mountNodeOffset
    }

    this.coords = { top, bottom, left, right, width, height }

    const { onOpen } = this.props
    if (onOpen) onOpen(e, this.props)
  }
}

[Avivhdr]

I Fixed it in the following wrapper component. Semantic-ui-react's Popup component has a _isStyleInViewport_ method .This method checks if the popup should be displayed in a different position, rather then the one it was passed to it in the _position_ prop, when there is not enough space for it in the current view port.
This wrapper component will overrides this method when passed a _fixed_ prop, causing the Popup to always open in it's giving _position_ prop.

The wrapper component:

/**
 *
 * PopupWrapper:
 *
 *
 * added 'fixed' prop:
 * will position the popup in it's original position,
 * regardless of the view port width/height.
 *
 */

import React from 'react';
import { Popup } from 'semantic-ui-react';
import PropTypes from 'prop-types';

class PopupWrapper extends React.Component {

  static propTypes = {
    fixed: PropTypes.bool,
  };

  constructor(props) {
    super(props);

    if (this.props.fixed) {
      Popup.prototype.isStyleInViewport = () => { };
    }
  }

  render() {

    const { fixed, ...popupProps } = this.props;

    return (
      <Popup {...popupProps}  />
    );
  }
}

export default PopupWrapper;

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 30 days if no further activity occurs. Thank you for your contributions.

[rijk]

I think we are still waiting for an official fix.. This stale bot is getting really annoying by the way.

[levithomason]

There is no official team addressing community needs. SUIR is and has always been a community addressing its own needs. There has been no activity here because the fix is not a high priority for anyone (maintainer or user).

Stale bot confronts an annoying but important reality. Many fixes and features simply will not get shipped. They are not invalid bugs or unhelpful features but the community has made it clear through collective inaction that these stale issues are not important enough to deliver on.

At some point, we have to admit to ourselves that no one actually cares enough to ship certain fixes and features. It then does not make sense to keep issues open for eternity. Instead, we focus on what is getting attention, which by nature are the higher priority and more impactful issues.

This is not a bad thing. It helps us in many ways including prioritization, focus, and feature set. As a maintainer, it also allows us to focus on helping PRs and issues that the community is most interested in. This is the best use of our extremely limited unpaid time.

This issue is well over a year old. Apparently, the maintainers and community at large believe everything else is more important. Since no one is willing to close it, the bot will help us move on with life :)

[Avivhdr]

Wasn't it already fixed here:
https://github.com/Semantic-Org/Semantic-UI-React/pull/2569
?

[levithomason]

Hm, I don't think so. I've updated the codepen to our latest build 0.80.0 using codesandbox here: https://codesandbox.io/s/846mr49219. I see the popup still scrolls with the page.

A popup will calculate its position and try to stay in the viewport by default. The linked PR allows users to opt out of this behavior by passing keepInViewport={false}.

Please let me know if I'm overlooking important nuance here.

[rijk]

@levithomason, first off this is a great package and I am extremely grateful for your and the other maintainers time and work.

The reason I said the bot is annoying (while I follow your reasoning), is because there were a number of issues I personally cared about, but either did not reply on in time, or my reply (via a thumbs down reaction on the stalebot) did not result in a 'unstalification'. These issues are now closed, and additional replies (after closing) have thus gone unnoticed.

This creates a subtle stress, and makes it seem like you don't care about an issue unless there is constant activity in the thread. In the worst case I am worried this will promote "+1" replies to keep issues 'alive', a thing that I was grateful we finally squashed here on Github.

However, to be honest, I understand your problem as well and I don't have a better solution (that won't cost you more time). It does 'revive' issues sometimes, like happened here, which is definitely not bad. I just want to make the point that just because you don't reply within X days doesn't mean you don't care about the issue — it might just be that you ran into it months (or years) ago, installed a workaround, and so you can not recall the exact issue from the top of your head anymore. This means you need some time to re-read the thread and look at your code to write a meaningful response (close or keep open please), and sometimes you cannot find that time within the bot's 14 days.

Anyway.. sorry for the off topic everyone. :)

[levithomason]

First, this is completely valid. I don't feel it is off topic as we don't have a dedicated channel for this kind of thing. I am just glad to see your care and initiative.

I'd like to address some key points you raised and perhaps provide a different view on others. At the least, we should consider changing the practice moving forward to be more helpful and accommodating.

These issues are now closed, and additional replies (after closing) have thus gone unnoticed.

We get notifications for all activity, including closed issues. We also try to respond to as many of those as we can. I do prioritize open PRs, then open issues, then closed issues/PRs when putting time into the project. I just want to make it clear that the maintainers are active even on closed issues and comments aren't unnoticed due to issue status.

Second, we can reopen issues easily, especially if they are unresolved! We do this a lot. If you feel an issue needs to be reopened, please let us know and it can be done.

This creates a subtle stress, and makes it seem like you don't care about an issue unless there is constant activity in the thread.

Stress around a project is not good and is something I'd like to resolve. We care about all issues but I do prioritize "hotter" issues over the less active ones. I think we can take steps to help with this. Perhaps we increase the stale timeframe and change the messaging. This way it happens less often and users know issues can be reopened easily.

just because you don't reply within X days doesn't mean you don't care about the issue

Also valid. Adding stalebot was a very tough decision I did not take lightly. It meant I was going to have to let go of things I deeply wanted to be done, but had to come to grips with the fact that I simply could not do them and no one in the community was willing or able to accomplish them either. This leaves me with the choice of perpetually keeping issues open solely for fear of closing them or in hopes that someone will come in the future and address them.

Proposals

We should probably increase the "days until stale" from 3 months to 6 months. The "days until close" can also be increased from 30 days to 60 days. This will make less stale issue stress. Also, if maintainers and the community cannot address an issue in 8mo, it seems reasonable to clean it up for housekeeping.

Update stalebot's messaging when labeling issues as stale. Community members shouldn't feel like they are being rejected nor that the maintainers don't care about the issue. The messaging should reflect that the issue is valid and that we'd love it to be resolved. It should also state the reality that, without a PR, the issue will not be resolved and therefore is being closed (not rejected!) for housekeeping.

The good news is that all issues closed for becoming stale are labeled stale. We can easily identify those issues and bring any of them back to life. This should be noted.

In the spirit of open source and doing a better job with community management, would you like to open a PR for this? The stale config is here and very easy to edit.

Finally, would you like to join us here? You've helped closed some issues, merged a PR, and clearly care for the project. I'd love for you to become a maintainer and continue input like this.

[rijk]

Thanks for the openness and explanation! It makes a lot of sense from your end, and it's great to see you care so much about the community interaction.

In the spirit of open source and doing a better job with community management, would you like to open a PR for this?

I will do a PR :)

Finally, would you like to join us here? You've helped closed some issues, merged a PR, and clearly care for the project. I'd love for you to become a maintainer and continue input like this.

Sure! I will try to help out where I can.

[fracmak]

Has there been much discussion around rendering the popup inline instead of in a portal? That is how SUI does it and it would solve this problem because the popup would move with the content of the window more naturally.

[layershifter]

@fracmak yep, I thought about this. I want to drop our positioning and use PopperJS there, should be exciting result.

[fracmak]

ooooh, I'll take a look. One of the projects I'm working on is looking into possibly building the inline popup, but if PopperJS is better, I can direct them that way instead.

[stale[bot]]

There has been no activity in this thread for 180 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 180 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

[CoryDuncan]

bump (running into this issue as well)

[averyga]

Popup probably listening to the scroll of a window only. If popup in a div it stays on one position when scrolling the div. Would be great to have scroll target as a prop
does anyone have a solution for this issue?
https://codesandbox.io/s/74mkmk5600

[averyga]

@layershifter is there any chance you've seen the issue?

[layershifter]

@averyga Please fill a separate issue for this as it exists in the latest master.

However, I can't offer you any estimations on it. It will be great if the issue will contain a resolution because now use Popper.JS and I almost sure that it can be fixed by an option.