Page MenuHomePhabricator

Related Article cards are not shown on Timeless skin
Closed, ResolvedPublic

Description

Related article cards are not shown with the Timeless skin, but with Vector and Minerva skins as RelatedArticles disables all skins by default and requires them to provide their own styles consistent with the skin's appearance.

To enable it the config should be updated and some styles should be provided.

Examples:

Changes

  • Add skinStyles styling for the ext.relatedArticles.readMore module in RelatedArticles
  • Config change
  • Feel free to add to the default whitelist in RelatedArticles extension as a follow up when this is done
$wgRelatedArticlesFooterWhitelistedSkins = [ 'minerva', 'vector', 'timeless' ];

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 393080 had a related patch set uploaded (by Jack Phoenix; owner: Jack Phoenix):
[mediawiki/extensions/RelatedArticles@master] [WIP] Use mw.util to get the element ID for skin-agnosticness

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

So RelatedArticles appears to have a configuration variable named $wgRelatedArticlesFooterWhitelistedSkins and by default only minerva is whitelisted in the extension's extension.json. This explains why on the aforementioned German Wikivoyage page the related articles stuff doesn't show up if using Cologne Blue or Modern.

Furthermore I'm a bit suspicious about the code in /extensions/RelatedArticles/resources/ext.relatedArticles.readMore.bootstrap/index.js as it certainly looks somewhat skin-specific and making assumptions about class names and IDs. My aforementioned untested patch attempts to work around that by utilizing mw.util.$content appropriately.

In general I feel that whitelists are a Bad Thing™ and should be avoided.

Jdlrobson subscribed.

There's two things going on here:

  1. $wgRelatedArticlesFooterWhitelistedSkins needs to include 'timeless' to whitelist the skin for use with this feature.
  2. Timeless needs to provide some styling.

This should require no changes for RelatedArticles.
Using mw.util.$content seems like a good idea but isn't needed to fix this. All you need is:

$wgRelatedArticlesFooterWhitelistedSkins = [ 'minerva', 'vector', 'timeless' ];

The whitelist is intentionally done this way to allow skins to opt into this experience at their leisure as we provide minimal styling and skins should provide skinStyles.

The current experience is broken and will needs styling before enabling:

Screen Shot 2017-11-27 at 8.48.13 AM.png (479×1 px, 103 KB)

So if I am understanding this correctly, the issue here is that this extension doesn't know where to add its content in the dom because skins vary and it's doing so via js?

Why not just use the SkinAfterContent hook to insert an empty placeholder after the content with a known id, add the stuff to that? Like flagged revisions, I think.

