Page MenuHomePhabricator

Page previews shouldn't be shown when user switches back to a tab
Open, MediumPublic

Description

Consider the scenario in which a user is reading a article (e.g.) God. He then opens another article (e.g.) monotheism linked in it, in a new tab using the shortcut Ctrl + click. After reading the article a little he switches back to the tab containing the previous article using the shortcut Ctrl + tab / Ctrl + Shift+ tab. In this case he wouldn't expect to see a link preview but a preview is shown as the keyup listener is triggered by the Control or Shift keys.

To prevent this situation it would be better if the popups aren't shown for when the keyup listener is triggered due to the Control and Shift keys. This shouldn't cause an issue as they seem to be meta-keys and don't much job by themselves.

Proposed solution

We listen to keyup so that we respond to <TAB> presses to show previews on tab. Right now all keys are processed when focused on a link to trigger the linkDwell action.

Using jQuery's event.which, we should whitelist TAB when the event is keyup instead of trying to blacklist all possible shortcut keys for moving around tabs in the browser or processing all of them unconditionally like right now.

Pseudocode:

diff --git a/src/index.js b/src/index.jsowser as it 
index 0ebe48e..181e66f 100644
--- a/src/index.js
+++ b/src/index.js
@@ -169,7 +169,8 @@ mw.requestIdleCallback( function () {
 
                previewLinks
                        .on( 'mouseover keyup', function ( event ) {
+                               // Skip keyups that are not TAB (T166610)
+                               if (event.type === 'keyup' && event.which !== 9) return;
+ 
                                boundActions.linkDwell( this, event, gateway, generateToken );
                        } )
                        .on( 'mouseout blur', function () {
                                boundActions.abandon( this );

Event Timeline

Kaartic created this task.May 30 2017, 7:31 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 30 2017, 7:31 PM

If there is no problem I could fix this issue.

Jdlrobson added subscribers: pmiazga, Jdlrobson.

I believe there is a good reason we don't do this.
@pmiazga in Sam's absence do you remember what that was?

Jdlrobson triaged this task as Low priority.May 31 2017, 4:55 PM

@Jdlrobson I couldn't see how ignoring the key events due to shift and control keys could be an issue. Anyways, what about making a change and seeing if it really affects anything rather than waiting, in case it really isn't a serious issue ?

There was an issue. We used to do that but removed it. I just have to dig into the history to remember why

There was an issue. We used to do that but removed it. I just have to dig into the history to remember why

I tried looking into git log src/index.js but in vain. Anyways, if you're that sure then you better close this issue after the reason strikes your mind.

Jdlrobson added a subscriber: ovasileva.

OK, I think I was getting confused with T159490 which doesn't seem to be related to this.

Really this is a product decision, but I think it's such an edge case and I'm not sure it is worth the additional code cost, especially given some users may prefer or not be bothered by this behaviour. What do you think @ovasileva ?

A typical patch would be the one found below. Warning : Browser question is questionable. See https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key

diff --git a/src/index.js b/src/index.jsowser as it 
index 0ebe48e..181e66f 100644
--- a/src/index.js
+++ b/src/index.js
@@ -169,7 +169,8 @@ mw.requestIdleCallback( function () {
 
                previewLinks
                        .on( 'mouseover keyup', function ( event ) {
-                               boundActions.linkDwell( this, event, gateway, generateToken );
+                               if (event.key !== "Control" && event.key !== "Shift")
+                                       boundActions.linkDwell( this, event, gateway, generateToken );
                        } )
                        .on( 'mouseout blur', function () {
                                boundActions.abandon( this );

@Kaartic If that is the issue then it seems easy to fix, nice. jQuery provides event.which, which should be the keyCode normalized across browsers (see [.keyup()](https://api.jquery.com/keyup/).

Also we need to think about the many keys that can be pressed. for example in OSX Cmd+Shift+[ or ] and Cmd+Shift+→ or ← also moves tabs in every browser.

Related (and when keyup was added) via @phuedx : T158631#3107834

Given that keyup is there for the TAB events when moving through links, maybe it would be better to whitelist TAB if the event is keyup instead of trying to blacklist all possible shortcut keys for moving around tabs in the browser.

Pseudocode:

diff --git a/src/index.js b/src/index.jsowser as it 
index 0ebe48e..181e66f 100644
--- a/src/index.js
+++ b/src/index.js
@@ -169,7 +169,8 @@ mw.requestIdleCallback( function () {
 
                previewLinks
                        .on( 'mouseover keyup', function ( event ) {
+                               // Skip keyups that are not TAB (T166610)
+                               if (event.type === 'keyup' && event.which !== 9) return;
+ 
                                boundActions.linkDwell( this, event, gateway, generateToken );
                        } )
                        .on( 'mouseout blur', function () {
                                boundActions.abandon( this );

Related irc discussion:

joakin
phuedx: so would it be safe to say the only key we want is TAB on keyup?
phuedx
joakin: i think that's right, yes
joakin
because whitelisting that instead of blacklisting all the possible keys of shortcuts to move tabs seems better
ok I'll comment
phuedx
12:29
^
absolutely
12:29
whitelists!

Given that keyup is there for the TAB events when moving through links, maybe it would be better to whitelist TAB if the event is keyup instead of trying to blacklist all possible shortcut keys for moving around tabs in the browser.

I'm surprised by how I missed this comment for so long. Anyways, the idea seems to be nice and a simple test I did with the browser console shows me that the 'Tab' keyup event doesn't occur when I switch back to the old tab using 'Ctrl+Shift+Tab'. Only the keyups for 'Ctrl' and 'Shift' are received.

ovasileva raised the priority of this task from Low to Medium.Feb 22 2018, 12:01 PM

@ovasileva a solution has been proposed: https://phabricator.wikimedia.org/T166610#3350952 but is this use case worth the associated risk given its an edge case?

Jhernandez updated the task description. (Show Details)Feb 22 2018, 6:41 PM

I've updated the description with the proposed solution.