Page MenuHomePhabricator

Page preview does not close upon closing other page
Closed, ResolvedPublic3 Story Points

Description

Steps to recreate:
OSX 10.11.6
Firefox: 51.0.1

  1. Open https://en.wikipedia.beta.wmflabs.org/wiki/Dog
  2. Hover over carnivorous (in the first sentences)
  3. Open in new tab clicking (Ctrl+click or CMD+Click on a Mac) the link
  4. Move mouse and go to other new tab, see hovercard disappear
  5. Close new Carnivore tab, when the Dog tab gets re-focused...

Expected

  1. There's no preview

Actual

  1. The carnivore hovercard will re-appear, and won't close until clicking somewhere or hovering the preview

Note: not observed in Chrome or Safari browsers

Using the ctrl+link method it also happens on the following OS and browsers:

  • Windows 10 - Edge & IE 11 & Chrome 56
  • Windows 8.1 - IE11 & Chrome 56

Sign Off AC


Seems like this is related to the focus event on the link being re-triggered when the tab is re-focused...

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 21 2017, 10:05 AM
ovasileva triaged this task as High priority.

@Nicholas.tsg - could you try to reproduce this one across different os/browsers? I'm not sure if it's OSX specific.

ovasileva updated the task description. (Show Details)Feb 21 2017, 5:16 PM

I've managed to reproduce, but differently (same browser and OS):

  1. Open https://en.wikipedia.beta.wmflabs.org/wiki/Dog
  2. Hover over carnivore
  3. Open in new tab clicking (Ctrl+click) the link
  4. Move mouse and go to other new tab, see hovercard disappear
  5. Close new Carnivore tab, when the Dog tab gets re-focused...

Expected

  1. There's no preview

Actual

  1. The carnivore hovercard will re-appear

Will update description.

Jhernandez updated the task description. (Show Details)Feb 21 2017, 5:23 PM
ovasileva set the point value for this task to 3.Feb 21 2017, 5:43 PM

I've checked with the redux devtools and a type: "focus" event gets triggered when you head back to the Dog tab which is what triggers the popup to show again.

Previously I could not reproduce the bug. Oddly enough, right-clicking the link and selecting open in a new tab from the options does not reproduce the behavior. However, I was able to reproduce it using the ctrl+link method on the following OS and browsers:

Windows 10 - Edge & IE 11 & Chrome 56
Windows 8.1 - IE11 & Chrome 56

Jhernandez updated the task description. (Show Details)Feb 22 2017, 11:33 AM

Updated description, thanks @ABorbaWMF

@Jhernandez - the behavior you're observing is acceptable. I'm getting something where the preview appears and won't disappear:

bmansurov updated the task description. (Show Details)Feb 22 2017, 2:19 PM

@ovasileva It looks the same to me. Did you click somewhere at the end to remove focus off the link? Without talking gifs and videos of this nature are confusing 😄

@Jhernandez - yup, clicking gets rid of it :)

pmiazga claimed this task.Mar 3 2017, 4:17 PM

Change 341818 had a related patch set uploaded (by Pmiazga):
[mediawiki/extensions/Popups] WIP: Hide Page Preview after opening link in different tab

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

Given fix works only for CMD+Click or Ctrl+Click. There is no way to detect "Open in new tab" action from context menu nor "middle mouse button click".

phuedx added a subscriber: phuedx.Mar 9 2017, 1:22 PM

^ rEPOP010ea211f48b: Hide Page Preview after opening link in different tab has a couple of comments from both @Jhernandez and myself around tidying up the state resetting part of the preview reducer.

phuedx added a subscriber: Nirzar.EditedMar 9 2017, 1:43 PM

Also, @ovasileva, @Jhernandez, and I discussed removing the keyboard interactions (as they are) from Page Previews entirely as we seem to be fighting the UA here. Below is a log of the conversation, which covers the initial idea of removing the keyboard interactions (focus/blur event listeners). @Jhernandez also proposed a possible refinement to the keyboard interaction: show the preview only after the user presses a key /cc @Nirzar.

We should discuss this during today's water cooler meeting.

