| Tacsipacsi | |
| Aug 7 2020, 9:55 AM |
| F31984234: Personal toolbar without orange.png | |
| Aug 7 2020, 9:55 AM |
When one gets a new talk page message, Echo turns the talk page link in the personal toolbar into a new message indicator. This indicator reads “You have new messages” and used to have an orange background. The background color is now gone, making this indicator almost unnoticeable. The background should be readded.
.mw-echo-alert
Wondering if lack of loading style is the same underlying reason as T259858: Sitenotice: Button for dismissing content isn't in the right place and does nothing
Looks like the related ResourceLoader module isn’t loaded on Vector; if I manually load it with
mw.loader.load( 'ext.echo.styles.alert' );
on the browser console, the talk page gets the background color. (So the cause is not a DOM change.)
It appears that the style for the alert isn't being loaded. The style is derived from modules/nojs/mw.echo.alert.less In extension.json, the ext.echo.styles.alert module is defined for it. When there is a new talk page message, includes/EchoHooks.php::onPersonalUrls will add that to the list of styles to load:
public static function onPersonalUrls( &$personal_urls, &$title, $sk ) { ... if ( $userHasNewMessages && !$user->getTalkPage()->equals( $title ) ) { if ( Hooks::run( 'BeforeDisplayOrangeAlert', [ $user, $title ] ) ) { $personal_urls['mytalk']['text'] = $sk->msg( 'echo-new-messages' )->text(); $personal_urls['mytalk']['class'] = [ 'mw-echo-alert' ]; $sk->getOutput()->addModuleStyles( 'ext.echo.styles.alert' ); } } }
.mw-echo-alert gets added to the talk link and the text is changed, so we know it's getting that far. However, once the page actually gets rendered, there's no call to load.php?modules=... that includes ext.echo.styles.alert, as @Tacsipacsi noted.
getRlClient in OutputPage.php is part of what creates those links. That method's documentation includes a warning that
Calling this too early may cause unexpected side-effects since disallowUserJs() may be called at any time to change the module filters retroactively. Skins and extension hooks may also add modules until very late in the request lifecycle.
And that appears to be what happened here. Previously, SkinTemplate::generateHTML called SkinTemplate::prepareQuickTemplate (which eventually called addModuleStyles) before it called VectorTemplate::execute (which eventually called getRlClient).
SkinMustashe::generateHTML used a similar process. However, in 8acef095b9f23674e41ccc008ce00d1e9ef06594, SkinMustashe::generateHTML was refactored, following the advice in T257630, to call OutputPage::headElement (which enventually called getRlClient) before SkinVector::getTemplateData (which eventually called addModuleStyles). This made the code look nicer, but it also broke it. By the time addModuleStyles tried to add a new style, getRlClient had already been called and wasn't accepting any new modules.
The change wasn't immediately apparent because Vector wasn't using SkinMustashe yet. When ee6974ad3559b87a9efe955611fb1f414ff2fa82 was merged and deployed, the bug appeared.
Change 619035 had a related patch set uploaded (by AntiCompositeNumber; owner: AntiCompositeNumber):
[mediawiki/core@master] SkinMustashe::generateHTML: Call headElement after getTemplateData
PersonalUrls shouldn't be used to add JS and CSS to the page. BeforePageDisplay or OutputPageBeforeHTML should be used. Can we have a patch to do that too?
Change 619074 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/extensions/Echo@master] Add all ResourceLoaderModules using supported hook
Change 619092 had a related patch set uploaded (by Krinkle; owner: AntiCompositeNumber):
[mediawiki/core@wmf/1.36.0-wmf.3] SkinMustache::generateHTML: Call headElement after getTemplateData
Change 619035 merged by jenkins-bot:
[mediawiki/core@master] skins: Call headElement() after getTemplateData() in SkinMustache
Change 619074 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Add all ResourceLoaderModules using supported hook
@Jdlrobson closed this task as Resolved.
What about https://gerrit.wikimedia.org/r/c/mediawiki/core/+/619092/ (a backport to wmf.3), which is still active? I hope it will be merged and deployed ASAP, but if it won’t, it should be abandoned to make this clear.
I don't have great availability until the Tuesday evening backport window. I should be able to have my patch deployed then though, I'll add it to the calendar.
The full fix will ride the train (group0 tomorrow, group1 Wednesday, group2 Thursday).
Change 619092 merged by jenkins-bot:
[mediawiki/core@wmf/1.36.0-wmf.3] skins: Call headElement() after getTemplateData() in SkinMustache
Mentioned in SAL (#wikimedia-operations) [2020-08-11T21:52:27Z] <krinkle@deploy1001> Synchronized php-1.36.0-wmf.3/includes/skins/SkinMustache.php: Ibe1f07346, T259872, T259858 (duration: 01m 04s)
@AntiCompositeNumber This is now backported so no need to make the backport window. :)
Change 668813 had a related patch set uploaded (by Alistair3149; owner: AntiCompositeNumber):
[mediawiki/core@REL1_35] skins: Call headElement() after getTemplateData() in SkinMustache
Cherry-picked the fix to 1.35 because of T259858.
Would someone mind to review it?
https://gerrit.wikimedia.org/r/668813
Change 668813 merged by jenkins-bot:
[mediawiki/core@REL1_35] skins: Call headElement() after getTemplateData() in SkinMustache