Page MenuHomePhabricator

Hovercards overrides NavPopups; should be the other way around
Closed, ResolvedPublic3 Story Points

Description

  • If NavPops is enabled don't load Hovercards
  • If NavPops gadget is removed show HoverCards

Steps to reproduce:

  1. register a new account on hu.wikipedia
  2. enable Hovercards
  3. enable Navigation popups

Expected: Navigation popups show up
Actual: Hovercards show up.
(Hovercards have an option to disable themselves, which works fine. This is about the default.)

This is not a problem now when Hovercards is in beta, but once it becomes default, having enabled NavPopups in the past (an intentional user action) should override Hovercards being default-on (not an intentional user action).


From merged task:

The current behavior is to disable navpops if hovercards are enabled, but navpops are the preferred tool of many editors and have features that are essential to their workflows. The reverse behavior should be true.

If I, as an editor, enable navpops, my hovercards should be disabled.

In the future, when we have finalized hovercards preferences, we will need a message explaining "hovercards cannot be enabled while navpops gadget is turned on"

This ticket reverses the behavior of: https://phabricator.wikimedia.org/T64952

Related Objects

StatusAssignedTask
ResolvedDereckson
Resolved Jdlrobson
Resolvedovasileva
ResolvedNone
InvalidNone
ResolvedCKoerner_WMF
Resolved Jdlrobson
Resolvedputnik
ResolvedJhernandez
ResolvedJhernandez
ResolvedMoushira
Resolvedphuedx
Resolveddr0ptp4kt
ResolvedJhernandez
Resolved Jdlrobson
ResolvedPcoombe
ResolvedAndyRussG
ResolvedNone
Resolveddr0ptp4kt
Resolved Tbayer
ResolvedJhernandez
Resolved Jdlrobson
Resolvedphuedx

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I'm not sure why but that's the way it's designed to work.

https://github.com/wikimedia/mediawiki-extensions-Popups/blob/85b27d8de83a094d104fd7650fa872eea515ac30/resources/ext.popups.renderer/desktopRenderer.js#L108-L109

There's work TBD about improving the opt-out experience and asking the users first time if they want to enable hovercards I think before rollout to stable. Going to ping @JKatzWMF and @dr0ptp4kt here so that they chime in about this.

Jhernandez added a comment.EditedMay 18 2016, 5:22 PM

Here's the first-run task T133230

Tgr added a comment.May 18 2016, 8:40 PM

I can see how discoverability is a problem if you don't forcibly enable, but even so, at least for power users I think some kind of opt-in process would be better. Maybe display hovercards the first time, or first few times, but require some sort of confirmation to keep it enabled, and disable if that does not happen.

@Jhernandez @Prtksxna I can't find documentation of it, but I could have sworn that we had an agreement to reverse this: T64952 and had put something in place. Anyone with Navpops should not see hovercards.

@Jhernandez @Prtksxna I can't find documentation of it, but I could have sworn that we had an agreement to reverse this: T64952 and had put something in place. Anyone with Navpops should not see hovercards.

I can't find a related task either, but yes, when the feature is default it should give priority to Navigation Popups (if enabled), and when its in Beta Features, Hovercards should be given priority.

I can see how discoverability is a problem if you don't forcibly enable, but even so, at least for power users I think some kind of opt-in process would be better. Maybe display hovercards the first time, or first few times, but require some sort of confirmation to keep it enabled, and disable if that does not happen.

Some version of T133230: First run opt-out experience for hovercards might be helpful here.

Tgr added a comment.May 20 2016, 1:57 PM

Some version of T133230: First run opt-out experience for hovercards might be helpful here.

Yes, but it should be an opt-in experience, not an opt-out one.

I'm not sure about the opt-in aspect of things, and anyway our approach will reflect the outcome from T133230: First run opt-out experience for hovercards.

But with respect to the behavior when both things are On - yes, the NavPopups should override Hovercards. I'll tag this for sprint 73.

Jdlrobson set the point value for this task to 3.

Change 291026 had a related patch set uploaded (by Bmansurov):
Disable Popups when the Navigatio Popups gadget is enabled

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

I've +2ed but note my comment - you might want to do a follow up - but the bug is now fixed.

@Jdlrobson left a comment there too.

Change 291026 merged by jenkins-bot:
Disable Popups when the Navigation Popups gadget is enabled

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