111:03:43 <phuedx> i really don't like this whole "blur on click" thing
211:04:27 <phuedx> but it'd require more state to mark the interaction as, say finalised but still open
311:06:17 <joakino> it is werid and hacky, yes
411:06:59 <joakino> i mentioned to piotr that if we don't find a proper way to do it I'd rather say that we can't do it without removing the focus,blur features of hovercards.
511:07:05 <joakino> because of implicit browser behaviors
611:09:42 <phuedx> ^
711:10:03 <phuedx> that's an interesting tradeoff
811:19:27 <phuedx> olliv: would that be acceptable?
911:19:39 <phuedx> basically, drop keyboard navigation support
1011:20:13 <phuedx> which would also get rid of this awkward "open in new tab, tab out, tab in, preview reappears"
1111:20:56 <olliv> phuedx: I'm not entirely following, keyboard navigation won't work only for the hovercards or for the links as well?
1211:21:14 <phuedx> only for hovercards
1311:21:32 <phuedx> sorry, should've been very clear, ONLY FOR HOVERCARDS!
1411:33:15 <phuedx> olliv: if you tab to a valid link, then it'll show a preview
1511:33:31 <phuedx> essentially, this is the behaviour that triggers the reappearing preview
1611:34:34 <phuedx> when you tab back to the page the browser says "you've tabbed to this link!"
1711:35:41 <olliv> phuedx: that's perfect
1811:35:47 <olliv> let's do it
1911:36:52 <phuedx> to be clear, let's remove the tabbing stuff from hovercards?
2011:40:46 <phuedx> olliv: so, yeah, remove keyboard-related stuff from page previews, i.e. only mouse interactions
2111:41:18 <olliv> phuedx: yes.
2211:41:40 <olliv> and I'll make a note to add that to the documentation
2311:53:05 <phuedx> olliv: do you mind if i copypasta this ^^^ and add it to the task
2411:53:12 <phuedx> without me swearing, obviously ;)
2511:53:40 <olliv> phuedx: sure. Could you also tag me with a reminder to update the documentation?
2611:54:20 <phuedx> sure
2711:54:25 <phuedx> (that's also on anyone btw)
2811:58:07 <phuedx> olliv: otoh limiting it to mouse interactions does make it less accessible, some of our users don't use their mouse
2911:58:10 <phuedx> i dunno
3011:58:14 <phuedx> it's a balancing act
3111:58:43 <phuedx> joakino: any thoughts?
3211:59:13 <phuedx> i'll think about this over lunch
3311:59:22 phuedx → phuedx|nom
3411:59:43 <olliv> if it's only for hovercards, I think it's okay. While I would love for all features to be accessible, I think we can have this as a special case
3512:01:31 <joakino> olliv: phuedx|nom: the problem is focus on links, not hovercards, it'd be removing the focus/blur behavior or changing it to require a key press to show the hovercard (like space or enter)
3612:01:38 <joakino> i'm out for lunch too, finishing a task
3712:08:10 <olliv> joakino: meaning a user could not focus a link to open via the keyboard, or only that they would not be able to open the hovercard? Also, would this alternative be possible - have a key to show the hovercard and then select the card to open the page?
3812:11:00 <joakino> olliv: they would be able to focus links, but instead of focus showing the hovercard, having a key (like enter) when focused show the hovercard, and next key (like enter) would open the page
3912:11:08 <joakino> we can chat later about it
4012:11:18 <joakino> kind of confusing to do in chat
4112:11:33 <olliv> joakino: sounds good
4212:11:48 <olliv> joakino, phuedx|nom ping me when you're back, maybe we can do a quick hangout
4312:13:07 <joakino> olliv: the thing is that removing focus on the link to avoid that unexpected behavior makes the focus be lost so if you come back and want to continue tabbing then you'll go to the start of the page
4412:13:20 <joakino> so fixing that bug means breaking the tabbing experience
4512:13:58 <olliv> joakino: hmm...okay, yeah, let's talk about it on a hangout and think through the options
4612:56:16 phuedx|nom → phuedx
4712:56:21 <phuedx> olliv, joakino: back
4813:01:21 <phuedx> right
4913:01:34 <phuedx> i'm going to grab that chat and remove my swears and whack it on the task for context
5013:01:42 <phuedx> but we should hangout about this, i agree

Jdlrobson added a subscriber: Jdlrobson.

I was going to merge this but I think it would be good to document this with adr. Anyone disagree?

I was going to merge this but I think it would be good to document this with adr. Anyone disagree?

ADRs are for complex architectural decisions. This feels like a product decision. @ovasileva has asked to be reminded to write up this decision on MediaWiki – that's where the on-wiki documentation lives, right?

We discussed this post kick off and a suggestion was made to document inside docs but not inside adr.

As I sat down to write my thoughts about this task, it occurred to me that having T160545: Bring back keyboard support to PagePreviews in the Sprint +1 column in the backlog is a little like punting this task, right?

@phuedx - for this sprint, yes. I'm tagging it with the next sprint, however, so we don't forget.

Huh? This is a little confusing. Why don't we just do it right the first time? Can we really not wait 2 weeks to fix this particular bug that it's worth a temporary disruption to keyboard support...?

I think the concern is that re-adding focus support would be too difficult, could create bit too much complex for such easy feature.

@Jdlrobson @phuedx @Jhernandez - The whole problem is caused by UA triggers focus event on last focused element when a user navigates through tabs. Another idea is to listen to keyup event, it works, everytime you press tab to navigate to another link it triggers the key.... events on specific link. Then we would listen to blur event to dismiss the PagePreview. It will re-add keyboard support but I'm not sure if screen-readers use keyboard events.

Huh? This is a little confusing. Why don't we just do it right the first time? Can we really not wait 2 weeks to fix this particular bug that it's worth a temporary disruption to keyboard support...?

Summary of the brief discussion the Reading Web team had around this:

It's not a given that we'll work on T160545: Bring back keyboard support to PagePreviews during the next sprint. We'd rather not release Page Previews to users on the stage0 wikis with this bug present. Given that we're releasing it to those wikis on Monday, 20th, let's drop keyboard support now and commit to implementing it properly later.

@pmiazga may have found a fix that addresses the bug AND keeps keyboard support in tact.

Should be fixed now. Page Previews are triggered on:

  • mousenter for mouse/pointer interactions
  • keyup for keyboard navigation

Script listens to the keyup event because a user can press and hold TAB key to quickly jump over the links, we don't want to send multiple AJAX requests for each link. The best solution was to listen to the moment when TAB button is released.

Change 341818 merged by jenkins-bot:
[mediawiki/extensions/Popups] Show Page Preview on mouseenter and keyup events

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

phuedx added a comment.EditedMar 16 2017, 9:01 PM

Script listens to the keyup event because a user can press and hold TAB key to quickly jump over the links, we don't want to send multiple AJAX requests for each link. The best solution was to listen to the moment when TAB button is released.

This would make an excellent inline comment 😉

phuedx updated the task description. (Show Details)Mar 17 2017, 4:37 AM
ovasileva reassigned this task from pmiazga to ABorbaWMF.Mar 17 2017, 4:40 PM
ovasileva added a subscriber: pmiazga.

Just checked on https://en.wikipedia.beta.wmflabs.org/wiki/Dog in Chrome - looks good. Over to you @ABorbaWMF

From testing on various OS and browsers specified, this seems to be fixed - the hovercard doesn't get stuck on screen when going between tabs. Here are screencaps to show what specifically happens on each -
Windows 8.1 Chrome 57


Windows 8.1 IE 11

Windows 10 Chrome 57

Windows 10 Edge

Windows 10 IE 11

While I think this is fixed I'll allow @ABorbaWMF to decide whether to move it to signoff.

phuedx updated the task description. (Show Details)Mar 21 2017, 4:30 PM

@Nicholas.tsg Performed the tests here. I took a quick look at a couple system/browsers. This appears to be fixed on the beta site.

phuedx reassigned this task from ABorbaWMF to bmansurov.Mar 22 2017, 7:31 AM
phuedx reassigned this task from bmansurov to ovasileva.
phuedx added a subscriber: bmansurov.
ovasileva closed this task as Resolved.Mar 22 2017, 4:36 PM

looks good, thanks all!