Page MenuHomePhabricator

Minerva skin styles conflict with content using CSS class "header" and "footer"
Closed, ResolvedPublic0 Estimated Story Points

Description

We had a problem with the tables on the Wikimania programme page displaying incorrectly on mobile: header cells were stacking vertically instead of horizontally in their row, meaning they didn't match up with the data cells.

I added a hack in MediaWiki:Mobile.css to override a style conflict where some Minerva skin styles were leaking into content; the offending CSS appears to be in `skins.minerva.base.reset%2Cstyles`:

.header {
  display: table;
  width: 100%;
  border-spacing: 0;
  border-collapse: collapse;
  height: 3.35em;
  white-space: nowrap;
  border-top: 1px solid #c8ccd1;
  margin-top: -1px;
}

Event Timeline

Jdlrobson added a subscriber: Jdlrobson.

the .header is pretty engrained into the Minerva skin and not going to change.

I'd argue the use of a generic class in a header is dangerous and should be revisited at the template level e.g. schedule-header. I'd also recommend reverting the Mobile.css change - Mobile.css is loaded via JS so you are introducing a reflow.

I agree use of such a generic class in skin code is dangerous; it should be changed to something using the 'mw-' prefix or else the styles need to be specified in a way that won't interact with content.

Re-adding MinervaNeue project; this is an obviously incorrect problem with the skin which should be fixed.

I was actually saying the oppsoite :) IMO a skin should be allowed to use generic classes. The responsibility lies to content developers.

We can bikeshed if you want, but I feel very strongly about this and that header is not going to change - it's not worth the effort or risk to rename that class that's subject to caching rules and is an important part of the skin.

Change 370482 had a related patch set uploaded (by Brion VIBBER; owner: Brion VIBBER):
[mediawiki/skins/MinervaNeue@master] Avoid setting skin-specific styles on content w/ 'header' class

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

I think it's extremely important to say that the responsibility is *not* on content developers to guess what will and won't work on our system. It's literally our job to make it work well for them.

Please see the patch. :) Thanks!

Fully agree with brion. Also, we have had a longstanding soft rule that Mediawiki generated class names and id's should be prefixed with mw- (unless they are terribly old and changing them would break every user script out there).

Change 370482 abandoned by Brion VIBBER:
Avoid setting skin-specific styles on content w/ 'header' class

Reason:
*nods* withdrawing this patch in favor of further looking at a cleaner prefix or other fix.

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

Sorry, didn't mean to start another CSS war. ;) Per Timo's notes on the withdrawn patch, the extra selector is a little janky, but it's probably worth looking more at prefixing the class. (Or if it's super tricky now for some reason, I'd love a general style naming cleanup to be something that's possible in future.)

We recently started using a footer element. If we're keen to rectify this in Minerva maybe use of a header element would be nicer and a motivation.

lso, we have had a longstanding soft rule that Mediawiki generated class names and id's should be prefixed with mw-

I've never liked the mw- prefixes and never really understood the logic. Is there any record of how this decision was come to?

I see this as a social thing and when these clashes occur it's an opportunity to think about whether they are appropriate usages
Why is an extremely generic class '.header' on a table column? Would a th be better? Does that semantically reflect what it does? Is the .header class in Minerva doing too much? Does that semantically reflect what that does?

FWIW long term one thing we are considering is removing cascading rules altogether (see T160128) to enforce us to be super semantic about all our class names. I feel any changes to class names now would be counter-productive and premature until that conversation has been had.

The reasoning was mostly: We cannot control how community editors name their classes (retroactively), but we can control how WE name our classes and IDs.

Jdlrobson edited projects, added MinervaNeue (Desktop); removed MinervaNeue.
Jdlrobson moved this task from Desktop to Blocked on the MinervaNeue board.
Jdlrobson edited projects, added MinervaNeue; removed MinervaNeue (Desktop).

Also, we have had a longstanding soft rule that Mediawiki class names and id's should be prefixed with mw-

I've never liked the mw- prefixes and never really understood the logic.

The mw- prefix is the namespace for MediaWiki core, and extensions should generally pick something more specific as well (e.g. mw-thing-, or other-thing- if not specific to MW).

It is a standard practice in software to avoid clashes at run-time by using namespaces or prefixes, because the environment is shared with many other sources of code, including (on the server-side) upstream PHP libraries and other MW extensions, and (on the client-side) with browsers' own native code, browser extensions, more upstream libraries (UI frameworks and JS plugins with styles), and also (in our case) user-scripts and gadgets.

A generic class name like .header is bound to cause issues at some point because there are many different things that could have a "header" without being related or intended as an override.

If a browser extension, or upstream library, or gadget, somewhere uses such generic name, they risk having their own stuff broken if another thing also does that. But as long as we avoid it in our own code, our code won't break.

I would ask to reconsider classifying this as a Local-Wiki-Issue and instead address this from Minerva' perspective.

Jdlrobson raised the priority of this task from Low to Medium.Oct 29 2019, 4:43 PM
Jdlrobson renamed this task from Minerva skin styles conflict with content using CSS class "header" to Minerva skin styles conflict with content using CSS class "header" and "footer".Apr 27 2020, 7:00 PM
Jdlrobson added a subscriber: Demian.

Just to add a small data point to this: I recently came across this issue because I used the CSS class "header" in TemplateStyles for the Arctic Knot Conference pages on Meta, but it would not do what I needed it to on mobile, even though I thought I did nothing wrong. It turned out that since the .header class from had a height attribute and my class didn't, Minerva's .header heightof course took precedence.

The page looked like F34184125, while it should have looked like what it does currently. The relevant page source code to look at is this and this.

So in summary, I really think .header & co should be changed to .mw-header to stay consistent and avoid accidents like this.

Patch is welcome mapping header to mw-header. The patch must consider the fact that we cache HTML so retain references to .header in CSS for at least a week. This is unlikely to be addressed by my team any time in the next few months given the focus on Vector (but I can provide code review if necessary).

Change 704569 had a related patch set uploaded (by ExE Boss; author: ExE Boss):

[mediawiki/skins/MinervaNeue@master] Fix style conflict with content using `class="header"`

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

Jdlrobson set the point value for this task to 0.
Jdlrobson updated Other Assignee, added: ExE-Boss.

I made a small suggestion on the ticket to use "minerva-header" as the class rather than "mw-header"

Change 704569 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Fix style conflict with content using `class="header"`

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

Jdlrobson removed Edtadros as the assignee of this task.
Jdlrobson added a subscriber: Edtadros.

Change 705502 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/MinervaNeue@master] Restore header class for cached HTML

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

Change 705504 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/MinervaNeue@master] minerva-header class is now in cached HTML, remove old header class

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

nray reassigned this task from nray to ExE-Boss.
nray added a subscriber: nray.

Change 705502 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Restore header class for cached HTML

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

Change 705504 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] minerva-header class is now in cached HTML, remove old header class

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