Page MenuHomePhabricator

Echo new message alert has no orange background in vector
Closed, ResolvedPublic

Description

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

Event Timeline

Restricted Application added a project: Growth-Team. · View Herald TranscriptAug 7 2020, 9:55 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Majavah added a subscriber: Majavah.Aug 7 2020, 3:53 PM
Pcoombe added a subscriber: Pcoombe.

Appears to only be an issue in Vector.

Xaosflux renamed this task from Echo new message alert has no orange background to Echo new message alert has no orange background in vector.Aug 7 2020, 5:00 PM
Xaosflux updated the task description. (Show Details)

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

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

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

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

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

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

Change 619035 merged by jenkins-bot:
[mediawiki/core@master] skins: Call headElement() after getTemplateData() in SkinMustache

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

Change 619074 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Add all ResourceLoaderModules using supported hook

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

Jdlrobson closed this task as Resolved.Aug 10 2020, 8:39 PM
Jdlrobson claimed this task.

@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).

.sEdivad removed a subscriber: .sEdivad.Aug 11 2020, 3:53 AM

Change 619092 merged by jenkins-bot:
[mediawiki/core@wmf/1.36.0-wmf.3] skins: Call headElement() after getTemplateData() in SkinMustache

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

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. :)

Thanks, works for me :)