Page MenuHomePhabricator

Navigational popups and page previews appearing simultaneously
Closed, ResolvedPublic2 Story Points

Description

Steps to recreate:

  1. Enable hovercards as a beta feature
  2. Enable navigational popups as a gadget (Go to preferences go to gadgets and tick "Navigation popups")
  3. Hover over any link

Expected behavior: navigational popup appears
Observed behavior: page preview and navigational popup appear

Note: easiest solution might be to add a note similar to the user preferences version: "Certain gadgets and other customisations May Affect the performance of this feature. If you experience problems please review your gadget and user scripts."

Details

Related Gerrit Patches:
mediawiki/extensions/Popups : masterSanitize gadget name
operations/mediawiki-config : masterpagePreviews: Enable NavPopups gadget detection
mediawiki/extensions/Popups : masterDisable Previews when Navigation Popups Gadget is enabled

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 9 2017, 3:21 PM
ovasileva triaged this task as Medium priority.Mar 9 2017, 3:21 PM
phuedx added a subscriber: phuedx.Mar 9 2017, 5:54 PM

@ovasileva: Is this really on normal priority and not high or UBN!?

ovasileva raised the priority of this task from Medium to High.Mar 9 2017, 8:23 PM

I guess high makes sense, but I still think it's an edge case. If you're a long-term nav popups user and you enable hovercards, I think it's common sense to see the bug and realize that you should probably turn one of the two off.

TheDJ added a subscriber: TheDJ.Mar 29 2017, 9:20 AM

This is F'ing annoying..

I'm not sure when this went wrong, but please fix it asap.

TheDJ added a comment.Mar 29 2017, 9:30 AM

This used to be handled by the function isNavPopupsEnabled() which nowadays seems to be inside src/settingsDialog.js.

Hey @TheDJ, have you enabled both the beta feature and the gadget or are you seeing this on a wiki where, say, both are enabled by default and are conflicting?

Jdlrobson updated the task description. (Show Details)Apr 5 2017, 5:35 PM
Jdlrobson added a subscriber: Jdlrobson.

Interestingly mw.config.get( 'wgPopupsConflictsWithNavPopupGadget' ) is true but I still see both :)

ovasileva set the point value for this task to 2.Apr 5 2017, 5:36 PM
ovasileva moved this task from Needs Analysis to To Do on the Reading-Web-Sprint-95 board.
bmansurov moved this task from To Do to Doing on the Reading-Web-Sprint-95 board.

@ovasileva is the expected behavior to completely turn off PagePreviews related code (Popup, EventLogging, etc.) when NavPopups is enabled?

@ovasileva is the expected behavior to completely turn off PagePreviews related code (Popup, EventLogging, etc.) when NavPopups is enabled?

The Popups schema provisions for the gadget being enabled. I'd hope that the EL-related code remains enabled.

Change 346792 had a related patch set uploaded (by Bmansurov):
[mediawiki/extensions/Popups@master] Disable Previews when Navigation Popups Gadget is enabled

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

Change 346792 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Disable Previews when Navigation Popups Gadget is enabled

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

The navigations popup gadget doesn't seem to work on the BC. So to sign off we'll either need to install it there or wait till we can verify on mediawiki.org on 11th April.

phuedx added a comment.Apr 6 2017, 8:50 PM

It's not a perfect test but enabling the gadget, even if it doesn't work, should disable Page Previews. This can be verified on the BC.

bmansurov removed bmansurov as the assignee of this task.Apr 6 2017, 9:07 PM
bmansurov added a subscriber: bmansurov.

Would someone other than Jon sign off on the task as he reviewed the patch? Thanks.

ovasileva added a subscriber: ABorbaWMF.EditedApr 7 2017, 1:34 PM

@Jdlrobson, @phuedx, @bmansurov : tested in beta cluster and works fine. However, let's wait until the 11th and have @ABorbaWMF
test it in production just in case:
Testing steps:

  1. When page previews are a beta feature:
    • - Enable navigational popups
    • - Enable page previews as a beta feature
    • - Hover over a link

Expected behavior:

  • Navigational popup appears (for beta cluster: nothing appears)
  1. When page previews are a live feature

