[go: up one dir, main page]

Page MenuHomePhabricator

Remove PagePreview popup from documentation link in MentorDashboard
Closed, ResolvedPublicBUG REPORT

Description

List of steps to reproduce (step by step, including full links if applicable):

What happens?:
A popup appears with a rendering error

What should have happened instead?:
A popup doesn't appear with a rendering error

Software version (if not a Wikimedia wiki), browser information, screenshots, other information, etc.:

chrome_FwXcUHethu.gif (650×775 px, 162 KB)

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Thanks for reporting this, @Iniquity! I can reproduce this issue on Czech Wikipedia as well. I originally thought this is an issue within PagePreviews themselves, but it turns out the link produced by Mentor dashboard is missing the extiw class, which MediaWiki adds when generating interwiki links. I verified that adding the class (via the devtools) fixes the issue.

The popup's content is constructed in JavaScript. The message that includes the link is growthexperiments-mentor-dashboard-mentee-overview-info-text, which is inserted via $( '<p>' ).html( mw.message( 'growthexperiments-mentor-dashboard-mentee-overview-info-text' ).parse() ). (modules/ext.growthExperiments.MentorDashboard/MenteeOverview/MenteeOverview.js).

Good question is how to add the extiw class in place. The "easiest" fix would be to parse the message servers-side and use ResourceLoader callback (docs), but that feels like a workaround to me. In an ideal world, the solution would be to make mediawiki.jqueryMsg aware of interwiki links, but that looks like a huge investment. Alternatively, we can construct the <a> manually in JS (with correct classes) and feed it to the message as a parameter.

@Tgr Do you have any recommendation/insights here please?

Changing mediawiki.jqueryMsg is not hard (the relevant logic is in the wikilink method in resources/src/mediawiki.jqueryMsg/mediawiki.jqueryMsg.js; in general, the parser is pretty easy to read once you wrap your head around the layout and the funky way it gets set up), but I don't think the list of interwiki prefixes is accessible from frontend code, and without that you can't tell whether it's an interwiki or a namespace or part of a mainspace title.

In any case, I do think this is an issue with PagePreviews. It shouldn't make requests to foreign domains - it has no reason to assume links pointing to another sites will always have the extiw class, and checking whether the URL is local is trivial.

Changing mediawiki.jqueryMsg is not hard (the relevant logic is in the wikilink method in resources/src/mediawiki.jqueryMsg/mediawiki.jqueryMsg.js; in general, the parser is pretty easy to read once you wrap your head around the layout and the funky way it gets set up), but I don't think the list of interwiki prefixes is accessible from frontend code, and without that you can't tell whether it's an interwiki or a namespace or part of a mainspace title.

Thanks for the info. Indeed, looks like knowing when to add the class is going to be tricky.

In any case, I do think this is an issue with PagePreviews. It shouldn't make requests to foreign domains - it has no reason to assume links pointing to another sites will always have the extiw class, and checking whether the URL is local is trivial.

That makes sense. However, I don't see how would one fix this in a trivial way from PagePreviews. The extension seems to decide where to (not) display a popup in JavaScript (it excludes a few CSS selectors), see source. The link we currently insert (via JS) is https://cs.wikipedia.org/wiki/mw:Special:MyLanguage/Growth/Feature_summary#NH at cs.wikipedia. The only part that makes the link external is the mw: in the title (which is hard to detect, as noted above). The domain etc. is local.

Hm, right, jqueryMsg can't expand interwiki prefixes for the same reason it can't detect external links. You could just add the extiw class via JS when the popup loads, I guess. Or use an external link as @Iniquity says.

@Urbanecm_WMF Have you been able to apply the solution that @Tgr has shared?

@Urbanecm_WMF Have you been able to apply the solution that @Tgr has shared?

See T312248#8129397 for my reply about the original solution. I think that using the external link directly (hardcoded to the message) is the best available option here. In the ideal world, client site would be aware of interwiki prefixes, but that's not really easy to implement, unfortunately.

Change 822418 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/GrowthExperiments@master] MenteeOverview: Do not use mw: for linking to MW.org

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

Urbanecm_WMF changed the task status from Open to In Progress.Aug 11 2022, 4:20 PM
Urbanecm_WMF claimed this task.
Urbanecm_WMF changed the task status from In Progress to Open.Aug 11 2022, 4:31 PM
Urbanecm_WMF moved this task from Code Review to QA on the Growth-Team (Sprint 0 (Growth Team)) board.

Change 822418 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] MenteeOverview: Do not use mw: for linking to MW.org

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

Etonkovidova added subscribers: Urbanecm, Etonkovidova.

In testwiki wmf.23 the Console displays the following:

GET https://test.wikipedia.org/api/rest_v1/page/summary/Mw:Special:MyLanguage/Growth/Feature_summary

type	"https://mediawiki.org/wiki/HyperSwitch/errors/not_found"
title	"Not found."
method	"get"
detail	"Requested resource is not found."
uri	"/test.wikipedia.org/v1/page/summary/Mw%3ASpecial%3AMyLanguage%2FGrowth%2FFeature_summary"

Tested the fix in betalabs:

  • no console errors
  • the popup warning (page preview popup) is not displayed

(click on the animated gif)

hover_mentorDashboard.gif (880×1 px, 125 KB)

@Urbanecm - there is some additional hint displayed to users when they hover over mw links, e.g.
Screen Shot 2022-08-15 at 5.57.18 PM.png (408×700 px, 36 KB)

Should such a hint be added to mw:Special:MyLanguage/Growth/Feature_summary#NH?

hover_mentorDashboard.gif (880×1 px, 125 KB)

@Urbanecm_WMF - there is some additional hint displayed to users when they hover over mw links, e.g.
Screen Shot 2022-08-15 at 5.57.18 PM.png (408×700 px, 36 KB)

Should such a hint be added to mw:Special:MyLanguage/Growth/Feature_summary#NH?

I don't think that's necessary (probably more work than worth it). Thanks for checking!

hover_mentorDashboard.gif (880×1 px, 125 KB)

@Urbanecm_WMF - there is some additional hint displayed to users when they hover over mw links, e.g.
Screen Shot 2022-08-15 at 5.57.18 PM.png (408×700 px, 36 KB)

Should such a hint be added to mw:Special:MyLanguage/Growth/Feature_summary#NH?

I don't think that's necessary (probably more work than worth it). Thanks for checking!

Yes, agree - makes sense.