improve hover outside handler logic #115

czabaj · 2021-02-05T13:45:36Z

This fixes the flickering problem when trigger="hover" and Trigger and Tooltip overlaps.

Partially relates #114

First commit also fixes some memoization bugs.

This fixes flickering Tooltip with `trigger="hover"` when the Tooltip overlaps the Trigger and cursor is moved to the overlaping section. Hovering overlaping section triggers `mouseleave` on the Trigger, which runs `hideTooltip()` but as the Tooltip disappears, the cursor encounters the Trigger again, `mouseenter` is triggered which runs `showTooltip()` and so it continues ad infinituum. This solution relies on `mousemove` instead and compares the cursor position with DOMRect of the Trigger. Also, when the `interactive` prop is enabled, it takes DOMRect of the Tooltip into consideration. This solution fixes another bug, which will occur with `trigger="hover"` in conjunction with `interactive` and big offset. The offset will create gap which will hide the Tooltip before the user hovers over it, so the `interactive` would not work, since the Tooltip will never be hoverable. This change virtualy increases the DOMRect of the Trigger to reach the Tooltip DOMRect so hovering over gap keeps the Tooltip opened.

codecov · 2021-02-05T13:47:46Z

Codecov Report

Merging #115 (29426fb) into master (3dfa356) will increase coverage by 2.77%.
The diff coverage is 92.10%.

@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
+ Coverage   90.97%   93.75%   +2.77%     
==========================================
  Files           2        2              
  Lines         133      160      +27     
  Branches       38       54      +16     
==========================================
+ Hits          121      150      +29     
+ Misses         12       10       -2

