@Isarra - do you want to allow gadgets to reveal portals? if so i can submit a patch provided you are happy to review.

Change 599437 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] WIP: addPortletLink should reveal hidden menus

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

Change 599439 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Do not assume and the existence of the emptyPortlet and remove it

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

I'd consider emptyPortlet a skin-agnostic core interface, like mw-body, mw-userlink and various others.

I understand this wasn't obvious or at all documented. But, now that we know, was there actually a problem with using that as-is in Vector? Or was it just renamed for consistency under the assumption it was owned by Vector?

I'm not seeing what problem this solved or would solve otherwise.

In T253938#6175076, @Krinkle wrote:

I'd consider emptyPortlet a skin-agnostic core interfacem, like mw-body, mw-userlink and various others.

I'd challenge that as this class it is only used by the Vector, Timeless, DarkVector and Metrolook skins according to codesearch. All other skins do not have a concept of hiding portlets. My read of the code history is that this code was not pulled out of core along with Vector when it should have been. All those other skins came after that move.

The fact it is not an mw- prefixed class also doesn't help.

Is there a problem with using that as-is in Vector? I'm not seeing what problem this solved or would solve.

Based on my experiences and your feedback in T236711 I was under the understanding that addPortletLink shouldn't have skin-specific behavior. Minerva supports icons, but no other skin does. That aside, relying on a class in this way seems terribly brittle to me as evidenced by the recent breakage. If we want to keep the existing behaviour the class should be provided by a method in Skin.php not left to the skin to add.

The interface for portlets is a core concept through sidebar configs, extension hooks, and indeed addPortletLink. Your recent improvement to its internal structure in PHP, and the removal of Vector-specific mw.util code last year, I think have turned this into a fairly solid example. To have more of in the skin system, perhaps?

I think the main difference between emptyPortlet and Minerva's icons are that portlets are a core primitive. The icons are (still) Minerva-specific, don't exist in the sidebar structure in PHP, and we don't yet have strategy for how they would be identified, known to core/extensions/gadgets, and dynamically insertable/loadable.

Specifically, the idea that a portlet area may be empty seems skin-agonistic. Noting that core requires portlets to be named, and that the default structure assumed by addPortletLink involves a <div> around a <ul> (the div typically accommodates the label as a heading, checkbox hack, or other wrapping styling). There is no native way in CSS to hide or otherwise target the wrapper for something containing an empty list, e.g. .portlet:has(ul:empty) doesn't exist (yet). I think it would be impossible or unlikely for a skin to satisfy the core interface without something like an "empty portlet" marker that the server outputs and client removes as-needed.

Minerva does accomplish this, but does so by omitting the assistive labels, by omitting some sections entirely, and by assuming there will be at least one entry, or by not supporting dynamic addition (e.g. the HTML isn't there). This is fine for now, and not a priority I suppose, but it's likely you'll encounter this use case there too, eventually.

Examples of sections that may be empty (filed in by gadgets, or live preview):

• p-variants is empty by default.
• p-cactions can be empty by default (depends on user rights)
• p-lang can be empty by default.

I see three paths forward:

1. Inaction. Keep one single line of JS code in core as-is. (Perhaps document it.)
2. Major work. Inflate it to 10-20 lines of runtime code in Vector JS, figure out what to to with other WMF skins, write release/migrate notes, figure out what a new/minimal skin should do.
3. Minor work. Perhaps change emptyPortlet to emptyPortlet mw-portlet-empty (or some such) in mw.util, and use only the latter in Vector going forward.

From T249372#6179732:

.vector-menu-empty has also caused trouble: T253912, T253819 (because of caching, not the name though). .emptyPortlet is misleading and non-compliant with the naming convention, so it's reasonable to rename, but coupling it to Vector only adds complexity (patch) without added functionality. I'd suggest using .mw-menu-empty and deprecating .emptyPortlet (addPortletLink in core needs to remove both until all skins migrate).

Change 602439 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] addPortletLink should show rather than use emptyPortlet class

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

I think my main hang up is the class here. I don't think skins should have to know about magic classes and their meanings - I think any classes used by core should be provided by core as part of a Skin method. @Krinkle how would you feel about instead using a show/hide ? Looking at the code, a show/hide applying a style would have the same effects here. .emptyPortlet with style="display:block" would appear. and .emptyPortlet with style="display:none" would be hidden. Using hide/show doesn't enforce the class on any skin while providing a means for hide portals if it wants. The work is minor here and allows us to do the major work in future. Thoughts?

In T253938#6193671, @Jdlrobson wrote:

The work is minor here and allows us to do the major work in future.

Which major work is impacted by whether one line of code uses a class or an inline style?

I think my main hang up is the class here. I don't think skins should have to know about magic classes and their meanings.

I'm not understanding what is magical here, assuming this is not an argument against CSS classes as a way to separate concerns between state changing and style control. Some examples

