Page MenuHomePhabricator

Sitenotice: Button for dismissing content isn't in the right place and does nothing
Closed, ResolvedPublicBUG REPORT

Description

Few users reported problem on Serbian Wikipedia's Discord server that "button" for dismissing content (Одбаци) isn't in the right place after promoting all wikis to 1.36.0-wmf.3.

It should be right (see capture on WebArchive.org from 31th July).

Current look:

This can be reproduced on all wikis which have something in the sitenotice.

Event Timeline

Kizule created this task.Aug 7 2020, 12:30 AM
Restricted Application added a project: Internet-Archive. · View Herald TranscriptAug 7 2020, 12:30 AM
Restricted Application added subscribers: Petar.petkovic, Aklapper. · View Herald Transcript

Removing Internet-Archive, as this isn't bug report related to this service. Also, change MediaWiki-General with something else, I wasn't sure which tag to use.

Kizule updated the task description. (Show Details)Aug 7 2020, 12:32 AM
Kizule changed the subtype of this task from "Task" to "Bug Report".Aug 7 2020, 12:46 AM
Kizule edited projects, added DismissableSiteNotice; removed MediaWiki-General.

I think that this one is correct. 😄

I have found tab in browser which I haven't refreshed whole day, with opened Serbian Wiktionary.

And in it, sitenotice was correct and I made screenshot (picture left). I've refreshed page before few seconds, and it now shows sitenotice after deployment. You can see differences down.

Sitenotice before deploymentSitenotice after deployment

@Zoranzoki21: If you still have the old, "working" tab open I'd be interested to know what the HTML source was before this bug happened. :)
Now the HTML source is <a tabindex="0" role="button">Одбаци</a>.

Ammarpad added a subscriber: Ammarpad.EditedAug 7 2020, 12:47 PM

@Zoranzoki21: If you still have the old, "working" tab open I'd be interested to know what the HTML source was before this bug happened. :)
Now the HTML source is <a tabindex="0" role="button">Одбаци</a>.

This is the outputs before the bug and after:

origin/wmf/1.36.0-wmf.2 (before bug)
<div id="siteNotice" class="mw-body-content"><div class="mw-dismissable-notice"><div class="mw-dismissable-notice-close">[<a tabindex="0" role="button">dismiss</a>]</div><div class="mw-dismissable-notice-body"><div id="localNotice" lang="en" dir="ltr"><p>Test site notice</p></div></div></div></div>
origin/wmf/1.36.0-wmf.3 (bug starts)
<div id="siteNotice" class="mw-body-content"><div class="mw-dismissable-notice"><div class="mw-dismissable-notice-close">[<a tabindex="0" role="button">dismiss</a>]</div><div class="mw-dismissable-notice-body"><div id="localNotice" lang="en" dir="ltr"><p>Test site notice</p></div></div></div></div>

In other words, there are no differences.

Ammarpad renamed this task from Sitenotice: Button for dismissing content isn't in the right place to Sitenotice: Button for dismissing content isn't in the right place and does nothing.Aug 7 2020, 12:49 PM
Ammarpad added a project: Vector.
Kizule added a comment.Aug 7 2020, 1:20 PM

Thanks @Ammarpad and @Aklapper for checks. Now also T259836 happens.

There are stylesheets that are not being included, and it started after ee6974ad. I am not sure why yet.

Kizule added a comment.Aug 7 2020, 2:27 PM

There are stylesheets that are not being included, and it started after ee6974ad. I am not sure why yet.

Umm, I've added extension on my wiki which I use for testing purposes and compared elements in capture of Serbian Wikipedia on WebArchive.org from July 31th and my wiki. You can see screenshots from "inspect element":

Capture of Serbian Wikipedia on July 31thMy testing wiki today

Looks like that elements of extension are missing. When I do copy-paste of elements from capture of Serbian Wikipedia to my testing wiki, sitenotice looks correctly.

