Page MenuHomePhabricator

Technical: Two instances of smart logout code are shipped to Minerva
Closed, ResolvedPublic3 Estimated Story Points

Description

Loading code twice sows confusion in developers and bloats code for end users.

Following on from T257265, the mediawiki.page.ready module was recently enabled on mobile.
This module contains currently inert code that binds a click handler to #pt-logout a[data-mw="interface"] to provide a "smart logout". This selector matches nothing in Minerva and is duplicated here: https://github.com/wikimedia/mediawiki-skins-MinervaNeue/blob/f915d69872b0f4e3e9c93903adb49889cdb6033a/resources/skins.minerva.scripts/initMobile.js#L269

I see two possible solutions for paying off this technical debt:

  1. Match the core selector - requires supporting IDs on MenuEntry items and adding data-mw=interface attributes
  2. Allow customization of the selector via the SkinPageReadyConfig that Minerva is already using and update the selector

it seems 1 would be preferable.

Developer notes

  • Matching the selector will require adding data-mw="interface" to main menu links. It seems this could easily be hardcoded to all links in includes/Skins/menuGroup.mustache.
  • The ID of the li item in menuGroups could be derived from the existing name e.g. id="pt-{{name}}" or provided explicitly via changes to the model

Gotchas:

  • Note in non AMC there are 2 links that would match such a selector
  • Mustache inherits parent properties. Be careful not to inherit the ID of the parent group e.g. #p-personal

QA steps

On both Vector and MinervaNeue skins:

  • Click the logout button
  • A notification message should show telling you that you are logged out
  • You should be logged out.

QA Results - Beta

ACStatusDetails
1T259200#6829409
2T259200#6829409

QA Results - Prod

ACStatusDetails
1T259200#6841368
2T259200#6841368

Event Timeline

Jdlrobson updated the task description. (Show Details)

I'm not 100% sure whether option 1 or 2 is better here, so would like some advice from someone else :)

I vote for 1 - Minerva being compatible with what core/vector provides. Adding support for MenuEntries shouldn't be a problem ( id's are already supported, you can pass those as an attribute when creating a component).

I vote for 1 - Minerva being compatible with what core/vector provides. Adding support for MenuEntries shouldn't be a problem ( id's are already supported, you can pass those as an attribute when creating a component).

+1

This could be as neat as

final class LogOutMenuEntry extends SingleMenuEntry {
  public function __construct( /* ... */ ) {
    /* ... */

    // Maintain compatibility with MediaWiki's logout button, in particular as referenced in the "mediawiki.page.ready" RL module.
    $this->attributes += [
      'id' => 'pt-logout',
      'data-mw' => 'interface',
    ];
  }
}
Jdlrobson renamed this task from Two instances of smart logout code are shipped to Minerva to Technical: Two instances of smart logout code are shipped to Minerva.Jul 30 2020, 2:59 PM
Jdlrobson updated the task description. (Show Details)

let me pick that, it should be simple and straightforward ;)

I'm still going to push the fix for that, but after ~4 months not working on MediaWiki code literally nothing works and the only solution is to drop entire repo and start it from scratch again. I struggled to get my env working again with everything what I had on the weekend but after couple hours I surrended.

@polishdeveloper sounds good :) I'll be keeping an eye out for it!

Ok, so a short summary of my struggles

We decided to go with proper approach - trying to match core selector. Yesterday I tried to do it right and it's not as easy at it seems.

  1. first of all, the $attributes is private, it would be great to not break encapsulation. Therefore I used setNodeId method to set the element id, and implemented new setDataAttribute($name, $value);
  2. the next problem is that when we render things, we do not iterate over component properties. The template is as simple as <a href="{{url}}" class={{classes}}"><span>{{text}}</span></a>. The id nor data-mw wasn't present, because we don't render those
  3. after fixing the template, I got the problem mentioned in Gotchas, where the menu entries without defined id got the parent id, for example p-personal. So I renamed the attribute to component-id so it doesn't conflict with group id. Changes were required also for PageActions menu items.
  4. When I fixed also that, code still didn't work because core expects <... id="pt-logout"><a data-mw="interface" ...</a></...>, not <a id="pt-logout data-mw="interface"></a>. The solution for that would be to wrap some (or all) menu entries with `<span>
  5. but even if we do that, then @Jdlrobson says there can be more than one logout button on the page, therefore we cannot depend upon #pt-logout

