Page MenuHomePhabricator

Gadget support: Review and improve addPortletLink functionality and tests
Closed, ResolvedPublic3 Estimated Story PointsBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

Seems to only happen when the page is first loaded, I can't reproduce using my console.

  • add portlet links via mw.util.addPortletLink

What happens?:
second portlet link has an empty icon

Screenshot 2023-02-13 at 2.48.19 PM.png (246×250 px, 16 KB)
Screenshot 2023-02-13 at 2.48.41 PM.png (97×174 px, 10 KB)
Screenshot 2023-02-13 at 2.48.31 PM.png (131×182 px, 12 KB)

What should have happened instead?:

  • No icon (no ID was passed)
  • If ID has passed strictly one icon

Software version (skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):

/* Edit personal nav links */
mw.loader.using('mediawiki.util').then(function () {
	mw.util.addPortletLink('p-personal', '/wiki/en:User:TerraCodes/dashboard', 'Dashboard', null, null, null, '#pt-betafeatures');
	mw.util.addPortletLink('p-personal', '/wiki/en:Special:ProblemChanges', 'Problem', null, null, null, '#pt-betafeatures');
	$(function () {
		mw.util.addPortletLink('p-personal-sticky-header', '/wiki/en:User:TerraCodes/dashboard', 'Dashboard', null, null, null, '#pt-betafeatures-sticky-header');
		mw.util.addPortletLink('p-personal-sticky-header', '/wiki/en:Special:ProblemChanges', 'Problem', null, null, null, '#pt-betafeatures-sticky-header');
	});
});

chrome_2HIEOoiTKV.png (268×380 px, 14 KB)

other issues

vector-tab-noicon class is removed unexpectedly
https://m.mediawiki.org/wiki/Talk:Reading/Web/Desktop_Improvements#addPortletLink

QA steps

  1. Run the following code when logged in:
mw.util.addPortletLink('p-personal', '/wiki/en:User:TerraCodes/dashboard', 'Dashboard', null, null, null, '#pt-betafeatures');

Expected: A link appears after beta features without an icon

  1. Run the following code when logged in:
mw.util.addPortletLink('p-personal', '/wiki/en:User:TerraCodes/dashboard', 'Dashboard', 'testing', null, null, '#pt-betafeatures');

Expected: A link appears after beta features with space allocated for an icon (but there should be no icon)

QA Results - Beta|Prod

ACStatusDetails
1T327369#8626885
2T327369#8626885

QA Results - Beta|Prod

ACStatusDetails
1T327369#8643215
2T327369#8643215

Event Timeline

Jdlrobson subscribed.

Can't replicate the duplicate icon right now. Seems like a race condition is occurring here.
The icons should also not be showing up as you are not passing an ID, so that looks unexpected.

Jdlrobson renamed this task from Adding portlet links causes duplicate icon elements to be added to Adding portlet links causes icon elements to be added when no ID is passed.Jan 31 2023, 12:45 AM
Jdlrobson renamed this task from Adding portlet links causes icon elements to be added when no ID is passed to Gadget support: Adding portlet links causes icon elements to be added when no ID is passed.
Jdlrobson updated the task description. (Show Details)
Jdlrobson renamed this task from Gadget support: Adding portlet links causes icon elements to be added when no ID is passed to Gadget support: Review addPortletLink functionality.Jan 31 2023, 4:24 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson renamed this task from Gadget support: Review addPortletLink functionality to Gadget support: Review and improve addPortletLink functionality and tests.Feb 2 2023, 6:41 PM
Jdlrobson triaged this task as High priority.
LGoto set the point value for this task to 3.Feb 9 2023, 6:16 PM

I also wasn't able to reproduce the race condition. I'm wondering if the ACs for this task could be clarified, is it primarily fixing the race condition?

I also took a look at the original discussion this task came from, and I'm not sure I understand the issue around .vector-tab-noicon being removed? addPortletLink adds that class .vector-tab-noicon to links in tab menus, so I'm unsure what it means for that class to be removed. In my testing addPortletLink is correctly adding that class to tab menus.

I think a screenshot would go a long way in the description :)
I was able to reproduce this issue when adding the following script to MediaWiki:Common.js on my local instance.

mw.util.addPortletLink('p-personal', '/wiki/en:User:TerraCodes/dashboard', 'Dashboard', null, null, null, '#pt-betafeatures');

It shows two empty icons beside the "Dashboard" link.

Screenshot 2023-02-13 at 2.36.59 PM.png (872×1 px, 196 KB)

I think the sticky header gadget support is particularly poor. If this turns out just to be a problem with race conditions I was talking to @nray Friday about https://phabricator.wikimedia.org/T329397

Change 889214 had a related patch set uploaded (by Jdrewniak; author: Jdrewniak):

[mediawiki/skins/Vector@master] Prevent addPortletLinkHandler from looping over links twice

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

Change 889215 had a related patch set uploaded (by Jdrewniak; author: Jdrewniak):

[mediawiki/skins/Vector@master] [WIP] Refactoring addPortletLinkHandler and adding tests

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

Change 889215 abandoned by Jdrewniak:

[mediawiki/skins/Vector@master] Refactor addPortletLinkHandler & update tests

Reason:

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

I talked to @alexhollender_WMF about which is the preferred state is when there is no ID (which produces an icon) provided to addPortletLink:

A.png (233×252 px, 14 KB)
B.png (245×225 px, 14 KB)
AB

The decision was A in order to be consistent with the handling of the "edit interlanguage links" item in the page tools menu.

The patch above changes this as well.

Change 889858 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] Register sticky header dropdown as icon capable

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

Change 889214 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Prevent addPortletLinkHandler from looping over links twice

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

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Ventura
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

Run the following code when logged in:

mw.util.addPortletLink('p-personal', '/wiki/en:User:TerraCodes/dashboard', 'Dashboard', null, null, null, '#pt-betafeatures');

✅ AC1: A link appears before beta features without an icon

Screenshot 2023-02-17 at 4.27.52 PM.png (482×1 px, 115 KB)

Run the following code when logged in:

mw.util.addPortletLink('p-personal', '/wiki/en:User:TerraCodes/dashboard', 'Dashboard', 'testing', null, null, '#pt-betafeatures');

✅ AC2: A link appears before beta features with space allocated for an icon (but there should be no icon)

Screenshot 2023-02-17 at 4.32.22 PM.png (490×1 px, 122 KB)

Edtadros subscribed.

Test Result - Prod

Status: ✅ PASS
Environment: enwiki
OS: macOS Ventura
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

Run the following code when logged in:

mw.util.addPortletLink('p-personal', '/wiki/en:User:TerraCodes/dashboard', 'Dashboard', null, null, null, '#pt-betafeatures');

✅ AC1: A link appears before beta features without an icon

Screenshot 2023-02-23 at 7.52.25 PM.png (599×1 px, 209 KB)

Run the following code when logged in:

mw.util.addPortletLink('p-personal', '/wiki/en:User:TerraCodes/dashboard', 'Dashboard', 'testing', null, null, '#pt-betafeatures');

✅ AC2: A link appears before beta features with space allocated for an icon (but there should be no icon)

Screenshot 2023-02-23 at 7.53.59 PM.png (578×1 px, 206 KB)

Change 889858 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Register sticky header dropdown as icon capable

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