T256213: Add a small delay to onMouseLeave #20

medied · 2020-07-09T13:55:08Z

Phab ticket: https://phabricator.wikimedia.org/T256213

What's needed:

add a small delay to the closing of the popup when the mouse leaves the word

We tested a few delays with this demo branch and we're setting delay to 300ms based on that.

stephanebisson

The idea is to prevent a quick "exit and re-enter" from closing the popup. This code is subject to race conditions between the closing after the delay and reopening in new mouseenter events. Put console.log in the events and you'll see what's going on. Ideally, a new mouseenter event should clearTimeout instead of trying to trigger a new opening.

medied · 2020-07-10T01:36:40Z

I had slightly misunderstood the ticket, you're right. How about 9c2f063? The more we test it, the better

hueitan · 2020-07-10T20:31:06Z

src/event.js

@@ -5,7 +5,22 @@ export const customEvents = popup => {
 			const toElement = e.toElement || e.relatedTarget || e.target
 			if ( toElement !== popup.element.currentTargetElement &&
 			!popup.element.contains( toElement ) ) {
-				popup.hide()
+				let timeoutId
+				const previewElement = popup.element.currentTargetElement,


popup.element.currentTargetElement used twice within the same scope, let's define previewElement in the top of the scope.

hueitan

👍 It looks good to me, ready for test.

jpita · 2020-07-14T07:44:15Z

Tested on android, windows (IE, Chrome and FF) and mac.
Found this small issue when 2 words are together.
When I move from one word immediately to the other, the popup on the 2nd word closes even though the mouse is over the 2nd word.
It seems we are not detecting the 2nd mouse-enter and stopping the timer.

hueitan · 2020-07-14T09:18:49Z

Tested on android, windows (IE, Chrome and FF) and mac.
Found this small issue when 2 words are together.
When I move from one word immediately to the other, the popup on the 2nd word closes even though the mouse is over the 2nd word.
It seems we are not detecting the 2nd mouse-enter and stopping the timer.

🥇 for u @jpita!!!! I can confirm this behaviour and we need to fix it.

review again after the fix

hueitan · 2020-07-14T20:51:12Z

Following what Pita found, what if when user mouse leaves the popup/previewElement to another wikipedia hyperlinks, then mouse move back to the popup, ALL within 300ms, do we show the new popup or keep the current popup? I think the latter. If so our code need to do extra guard here.

EDIT : just check on PagePreviews on the Wikipedia website, it opens the new popup immediately when use mouse hover the new hyperlink.

src/index.js

hueitan · 2020-07-16T20:59:50Z

note : to review this beb4a3a

jpita · 2020-07-19T13:21:23Z

this still has pending changes, right?

stephanebisson · 2020-07-21T12:54:48Z

Works well as far as I can tell but we need unit tests to verify the different scenarios: exit, exit and re-enter the same link or popup, exit and re-enter a different link.

medied · 2020-07-21T14:43:59Z

Yes as of e1e18a4 the expected behavior is overall working well but under the hood there are some event listeners we are still failing to remove. To address this, we are in the process of merging #23 into this PR.

Also, as noted in #23, when you are QAing this PR @jpita keep in mind that transitioning from a preview in one language to another one in a different language (all within 300ms) you will see a buggy behavior. We plan to address this in a new ticket

* POC - stack all event listener and timeout event * remove unused comment line * naming clearAllEventListener * var -> let

medied · 2020-07-22T03:19:29Z

src/index.js

@@ -16,7 +16,9 @@ function init( {
 			createPopup( popupContainer ),
 		events = customEvents( popup ),
 		showPopup = ( { target } ) => {
-			popup.hide()
+			if ( popup.element.style.visibility === 'visible' ) {


@hueitan just FYI: I realized tonight that always executing popup.hide() without a check was breaking the previews in mobile with the error below (it tries to remove a background screen that's not there), so I'm including this conditional

Uncaught TypeError: Node.removeChild: Argument 1 is not an object. removeBackgroundScreen webpack://wikipediaPreview/./src/touchPopup.js?:17 hide webpack://wikipediaPreview/./src/touchPopup.js?:49 showPopup webpack://wikipediaPreview/./src/index.js?:31

medied · 2020-07-22T03:31:38Z

...but we need unit tests to verify the different scenarios

Huei and I suspected these tests would be somewhat tricky/particular, I gave it a first try today and indeed ran into issues on how to approach them and think about measuring timing. I'm looking to pick someone's brain to understand what makes most sense. For what it's worth, this sub-branch shows what I tried: T256213-hover-closing-delay-tests-exploration.

@stephanebisson would you say the tests here are a merge blocker?

Either way, @jpita I'd say you can now start QAing this ticket. Just don't merge to master until we settle on tests

jpita · 2020-07-22T06:48:50Z

Either way, @jpita I'd say you can now start QAing this ticket. Just don't merge to master until we settle on tests

will move the task to code review until the code is finished so it's not stuck in QA

hueitan · 2020-07-22T10:46:58Z

Huei and I suspected these tests would be somewhat tricky/particular, I gave it a first try today and indeed ran into issues on how to approach them and think about measuring timing. I'm looking to pick someone's brain to understand what makes most sense. For what it's worth, this sub-branch shows what I tried: T256213-hover-closing-delay-tests-exploration.

@stephanebisson would you say the tests here are a merge blocker?

This is really tricky, this approach is going into an integration test which is not a unit test anymore.

I would suggest don't let this be the merge blocker, the unit test (only event.js) or e2e test (integration) should cover this scenario + this as well #23 (comment),

jpita · 2020-07-22T10:59:29Z

I can see what I can do with cypress
https://phabricator.wikimedia.org/T258574

hueitan · 2020-07-23T08:13:59Z

We should move on, @jpita, currently there is 0% in the integration test, let's qa this then feel free to add them later.

jpita · 2020-07-24T05:27:28Z

ok, this is tested, feel free to merge when you think is ready

Add 300 ms delay to onMouseLeve

c8ae4b8

stephanebisson requested changes Jul 9, 2020

View reviewed changes

Handle quick exit and re-enter

9c2f063

hueitan reviewed Jul 10, 2020

View reviewed changes

Define previewElement at top of the scope

4ed40f7

hueitan previously approved these changes Jul 13, 2020

View reviewed changes

Fix issue when quickly transitioning to a new popup

e1e18a4

hueitan reviewed Jul 14, 2020

View reviewed changes

src/index.js Show resolved Hide resolved

hueitan mentioned this pull request Jul 17, 2020

stack timeout listener #23

Merged

hueitan and others added 2 commits July 21, 2020 10:48

stack timeout listener (#23)

27cdea0

* POC - stack all event listener and timeout event * remove unused comment line * naming clearAllEventListener * var -> let

Fix touch interaction

b423cc2

medied commented Jul 22, 2020

View reviewed changes

hueitan approved these changes Jul 23, 2020

View reviewed changes

stephanebisson merged commit 513b876 into master Jul 27, 2020

stephanebisson deleted the T256213-hover-closing-delay branch July 27, 2020 10:49

stephanebisson restored the T256213-hover-closing-delay branch May 13, 2021 17:27

stephanebisson deleted the T256213-hover-closing-delay branch May 13, 2021 17:31

stephanebisson restored the T256213-hover-closing-delay branch July 28, 2022 12:56

stephanebisson deleted the T256213-hover-closing-delay branch July 28, 2022 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T256213: Add a small delay to onMouseLeave #20

T256213: Add a small delay to onMouseLeave #20

medied commented Jul 9, 2020

stephanebisson left a comment

medied commented Jul 10, 2020

hueitan Jul 10, 2020

hueitan left a comment

jpita commented Jul 14, 2020

hueitan commented Jul 14, 2020

hueitan commented Jul 14, 2020 •

edited

Loading

hueitan commented Jul 16, 2020

jpita commented Jul 19, 2020

stephanebisson commented Jul 21, 2020

medied commented Jul 21, 2020

medied Jul 22, 2020

medied commented Jul 22, 2020

jpita commented Jul 22, 2020

hueitan commented Jul 22, 2020

jpita commented Jul 22, 2020 •

edited

Loading

hueitan commented Jul 23, 2020

jpita commented Jul 24, 2020

T256213: Add a small delay to onMouseLeave #20

T256213: Add a small delay to onMouseLeave #20

Conversation

medied commented Jul 9, 2020

stephanebisson left a comment

Choose a reason for hiding this comment

medied commented Jul 10, 2020

hueitan Jul 10, 2020

Choose a reason for hiding this comment

hueitan left a comment

Choose a reason for hiding this comment

jpita commented Jul 14, 2020

hueitan commented Jul 14, 2020

hueitan commented Jul 14, 2020 • edited Loading

hueitan commented Jul 16, 2020

jpita commented Jul 19, 2020

stephanebisson commented Jul 21, 2020

medied commented Jul 21, 2020

medied Jul 22, 2020

Choose a reason for hiding this comment

medied commented Jul 22, 2020

jpita commented Jul 22, 2020

hueitan commented Jul 22, 2020

jpita commented Jul 22, 2020 • edited Loading

hueitan commented Jul 23, 2020

jpita commented Jul 24, 2020

hueitan commented Jul 14, 2020 •

edited

Loading

jpita commented Jul 22, 2020 •

edited

Loading