Page MenuHomePhabricator

setPageTitle to empty string or false should prevent rendering of H1 on page otherwise Wikivoyage will print 2 h1 elements in Vector
Open, HighPublic

Description

In various places, Flow, Gather, Wikidata want to rendering the h1 differently.
To do this they use $out->setHTMLTitle( '' ); which creates an empty h1.
This can lead so side effects.

For example:
http://en.wikipedia.org/wiki/Special:Gather/id/2?useformat=desktop has an empty h1 dom element at the top of the page with a black bottom border.

Can we make set $out->setHTMLTitle( false ); kill this h1 element?

Update

This was reopened in T97891#5079173 and broke the rendering of all Wikivoyage.org. Now 2 h1s are shown on a page (one is visible above the banner with an underline - see T97891#5079408) .

SEO is sensitive for this project given the Wikitravel.org relationship and 2 h1s can cause problems. We should revert or restore the previous behaviour of a single h1 ASAP.

Event Timeline

Jdlrobson raised the priority of this task from to Needs Triage.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: Jdlrobson.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 2 2015, 11:56 AM
Florian added a subscriber: Florian.May 2 2015, 4:15 PM

I think you mean OutputPage::setPageTitle()? OutputPage::setHTMLTitle() only set's the title-tag in head.

The problem is, that the h1 element isn't created by mediawiki core. Each skin uses the title (which can be set by OutputPage) to create the h1 tag alone, see e.g. Vector. So, for compatible reasons,it's abit more complicated to find a standard way (but would be possible and we could start implementing a way for wmf-deployed skins).

Maybe @matmarex has some good ideas here. Overriding the h1 and having full control of rendering views seems to be something we should support.

Lydia_Pintscher triaged this task as Normal priority.May 4 2015, 12:54 PM

@Jdlrobson can you give us a sense of the urgency of this from your POV? Trying to figure out how to prioritize this.

It can be hacked around so not urgent but I keep hitting this bug in all the projects I have worked on. It's something that should be done in core as there is clearly a need. Do we have an appropriate project for skin architecture @matmarex?

Do we have an appropriate project for skin architecture @matmarex?

MediaWiki-Interface ? :)

Can we make set $out->setHTMLTitle( false ); kill this h1 element?

This sounds sensible to me. Like Florian says, this would have to be implemented in every skin you care about separately (and others would probably continue outputting empty <h1></h1> tag with a false title, since it casts to empty string in PHP).

Doing this for Vector and mobile skin seems like a good start.

Change 209870 had a related patch set uploaded (by Florianschmidtwelzow):
Don't output an empty h1 element

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

Change 209871 had a related patch set uploaded (by Florianschmidtwelzow):
Don't output an empty h1 element

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

Florian claimed this task.May 9 2015, 2:11 PM

Doing it for Vector and MonoBook, Minerva already checks, if a title is set before creating the h1 element.

Change 209871 merged by jenkins-bot:
Don't output an empty h1 element

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

Change 209870 merged by jenkins-bot:
Don't output an empty h1 element

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

Florian renamed this task from setHTMLTitle to empty string or false should prevent rendering of H1 on page to setPageTitle to empty string or false should prevent rendering of H1 on page.May 12 2015, 12:50 PM
Florian closed this task as Resolved.

I left a short documentation in mediawiki's Manual:Skinning.

This has been effectively reverted in Vector in https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/500805 as a result of T219864.

Krinkle reopened this task as Open.Apr 2 2019, 7:42 PM
Krinkle added a subscriber: Krinkle.

Change 209870 had a related patch set uploaded (by Florianschmidtwelzow):
Don't output an empty h1 element
https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/209870/

This was effectively reverted in d56792addb (change 500805) to fix T219864.

I think the original use case of allowing extensions to control part of the skin layout is interesting and worth exploring still. Having said that, it seems Flow, Echo and Wikibase no longer use this feature (and Gather was undeployed). I wasn't able to find a use of it in Codesearch, but maybe I missed it?