bmansurov removed bmansurov as the assignee of this task.May 28 2016, 12:15 AM
bmansurov added a subscriber: bmansurov.
This comment was removed by Jdlrobson.
phuedx added a subscriber: phuedx.EditedMay 31 2016, 9:15 AM

017cb24 has been pulled by our staging server.

phuedx added a comment.EditedMay 31 2016, 9:18 AM

I've confirmed that this bug is fixed on our staging server.

N.B. that the Popups extension isn't enabled on the Beta Cluster /cc @Jhernandez

phuedx closed this task as Resolved.May 31 2016, 9:18 AM
phuedx claimed this task.
phuedx updated the task description. (Show Details)

If NavPops is enabled don't load Hovercards

This isn't strictly fulfilled as the Popups code is loaded but disabled.

bmansurov reopened this task as Open.Jun 2 2016, 10:04 PM

The patch caused this bug: T135630#2351005. Re-opening. A temporary fix at https://gerrit.wikimedia.org/r/#/c/292487/ will be SWAT deployed in about an hour.

The above patch has been SWAT deployed and fixed the problem.

Qgil moved this task from Backlog to Team radar on the Developer-Advocacy board.
phuedx removed phuedx as the assignee of this task.Jun 3 2016, 9:49 AM

@bmansurov Is this task ready for signoff on the sprint board?

@MBinder_WMF not yet, that was a temp fix.

Change 292744 had a related patch set uploaded (by Bmansurov):
Detect whether NavPopups gadget is enabled before showing Hovercards

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

@dr0ptp4kt currently we don't take the Navigation Popups gadget into account when logging events, which may distort your EL analysis. When the gadget is enabled with the change above, Hovercards will be disabled. This means that even though a user sees a navigation popup (using the gadget), 'dwelledButAbandoned' event will be logged when the user dismisses the popup. Is that OK? Or should we also modify the schema to indicate that the gadget is enabled (thus the Hovercards extension is not)?

Tgr added a comment.Jun 7 2016, 11:52 AM

The old behavior was that enabling NavPopups had the same effect as manually disabling Hovercards: there was a link in the footer to reenable, and that could be used to switch between NavPopups and HoverCards, without having to change gadget preferences. If it's possible to do that, it would be preferable to the current behavior (where Hovercards seems to totally disappear when NavPopups is enabled).

Tgr added a comment.Jun 7 2016, 11:58 AM

Also it would be nice if there was a way to select "Hovercards when available, NavPopups otherwise", given that NavPopups supports various link types that Hovercards doesn't (user page, diff etc).

@dr0ptp4kt currently we don't take the Navigation Popups gadget into account when logging events, which may distort your EL analysis. When the gadget is enabled with the change above, Hovercards will be disabled. This means that even though a user sees a navigation popup (using the gadget), 'dwelledButAbandoned' event will be logged when the user dismisses the popup. Is that OK? Or should we also modify the schema to indicate that the gadget is enabled (thus the Hovercards extension is not)?

Okay, I think I understand. You're saying that we might, at least for logged in users, see a decent number of dwelledButAbandoned events for the navpopups gadget users, who are logged in.

@bmansurov, if it's a very simple change you can do in this part of the task, sure, go ahead and add it as `navpopGadgetEnabled' to the schema.

@Tbayer & @JKatzWMF, note for any future queries you might need to add results from two tables for time series analysis.

@bmansurov, if you feel a new task needs to be created, please do so and then tag it for sprint 75.

Good catch!

@Tgr, your comments noted. Right now we're trying to keep it simple, but let's ensure this is visible in our collective backlog so we tackle it when we come back around to the preferences workflow.

T137203 has been created as suggested by @dr0ptp4kt.

Rechecking and merging this is blocked on T137265.

Change 292744 merged by jenkins-bot:
Detect whether NavPopups gadget is enabled before showing a hovercard

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

bmansurov removed bmansurov as the assignee of this task.Jun 9 2016, 8:10 AM

@bmansurov Should we cherry-pick the change to the wmf/1.28.0-wmf.5 branch so that it can rid this week's train and, hopefully, be signed off towards the end of the sprint?

@phuedx I'm not sure as this is not an urgent bug and things were not broken on the wmf/1.28.0-wmf.5 branch.

phuedx closed this task as Resolved.Jun 15 2016, 8:49 AM
phuedx claimed this task.

This LGTM. I've tested this on our staging server by waiting for the page to finish loading and then loading the NavPopups gadget using mw.loader.load, simulating a race condition.