Impacted Files	Coverage Δ
src/usePopperTooltip.ts	`92.80% <88.00%> (+2.70%)`	⬆️
src/utils.ts	`97.14% <100.00%> (+1.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3dfa356...29426fb. Read the comment docs.

src/usePopperTooltip.ts

czabaj · 2021-02-05T13:57:31Z

src/usePopperTooltip.ts

    };
  }, [triggerRef, isTriggeredBy, showTooltip, hideTooltip]);
+  React.useEffect(() => {
+    if (!visible || triggerRef == null || !isTriggeredBy('hover')) return;


This follows the same condition as hover but it turns on only when visible is truthy.

src/usePopperTooltip.ts

mohsinulhaq · 2021-02-06T08:39:46Z

So the memo fix was definitely needed, but I'm wondering if instead of avoiding calculations on every mousemove, we could avoid flickering by using pointer-events: none, as you also had talked about. While the gap issue can be solved by passing a delayHide, as mentioned in the docs so that the user gets ample time to move over to the tooltip.

What are the pros and cons here according to you?

czabaj · 2021-02-07T07:56:55Z

@mohsinulhaq the pointer-events: none is a very simple fix but it only works with an arrow, you simply cannot apply pointer-events: none to the Tooltip as it wouldn't be able to be interactive, thus it won't work with negative offset, which is a very rare use case, but it adds another little bit to the caveats of the library.

The second problem with the potential gap between Trigger and the Tooltip in interactive mode is another caveat which users of the library must be aware of and the delayHide might not work very well in all scenarios.

The solution proposed here solves all the problems quite clearly without any caveats. If you are concerned with performance, I admit the getBoundingClientRect might be costly, but I did not measure its impact. Tooltip is not active most of the time, so a slight increase in its performance cost is fine by me. I could implement throttling for the mousemove event if you wish, although it is not trivial to implement event throttling in React without memory leaks, so I would rather not complicate this solution if it is not necessary.

mohsinulhaq · 2021-02-07T11:41:14Z

@czabaj then how about attaching to mouseleave event when interactive is false, adding pointer-events none to both tooltip and arrow (we already add that to tooltip with followCursor, you can add one more condition there) and attach to mousemove event only when interactive is true.

czabaj · 2021-02-07T14:10:58Z

@mohsinulhaq that sounds complicated, wouldn't be better to be free of thinking about when and why attach pointer-events: none? With this change, you can even remove the pointer-events: none for followCursor mode and make the logic simple IMHO. And, there is still the problem with the gap between Trigger and Tooltip, which this solution solves nicely and it could be even improved further, like increase the hover area for better usability.

The more I think about it, the more I'm sure that this logic clearly expresses what you need to achieve -- hide the tooltip when the cursor leaves some area. The CSS z-index is an obstacle, which this solution gets rid of completely. The only drawback is theoreticly performance, but I don't think it is that critical and if you think it is, throttling the mousemove event is possible.

mohsinulhaq · 2021-02-08T16:28:52Z

@czabaj If you could re-use the same logic to integrate with the virtual element construct from react-popper that we are using for follow-cursor, that would be even better.

czabaj · 2021-02-08T18:25:37Z

@czabaj If you could re-use the same logic to integrate with the virtual element construct from react-popper that we are using for follow-cursor, that would be even better.

Can you please elaborate more on this? I see the possibility to use only a single mousemove event for both cases, is that what you mean?

mohsinulhaq · 2021-02-08T20:57:06Z

@czabaj yep, that's exactly what I meant

czabaj · 2021-02-09T18:44:43Z

@mohsinulhaq look at the changes, I also added tests for isMouseOutside

czabaj · 2021-02-11T08:05:58Z

@mohsinulhaq hold on, I identified one bug. The mousemove event calls hideTooltip every time the mouse moves outside the elements until the visible = false. When there is hideDelay set, then every move outside the hover area prolongs the delay by calling hideTooltip thus the Tooltip hides effectively after the delay after the mouse stops moving.

I must call hideTooltip only once.

czabaj · 2021-02-11T08:18:17Z

@mohsinulhaq I added check which calls hideTooltip only when there is change mouseInside => mouseOutside

mohsinulhaq · 2021-02-11T08:27:27Z

ok thanks @czabaj, will review it soon

mohsinulhaq · 2021-02-13T16:16:31Z

@czabaj can you allow edits to the PR/branch. I have some changes to push to this before merging.

czabaj · 2021-02-13T18:34:38Z

@mohsinulhaq the "Allow edits by maintainers" is checked, is there any more options?

mohsinulhaq · 2021-02-13T19:28:21Z

@czabaj I have removed the pointer-events none we were using for follow-cursor, because I don't think it's needed anymore after your change. Correct me if I'm wrong.

czabaj · 2021-02-14T02:32:27Z

@mohsinulhaq correct.

czabaj · 2021-02-14T03:16:31Z

@mohsinulhaq as finishing touch, I improved the test for the "gap between" and added ESLint suppression pragma for useCalback dependencies. Feel free to revert the ESLint suppression, but it shall be there IMHO, for it expresses that violation of the rule is intended (ESLint complaints about the isArray ternary expression and usage of the array right away, as it is not able to assess correctness).

mohsinulhaq · 2021-02-16T07:50:50Z

@czabaj shall I go ahead and merge it?

czabaj · 2021-02-16T10:57:02Z

@mohsinulhaq I'm confident. Go ahead!

czabaj · 2021-02-16T12:29:16Z

@mohsinulhaq It was a pleasure, I like the selection of tooling in the repo and the code is easy to follow. If this creates some regression, don't hesitate to mention me in the issue. I will be glad to help.

mohsinulhaq · 2021-02-16T12:46:10Z

Thanks 😄

czabaj added 2 commits February 5, 2021 11:31

refactor: Improve React memoization

f30773f

czabaj commented Feb 5, 2021

View reviewed changes

src/usePopperTooltip.ts Show resolved Hide resolved

czabaj commented Feb 5, 2021

View reviewed changes

src/usePopperTooltip.ts Show resolved Hide resolved

czabaj commented Feb 5, 2021

View reviewed changes

src/usePopperTooltip.ts Show resolved Hide resolved

czabaj commented Feb 5, 2021

View reviewed changes

src/usePopperTooltip.ts Show resolved Hide resolved

mohsinulhaq requested a review from denisborovikov February 5, 2021 14:23

test: Add test for isMouseOutside

9747724

refactor: followCursor in global mousemove listener

894b6d0

czabaj force-pushed the fix/tooltip-flicker branch from c54499a to 894b6d0 Compare February 10, 2021 08:01

denisborovikov requested review from mohsinulhaq and removed request for denisborovikov February 10, 2021 17:31

fix: Mousemove does not prolong hide

5276d2a

mohsinulhaq added 2 commits February 14, 2021 00:55

remove pointer-events none for follow-cursor

5f3a20d

revert default offset to [0, 6]

d7112c7

czabaj added 2 commits February 14, 2021 04:09

test: Improve the "gap between" tests

961b379

chore: Suppress hook warnings

0ba14c5

mohsinulhaq approved these changes Feb 14, 2021

View reviewed changes

mohsinulhaq changed the title ~~Fix/tooltip flicker~~ improve hover outside handler logic Feb 14, 2021

bump version

29426fb

mohsinulhaq merged commit babbffc into mohsinulhaq:master Feb 16, 2021

afurnell mentioned this pull request Feb 18, 2021

Tooltip appears even after mouse leaves the trigger #117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve hover outside handler logic #115

improve hover outside handler logic #115

czabaj commented Feb 5, 2021

codecov bot commented Feb 5, 2021 •

edited

czabaj Feb 5, 2021

mohsinulhaq commented Feb 6, 2021

czabaj commented Feb 7, 2021

mohsinulhaq commented Feb 7, 2021

czabaj commented Feb 7, 2021

mohsinulhaq commented Feb 8, 2021

czabaj commented Feb 8, 2021

mohsinulhaq commented Feb 8, 2021

czabaj commented Feb 9, 2021

czabaj commented Feb 11, 2021

czabaj commented Feb 11, 2021

mohsinulhaq commented Feb 11, 2021

mohsinulhaq commented Feb 13, 2021

czabaj commented Feb 13, 2021

mohsinulhaq commented Feb 13, 2021

czabaj commented Feb 14, 2021

czabaj commented Feb 14, 2021 •

edited

mohsinulhaq commented Feb 16, 2021

czabaj commented Feb 16, 2021

czabaj commented Feb 16, 2021

mohsinulhaq commented Feb 16, 2021

improve hover outside handler logic #115

improve hover outside handler logic #115

Conversation

czabaj commented Feb 5, 2021

codecov bot commented Feb 5, 2021 • edited

Codecov Report

czabaj Feb 5, 2021

Choose a reason for hiding this comment

mohsinulhaq commented Feb 6, 2021

czabaj commented Feb 7, 2021

mohsinulhaq commented Feb 7, 2021

czabaj commented Feb 7, 2021

mohsinulhaq commented Feb 8, 2021

czabaj commented Feb 8, 2021

mohsinulhaq commented Feb 8, 2021

czabaj commented Feb 9, 2021

czabaj commented Feb 11, 2021

czabaj commented Feb 11, 2021

mohsinulhaq commented Feb 11, 2021

mohsinulhaq commented Feb 13, 2021

czabaj commented Feb 13, 2021

mohsinulhaq commented Feb 13, 2021

czabaj commented Feb 14, 2021

czabaj commented Feb 14, 2021 • edited

mohsinulhaq commented Feb 16, 2021

czabaj commented Feb 16, 2021

czabaj commented Feb 16, 2021

mohsinulhaq commented Feb 16, 2021

codecov bot commented Feb 5, 2021 •

edited

czabaj commented Feb 14, 2021 •

edited