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

Fix popover issues with hover display. #13104

Merged
merged 5 commits into from
Dec 5, 2018
Merged

Fix popover issues with hover display. #13104

merged 5 commits into from
Dec 5, 2018

Conversation

goldengecko
Copy link
Contributor

There are two issues addressed in this pull request:

  1. When the trigger is set to "hover", the popover is also triggered on focus, which is incorrect behaviour. Fixed by setting the handling of handleFocus and handleBlur to just show/hide the popover when the trigger is set to "click" or "focus".

  2. When the trigger is set to "hover" the timer is established on hover, but if the thing that you are hovering over is a button that you click to go to another page, then the timer is not cleared. The issue with this is most visible if the component is a keepalive one, so it is not destroyed. The timer fires after the page is gone, leaving a rogue popover at the top of the page.

I therefore implemented a "beforeDestroy" function and "deactivated" function to handle both keepalive and non keepalive components. Both of these call the cleanup function which cancels the timer and makes sure the popover isn't displayed.

…it to show on hover.

Fix issue with popover timer still running when button clicked and parent element no longer visible (e.g. keepalive component).
@element-bot
Copy link
Member

element-bot commented Oct 21, 2018

Deploy preview for element ready!

Built with commit fbfd588

https://deploy-preview-13104--element.netlify.com

@goldengecko
Copy link
Contributor Author

I'm not sure why but the Travis build says it's still in progress even when the details show that it completed successfully. Hope this doesn't block merge.

@goldengecko
Copy link
Contributor Author

The failure of the test seems to be completely unrelated to the change I made since I only edited the documentation which was missing some format options which are supported in utils/date.js

@ziyoung
Copy link
Contributor

ziyoung commented Oct 23, 2018

Which issue does this pr fix? Could you please provide a demo?

@goldengecko
Copy link
Contributor Author

goldengecko commented Oct 23, 2018

The first issue is simply demonstrated:

<el-popover content="Something" trigger="hover" :open-delay="5000">
  <el-button slot="reference">Click me</el-button>
</el-popover>

Expected: you need to hover over the button for 5000ms before the popover shows and there is no way to show the popover before the open delay is complete.
Actual: the popover shows immediately when you click the button.
Fixed by: making the popover only show on focus when the trigger is set to focus or click

The second issue is slightly more work. Set up your router to have a page kept alive:

<keep-alive include="mypage">
    <router-view>
    </router-view>
</keep-alive>

Now in the mypage component, have a popover on a button which takes you to a different page:

<el-popover content="Something" trigger="hover" :open-delay="5000">
  <el-button @click="$router.push('/anotherpage')">Click me</el-button>
</el-popover>

Expected: when you click the button, the popover will not show.
Actual: when you click the button, the popover is shown at the top left corner of the browser window (presumably since its anchor element is missing)
Fixed by: ensuring that when the component is deactivated or destroyed, any pending timer is destroyed and the popover is hidden. To be even more thorough, the this.showPopper = false; line could even be moved below the if statement to clean up in other cases like manual display.

@goldengecko
Copy link
Contributor Author

Is it possible to get this merged soon? I have a project going into beta testing shortly and without this bugfix it leaves a popover from the previous page showing, as previously described.

The attached video shows both the problems:

  1. Popover appears as soon as I click even though I configured it to show only on hover after delay.
  2. Popover remains even after next page is loaded for reasons previously described.

My pull request fixes both of these issues.

popover.mp4.zip

@ziyoung ziyoung self-assigned this Nov 26, 2018
@ziyoung
Copy link
Contributor

ziyoung commented Nov 26, 2018

add a issue link: https://jsfiddle.net/dpc78qb4/

packages/popover/src/main.vue Outdated Show resolved Hide resolved
packages/popover/src/main.vue Outdated Show resolved Hide resolved
@ziyoung ziyoung merged commit 911ca68 into ElemeFE:dev Dec 5, 2018
imzjy pushed a commit to imzjy/element that referenced this pull request Dec 5, 2018
* Fix issue with popover displaying on focus when configuration is for it to show on hover.
Fix issue with popover timer still running when button clicked and parent element no longer visible (e.g. keepalive component).

* Updated to pass tests

* Correct documentation for el-date-picker

* Changes as per ziyoung review
@island205 island205 mentioned this pull request Jan 25, 2019
3 tasks
weisiren168 pushed a commit to weisiren168/element that referenced this pull request Jun 20, 2019
* Fix issue with popover displaying on focus when configuration is for it to show on hover.
Fix issue with popover timer still running when button clicked and parent element no longer visible (e.g. keepalive component).

* Updated to pass tests

* Correct documentation for el-date-picker

* Changes as per ziyoung review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants