Page MenuHomePhabricator

Fix conditional presence of 'sitenotice' data and #siteNotice element
Open, Needs TriagePublic

Description

I noticed the following in page view HTML:

	<div id="siteNotice" class="mw-body-content"><!-- CentralNotice --></div>

There is no anon notice, site notice, local notice, central notice or other notice present for me at the moment, but still this odd comment being output. No big deal, but it looked odd to me.

CentralNoticeHooks.php
	public static function onSiteNoticeAfter( &$notice ) {
		// Ensure that the div including #siteNotice is actually included!
		$notice = "<!-- CentralNotice -->$notice";
		return true;
	}

This suggest it isn't just boilerplate left over where there could sometimes be more output. Instead, we have found a workaround as this is the only thing it outputs to trigger a conditional that some skins presumably have...

MonoBookTemplate.php
		$this->getIfExists( 'sitenotice', [
			'wrapper' => 'div',
			'parameters' => [ 'id' => 'siteNotice', 'class' => 'mw-body-content' ]
		] ) 

 function getIfExists (  ) {
	 if (  != '' ) {
		
	}
 }

Interesting, so MonoBook doesn't want to output the siteNotice element when it's empty. Maybe because it has things like margins or borders around it that would look odd otherwise. Although nowadays that could be done with CSS :empty() instead.

In any event, regardless of what MonoBook's intention is, with CentralNotice installed, the reality is that the element will always be output regardless. So it's not working as intended.

This means either there's a potential for bugs with MonoBook right now on Wikipedia, or the condition is outdated and no longer needed in MonoBook.

In Vector:

VectorTemplate.php
		// Conditional values are null if absent.
		$params = [
			…
			'html-sitenotice' => $this->get( 'sitenotice', null ),
vector.mustache
	{{#html-sitenotice}}<div id="siteNotice" class="mw-body-content">{{{html-sitenotice}}}</div>{{/html-sitenotice}}

This one is different. It intends for the element to be shown for any non-null value. Which means for empty string it is fine to be added to the HTML. And indeed, Vector doesn't apply any styling to the element by default, so it is harmless to be there unconditionally.

But, Vector allows the element to be disabled by legacy SkinTemplateOutputPageBeforeExec hooks if they set the sitenotice SkinTemplate data key to null. In any other case it is added to the output.

But... trying this out locally (without CentralNotice installed), I find this is also not working as intended. The div#siteNotice element is not output in Vector by default when there is no site notice.

In MediaWiki core.

SkinTemplate.php
		$tpl->set( 'sitenotice', $this->getSiteNotice() );
Skin.php
	/**
	 * Get the site notice
	 * @return string HTML fragment
	 */
	function getSiteNotice() {
		$siteNotice = '';
		if ( Hooks::run( 'SiteNoticeBefore', [ &$siteNotice, $this ] ) ) {
			if (  ) {
				$siteNotice = $this->getCachedNotice( 'sitenotice' );
			} else {
				
			}
		}
		Hooks::run( 'SiteNoticeAfter', [ &$siteNotice, $this ] );
		return $siteNotice;
	}

Suspicious indeed. It certainly looks like it is always a string. If not, it would be odd for the CentralNotice hook to prepend an HTML comment to a non-string value, right?

Event Timeline

@AndyRussG Hey, do you know why this is happening? I want to help but I'm slightly confused here.

@Ladsgroup From core's perspective, "sitenotice" is an optional element. And some skins even use this to sometimes omit the DIV that would otherwise be empty.

This hack exists in CentralNotice because on the PHP side it does not know whether a banner will be displayed, so it makes the string non-empty with this placeholder HTML comment as content, so that the skin will always output the DIV, and thus its JS code can hook into it to insert a banner if needed.

Jdlrobson subscribed.

Can this be declined? Sitenotice is an optional element when it comes to skins.

[…] Sitenotice is an optional element when it comes to skins.

In theory, yes. In reality, no, as per the task description.

Whatever a skin thinks it is accomplishing, isn't. E.g. if a skin would want to give the sitenotice a margin or border only if present, then the reality is, that this margin or border will show all the time even without a notice, as per the task description.

Ladsgroup subscribed.