(It looks like the broken styles in the example are because it got added to the wrong element there - those seem to be the content header styles, but it looks like this shouldn't even be in the content to begin with based on the linked example?)

...also why is it headers at all?

So it looks like the headers styling issue has since been fixed, regardless of why, and the reason why it's not using that hook is because Minerva doesn't support that hook and this extension is apparently primarily for Minerva, based on the default whitelist only including Minerva.

...Minerva should really support that hook. Filed T226143.

Change 518032 had a related patch set uploaded (by Isarra; owner: Isarra):
[mediawiki/extensions/RelatedArticles@master] Use SkinAfterContent hook to place cards in the DOM

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

All right, so experimenting with using the hook, it looks like it works pretty much as expected in most skins: Minerva (once support is added) is unchanged, Timeless, Modern, and other skins behave as expected, etc.

The main problems seem to be CologneBlue, MonoBook, and Vector:

  • CologneBlue just doesn't style the DataAfterContent properly in general: on short pages anything there typically winds up overlapping the sidebar (PendingChanges also has this issue). Filed T226201, but we probably don't particularly care for the sake of this regardless.
  • Vector and MonoBook work just fine, they're totally usable and it looks okay, it's just in a different place in them now due to how they place the DataAfterContent in their skin structure:

image.png (1×1 px, 251 KB)

I'm going to go out on a limb and guess whoever was behind this design placed the related article cards outside the content block for a reason, however, and frankly I think I agree - as a designer myself, I've likewise moved the entire DataAfterContent block outside of the content block in quite a few skins because in terms of related content, it tends to just make more sense this way for what it is and the way extensions use it. We should consider doing this in MonoBook/Vector as well: T226199

Change 518100 had a related patch set uploaded (by Isarra; owner: Isarra):
[mediawiki/skins/MonoBook@master] Style RelatedArticle cards

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

Change 518102 had a related patch set uploaded (by Isarra; owner: Isarra):
[mediawiki/skins/Vector@master] Style RelatedArticle cards

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

Sorry, apparently this is a site request because apparently it's only enabled for some skins some places regardless. I misunderstood that.

Anyway, the patch pile here is thus:

At which point we should basically be exactly where we started in terms of what people actually see, but such that RelatedArticles should mostly just work with whatever if enabled. May also need to discuss/look into how this will affect pending changes and anything else using the same hook.

No patches needed to wmf-config aside from one to address this task specifically and, er, actually enable RelatedArticles for Timeless (and possibly other skins) on whichever project(s) were wanting it, probably WikiVoyage, based on the example.

Change 518102 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Style RelatedArticle cards

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

Change 518032 merged by Niedzielski:
[mediawiki/extensions/RelatedArticles@master] Use SkinAfterContent hook to place cards in the DOM

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

The effect of this change was that related articles feature was enabled for all readers and users in the default skin, Vector. In fact, this has done two things, not one, for wikis using FlaggedRevs extension, since FlaggedRevs interface was moved outside of content area and enabled in Minerva.

I am not against this, but I have two questions:
a) Was this announced in tech news at all? (Figuring out where the change with RelatedArticles happened is how I landed here.) Will this be announced there?
b) Now that this is done, is there anything blocking doing RelatedArticles server-side? It’s a +1 API request on every view, unless that is not a problem at all.

