Page MenuHomePhabricator

Remove ULS extension hacks from Vector and MobileFrontend
Closed, ResolvedPublic

Description

There are some unnecessary hacks in Vector and MobileFrontend for this extension and these should be cleaned up as they cannot be sustained in perpetuity.

It's the responsibility of the extension to make sure it looks good and works correctly with the existing infrastructure, just like how every other extension does.

Acceptance criteria

  • Removed from Vector
  • Removed from Mobilefrontend

Event Timeline

Ammarpad created this task.Sep 21 2020, 6:12 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 21 2020, 6:12 AM
Ammarpad updated the task description. (Show Details)Sep 21 2020, 6:15 AM

"Unnecessary" in what sense?

Yes, ideally certain features would be supported in core and implemented by all skins, but that's not always practical.

Change 628740 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/skins/Vector@master] Remove code that does not belong to Vector skin

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

Ammarpad triaged this task as Medium priority.Sep 21 2020, 10:09 AM
Ammarpad updated the task description. (Show Details)
This comment was removed by Jdlrobson.
Ammarpad added a comment.EditedSep 21 2020, 9:24 PM

The code in Vector is used to make sure the ULS switcher comes before the user login:

The hack's comment says "first item in the personal menus", and that's how I see it in my local. And that's how it is currently in production, Wikimedia Commons: https://commons.wikimedia.org/wiki/Main_Page or MetaWiki: https://meta.wikimedia.org/wiki/Main_Page

Before your gerrit patch:

Is this image from your local? It's different from what's in production.

After gerrit patch:

Yes, and that's how production looks currently. That means no change, and the patch works correctly.

Jdlrobson added a comment.EditedSep 21 2020, 9:34 PM

Sorry my screenshot was confusing. Let me try that again.

When logged out Vector injects a menu item #pt-anonuserpage at the front. This is a hack on Vector's part and should be done in core, but is currently done in Vector.

This should always appear in front of ULS like so:

It's my understanding that ULS hides this in our production configurations, however with a very visible flash of unstyled content (which is a bug BTW and should be fixed).
You can however replicate this incognito window with https://commons.wikimedia.org/wiki/Main_Page?safemode=1

After your change, ULS moves to after the #pt-anonuserpage element.

Commons as it looks to me now. (Note the personal tools items order)

^ see above comment

When logged out Vector injects a menu item #pt-anonuserpage at the front. This is a hack on Vector's part and should be done in core, but is currently done in Vector.

Thanks. I see what you mean. That needs fixing itself. I will attempt that

After your change, ULS moves to after the #pt-anonuserpage element.

Yes, of course that's because the anon placeholder is a hack string concatenation that's coming after ULS already claims the first spot via array union. Once the placeholder item becomes integrated menu data item, the ULS will get what it wants.

Change 629324 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Provide basic anon personal menu placeholder.

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

Ammarpad added a comment.EditedSep 23 2020, 10:06 AM

After moving the placeholder to core, ULS will get what it wants. The patch will also allow removal of the placeholder duplication in Monobook as well as removal of multiple hacks for the extension in both Vector and MonoBook as well as another skin that seems to be replicating the same thing.

Default basic placeholder by coreVector styles itMonoBook styles itTimeless unsets it**

Note: Timeless did unsets it in this image otherwise duplicate 'Not logged in' text will appear in between ULS extension trigger and Talk link.

Thanks for working on this! We will need patches for Vector and Monobook removing this code to avoid duplicating the menu item.
I think it's fine to ship this to Modern.

Change 629457 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/skins/Timeless@master] Timeless: Unset anon placeholder item

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

Change 629465 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/skins/MonoBook@master] Remove ULS extension and anon placeholder hacks.

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

Change 629457 merged by jenkins-bot:
[mediawiki/skins/Timeless@master] Timeless: Unset anon placeholder item

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

Change 628740 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Remove ULS extension hack and anon placeholder code

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

Change 629465 merged by jenkins-bot:
[mediawiki/skins/MonoBook@master] Remove ULS extension and anon placeholder hacks.

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

Change 629324 merged by jenkins-bot:
[mediawiki/core@master] Provide basic anon personal menu placeholder.

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

Jdlrobson updated the task description. (Show Details)

@Jdlrobson I'd need your help on how to reproduce the problem that $wgULSPosition ='none' hack is supposed to solve in MobileFrontend. Because me I am not seeing anything if I removed it (MobileFrontendHooks.php#L123).

For context, the hack was added in T59091 several years ago.

Change 630720 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/extensions/MobileFrontend@master] Remove $wgULSPosition hack for ULS extension

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

Change 630720 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove $wgULSPosition hack for ULS extension

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

Ammarpad closed this task as Resolved.Sep 29 2020, 4:48 PM
Ammarpad claimed this task.
Ammarpad removed a project: Patch-For-Review.
Ammarpad updated the task description. (Show Details)