Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tooltips remain visible after page navigation #2118

Closed
davwheat opened this issue Apr 10, 2020 · 3 comments · Fixed by #2166
Closed

Tooltips remain visible after page navigation #2118

davwheat opened this issue Apr 10, 2020 · 3 comments · Fixed by #2166

Comments

@davwheat
Copy link
Member

davwheat commented Apr 10, 2020

Bug Report

Current Behavior
Tooltips remain on screen when hovering over an element and starting a page navigation.

Steps to Reproduce
(video below)

  1. Go to https://discuss.flarum.org/
  2. Click on a stickied thread
  3. Hover over the sticky icon
  4. Press the back button on your mouse, or ALT + Left arrow
  5. MYSTERY TOOLTIP REMAINS :O

They also stack too, meaning you can start a 'who can get the most tooltips on screen at once' competition. I've gotten 11 so far :P

Expected Behavior
The tooltip should probably disappear, along with the rest of the page's content.

Screenshots
https://puu.sh/FvEHo/ff5810b7f5.mp4

Environment

  • Flarum version: 0.1.0-beta.12, I assume
  • Website URL: https://discuss.flarum.org/
  • Webserver: you tell me!
  • Hosting environment: VPS, I hope
  • PHP version: 7.0.0
  • Browser: Chrome 80

I've reproduced this on other Flarum sites, too.

@kalinjul
Copy link
Contributor

kalinjul commented May 7, 2020

I looked into this because i somehow liked the bug.
This appears to be only an issue with "Badges".
In Badges.js we have this:
https://github.com/flarum/core/blob/c3b2fa5a99e9ffbc6f9e8ff57a4bde69276af413/js/lib/components/Badge.js#L41

The other tooltips (for not-badges such as hovering over a user pic in the discussion list) , however, are created like this:
https://github.com/flarum/core/blob/85e26236221cdac3cfb1198c45c8ea13cce1c000/js/src/forum/components/DiscussionListItem.js#L107
So there is no container: 'body'

I tried to simply remove the container attribute, which fixed this bug. However, the container: 'body' was added in this commit: c3b2fa5. This fixed cut-off badges in the notification area.

So i tried other container attributes and indeed, using main works as tooltips correctly disappear while they're not cut off in the notification area.
Edit: I was mislead while i tried to turn off all caching for development.
What fixed the issue for me was adding this:

onunload() {
    if (this.props.label) this.$().tooltip('hide');
}

I'm going to provide a pull request so someone can decide whether this is a suitable fix.

@kalinjul
Copy link
Contributor

kalinjul commented May 7, 2020

Another, maybe better possible fix:
Remove the container attribute and also remove the overflow: 'hidden' for .Dropdown-menu less class. This way, tooltips can overflow the notification area and this bug is fixed, too.

This would also solve another bug: container: 'body', badges inside the notification area will not stick to their position when scrolling. Instead, they are fixed to their position inside body and will happily scroll away.

@kalinjul
Copy link
Contributor

kalinjul commented May 7, 2020

Video of scrolling badge tooltip bug which is also fixed in the second variant #
293c1cc:
https://imgur.com/a/AQOA7t7

askvortsov1 pushed a commit that referenced this issue May 16, 2020
…#2166)

* Don't use body as tooltip container, allow notification area overflow

Badge tooltips are using container: 'body', so they can overflow the
notification area. When the user navigates back while a badge tooltip is
showing, the tooltip remains visible.
This commit removes the body container attribute and instead allows the
notificationDropDown to overflow, so badge tooltips aren't cut off.
Instead, this adds overflow: hidden to NotificationList.
Fixes #2118.

* Remove newline
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants