Page MenuHomePhabricator

Site logo should not be an inline background-image in the html output
Closed, ResolvedPublic

Description

Let's not put logo URL in an inline background-image style, these can be cached forever and thus are very annoying to change. It should be trivial to make a ResourceLoader module that would just output '#p-logo a { background-image: url($wgLogo); }'.

(This affects Vector and Monobook skins.)


Version: 1.23.0
Severity: enhancement
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=30113
https://bugzilla.wikimedia.org/show_bug.cgi?id=71334

Details

Reference
bz56257

Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 2:13 AM
bzimport set Reference to bz56257.
matmarex created this task.Oct 28 2013, 5:07 PM

Sounds sensible, though maybe avoid adding a new module for it. Perhaps add it to the 'site' module.

On that note, we might also want to move usercssprefs into user (it makes sense to separate a.t.m. because one is embedded as only=styles). And user.groups is separate from 'site' to avoid having 'site' be user specific, it could perhaps be merged into 'user' though.

Change 98356 had a related patch set uploaded by Tholam:
Site logo loaded using UserCSSPrefsModule instead of inline

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

t.lam wrote:

How do I add it to the site module? I tried doing something similar to what's in the UserCSSPreferences but that didn't seem to work.

I didn't test the code below, but it should be enough to override the getStyles() method:

class ResourceLoaderSiteModule extends ResourceLoaderWikiModule {

public function getStyles( ResourceLoaderContext $context ) {

		$styles = parent::getPages( $context );
		$styles['all'][] =
			"#p-logo a { background-image: url($wgLogo); }";
		return $styles;

}

}