Does anyone know how we can test this? (i.e. - do we have a list somewhere with the wikis that have navpops as a gadget and what the name of the gadget is for each wiki?

Change 347291 had a related patch set uploaded (by Phuedx):
[operations/mediawiki-config@master] pagePreviews: Enable NavPopups gadget detection

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

  1. When page previews are a live feature

Does anyone know how we can test this? (i.e. - do we have a list somewhere with the wikis that have navpops as a gadget and what the name of the gadget is for each wiki?

I wrote a script that attempts to automatically generate this list and the corresponding config. ^ I'll deploy that config today.

phuedx claimed this task.Apr 10 2017, 8:23 AM
phuedx moved this task from Ready for Signoff to Doing on the Reading-Web-Sprint-95 board.

Change 347291 merged by jenkins-bot:
[operations/mediawiki-config@master] pagePreviews: Enable NavPopups gadget detection

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

Mentioned in SAL (#wikimedia-operations) [2017-04-10T13:16:25Z] <hashar@tin> Synchronized wmf-config/InitialiseSettings.php: pagePreviews: Enable NavPopups gadget detection - T160081 (duration: 00m 40s)

phuedx removed phuedx as the assignee of this task.Apr 10 2017, 1:48 PM
phuedx moved this task from Doing to Needs More Work on the Reading-Web-Sprint-95 board.
phuedx added a subscriber: pmiazga.

The above config works for dewiki, for example, but not for fr- or simplewiki. @pmiazga: I'm hoping that you might be able to shed some light on this.

I'll check it today

pmiazga claimed this task.Apr 10 2017, 5:10 PM

Change 347523 had a related patch set uploaded (by Pmiazga):
[mediawiki/extensions/Popups@master] Sanitize gadget name

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

pmiazga added a comment.EditedApr 10 2017, 10:52 PM

I tried to find what's wrong with Popups and Gadgets integration but it's pretty strange. I pushed a fix that should help in some cases (https://gerrit.wikimedia.org/r/#/c/347523/2 - will fix es, fr, nl and similar names with spaces) but the simple English is still broken as wiki has a correct/sanitized gadget name. If I copy&paste gadgets definitions to my local instance everything works like a charm 🤔.

What's more strange, on the https://yi.wikipedia.org I after enabling both PagePreviews and NavPopups I can see both. If I type in console mw.config.values.wgPopupsConflictsWithNavPopupGadget I get true -> which should disable PagePreviews but somehow both features are visible

@pmiazga: After panicking about this for the last 30 minutes – my shower was ruined – @bmansurov's change only started riding the train this week, i.e. you'll see it on yiwiki on Thursday, 13th.

^ There's a minor issue with the unit tests in rEPOPa74e3a7dfa0e: Sanitize gadget name.

Change 347523 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Sanitize gadget name

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

phuedx reassigned this task from pmiazga to Jdlrobson.Apr 18 2017, 8:55 AM

^ This'll be released next week (the week of 25th, April) and can be tested on any of the wikis in the $wgPopupsConflictingNavPopupsGadgetName config.

Jdlrobson changed the task status from Open to Stalled.Apr 18 2017, 9:06 PM
Jdlrobson reassigned this task from Jdlrobson to ovasileva.

The problem is already resolved on https://www.mediawiki.org/wiki/MediaWiki but we won't be able to check simple wiki and friends until Wednesday 26th as all are group1 wikis.

I created a calendar invite to remind us.
It doesn't make sense to me to keep it open... but I guess that's not my call.

the reminder should work, closing this for now.

Jdlrobson closed this task as Resolved.Apr 19 2017, 12:00 AM

^ per

Quoting my response to the invitation:

What happens if we find a bug next Wednesday? Do we reopen [this task] and fix it? In that case is it unplanned sprint work? To an external observer (anyone who hasn't been invited to this event), yes. To us, no.
Why not put this task in the Blocked Externally column of sprint 96 and commit to QAing and signing it off per our usual process?

I'm a fan of distinct tickets when possible, so I don't think it's inappropriate to create a followup task if further work has to be done. That way, this work can remain logged as resolved effort, and you get the benefits of granularity. Does that work?

I'm not sure about this - I'll make a note to discuss for retrospective. If signoff is all that's needed in a task and we've done the work, I think creating additional tasks/reminders only increases time spent and confusion. My preference would be to keep the task open - if it's sitting in signoff or blocked externally as Sam suggested it doesn't distract from the remainder of the sprintwork significantly. For this task in particular - I think it's fine with a reminder, as it is working on mediawiki, but I still don't see the harm in waiting for a few days and resolving then rather than introducing more complexity

Correction.. the fix is in wmf21 which is not live on simple wiki at time of writing. I've updated calendar invite for tomorrow.

This does not appear to be working on simple wiki but Yiddish and others are fine. Any ideas to why please add them over on T164044.