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).

In T259200#6348215, @polishdeveloper wrote:

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',
    ];
  }
}

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

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

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

✅ AC2: You should be logged out.
See Above

Change 625663 abandoned by Jdlrobson:

[mediawiki/skins/MinervaNeue@master] DONOTMERGE: Use `component-id` instead `id` when specifing menu entry ids

Reason:

Abandoning old patchset. Can be restored at later date if needed.

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