Page MenuHomePhabricator

Minerva should support skin agnostic mw.util.addPortletLink or similar
Closed, ResolvedPublic

Description

MediaWiki core provides a method mw.util.addPortletLink for adding menu items. However it makes a lot of assumption about the HTML structure of the menu and what needs to be added. It is used widely by gadget developers. In Minerva it doesn't work as the selectors passed don't match anything, and menu items are more complicated than a single li item - they can contain icons for example.

Minerva (the mobile skin) doesn't support this, meaning editors often use hacks to add things to menus with undesired results.
Examples:
Map gadget Wikipedia:


Wikisource:

Instead of passing a jQuery object and selector to mw.util.addPortletLink a new method is needed that replaces mw.util.addPortletLink and uses models, leaving presentation up to the skin.

Something like:

mw.menus.addItem( new mw.menu.Item( label, href, groupOfMenu, iconNameWhichDefaultsToSomePlaceholderIcon ) );
// addItem might throw an exception or warning if the group doesn't exist in the skin.

mw.hookSubscribe( 'menu-changed', function ( menu ) {
  // skin decides how to render menu.
} );
// if nothing has subscribed to hook gadget developer gets a warning telling them to ask skin to implement it.

Acceptance criteria

  • A method is available to gadgets that manipulates a model that corresponds to all menu contents
  • When the menu contents change, a signal is sent to the skin to render it
  • A skin must register how to add new items to menus.

Developer notes

Menus in mobile

Event Timeline

Jdlrobson created this task.Sep 3 2019, 5:55 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 3 2019, 5:55 PM
Isarra added a comment.Sep 3 2019, 5:59 PM

We may want to consider an approach here that would also be consistent with other skins that use icons for menus and whatnot - for instance as I recall, bluesky had an option to just shove icons into the mediawiki:sidebar that it would parse and use for its main menu. I dunno if that's necessarily indicative of an approach that would actually work here, but it's something to consider - we probably need to establish a consistent way of handling icons for skins that use them regardless.

Isarra added a comment.Sep 3 2019, 6:01 PM

(But yeah, tacking it onto the end with a default fallback seems fine to me.)

Isarra added a comment.Sep 3 2019, 6:09 PM

BaseTemplate should probably just come with a default handler for older skins (Vector etc) where it just does the same as the current on in effect so folks can safely migrate and not worry about what skins they're targetting.

So the gist of this proposal is that the new API to replace addPortletLink() would call some method/hook/whatever defined by the skin, which would do the actual work of creating a list item and inserting it. Why not just do this in addPortletLink()?

I'll also note that addPortletLink() already has a hardcoded special case for Vector, so there are more use cases for this improvement (or a new API) than just Minerva :)

I think the hard part in making it skin-agnostic is in allowing the caller to decide where to insert the new item. There is a lot of variety in the layout of navigational menus (let us not even think about Minerva and Timeless, both of which go crazy here – even the different layout of page tabs between MonoBook and Vector has forced us to handle them separately in some VE code that uses addPortletLink()).

So the gist of this proposal is that the new API to replace addPortletLink() would call some method/hook/whatever defined by the skin, which would do the actual work of creating a list item and inserting it. Why not just do this in addPortletLink()?

Maybe we could.
So working with this idea, let's take this example:

mw.util.addPortletLink('p-interaction', 'http://en.wikipedia.org/wiki/Wikipedia:Dashboard', 'Dashboard', 'n-dashboard', 'Visit the dashboard', 'd');

In this case Minerva would probably want to add this to to the following menu:

but that's nothing to do with an element with id p-interaction.

If we consider the group name p-interaction, and mw.util.addPortletLink doesn't add a link but rather triggers a hook with implementation details provided by the skin, I think we could work along those lines. An incomplete Minerva example might look a little like this for example:

mw.util.addPortletLink = function (portletId,href,text,id,tooltip,accesskey,nextnode){

  if (portletId === 'p-interaction' ) {
    mw.config.values.wgMinervaMenuData.groups[1].push( {
      components: [ { text: text, href: href } ],
     name: id
    } );
  }
 // @todo: refresh menu component
};
Krenair added a subscriber: Krenair.Sep 3 2019, 9:11 PM
ovasileva triaged this task as Medium priority.Sep 4 2019, 12:09 PM
ovasileva added a project: Readers-Web-Backlog.
ovasileva moved this task from Incoming to Upcoming on the Readers-Web-Backlog board.

@matmarex looking into this some more the following code in Minerva gets portlet links added where they need to be:

$('.minerva-user-menu .toggle-list__list--drop-down').attr( 'id', 'p-personal' );
$( '#mw-mf-page-left .menu:first-child' ).attr( 'id', 'p-navigation');
$('#page-actions-overflow .toggle-list__list--drop-down').attr( 'id', 'p-tb' );

However Minerva uses a different template for these items, so the do not render properly:

I think addPortletLink will need to be clever enough to work out how to template these links being added.

A data-template attribute on the list might be one solution.

Change 545636 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] addPortletLink should be compatible with Minerva skin

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

Change 545637 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Menu groups have identifiers

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

Made a stab at this. Since the goal is to give editors something that works - I think we can get away with just the above 2 patches here.

@Jdlrobson On non-AMC view only the main menu is available. How addPortletlink will be rendered in non-AMC mode without a personal and overflow menu?

@Jdlrobson On non-AMC view only the main menu is available. How addPortletlink will be rendered in non-AMC mode without a personal and overflow menu?

Those menus are not present so editors won't be able to use them. They will have no impact like the current situation. This is specifically targeted at Minerva desktop users and AMC users. We can't add to menus that don't exist.

Jdlrobson added a comment.EditedOct 24 2019, 5:35 PM
without icon placeholderwith icon placeholderwithout span
Jdlrobson updated the task description. (Show Details)Oct 24 2019, 10:12 PM

Change 546278 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Portlet links should generate valid HTML in Minerva

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

Change 545636 abandoned by Jdlrobson:
mediawiki.util: Make addPortletLink compatible with Minerva skin

Reason:
As much as I'd love to fix this properly, I don't have product support to do so.

I've downscaled this patch to
https://gerrit.wikimedia.org/r/#/c/mediawiki/core/ /546278 Portlet links should generate valid HTML in Minerva

It's a bit frustrating as gadgets will look weird in Minerva without the additional changes, but at least this puts control back in the editors hands.

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

Change 545637 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Menu groups have identifiers

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

Change 546278 merged by jenkins-bot:
[mediawiki/core@master] Portlet links should generate valid HTML in Minerva

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

Jdlrobson added a subscriber: Mooeypoo.EditedDec 10 2019, 9:58 PM

Some discussion here @Mooeypoo in particular discussion with @Krinkle on https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/545636/

Thanks for working on this! Note that it not only affects gadgets and user scripts, but also WMF software like PageTriage and Who Wrote That:

@kaldari would you (or someone equally informed) mind opening a new ticket for that example and pointing to the relevant code and how to replicate it locally? I don't want this to get lost!

Change 545636 restored by Jdlrobson:
mediawiki.util: Make addPortletLink compatible with Minerva skin

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