I'd be interested in re-considering this feature with a specific use case in mind to see how we can best balance the need for flexibility, and the maintenance cost + developer usability involved. There are also other ways to do this, such as through a Skin method, or through a documented CSS class. It might then also make sense to tailer the interface to a more specific semantic intent so that the skin can respond in some way rather than just leaving a gap in its design. The notion of "Don't render the h 1" seems rather specific and hard to accommodate universally in all designs. Not to mention the impact on other extensions and gadgets from a central element missing on the page.

Whoops, and we have a regression on Wikivoyage relating to this change: https://en.wikivoyage.beta.wmflabs.org/w/index.php?title=Bancouver&mobileaction=toggle_view_desktop

Note the line at the top of the page - that's an empty h1:

The empty h1 should not be shown there for SEO reasons and for aesthetic...

I've uploaded a revert, but other solutions are welcomed.

Change 500838 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Revert "Fix <h1> to be present even if title is "0""

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

oh shoot.. timo already SWATed this, so now we've broken production Wikivoyage nooooo: https://en.wikivoyage.org/wiki/Singapore

Jdlrobson raised the priority of this task from Normal to Unbreak Now!.Apr 2 2019, 8:58 PM
Restricted Application added subscribers: Liuxinyu970226, TerraCodes. · View Herald TranscriptApr 2 2019, 8:58 PM
Jdlrobson renamed this task from setPageTitle to empty string or false should prevent rendering of H1 on page to setPageTitle to empty string or false should prevent rendering of H1 on page otherwise Wikivoyage will print 2 h1 elements in Vector.Apr 2 2019, 9:14 PM
Jdlrobson removed Florian as the assignee of this task.Apr 2 2019, 9:16 PM
Jdlrobson added a project: User-notice.
Jdlrobson updated the task description. (Show Details)
Johan added a subscriber: Johan.EditedApr 2 2019, 10:32 PM

@Jdlrobson: Is the User-notice tag mainly because of the production issues on Wiktionary (edit: eh, Wikivoyage, of course)? If so, is this a "there was a problem but it was [hopefully] fixed" when this goes out on Monday? Or will it, five days from now, require action from the editors?

@Johan not 100% sure whether this is user notice worthy, but I consider this a big UI regression for the wikivoyage community that is likely to be flagged on the Wikivoyage pubs (village pumps). The user notice would be, that we are aware of this problem and tracking here and hope to have a fix shortly. Is that appropriate for user notice?