• OOUI or VE adding a class loading and loaded to indicate current state, which consumers can then use to modify parts of the page as needed to accomodate that state in their component.
• Ajax watch in core indicating watched/unwatched loading.
• Twitter Bootstrap dropmenus gainig a class show to make them visible. Consumers decide whether that is a simple display change, animation or something else.
• mw-collapsible, mw-collapsed. Core module controls state. Consumer can control what it means to be expanded or collapsed visually.
• mw-userlink, mw-headline, mw-content-rtl, etc.
• Other CSS classes from core that skins need to know about: mw-body, mw-body-content, mw-content-rtl, mw-selflink, etc.

Before 2011, we didn't use lowercase and mw-prefixed class names much (I pushed us more toward that). Perhaps we should rename this to something like mw-portlet-empty to make it seem less out of place. Would that be an improvement?

Looking at the code, a show/hide applying a style would have the same effects here. .emptyPortlet with style="display:block" would appear. and .emptyPortlet with style="display:none" would be hidden. Using hide/show doesn't enforce the class on any skin while providing a means for hide portals if it wants. The work is minor here and allows us to do the major work in future. Thoughts?

The current patch uses .show() is generally stands out as an anti-pattern due to the akward legacy semantics is has to uphold. Consider the question, given an alement that has no inline style attribute, but some css classes set that make it display: none, how would jQuery know which inline style to set? Is it display: inline, display: block, display: table, display: list-item, etc. Moreover, consider a page with a <div> and a CSS rule that makes it behave as a table (like our Table of Contents), or a page with <whatsthatthingthere> and some default display rule in your site's stylesheet. jQuery does a lot of guess work to try and get this right (involving temporary DOM elements and what not).

The caller ought to know which display is preferred, using .css('display', …). That isn't only orders of magnitude faster, it also less likely to get it wrong and is easier to debug. However we can't do that here as we shouldn't assume what element is used here (I think). jQuery does kind of make this viable, but I'd also like to reduce use of jQuery, especially if it hides a very complex contract like "It will somehow set the right display inline style - and you have no control over it".

I also don't think it is helpful to skin developers and gadget authors to have elements in the DOM with a class of emptyPortlet that are in fact not an empty portlet. That's bound to waste people's time.

However if you want to standardise on an inline style, there is a less fragile to achieve that. We could require skins to set display: none server-side (instead of setting class=mw-portlet-empty like they do currently), then mw.util could simply remove that through .css('display', ''). This requires no knowledge of the skin's default appearance. This would be a breaking change, and we' require skins to remove their emptyPortlet classes as core would no longer unset them. But perhaps we can chain .css('display', '').removeClass('emptyPortlet') in MW 1.35 and remove the second half of that line in MW 1.36 :)

In T253938#6193671, @Jdlrobson wrote:

I think my main hang up is the class here. I don't think skins should have to know about magic classes and their meanings

Finding the meaning of .emptyPortlet was difficult, it's kind of "magical", I agree on that, but it's not unique in that sense. One could use the universally understood .hidden class, assuming there is no skin that implements a different behavior for empty menus (why would though).

The work is minor here and allows us to do the major work in future. Thoughts?

What's the major work that's not allowed by the current state?

In T253938#6194923, @Krinkle wrote:

But perhaps we can chain .css('display', '').removeClass('emptyPortlet') in MW 1.35 and remove the second half of that line in MW 1.36 :)

That sounds feasible.

Change 599437 abandoned by Jdlrobson:
WIP: addPortletLink should reveal hidden menus

Reason:
I'll circle back to this one in a few weeks time. My current thinking is that I should focus on integrating the SkinMustache class in Vector. Once that has happened I believe that some of the code in SkinVector::getMenuData including the emptyPortlet logic can be moved into core. I believe this will lead to all emptyPortlet logic living in core.

At minimum this will mean a Skin function that adds the appropriate class, a ResourceLoaderSkinModule that adds appropriate JS and CSS and a JS function that vector's collapsible tabs can query.

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

Change 599439 abandoned by Jdlrobson:
Do not assume and the existence of the emptyPortlet and remove it

Reason:
Per https://phabricator.wikimedia.org/T253938#6226228

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

Change 602439 abandoned by Jdlrobson:
addPortletLink should show rather than use emptyPortlet class

Reason:
Per https://phabricator.wikimedia.org/T253938#6226228

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

Change 615321 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Standardize portlets across skins

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

Change 615322 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Use newly available Skin::getPortletData method

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

Change 624899 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Expose portlet data in SkinMustache

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

Change 615321 merged by jenkins-bot:
[mediawiki/core@master] Standardize portlets across skins

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

Change 615322 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Use newly available Skin::getPortletData method to get mw-portlet class

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

@Jdlrobson this change to mw.util creates errors when someone makes it lose it's context i think.

var add = mw.util.addPortletLink;
			$(add('p-tb', '#', 'DATES to dmy', 'dmy-unitfixer', 'Align all dates to dmy', '', '')).click(ohc_all_to_dmy_driver);

Triggers: Exception with thrown value: TypeError: undefined is not an object (evaluating 'this.showPortlet')

https://en.wikipedia.org/wiki/Wikipedia:Village_pump_(technical)#Scripts_do_not_show_up

In T253938#6543120, @TheDJ wrote:

@Jdlrobson this change to mw.util creates errors […]

https://en.wikipedia.org/wiki/Wikipedia:Village_pump_(technical)#Scripts_do_not_show_up

Reported at T265809.