I'm not sure what the guidance is for solving this.

If you right click a link you trigger a mouseleave/blur event which hides the popup.

The only way I can think to solve this is as follows:
Don't log anything in this situation - if the user is using the right clicking on a link (not the hovercard) are they really interacting with hovercards?

Thoughts?

I've written some hygiene patches during my investigations to untangle what's going on here - Ia9f4ae6e343c9114c9fde7d9e98899d4eb43998a

Someone should feel free to take this over from me before next standup.
I've been mostly getting familiar with the logging code and trying to get it to a position where I can add unit tests and keep it in one place. @Jhernandez you've got mail => https://github.com/joakin/hovercards-tests/pull/1
@ovasileva => https://phabricator.wikimedia.org/T147846#2710768

Just to clarify:

• right-clicking does not trigger a focus event and is recorded correctly
• ⌘-clicking triggers a focus event which zeroes the time causing the low interaction times

Do we have a way of filtering these?
How do we track ⌘-click on other browsers?
Do we know why this is only an issue for hovercards off? (is it?)

In T147846#2710768, @Jdlrobson wrote:

I'm not sure what the guidance is for solving this.

If you right click a link you trigger a mouseleave/blur event which hides the popup.

The only way I can think to solve this is as follows:
Don't log anything in this situation - if the user is using the right clicking on a link (not the hovercard) are they really interacting with hovercards?

Thoughts?

I think they're both important, in terms of measuring interaction times with hovercards on and off.

Hovercards off:

1. User sees page of interest
2. User opens in new tab

expected - fairly short interaction time

Hovercards on:

1. User sees page of interest
2. User hovers and views hovercards
3. User continues to open page in new tab