@Krinkle let me know how we should proceed here. I would personally like to revert the change and undo the deploys (I think the broken "0" page is much less of a concern than the current situation we find ourselves in.

Johan added a comment.Apr 2 2019, 11:09 PM

Yes, I'd say it is.

Krinkle added a comment.EditedApr 3 2019, 4:57 PM

@Krinkle I think the broken "0" page is much less of a concern than the current situation we find ourselves in.

I see the underline bug on Beta indeed. I don't see any such bug at https://en.wikivoyage.org/wiki/Singapore, however.

This has been flagged on the travellers pub now - https://en.wikivoyage.org/wiki/Wikivoyage:Travellers%27_pub#Something_I've_never_seen_before - the problem is there are now 2 h1s on every page on Vector, the first one is empty.

Krinkle added a comment.EditedApr 3 2019, 5:07 PM

OK. Looks like it's still Varnish cached for most pages. But I see when logged-in now. I've hot-fixed for now by hiding it with CSS.

It's been visible on Minerva/MobileFrontend for years without issue, but it's less obvious there given Minerva's heading element neatly merges with the surrounding elements when empty (no forced margin or border).

In terms of medium/long strategy, it seems to me like the obvious way forward is for there not to be two headings. Rather then trying to suppress one, and insert another (neither for which the WPB extension controls the position or layout) - would it make sense to instead replace the existing one? The skin method behind this supports HTML (nor just text), which we can make use of.

Alternatively, to really tailor-fit the integration (such as on Minerva where you'll probably want the image to be full-width rather than 90% as page-heading would be), that's something we'll probably want to codify explicitly rather than work by accident. With a conditional block somewhere to explicitly support this. E.g. SkinMinerva::disableFirstHeading called from WPB. It could be generalised for all skins, but that's a breaking change, and as mentioned higher up, I'm not sure we can expect that for all skins. In core we'd have to go a step further and e.g. offer canDisableFirstHeading as well, so that extension can decide accordingly how to approach it.

Jdlrobson added a comment.EditedApr 3 2019, 5:15 PM

OK. Looks like it's still Varnish cached for most pages. But I see when logged-in now. I've hot-fixed for now by hiding it with CSS.

What about all the other Wikivoyage projects? Are we confident this (having 2 h1s in the page in the heading) is not having a negative impact on SEO for Wikivoyage?

, but it's less obvious there given Minerva's heading element neatly merges with the surrounding elements when empty (no forced margin or border).

I think that's pure luck to be honest.

Rather then trying to suppress one, and insert another (neither for which the WPB extension controls the position or layout) - would it make sense to instead replace the existing one?

Replacing is obviously the solution here, but I think it's more than the title and it's tricky since all skins do something different here. I think the challenge here is the heading is inside the banner and the ordering relating to other things e.g. subtitle (notice subtitle position is different on Vector and Minerva). I think the suppression is the easiest thing to do here without huge changes to the skin architecture.

Krinkle lowered the priority of this task from Unbreak Now! to High.Apr 3 2019, 5:16 PM

The unintended regression from Wikivoyage was only live for a few hours and is now mitigated.

@Johan If we want to highlight the "title 0" bug in Tech-News (which was live 4-8 days, depending on the wiki), the conversation about that is mainly at T219864. I would summarise it as: A regression in last weeks' branch caused the Vector skin to no longer show the top heading for pages with the title "0". This was fixed on Tuesday for all wikis.

Krinkle added a comment.EditedApr 3 2019, 5:22 PM

OK. Looks like it's still Varnish cached for most pages. But I see when logged-in now. I've hot-fixed for now by hiding it with CSS.

What about all the other Wikivoyage projects? Are we confident this (having 2 h1s in the page in the heading) is not having a negative impact on SEO for Wikivoyage?

The SEO case isn't relevant. Similar to T198947. In part because crawlers don't care whether there are multiple same-level headings on a page. I've yet to see evidence indicating that the document structure is in any way considered. If there were no headings at all, that might pose a problem, but that's not the case here.

Between WMF sites and Google in particular, our web views aren't actually indexed. Parsoid is used instead.

Johan added a comment.Apr 3 2019, 5:24 PM

OK, thanks, I've rewritten it to something very generic (so editors who've seen it knows it has been fixed) and linked to this discussion for those who want the details.

OK. Looks like it's still Varnish cached for most pages. But I see when logged-in now. I've hot-fixed for now by hiding it with CSS.

What about all the other Wikivoyage projects? Are we confident this (having 2 h1s in the page in the heading) is not having a negative impact on SEO for Wikivoyage?

It's still on Russian Wikivoyage https://ru.wikivoyage.org/wiki/%D0%A3%D0%BF%D0%BF%D1%81%D0%B0%D0%BB%D0%B0

I was still going through them :)

Found floating headline and fixed it: en, bn, es, fi, fr, he, it, ps, ro, ru, uk, vi, zh.
Appear unaffected: de, el, fa, pl, nl, pt, sv.
Broken for unrelated reasons (and fixed): hi.

t's been visible on Minerva/MobileFrontend for years without issue,

@Krinkle that's not true. Your patch https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/500814/1/includes/skins/SkinMinerva.php is why the h1 is present on Minerva.

matmarex removed a subscriber: matmarex.Apr 3 2019, 9:27 PM

t's been visible on Minerva/MobileFrontend for years without issue,

@Krinkle that's not true. Your patch https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/500814/1/includes/skins/SkinMinerva.php is why the h1 is present on Minerva.

No, it's not. Take another look and explain where the previous code results in the Minerva method not returning an <h1> element.

So do we want WikidataPageBanner to apply the CSS rule to hide this additional heading or would it be better to add to Vector/Minerva?

h1:empty { display: none; }
Krinkle added a comment.EditedJun 17 2019, 5:54 PM

I think it would be best suited as a skinStyle that WikidataPageBanner adds to one of its modules for specific skins it knows how to handle.

Change 500838 abandoned by Jdlrobson:
Revert "Fix <h1> to be present even if title is "0""

Reason:
This doesn't seem to be wanted.

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