(In reply to comment #4)

$styles = parent::getPages( $context );

This of course should be getStyles, silly me. And I'm missing
global $wgLogo;.

t.lam wrote:

I tried putting this
public function getStyles( ResourceLoaderContext $context ) {
global $wgLogo;
$styles = parent::getStyles( $context );
$styles['all'][] = "#p-logo a { background-image: url($wgLogo); }";
return $styles;
}
in the ResourceLoaderSiteModule class but it doesn't get loaded. I already got rid of the inline styling but now there's just no logo.

Blaaaghh.

I spent way too much time tracking this down.

This is because the module is not loaded at all if its isKnownEmpty() method returns true, and it does return true for ResourceLoaderWikiModule (of which ResourceLoaderSiteModule is a subclass) if none of the pages it's supposed to check exist (as returned by getPages() method).

Of course, on a new wiki none of these pages exist, so the module is not even loaded.

The simplest solution that makes this work is adding public function isKnownEmpty( ResourceLoaderContext $context ) { return false; } to ResourceLoaderSiteModule, but this looks like an awful hack. We might want to create a new module after all per comment 0, but I'd like Krinkle to comment on this.

(I knew this is going to cause trouble, and Krinkle, I told you so. :P )

t.lam wrote:

I'll just go ahead and do it both ways and see which one we like more in the end.

Change 98460 had a related patch set uploaded by Tholam:
Create ResourceLoaderLogoModule to load logo

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

I had a little chat about this with Krinkle on #wikimedia-dev today.

Basically, it looks like we'll go with the original approach, it just needs some fixes – and while we're at it, let's change the selector for logo from "#p-logo a" to something saner like ".mw-wiki-logo" (and add this class to the appropriate element in the skins themselves).

Sorry, but I don't have much time for Wikimedia things today/tomorrow, so let me just paste the log here:

<Krinkle> MatmaRex: What about the sitelogo thing? Why is extending

the wikmodule a problem?

<MatmaRex> well, we need to override isKnownEmpty(), which looks hacky

to me, and might not be the most efficient thing ever in general

<MatmaRex> and we need to combine logic for cache invalidation with

the existing one

<MatmaRex> both of these mean that we need to much internal knowledge

of RLWikiModule than i'm comfortable with when subclassing it like
this

<MatmaRex> (unless i'm missing something obvious to you)
<Krinkle> MatmaRex: overriding isKnownEmpty is fine, because it

unconditionally adds the css rule. Unless you make that css rule
optional, there is no need to implement any known-empty logic.

<Krinkle> As for the cache invalidation, that's just parent::() and

then return max( $parentMTime, wglogomtime )

<Krinkle> which you calculate using the value-hash-cache-timestamp

logic

<MatmaRex> we have no wglogomtime i'm aware of, unless we store the

time we last noticed it changing

<Krinkle> yes
<MatmaRex> ugh.
<Krinkle> that's what we do for all values we cache that don't have

timestamps.

<Krinkle> we do the same for the language globals in langdata module
<MatmaRex> sounds unpleasant, but alright
<MatmaRex> so we stick to adding the logo to ResourceLoaderSiteModule?
<Krinkle> and definition timestamp, and git version hash, etc. there

is no other way to calculate it without lots of spurious invalidations
(e.g. you'd need an array of all files that can possibly affect this
global, and then some, and hte max() of their files, which is terrible
and doesn't even always work with git and wmf-config)

<Krinkle> MatmaRex: Yep
<MatmaRex> Krinkle: while we're at it, let's change "#p-logo a" to

".mw-logo", hm?

<MatmaRex> .mw-wiki-logo, rather
<Krinkle> MatmaRex: Be sure to document that this means we will now

always add a request for modules=site&only=styles on every page, where
previously this only happened on wikis that enabled sitejs/css and
actually created 1 or more wiki pages

<Krinkle> MatmaRex: Sounds good
<MatmaRex> document where?
<Krinkle> MatmaRex: Commit message
<Krinkle> MatmaRex: Also, given the above, this will now depend on

https://gerrit.wikimedia.org/r/#/c/95463/

<MatmaRex> ah, duh. sure
<MatmaRex> hmm?
<Krinkle> MatmaRex: If not, you need to change OutputPage to take

these new factors into account, but probably easier to make it
dependent on https://gerrit.wikimedia.org/r/#/c/95463/

<MatmaRex> why would it? it seemed to work when i tested very biriefly
<Krinkle> because we need to load site module unconditionally
<Krinkle> MatmaRex: Delete your local site js/css pages
<Krinkle> and try again :)
<MatmaRex> i have none. it loaded the site module when i changed

isKnownEmpty to return false, i think

<Krinkle> MatmaRex: OK, in addition, disable the wg variarbles that

enable it.

<Krinkle> They are false on a new wiki by default.
<MatmaRex> i'mmm pretty sure they're true by default
<MatmaRex> wgUseSiteJs / Css?
<MatmaRex> or something
<MatmaRex> ok anyway, will have to check this
<Krinkle> MatmaRex: okay, they weren' always true by default.

Whatever, the point is, OutputPage has some harccoded logic beyond
isKnownEmpty that will break your change if you don't fix it.

<MatmaRex> got it, thanks
<Krinkle> Which is ugly, and why I remove it with

https://gerrit.wikimedia.org/r/#/c/95463/

Change 98460 abandoned by Tholam:
Create ResourceLoaderLogoModule to load logo

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

Question: will this solve the problem of MediaWiki links published in Facebook or Google+ featuring the "Powered by MediaWiki" image at the footer (in pages without images added by editors) because these services can't find any other image in the page?

If this bugfix would make those services show the logo of the MediaWiki instance by default, this would be a great collateral effect. If not, then I might file a new enhancement request.

No, it won't; that would (to my understanding) require inlining the logo URL in the HTML output, but in a different way, or coming up with some magical solutions. That's already filed as bug 30113.

Change 162000 had a related patch set uploaded by Krinkle:
Use mw-wiki-logo class instead of inline background-image

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

Change 162001 had a related patch set uploaded by Krinkle:
Use mw-wiki-logo class instead of inline background-image

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

Change 98356 merged by jenkins-bot:
Set site logo url in ResourceLoaderSiteModule instead of inline styles

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

Change 162000 merged by jenkins-bot:
Use mw-wiki-logo class instead of inline background-image

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

The patch needs to use a separate RL module rather than being included in the site module.

Just for the record, I am questioning my initial reasoning for this. My current opinion is that logo should just use <img src=$wgLogo alt=$wgSitename />, and who cares about caching forever.

Change 163043 had a related patch set uploaded by Krinkle:
Set site logo in mediawiki.skinning.interface module instead of inline styles

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

Change 163043 merged by jenkins-bot:
Set site logo in mediawiki.skinning.interface module instead of inline styles

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

Change 163291 had a related patch set uploaded by Krinkle:
Use mw-wiki-logo class instead of inline background-image

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

Change 162001 merged by jenkins-bot:
Use mw-wiki-logo class instead of inline background-image

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

Change 163291 merged by jenkins-bot:
Use mw-wiki-logo class instead of inline background-image

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

Re-merged into core, and patched for the tarball skins that display a logo (MonoBook and Vector).