I solved 1 to 3, but 4 and 5 are remaining, and at this moment I'm almost sure that this cannot be achieved without modifications in core. Therefore I change my vote and propose to go with option 2 - customize the SkinPageReadyConfig and provide selector for logout button and leave minerva templates as it is (only remove the logic for dynamic logout button).

/cc @phuedx

Makes sense to me. @polishdeveloper would you be interested in pushing a patch to allow configuration of the logout button selector? I guess we'd need to enforce the data-mw=interface portion however to avoid challenges with user content posing as logout buttons.

data-mw="interface" definitely should stay. And on top of that we should use something else instead of #pt-logout, also I don't see real value in wrapping elements menu elements with additional wrappers, so we would have to search for element that has both classname and data-mw element. Something like .menu a[data-mw=interface][class*=menu__item--logout] should do the job. I'll create a patch.

Change 625663 had a related patch set uploaded (by Polishdeveloper; owner: Polishdeveloper):
[mediawiki/skins/MinervaNeue@master] Use component-id instead id when specifing menu entry ids

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

Krinkle updated the task description. (Show Details)

Repaired links by using permalinks. For future reference, pressing Y on a github page will turn the current address into a permalink.

ovasileva lowered the priority of this task from High to Medium.Sep 23 2020, 4:36 PM

I see some action in phab, give me this week to push what I have so far.

@Jdlrobson, @phuedx - cleaning up the board a bit and moving this to triaged but future for the time being. Let me know if there's more urgency needed

While this is technical debt, I'd personally say this is high priority as we're shipping unnecessary JS to every single page view (that includes anonymous users) and given the timeline on this task it's likely to linger here without proper prioritisation.

Change 660911 had a related patch set uploaded (by Polishdeveloper; owner: Polishdeveloper):
[mediawiki/core@master] Introduce possibility to override logout link selector

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

Change 660913 had a related patch set uploaded (by Polishdeveloper; owner: Polishdeveloper):
[mediawiki/skins/MinervaNeue@master] Drop smart logout handling in favor of core ready.js

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

Pushed the smallest/simplest change to get this task done. Menu handling still requires some small refactoring but that can be a part of different task.

Change 660911 merged by jenkins-bot:
[mediawiki/core@master] Introduce possibility to override logout link selector

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

Change 660913 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Drop smart logout handling in favor of core ready.js

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

Test Result - Beta

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

Test Artifact(s):

QA Steps

On both Vector and MinervaNeue skins:

Click the logout button
✅ AC1: A notification message should show telling you that you are logged out

Screen Recording 2021-02-14 at 1.34.23 PM.mov.gif (850×838 px, 170 KB)

✅ AC2: You should be logged out.
See Above

Test Result - Prod

Status: ✅ PASS
Environment: cawiki
OS: macOS Big Sur
Browser: Chrome, Firefox
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA Steps

On both Vector and MinervaNeue skins:

Click the logout button
✅ AC1: A notification message should show telling you that you are logged out

Screen Recording 2021-02-18 at 9.11.29 AM.mov.gif (872×944 px, 261 KB)

Screen Recording 2021-02-18 at 9.09.50 AM.mov.gif (872×944 px, 252 KB)

Screen Recording 2021-02-18 at 9.07.32 AM.mov.gif (872×944 px, 308 KB)

✅ AC2: You should be logged out.
See Above

Looks good, resolving