Page MenuHomePhabricator

JavaScript addPortletLink method differs from PHP equivalent leading to gadget inconsistencies in modern Vector
Open, MediumPublic3 Estimated Story PointsBUG REPORT

Description

Spun out of T288367

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

mw.util.addPortletLink('p-cactions', '#test', 'Test link')
  • Click the more menu

What happens?:

A link is created, but the resulting HTML is inconsistent with other menu items, leading to an inconsistent font-size

What should have happened instead?:
The resulting item should have been wrapped in a span.
The options in Skin->defaultLinkOptions should be known to the addPortletLink method in a similar way to Skin::makeLink

e.g.

		var portletOptions = mw.config.get('wgSkinPortletLinkOptions' );
...
		link = document.createElement( 'a' );
		link.href = href;
		if ( portletOptions['text-wrapper'] ) {
                   wrapper = document.createElement( portletOptions['text-wrapper']  );
                   wrapper.textContent = text;
                    link.appendChild( wrapper );
                } else {
                    link.textContent = text;
               }

Acceptance criteria

  • The mw-list-item class should always be on the LI element
  • The mw-list-item-js class should be on LI elements that have been added with addPortletLink
  • The JS should build list items based on the text-wrapper option

QA

  • Use addPortletLink in the the developer console to test, i.e. mw.util.addPortletLink('p-cactions', '#test', 'Test link')
  • Ensure portlet links have the correct classes as specified by the AC above
  • Ensure that portlet links added by addPortletLink have the same markup as other portlet links generated by the server
  • Update the "text-wrapper" option in skin.json and ensure the portlet links are still wrapped correctly, i.e.
"text-wrapper": {
  "tag": "div"
}

or

"text-wrapper": [{
  "tag": "div"
},{
  "tag": "span"
}]

QA Results - Beta

QA Results - Prod

Event Timeline

Jdlrobson renamed this task from JavaScript addPortletLink method differs from PHP equivalent. to JavaScript addPortletLink method differs from PHP equivalent breaking gadgets in modern Vector.Aug 18 2021, 3:13 PM
Jdlrobson renamed this task from JavaScript addPortletLink method differs from PHP equivalent breaking gadgets in modern Vector to JavaScript addPortletLink method differs from PHP equivalent leading to gadget inconsistencies in modern Vector.
Jdlrobson updated the task description. (Show Details)
ovasileva lowered the priority of this task from High to Medium.Aug 31 2021, 5:16 PM

Change 720112 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/core@master] Update addPortletLink to use 'text-wrapper' option provided by $wgSkinPortletLinkOptions

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

Change 720112 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/core@master] Update addPortletLink to use 'text-wrapper' option

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

Change 720820 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/core@master] Add a new wgSkinOptions variable to be used by addPortletLink.

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

Change 720822 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/core@master] Add Skin::getSkinConfig to be used by addPortletLink.

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

We'll likely need some input from performance team around the best way to communicate the configuration from skin to JavaScript (e.g. which of the above two options looks preferable).

Krinkle added a subscriber: Krinkle.

Definitely the package file approach. Details in CR. Carry on :)

Change 720820 abandoned by Bernard Wang:

[mediawiki/core@master] Add a new wgSkinOptions variable to be used by addPortletLink.

Reason:

abandoning in favor of Ic7bccba14f57acbc86eb91ec6995b585b2fb3a6f

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

There is a hook in addPortletLink(): (added in T236711 for Minerva)

		mw.hook( 'util.addPortletLink' ).fire( item, {
			id: id
		} );

Is that something you might want to use here?

Change 720822 abandoned by Bernard Wang:

[mediawiki/core@master] Add Skin::getSkinConfig to be used by addPortletLink.

Reason:

Squashing these changes into I801e7d583cb0b0c7da51f4da503268be736bb60c, per discussion here: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/720112/9#message-2f4551dd66bda37b0c03895565af90c752d3e7e4

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

bwang removed bwang as the assignee of this task.Sep 15 2021, 4:34 PM
bwang added a subscriber: bwang.

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

[mediawiki/skins/Vector@master] Do not use User session in the constructor

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

Change 722481 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Do not use User session in the constructor

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

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

[mediawiki/skins/Vector@master] Vector menu items are wrapped in spans + improve Vector addPortletLink support

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

