Page MenuHomePhabricator

Remove styles in hacks.less that no longer apply
Closed, ResolvedPublic2 Estimated Story Points

Description

Background

We ship various styles in the mobile skin that should be no longer necessary.
In preparation for applying this to Vector 2022 we should remove styles that no longer serve a purpose.
Based on discussion in T358078 we can remove some of these styles.

User story

As a reader I do not want to load styles that are not serving any purpose for my experience.
As a developer I do not want undocumented code that serves no purpose.

Acceptance criteria

The following styles in can be removed from https://github.com/wikimedia/mediawiki-skins-MinervaNeue/blob/master/resources/skins.minerva.base.styles/content/hacks.less

QA

A file table of contents should appear at the top of this page:
https://commons.m.wikimedia.org/wiki/File:Dzieje_malarstwa_w_Polsce._Cz._1_1925_(155225517).jpg?uselang=de
It should look like so:

image.png (1×702 px, 1 MB)

QA Results - Prod

AC Status Details
1 T360387#9685889

Event Timeline

.multicol

Col-begin still appears to use multicol for columns? (It's another on the list of things to change, but I haven't jumped at this one because there's a lot of articles that could be affected by any div-i-fication.)

.content .reflist { column-gap: 2em; } doesn't apply to elements that are display: block

This is not a true statement. I think this still merits removal for the reason I expressed in the discussion task.

In T360387#9655670, @Izno wrote:

Col-begin still appears to use multicol for columns? (It's another on the list of things to change, but I haven't jumped at this one because there's a lot of articles that could be affected by any div-i-fication.)

Could be easily rewritten into TemplateStyles like https://ru.wikipedia.org/wiki/Шаблон:Столбцы/styles.css

.content .reflist { column-gap: 2em; } doesn't apply to elements that are display: block

This is not a true statement. I think this still merits removal for the reason I expressed in the discussion task.

Source:

Screenshot 2024-03-22 at 5.31.33 PM.png (720×1 px, 164 KB)

Col-begin still appears to use multicol for columns? (It's another on the list of things to change, but I haven't jumped at this one because there's a lot of articles that could be affected by any div-i-fication.)
ould be easily rewritten into TemplateStyles like

multicol exists but it's not using tables so I'm pretty sure these rules are no longer doing anything: https://gerrit.wikimedia.org/g/mediawiki/skins/MinervaNeue/+/18d194b9572f19c079629bba3dff5d77344a73a3/resources/skins.minerva.base.styles/content/hacks.less#164
But yes, my expectation is these hacks have lived here longer than expected and should have been upstreamed to templates by now. They are poorly documented and added almost a decade ago so we shouldn't be maintaining them in Minerva. @Izno if you are worried, would it make sense to copy it to a TemplateStyle scoped to body.skin-minerva in preparation for this change now? Happy to do that in my volunteer time if you think it's useful.

I had hoped to be having this discussion in the community discussion task just so we could be very clear before making to-be-actioned tasks......

.content .reflist { column-gap: 2em; } doesn't apply to elements that are display: block

This is not a true statement. I think this still merits removal for the reason I expressed in the discussion task.

Source:

Screenshot 2024-03-22 at 5.31.33 PM.png (720×1 px, 164 KB)

Chrome is giving you a poor-quality error message, here is Firefox's version of the same:

image.png (116×272 px, 5 KB)

column-gap acts as documented on MDN, which may be used not just with CSS flex or grid but also CSS columns (see particularly the example).

As I said, I still think this should be removed, but for the reason that it has no real reason to be here. It's not working around any loss of functionality, it's not adding functionality, it's not working around big tables, it appears solely to be used (badly) to style specifically reflist templates to be slightly better looking. It appears to have been for tablet resolution (at least when I looked at things last) getting... maybe more than two columns? I don't know. Either way, it really is a hack, with little to no reason for existence.

Col-begin still appears to use multicol for columns? (It's another on the list of things to change, but I haven't jumped at this one because there's a lot of articles that could be affected by any div-i-fication.)
Could be easily rewritten into TemplateStyles like

multicol exists but it's not using tables so I'm pretty sure these rules are no longer doing anything: https://gerrit.wikimedia.org/g/mediawiki/skins/MinervaNeue/+/18d194b9572f19c079629bba3dff5d77344a73a3/resources/skins.minerva.base.styles/content/hacks.less#164

Yes, it is using a table. The content of the template right now is (eliding the noincluded stuff):

<div>
{| class="{{{class|}}} multicol" role="presentation" style="{{#if:{{{small|}}}|font-size:90%; }}border-collapse: collapse; padding: 0; border: {{{border|0}}}; background:{{{bgColor|{{{bgcolor|transparent}}}}}}; width:{{{width|100%}}}; {{{style|}}}"

The next template you would use with this is col-break, the content of which is:

| style="{{safesubst<noinclude />:#if:{{{width|}}}|width: {{{width}}}; }}text-align: {{{align|left}}}; vertical-align: {{{valign|top}}}; {{safesubst<noinclude />:#if: {{{gap|}}}|padding-left: {{{gap}}};}} {{{style|}}}" |

The div isn't meaningfully doing anything there and I don't know why it was added in that diff, but that's irrelevant to the question ultimately. The structure of this would be:

<div>
  <table class="multicol"><!-- end col-begin -->
    <td><!-- col-break -->
      Some content between uses of col-break.
    <td><!-- a second use of col-break -->
      Some more content, in a second "column".
  </table> <!-- start col-end -->
</div> <!-- end col-end -->

Which is clearly a table column template. womp womp :( (here's our nav template for how to make columns based on their technology, this is the last one using tables on English WP but it has a ton of uses and it really hasn't been the priority - another I previously successfully had deleted after doing the work to pare down obscene uses).

I did say this should probably get fixed on wiki, so someone could probably go forth on TemplateStyling the main template and then seeing if the others still need to have the class. Looks like pretty few on en.wiki, from this search I'd say only col-begin needs these styles and I can add this class to the to do page for any further cleaning.

But yes, my expectation is these hacks have lived here longer than expected and should have been upstreamed to templates by now. They are poorly documented and added almost a decade ago so we shouldn't be maintaining them in Minerva. @Izno if you are worried, would it make sense to copy it to a TemplateStyle scoped to body.skin-minerva in preparation for this change now? Happy to do that in my volunteer time if you think it's useful.

Using TemplateStyles here is a fair suggestion, but doesn't solve your 800 wiki problem having this removed before wikis can correct display (well, Wikidata looks like it's tracking about 300 wikis). That is one reason why I think the config idea has merit, you can either clean this up everywhere yourself or you can at least get wikis to be thinking about opting in/out... I guess you could plop some version of the CSS currently in hacks.less into every Minerva.css page (perhaps with some media query for where exactly these are being targeted - though I'd hold off on that until have container queries at least). IDK what the right way is to motivate cleanup in the fleet. (Age old problem, you'd think we'd have figured out the right answer by now. Maybe it is just to make changes on as many as you can control and letting users re-figure out their stuff looks bad at low resolutions, perhaps with an announcement of "hey, this will be breaking things, you can copy this precise paste to this precise location or tell us you don't feel comfy doing that".)

(I did notice this class has spread a bit beyond this template also in my search, but I think that can be cleaned without Styling things?)

Drop .collapsible td. This is antiquated code that has been replaced with mw-collapsible.

This isn't what I said, so I'm not sure how you came to this conclusion. I said that there is a replacement, but that we still have many uses of the old .collapsible. And that I didn't know precisely what the line of interest was intended to do. It's probably still fair to remove it ultimately, but it's a risk that you take that no one will be super-concerned with it.

Ok for multicol I've now added it to the to do page, I'll go add the styles now.

Change #1014547 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/MinervaNeue@master] Remove styles in hacks.less that no longer apply

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

bwang removed bwang as the assignee of this task.Mar 26 2024, 4:17 PM
bwang moved this task from Doing to Code Review on the Web-Team-Backlog (FY2023-24 Q3 Sprint 6) board.
bwang subscribed.

Change #1014547 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Remove styles in hacks.less that no longer apply

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

And that I didn't know precisely what the line of interest was intended to do. It's probably still fair to remove it ultimately, but it's a risk that you take that no one will be super-concerned with it.

Hey @Izno I agree it's a risk, but I'd rather break this, understand the issue it fixes and restore it if needed with a more detailed comment. Generally the intention of these hacks was to apply rules to highly utilised templates which globally impact multiple projects rather than individual templates with less reach. By this criteria I think it makes sense to remove this (but happy to learn that I'm wrong).

Ok for multicol I've now added it to the to do page, I'll go add the styles now.

Thank you for cleaning this up 🙏. Given the low usage for the same reason above, I don't think this belongs here.

If I'm not misunderstanding this, a short term fix should be as simple as adding the follow rule to a stylesheet associated with https://en.m.wikipedia.org/wiki/Template:Col-begin to make sure these stack to a single row on low resolutions? If so should I write a User-notice for this week or is there a bug or issue that stops us from implementing that?

@media all and ( max-width: @width-breakpoint-tablet ) {
table.multicol > tbody > tr > td,
table.multicol > tr > td {
	display: block !important;
	width: auto !important;
}
}

IDK what the right way is to motivate cleanup in the fleet. (Age old problem, you'd think we'd have figured out the right answer by now.

Agreed :-(.

Probably the task in general is worth a user notice. "Minerva, the skin used on mobile, has had some CSS in place to make some templates display better at mobile resolutions. Skins shouldn't decide how templates display, so some of this CSS will be removed this week. If you use one of the set of affected classes somewhere on your wiki (put list in description, with a link to Special:Search maybe for each class), you may need to add TemplateStyles to support the same display." or something like that... I don't really like the description of how to fix it inline since the changes may be pretty different (or not needed as with e.g. .collapsible td) so maybe leave that to a description in this task or another task to point users to.

@media all and ( max-width: @width-breakpoint-tablet )

Yup, 720px is fine. I'll take care of enwiki myself. These tables are marked as presentational anyway so not like we're losing anything for screen readers.

The notice should serve for the rest of the wikis.

Edtadros subscribed.

Test Result - Prod

Status: ✅ PASS
Environment: commonswiki
OS: macOS Sonoma
Browser: Chrome
Device: MBA
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: A file table of contents should appear at the top of this page:
https://commons.m.wikimedia.org/wiki/File:Dzieje_malarstwa_w_Polsce._Cz._1_1925_(155225517).jpg?uselang=de

screenshot 164.png (826×1 px, 520 KB)

Jdlrobson claimed this task.