I can replicate on https://sr.wikipedia.org/wiki/%D0%93%D0%BB%D0%B0%D0%B2%D0%BD%D0%B0_%D1%81%D1%82%D1%80%D0%B0%D0%BD%D0%B0
and can confirm neither of its modules are being added
:03.330 mw.loader.getState('ext.dismissableSiteNotice')
08:29:03.370 "registered"
08:29:10.972 mw.loader.getState('ext.dismissableSiteNotice.styles')
08:29:11.009 "registered"

According to these hooks, these modules only get added for logged in users or if wgDismissableSiteNoticeForAnons is true:
https://github.com/wikimedia/mediawiki-extensions-DismissableSiteNotice/blob/master/includes/DismissableSiteNoticeHooks.php

I am however logged in.

The fact this is using onSiteNoticeAfter to add JS and CSS is a bit suspect to me.

The SkinNoticeAfter skin is executed inside Skin::getSiteNotice which is executed by SkinMustache inside the processTemplate method.

I believe the issue lies in SkinMustache. It calls $html = $out->headElement( $this ); before getTemplateData.

I would suggest 2 patches:

  1. Update SkinMustache::generateHTML to call getTemplateData before headElement (short term fix)
  2. Update the DismissableSiteNotice extension to use a different hook for adding JS and CSS. SiteNotice. https://www.mediawiki.org/wiki/Manual:Hooks/BeforePageDisplay is the more appropriate hook.
Kizule added a comment.Aug 7 2020, 3:51 PM

Thanks @Jdlrobson. I think that option 2 is good.

But I have option 3: We maybe should add this functionality in Mediawiki, instead of having extension.

Ammarpad triaged this task as Medium priority.Aug 7 2020, 4:06 PM
In T259858#6368871, @Zoranzoki21 wrote:

But I have option 3: We maybe should add this functionality in Mediawiki, instead of having extension.

I was thinking of that. I find it very odd that a trivial dismiss button needs separate extension while the notice lives in core. Does that mean in vanilla mediawiki notice cannot be dismissed at all?

I would suggest 2 patches:
Update SkinMustache::generateHTML to call getTemplateData before headElement (short term fix)
Update the DismissableSiteNotice extension to use a different hook for adding JS and CSS. SiteNotice.
https://www.mediawiki.org/wiki/Manual:Hooks/BeforePageDisplay is the more appropriate hook.

Thanks Jon, I will fix the extension. It should use proper hook.

Ammarpad claimed this task.Aug 7 2020, 4:07 PM
Kizule added a comment.Aug 7 2020, 4:13 PM

Thanks!

@Ammarpad Can I open another task for adding this functionality in mediawiki/core?

@Zoranzoki21: Off-topic here as this task is about a specific software bug; please see the task tree in T28751 for existing requests - thanks! :)

Kizule added a comment.Aug 7 2020, 5:17 PM

@Zoranzoki21: Off-topic here as this task is about a specific software bug; please see the task tree in T28751 for existing requests - thanks! :)

Thanks, I've created T259905: Merge DismissableSiteNotice into core.

Kizule added a comment.EditedAug 7 2020, 5:21 PM
In T259858#6369101, @Zoranzoki21 wrote:

Oh, @Ammarpad and I've created tasks in same time, so I closed my as duplicate.

Change 619077 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/extensions/DismissableSiteNotice@master] Use BeforePageDisplay hook to add ResourceLoader modules

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

MKar added a subscriber: MKar.Aug 8 2020, 3:18 PM
Krinkle raised the priority of this task from Medium to High.Aug 10 2020, 5:18 PM
Jdlrobson closed this task as Resolved.Aug 10 2020, 9:07 PM

Should be fixed in current master by https://gerrit.wikimedia.org/r/c/mediawiki/core/+/619035. Can be backported if necessary.

We can open a new task for https://gerrit.wikimedia.org/r/c/619077 but let's keep the patch open for discussion around the long term solution based on whatever direction we can agree on as part of T259955.

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)

This is now backported.

Change 619077 abandoned by Ammarpad:
[mediawiki/extensions/DismissableSiteNotice@master] Use BeforePageDisplay hook to add ResourceLoader modules

Reason:
Not necessary, in light of T262118

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