Change 720112 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.util: Update addPortletLink to support 'text-wrapper' option

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

Is that something you might want to use here?

@matmarex the span wrapping is part of the Skin PHP code that skins can configure via a skin option so this should work for skins without a hook but that hook is being used for other inconsistencies.

@bwang tagging with you. I believe the remaining patches are:

Change 723320 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Vector menu items are wrapped in spans + improve Vector addPortletLink support

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

I'm now seeing inconsistent font sizing of menu items in legacy Vector on 1.38-wmf.2, and the padding is bigger (which might have been intentional, not sure).

wmf.2:

Screenshot from 2021-09-29 12-14-26.png (307×296 px, 21 KB)

The "Contribs" and "SUL" links are added using addPortletLink. The other three are #p-views tabs that were added to the More menu responsively. I've no idea why "Create" decided to be larger than the "View on..." and "Create source" links.

wmf.1:

Screenshot from 2021-09-29 12-14-34.png (364×235 px, 36 KB)

Same situation here; a combo of links added with addPortletLink and responsive p-views tabs. This is the font sizing and padding I would expect.

The inconsistent font sizing is also present in new Vector on wmf.2, and in my opinion also suffers from the padding being too big:

Screenshot from 2021-09-29 12-21-23.png (260×199 px, 12 KB)

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

[mediawiki/skins/Vector@master] Restore original more menu padding in legacy Vector

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

Test wiki created on Patch Demo by Jdlrobson using patch(es) linked to this task:

https://patchdemo.wmflabs.org/wikis/30fc2cf65c/w/

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

[mediawiki/skins/Vector@wmf/1.38.0-wmf.2] Restore original more menu padding in legacy Vector

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

Change 724840 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Restore original more menu padding in legacy Vector

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

Change 724798 merged by jenkins-bot:

[mediawiki/skins/Vector@wmf/1.38.0-wmf.2] Restore original more menu padding in legacy Vector

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

Mentioned in SAL (#wikimedia-operations) [2021-09-30T18:58:02Z] <thcipriani@deploy1002> Synchronized php-1.38.0-wmf.2/skins/Vector/resources/skins.vector.styles.legacy/components/MenuDropdown.less: Backport: [[gerrit:724798|Restore original more menu padding in legacy Vector (T289163)]] (duration: 01m 08s)

Test Result - Beta

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

Test Artifact(s):

QA Steps

✅ AC1: Use addPortletLink in the the developer console to test, i.e. mw.util.addPortletLink('p-cactions', '#test', 'Test link')

Screen Shot 2021-10-06 at 6.49.32 PM.png (886×754 px, 192 KB)

✅ AC2: Ensure portlet links have the correct classes as specified by the AC above
See AC1

✅ AC3: Ensure that portlet links added by addPortletLink have the same markup as other portlet links generated by the server

Screen Shot 2021-10-22 at 5.18.07 PM.png (1×1 px, 467 KB)

Screen Shot 2021-10-22 at 5.19.14 PM.png (1×1 px, 512 KB)

✅ AC4: Update the "text-wrapper" option in skin.json and ensure the portlet links are still wrapped correctly, i.e.

"text-wrapper": {
  "tag": "div"
}

Screen Shot 2021-10-12 at 1.09.37 PM.png (834×966 px, 170 KB)

"text-wrapper": [{
  "tag": "div"
},{
  "tag": "span"
}]

Screen Shot 2021-10-12 at 1.11.21 PM.png (1×1 px, 266 KB)

@bwang thanks for your help!

Edtadros added a subscriber: Edtadros.

Test Result - Prod

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

Test Artifact(s):

QA Steps

✅ AC1: Use addPortletLink in the the developer console to test, i.e. mw.util.addPortletLink('p-cactions', '#test', 'Test link')

Screen Shot 2021-10-22 at 5.25.25 PM.png (1×1 px, 679 KB)

✅ AC2: Ensure portlet links have the correct classes as specified by the AC above
See AC1

✅ AC3: Ensure that portlet links added by addPortletLink have the same markup as other portlet links generated by the server
Compare with AC1

Screen Shot 2021-10-22 at 5.27.22 PM.png (1×1 px, 622 KB)

⬜ AC4: Update the "text-wrapper" option in skin.json and ensure the portlet links are still wrapped correctly, i.e.
Not testable in Prod