Page MenuHomePhabricator

User links: Logged-in user links menu need sorting
Closed, ResolvedPublic3 Estimated Story Points

Description

Background

Coming out of T276561, the user links menu for both logged in/out users needs to sort the menu items so that the "Logout" or "Login" links appear last in the list.

Description

As a logged-in user, when I access my user links, I should see the login/logout menu links last, even if gadgets are added to the menu.

Note that in the screenshot below, the UTC Clock gadget is appearing below the "Log out" link.

Screen Shot 2021-05-03 at 9.48.22 PM.png (704×1 px, 328 KB)

Acceptance criteria

  • The "Log in" and "Log out" links in the personal dropdown vector menu (when feature flag is enabled) appear last in the list.
  • Gadgets that currently add to the personal tools appear below the "Log in" and above the "Log out" items - e.g. if I call mw.util.addPortletLink('p-personal', '#', 'My link', 'mmssss', 'hey there') I get "random gadget" in the menu after the "Log in" or before "Log out" items.

QA steps

TK

QA Results - Beta

ACStatusDetails
1T281791#7163723
2T281791#7163723

Event Timeline

LGoto lowered the priority of this task from High to Medium.

Change 688550 had a related patch set uploaded (by Nray; author: Nray):

[mediawiki/core@master] POC: Add check for `data-items-before` attribute in `addPortletLink`

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

Change 688551 had a related patch set uploaded (by Nray; author: Nray):

[mediawiki/skins/Vector@master] POC: Add `data-items-before` attribute to consolidated user menu

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

Change 688551 abandoned by Nray:

[mediawiki/skins/Vector@master] POC: Add `data-items-before` attribute to consolidated user menu

Reason:

POC only

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

Change 688550 abandoned by Nray:

[mediawiki/core@master] POC: Add check for `data-items-before` attribute in `addPortletLink`

Reason:

POC only

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

There are several options I've thought of with this ticket (please feel free to add others!):

A) Do nothing

mw.util.addPortletLink already includes a param nextnode that clients (e.g. gadget authors) can use to specify which element the link should be added before. Therefore, a gadget author could pass #pt-logout for this param to make their gadget appear before the logout button menu.

Pros: No additional code from our side
Cons: Puts a burden on gadget authors to know/remember our rules for modern Vector (e.g. gadgets should always be added before the logout button). Other skins might not have the same constraints. As a result, compliance might be difficult.

B) Apply order: 1 css style on #pt-logout

As @Jdrewniak brought up, we could make the visual order different than the source order by using the order property. By using this property, the logout button would visually appear last in the menu (assuming no other styles were applied), but gadgets would still be added after the logout button in the DOM (if the nextnode param was not passed to mw.util.addPortletLink).

I have some accessibility questions/concerns with this approach though. For example, keyboard navigation could become confusing/unintuitive as users might jump from the logout button to a gadget due to the visual order/DOM order disconnect as I've demonstrated below:

2021-05-11 18.31.55.gif (542×488 px, 52 KB)

w3 recommendations for a navigation button with menu links include adding keyboard support. As an aside, they also suggest taking the links out of the tab order by using tabindex=-1 and instead suggest relying on the up/down arrow keys to focus the menu item which differs from the status quo for our menus, but that's a conversation for another day.

Pros: Easy implementation. Would be 1 line of CSS.
Cons: Accessibility concerns

C) Add logic to mw.util.addPortletLinkthat looks for a preferred nextnode

We could add a small amount of logic to the addPortletLink method that looks for a preferred position if the nextnode param wasn't passed from the client. This could be in the form of a data attribute added to the portlet container (e.g. data-items-before="#pt-logout") for example.

https://gerrit.wikimedia.org/r/688551
https://gerrit.wikimedia.org/r/688550

Pros: DOM/visual order would be correct by default (potentially better accessibility than option B. No breaking changes (new logic would be opt-in).
Cons: More involved implementation than the other options as it involves core and Vector changes (but not too bad).

Conclusion

Personally, I feel the burden placed on gadget authors associated with option A and the accessibility concerns associated with option B make me lean toward option C. If anyone has any concerns with option C that I haven't thought of or feel strongly toward option A/B/another option, please let me know! :)

I think there's an option D:
unset logout link and make it a separate menu/button that's impossible to add to by gadgets. @bwang has an interesting approach in the anonymous users implementation where he's thinking about taking this approach for create account/login. Perhaps he could share his thoughts here?

I would rather not take option C - it adds complexity to an existing method with no guarantee it will ever be used.

Jdlrobson added a subscriber: nray.

@Jdlrobson Yes, I think we could definitely unset the logout link in onSkinTemplateNavigation in Hooks.php, and handle it manually. I was also thinking an even simpler solution could be to just reorder $content_navigation['user-menu']['logout'] in that hook instead of unsetting it.

Edit: Nevermind, I forgot that gadgets add menu items after Vector haha. What Jon suggested still makes sense though. After this patch merges, it would be really easy to add the log out button to the new UserMenu.mustache template

ovasileva raised the priority of this task from Medium to High.May 13 2021, 8:16 AM

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

[mediawiki/skins/Vector@master] Move "logout" button to bottom of user links menu in modern Vector

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

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

[mediawiki/core@master] Factor out data required for creating a logout link

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

Change 699068 merged by jenkins-bot:

[mediawiki/core@master] Factor out data required for creating a logout link

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

Change 698192 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Move "logout" button to bottom of user links menu in modern Vector

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

Edtadros added a subscriber: Edtadros.

Test Result - Beta

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

Test Artifact(s):

QA Steps

✅ AC1: The "Log in" and "Log out" links in the personal dropdown vector menu (when feature flag is enabled) appear last in the list.

✅ Logged Out:  See below.

✅ Logged In: See below.

✅ AC2: Gadgets that currently add to the personal tools appear above the "Log in" or "Log out" items - e.g. if I call mw.util.addPortletLink('p-personal', '#', 'My link', 'mmssss', 'hey there') I get "random gadget" in the menu before the "Log in" or "Log out" items.

​✅ Logged Out:  See below.

✅ Logged In:  See below.
Logged OutLogged In
Screen Shot 2021-06-19 at 11.29.36 AM.png (795×1 px, 197 KB)
Screen Shot 2021-06-19 at 11.32.49 AM.png (795×1 px, 246 KB)

UPDATE: Passing this per Pass T281791#7166064

This comment was removed by Jdlrobson.
Jdlrobson updated the task description. (Show Details)

Edward, this one is behaving correctly, as you say the steps were unclear. Have revised.

User Links will be tested in Prod when the feature is turned on.