hypothesis - perhaps a longer interaction time? (assuming they're reading the hovercard)

Change 315583 had a related patch set uploaded (by Phuedx):
Log events in one central place

https://gerrit.wikimedia.org/r/315583

In T147846#2710772, @Jdlrobson wrote:

I've written some hygiene patches during my investigations to untangle what's going on here - Ia9f4ae6e343c9114c9fde7d9e98899d4eb43998a

@gerritbot can write that comment for you!

In T147846#2712071, @ovasileva wrote:

Do we know why this is only an issue for hovercards off? (is it?)

IIRC it is only an issue for the Hovercards OFF. Hovercards ON keeps track of which link was last interacted with by the user so that it can handle situations like this.

I believe this task is effectively a duplicate of T142723: Title attributes not properly removed on hover. Fixing the problems with the focus event described in that task will most likely solve this issue as I believe some of the logging is done from the related code. I'd like to suggest merging this task into T142723 and pulling T142723 into the sprint. We can always reopen this task if it turns out the two are not related.

I'm sorry, I've not made much progress on this. The event logging is a maze and I'm really keen to make this code easier for us all to work with.

Devs: We currently bind data to every single link and use this when we log. I'd rather we were event driven and let the events set this data in one central place. For example there are 2 events we do not log but events in the Hovercards lifecycle - the hovering of a link and the display of the hovercard. I'd like to record when these events happen and use them to drive the population of the data we log. So for example perceivedWait would be calculated on a display event and dwellStartTime on a hover event. I wanted to check this sounds sane to everyone before continuing with this approach. I've jotted some rough ideas down in patch form here:
https://gerrit.wikimedia.org/r/315862

If anyone has a better idea for an approach also please chip in with those ideas.

In T147846#2715077, @Jdlrobson wrote:

I'm sorry, I've not made much progress on this. The event logging is a maze and I'm really keen to make this code easier for us all to work with.

IMO, that would be amazing and worth the time, even if it takes a little while, especially considering all the issues we've had. Do you have any more concrete ideas on how to organize this?

@Jdlrobson: [We use this same approach in MobileWebSearchLogger](https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/resources/mobile.search/MobileWebSearchLogger.js). The tests are also easy to read as they simply send events in (by triggering the event handlers) and make asserts about the events sent out.

Change 316481 had a related patch set uploaded (by Jdlrobson):
Multiple hover events should not clear dwellStartTime

https://gerrit.wikimedia.org/r/316481

Okay I've got a simplified approach I'm happy to run with for the time being. I've opened up a new task T148468 to describe the things that I'm seeing that I do not like.

I've not sure if it's out of scope for this task, but if a user right clicks on a menu and clicks a link, it's worth pointing out we don't currently log a "opened in new tab" event.

See my comments on rEPOPddf88c5cb462: Event and hover events are logged but not recorded and rEPOP4b0e04fd83ca: ext.popups.schemaPopups now manages logic of whether to log an event.

@phuedx is there actually a way to view your comments in diffusion or a way to navigate to the gerrit patch via these links you paste? I see no way....

See the ChangeId property in the Details section of the Diffusion page.

I misread your comment, ignore my previous response.

FYI https://gerrit.wikimedia.org/r/#/c/316415 also needs review.

@Jdlrobson: In future, could you keep hygiene changes like rEPOP7dfad14da17c: Directory structure should reflect the ResourceLoader definitions out of the change of changes? While it's a useful change, it's not clear to me why it's critical, i.e. why it should block merging the other changes that fix this bug.

Further to my review of PS3 of rEPOPee1db4eda616: Event and hover events are logged but not recorded and T147846#2717292, I drew the following, which I hope clarifies what I was thinking:

@phuedx as I explained earlier the hygiene patches helped me understand the code and problem so was necessary and critical for me to get this work done, even if they seem unrelated to the goal. I was getting extremely confused with what code went where and when it was loaded.

Update on this task - please help:

I'm hoping rEPOP7dfad14da17c: Directory structure should reflect the ResourceLoader definitions helps reviewers too but I should note that if it's really seen as getting in the way you can manually cherry pick the patch that follows onto the master branch without problem (note not via Gerrit interface as Gerrit cannot handle renaming folders).

rEPOP74231eba4a86: Hygiene: Remove dead code is +2ed. If someone is impatient and would like to see this merged see above comment, this can easily be merged now.

rEPOP9ea5d8dbcc84: Hygiene: Rename track topic to be more consistent with new plans has been amended and needs review. Hopefully +2able.

rEPOP87e7b0520ac7: Event and hover events are logged but not recorded is -1ed by Sam but I've replied to his comments and am waiting for a reply on rEPOP7b37314cc4a8: ext.popups.schemaPopups manages logic of whether to log events. @jhobs @Jhernandez
@bmansurov @pmiazga input encouraged.

rEPOP6df26eedbbf4: Multiple hover events should not clear dwellStartTime which finally fixes the problem is still needing some kind of review.

rEPOP7dfad14da17c: Directory structure should reflect the ResourceLoader definitions and rEPOP74231eba4a86: Hygiene: Remove dead code have been merged. Neither change should change the behaviour of the EventLogging code.

rEPOP6df26eedbbf4: Multiple hover events should not clear dwellStartTime was reviewed by @jhobs on Friday.

I've reviewed rEPOP366eddd7addf: Hygiene: Rename track topic to be more consistent with new plans but it needs testing and I'm about to go on a break.

… and rEPOP073075936018: Hygiene: Move logic for duplicate events into getMassagedData needs a little work but looks promising.

@Jdlrobson: I'm having a hard time following the chain in Gerrit. The new interface lists a lot of related commits, which change as you move through the chain. Could you list the changes that need merging again?

I'm having a hard time myself and I wrote them :)
I'm having a lot of trouble rebasing so please focus on the earlier patches in the chain - I want some reassurance I am taking the right approach.

Remaining patches are:

• Hygiene: Move logic for duplicate events into getMassagedData - rEPOP22e4ebe57c85: Hygiene: Move logic for duplicate events into getMassagedData
  • @phuedx please clarify if you mean I should rename the method as this effects all the patches that follow it.
• Event and hover events are logged but not recorded (currently +2ed)- rEPOP9a9b3e35524c: Event and hover events are logged but not recorded
• ext.popups.schemaPopups manages logic of whether to log events -

rEPOP7b37314cc4a8: ext.popups.schemaPopups manages logic of whether to log events

• Multiple hover events should not clear dwellStartTime{6df26eedbbf4677cc1b4526c0cba40474d2cea99}

Note well that only rEPOP6df26eedbbf4: Multiple hover events should not clear dwellStartTime needs to be reviewed now.

In T147846#2715077, @Jdlrobson wrote:

I'm sorry, I've not made much progress on this. The event logging is a maze and I'm really keen to make this code easier for us all to work with.

Devs: We currently bind data to every single link and use this when we log. I'd rather we were event driven and let the events set this data in one central place. For example there are 2 events we do not log but events in the Hovercards lifecycle - the hovering of a link and the display of the hovercard. I'd like to record when these events happen and use them to drive the population of the data we log. So for example perceivedWait would be calculated on a display event and dwellStartTime on a hover event. I wanted to check this sounds sane to everyone before continuing with this approach. I've jotted some rough ideas down in patch form here:
https://gerrit.wikimedia.org/r/315862

So from the data analysis perspective, things have been facilitated a little by the fact that the schema was designed to log only one event per link interaction (apart from the case of "tapped settings cog" + "disabled", which occurred only very rarely in the huwiki experiment).
But I agree that expanding the schema in this way could make sense - it will cause a little extra work during analysis, but should also yield some useful additional information.

I have made a state diagram for the current Schema:Popups instrumentation, which should also help thinking about these issues. E.g. the two additional events you propose would correspond to the arrows on the bottom right and top center left.

That's a great diagram @Tbayer

Notes that I've made while reviewing @Jdlrobson's change:

• When the page loads I should see a page loaded event 👍
• After the page has loaded, when I hover over a link and left-click it I should see an opened in same tab event 👍
  • Should the event have the perceivedWait property?
• After the page has loaded, when I hover over a link and ⌘-click it I should see an opened in new tab event 👍
  • Should the event have the perceivedWait property?
• After the page has loaded, when I hover over a link, wait, and then move my mouse away from the link I should see a dwelledButAbandoned event 👍
• After the page has loaded, when I hover over a link, wait until the popup appears, and then move my mouse away from the link I should see a dismissed event 👍
  • And I repeat the interactions for the same link I should see a dismissed event with a different linkInteractionToken property 👍
    • BUG perceivedWait and totalInteractionTime always increase.

In T147846#2748417, @phuedx wrote:

Notes that I've made while reviewing @Jdlrobson's change:

• When the page loads I should see a page loaded event 👍
• After the page has loaded, when I hover over a link and left-click it I should see an opened in same tab event 👍
  • Should the event have the perceivedWait property?

No, only if the link is clicked from within an already rendered card. (At least that has been my understanding, see also the description in the schema documentation, which defines perceivedWait as the "Amount of time (in milliseconds) between the instant the user started dwelling on a link and the instant the preview text rendered visibly, as determined strictly client-side. Not set if hovercards disabled.")

• After the page has loaded, when I hover over a link and ⌘-click it I should see an opened in new tab event 👍
  • Should the event have the perceivedWait property?

same as above

• After the page has loaded, when I hover over a link, wait, and then move my mouse away from the link I should see a dwelledButAbandoned event 👍
• After the page has loaded, when I hover over a link, wait until the popup appears, and then move my mouse away from the link I should see a dismissed event 👍
  • And I repeat the interactions for the same link I should see a dismissed event with a different linkInteractionToken property 👍
    • BUG perceivedWait and totalInteractionTime always increase.

Change 316481 merged by jenkins-bot:
Multiple hover events should not clear dwellStartTime

https://gerrit.wikimedia.org/r/316481

From my review of rEPOP9477fd5fe6a2: Multiple hover events should not clear dwellStartTime:

This LGTM.

I went over all the scenarios described in T147846#2748417 as well as the scenario described in the original bug report. I also re-checked that perceivedWait was being added to events that are logged after the "display" lifecycle event, i.e. "dismissed", "tapped settings cog", "disabled".

The bug is still happening. Steps to reproduce:

1. Visit http://reading-web-staging.wmflabs.org/wiki/Book
2. Hover over the first link, which is the word "ink" in the first paragraph, and wait about 5 seconds.
3. Command + Click.
4. Inspect the Network tab, you'll see that the totalInteraction time is around 100ms.

Here is a screencast of the above steps:

Thanks, @phuedx.

The interaction time seems to be correct. Opening a link in a new tab using the context menu (right click on the mouse) is sending the "dwelledButAbandoned" and not "opened in new tab" event.

@bmansurov: I thought that I'd noted this during my review but then I found that @Jdlrobson and I discussed it in IRC.

Which browsers fire an event for a context menu item being clicked? Chrome doesn't appear to.

11:12:48 <phuedx> jdlrobson: hover, wait for popup to appear, right-click and open in new tab -> dismissed event
11:13:33 <jdlrobson> phuedx: what's the problem? it shouldnt fire a dismissed event?
11:14:49 <phuedx> jdlrobson: i //assumed// (keyword) that i'd opened the link in a new tab, that i'd see an opened in new tab event
11:15:32 <phuedx> i can't write a failing test for it as there's no "click" lifecycle event
11:15:53 <jdlrobson> phuedx: https://phabricator.wikimedia.org/T147846#2710768
11:15:53 <jdlrobson> If you right click a link you trigger a mouseleave/blur event which hides the popup.
11:16:00 <jdlrobson> hence the dismissed event
11:16:10 <jdlrobson> The only way I can think to solve this is as follows: Don't log anything in this situation - if the user is using the right clicking on a link (not the hovercard) are they really interacting with hovercards?
11:17:42 <jdlrobson> we can add a click event, but it's also going to trigger a dismissed event
11:17:52 <jdlrobson> which will leave it in an ambiguous state
11:18:39 <jdlrobson> unless im missing something obvious..?
11:20:03 <phuedx> hrrrm
11:20:08 <phuedx> olliv ^
11:20:39 <phuedx> jdlrobson: to your point about "interacting with hovercards": we're logging interactions with links too
11:25:07 <@olliv> jdlrobson:  there's no way to know what they did after the right click?  We should log it as a link interaction either way + whatever they decided to do afterwards.  I agree that they're not really interacting with the hovercard though
11:26:01 <jdlrobson> so consider a case where the hovercard is open and the user right clicks the link
11:26:09 <jdlrobson> say the hovercard stays open
11:27:09 <jdlrobson> if the user clicks any context menu item we can detect it, but i dont think we can detect which one.
11:27:20 <jdlrobson> so if they click copy link in address - what do we do? trigger an open in new tab event?
11:28:32 <jdlrobson> let's say we trigger a new event with an update in schema 'clicked-context-menu-item' we could then end the hovercard lifecycle, trigger the event.
11:30:48 <phuedx> i agree that it's tricky
11:31:57 <phuedx> if there's a dom event associated with the "open link in new tab" entry of the context menu, then i don't see why we shouldn't log it
11:32:34 <phuedx> but if the schema doesn't support this, then we shouldn't extend it right this instant
11:36:13 <phuedx> jdlrobson: okay, chrome doesn't fire such an event
11:39:01 <phuedx> olliv, jdlrobson: yeah i think we can detect the right click but the schema doesn't provide for it at the moment