@stjn the extension was setup to allow disabling of Related Articles on certain skins (for example on English Wikipedia the community didn't want RelatedArticles on desktop but did on mobile - so it is only enabled on Minerva). We shouldn't be enabling this feature on skins it wasn't shown on before. I don't want to speak for @Isarra but I don't think enabling it on Vector was her intention.

There is a config variable specifically for this - $wgRelatedArticlesFooterWhitelistedSkins. If it's empty it shows on all skins, otherwise it's used as a whitelist against skin keys.

It looks like $wgRelatedArticlesFooterWhitelistedSkins got converted to an empty array from ['minerva'] in Isarra's patch.

In mediawiki-config we set:

wmgRelatedArticlesFooterWhitelistedSkins = [ 'minerva' ]

but for some reason that doesn't seem to be happening - it's functioning as if it wmgRelatedArticlesFooterWhitelistedSkins = [] - an empty array.

I seem to remember an issue with empty arrays in mediawiki config (and I seem to remember @Legoktm bringing up the problem)
It likely needs an associated change in mediawiki-config (Wikimedia-Site-requests ) to disable this again explicitly on enwiki (also a great time to check in on whether that's wanted - but I expect not much has changed there).

What Jon said. Default behaviour was changed, but that was definitely not supposed to override projects where a value is actually explicitly set, as is the case here. Uh.

Hmm.

Does default config include Vector? Because I was not speaking about English Wikipedia, btw (I cannot reproduce it there, if that matters, while seeing the change in Russian Wikipedia, both logged in and logged out). The Related articles block was previously not shown on Vector everywhere, now it gets shown. I do not have complaints about that, and did not, certainly, came to grumble from English Wikipedia :-)

Only twiki, hewiki, and wikivoyage configs currently include vector. These changes were just supposed to better open up the option for enabling and using it more widely; whether or not we actually do that is more... I dunno, up to the projects, probably? And perhaps it would also make sense to just enable it for all skins on the ones that currently do have it on vector, since they're more likely to expect it on all of them to begin with (as evidenced by this task, for instance)...

Whatever the case, we haven't actually touched the settings yet, merely broken them. >.>

Anyway, I have no idea how to fix this without some more information on what the bug actually is, but if this is something we've seen this before, we can probably just copy whatever was done then?

Elitre subscribed.

Adding the TN tag only for the writer to evaluate - if the unintended change affected more than one wiki, then it may be a good idea to mention it.

Trizek-WMF subscribed.

Tech News usually don't cover Timeless display issues.

Change 527570 had a related patch set uploaded (by Isarra; owner: Isarra):
[mediawiki/extensions/RelatedArticles@master] Untested attempt to avoid triggering wmf-config bug

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

Is there an actual bug for the wmf-config empty array issue?

Tech News usually don't cover Timeless display issues.

To be clear, this is not a Timeless display issue, this became an issue in all skins, including default one, Vector. Since there are some forum topics with mobs assembling, it might be a good thing to mention that this was a mistake.

To be clear, this is not a Timeless display issue, this became an issue in all skins, including default one, Vector. Since there are some forum topics with mobs assembling, it might be a good thing to mention that this was a mistake.

Thank you for the clarification, @stjn. Would the following explanation be correct? "

Due to an accidental configuration change, RelatedArticles local configurations became ineffective. This has been fixed.

I can suggest something like:
‘The change in RelatedArticles extension accidentally enabled it for everyone, not just on mobile. This has been fixed.’

(assuming it will be, of course)

Local configuration still continues to work, global configuration doesn’t.

I can suggest something like:
‘The change in RelatedArticles extension accidentally enabled it for everyone, not just on mobile. This has been fixed.’

(assuming it will be, of course)

Thank you. I'm assuming it will be fixed by Monday.

By the way, the current task title and description is not describing this issue at all. It should be updated.

Actual task for unintended enabling is T229644. The discussion just started here because folks weren't sure what was going on, but I'll updating that one to be a tad more descriptive now.

Señores programadores! Por favor, desactivar esta opción para verciones de ordenador en Ru-Wiki. La comunidad esta muy sorprendida por los repentinos cambios no consensuados y realizados sin previo aviso y discusión. Ver aqui. No sé cómo solicitar una corrección correctamente, así que escribo aquí. Sinceramente, S.I.

Jdlrobson claimed this task.

I think the bug is the config uses wmg rather than wg.

Let's move conversation to T229644 which is clearer about the problem. This task is resolved.

This is still ongoing; we haven't actually enabled RA anywhere new (intentionally) in timeless.

Change 527570 abandoned by Isarra:
Use null instead of [] for default whitelist to avoid triggering wmf-config bug

Reason:
lolololol

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

Change 528506 had a related patch set uploaded (by Isarra; owner: Isarra):
[operations/mediawiki-config@master] Enable Related Article cards in Timeless across all projects

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

Change 518100 merged by jenkins-bot:
[mediawiki/skins/MonoBook@master] Style RelatedArticle cards

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

@Jdlrobson If I redo the config patch to actually enable RL cards on timeless, how can we go about applying it? Could you handle that, or let me know what I would need to do to do so? Thanks!

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

(Or if you think there's something about how it is now that's a bad idea, etc, or wrong, or whatever, please let me know.)

@Jdlrobson If I redo the config patch to actually enable RL cards on timeless, how can we go about applying it? Could you handle that, or let me know what I would need to do to do so? Thanks!

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

https://gerrit.wikimedia.org/r/528506 just needs a SWAT arranged via https://wikitech.wikimedia.org/w/index.php?title=Deployments
It's your skin so I leave the decision making to you. Some people like the feature, some dislike it so that's why it's on some skins and not on others. Personally I don't think it's worth enabling anything unless asked for explicitly as you end up having to maintain it. Looks like @RolandUnger as the bug filer wanted it, but not sure which wikis they had in mind.

All right, thanks!

As someone still vaguely involved with maintaining AFTv5, though, I gotta say that doesn't daunt me at all... :P

Added to 18:00–19:00 UTC SWAT window mondaaaaaay.

Change 528506 merged by jenkins-bot:
[operations/mediawiki-config@master] Enable Related Article cards in Timeless across all projects

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

Muahahahahahah! I'm evil, so I added a 'mua' to the beginning!

Change 393080 abandoned by Jdlrobson:
[WIP] Use mw.util to get the element ID for skin-agnosticness

Reason:
The associated phabricator ticket has been resolved so this issue should be taken care of now.

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