Page MenuHomePhabricator

[M] [Dev cleanup] Rename all mw- prefixed classes in Vector
Closed, ResolvedPublic1 Estimated Story Points

Description

Classes prefixed with "mw-" should be generated by core yet we've invented quite a few. Please prefix with "vector-" if these classes do not have styles or HTML generated in MediaWiki core. We need to clean this up before deploying everywhere, as gadgets may interpret these as APIs.

  • mw-sidebar-action
  • mw-article-toolbar-container
  • mw-sidebar-action-heading
  • mw-sidebar-action-content
  • mw-body-subheade
  • mw-header-content

Do not touch mw-ui-button and mw-ui-icon

Where possible IDs should also be updated, with exceptions for things that are historically using that ID:
e.g.

  • mw-panel

Developer notes

This will require 2 steps so we don't break cached HTML:

  • Add the new selector to the HTML and CSS, retaining the old selector in HTML and CSS. Update CSS to style both. Add some FIXMES to the CSS to make it clearer what needs to be cleaned up.
  • Wait for the next release cut.
  • Remove the old selectors from HTML and CSS.

Event Timeline

Jdlrobson lowered the priority of this task from Medium to Low.Sep 6 2022, 5:27 PM
LGoto set the point value for this task to 1.Sep 29 2022, 5:17 PM

Would it also make sense to take this opportunity to update the classes that overlap with https://phabricator.wikimedia.org/T317437?

In particular 'sidebar' would be replaced with 'main-menu' and 'article-toolbar' replaced with 'page-toolbar'?

LGoto renamed this task from [Dev cleanup] Rename all mw- prefixed classes in Vector to [M] [Dev cleanup] Rename all mw- prefixed classes in Vector.Oct 4 2022, 5:39 PM

It seems that according to our current documentation https://www.mediawiki.org/wiki/Manual:Coding_conventions/CSS#Naming the prefix .skin-vector-seems more expected than just the .vector-.

The .vector- prefix might also be confused with legacy vector. How about .skin-v22- for brevity? (.skin-vector-2022- might add a lot of bytes to the CSS/HTML payload).

On .vector- vs .skin-vector-2022-: After gzipping this verbosity pretty much makes no impactful difference. Have tested once on the huge number of OOUI selectors in comparison once and it's low bytes area.

It's more a question of developer experience (onboarding, skimming, writing, debugging). Every abbreviation is another point for confusion. Debugging might be more problematic with longer terms in DOM inspecting.

Regarding the CSS naming wiki page - not set in stone and we should feel free to fix it. I personally would steer away from the "skin-" prefix as its redundant. There's no Vector extension so I would prefer we retain "vector-" prefix for our own sanity (note we use cdx- for Codex).

@bwang yes this would be a great time to make progress on T317437 - if we are changing the classes, feel free to also change the names.

@Jdlrobson Can we safely update mw-sidebar? I don't see any mentions of it in core, though i see some extentions in code search reference it

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

[mediawiki/skins/Vector@master] Replace incorrectly mw- prefixed classes

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

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

[mediawiki/skins/Vector@master] Remove CSS classes and selectors from cached HTML

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

Regarding the CSS naming wiki page - not set in stone and we should feel free to fix it. I personally would steer away from the "skin-" prefix as its redundant. There's no Vector extension so I would prefer we retain "vector-" prefix for our own sanity (note we use cdx- for Codex).

@bwang yes this would be a great time to make progress on T317437 - if we are changing the classes, feel free to also change the names.

There was a discussion about the relevant section quite recently, see https://www.mediawiki.org/w/index.php?title=Topic:Wr2mgdpq9ztfd1b6 , which you participated in, where you didn't seem to have an issue with adding that...

I much prefer to keep the set of potential selectors minimal to help avoid clashing with names for templatestyles, common.css, and gadgets (see also the .hlist problem). skin-vector helps with that; vector- unprefixed invites timeless- and monobook- and so on.

Why is skin-vector better than vector-? Are templates using the vector- class prefix?

Why is skin-vector better than vector-? Are templates using the vector- class prefix?

I'm less worried about Vector particularly and more thinking about the many different skins that could exist each with their own names and the potential to overlap. Having a narrow expectation on the skinning side doesn't seem like any sort of large cost and significantly reduces the potential overlap (at that point it's just skins needing to ensure they don't have the same name as another skin). It's also easier to teach on the user side: "don't make a name with mw-, ext-, or skin- or you may run into unexpected styling that you have to override" as well as the corollary that shows up which is "if you're using these names, they are the responsibility of the skin, which means you deliberately must override their styling at arbitrary times into the future" +- the related discussion about name stability.

Skins and extensions using their name as the highest namespace makes that a more difficult expectation to let others know about.

Which is also why I am a general proponent of mw- for everything and then developers can deal with name collisions entirely within that space with only a single "avoid this" for user space, but that discussion didn't tend that way for whatever reason.

bwang removed bwang as the assignee of this task.Oct 19 2022, 2:54 PM

@Izno where are these guidelines? The set of skins is finite right now. I think the concern here is if we create a new skin with a name that matches a common prefix. I think for now, guiding gadget developer to not prefix classes with names of deployed skins should be sufficient guidance, no? For Vector I think it would be too risky at this point to rename all vector- prefixed classes to skin-vector-

where are these guidelines?

The discussion already pointed to above and corresponding guidelines? I realize these don't have the inverse "if you're an X developer that isn't in one of these sets, don't run into these names please", but one can reasonably say that's inferred.

The set of skins is finite right now.

The set of skins on Wikimedia is finite (and small). The set of skins outside Wikimedia is finite (but not as small). The set of template and script names... is finite and much larger. Vector particularly has this issue since it has other meanings that could plausibly end up in a template or script name (whereas Monobook and Minerva, not so much).

I think the concern here is if we create a new skin with a name that matches a common prefix.

This is also an issue. :)

I think for now, guiding gadget developer to not prefix classes with names of deployed skins should be sufficient guidance, no

This rule is also simple, but when you start unrolling there's 5 skins just on Wikimedia and others in other places (so it goes from "don't prefix with skin-" to "don't prefix with monobook- and vector- and...). And let's not forget templates of course.

For Vector I think it would be too risky at this point to rename all vector- prefixed classes to skin-vector-

Yes, I do think the Vector cat is out of the bag for old names.

The followon question is whether it's good for the skin to have inconsistent names where some names are (slightly) 'better' or consistent names and all of them are the slightly inferior version. That's a different question than the more general point which already seems to have been settled in that discussion........ Add more correct names everywhere resolves it and then maybe at some point someone can delete the old names. (I think it would be better for Vector to follow the relevant guidelines directly so as to be an exemplary citizen of the skinning community.) This is basically what the task proposes to do, you just might want to wait longer.

Names are cheap.

Change 843573 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Replace incorrectly mw- prefixed classes Rename template and CSS files to match updated naming conventions, replaces instances of "sidebar" with "main menu"

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

Change 843584 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Remove CSS classes and selectors from cached HTML

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

So finally got round to reviewing this. I'm noting the following classes that seem to be generated by Vector not core and using the mw- prefix (have ignored legacy Vector classes):

  • .mw-table-of-contents-container
  • .mw-content-container
  • .mw-page-container-inner
  • .mw-header
  • .mw-body
  • .mw-body-content
  • .mw-footer
  • .mw-page-container
  • #mw-sidebar-checkbox
  • #mw-panel-toc
  • #mw-panel-toc-list
  • #mw-sidebar-button
  • .mw-footer-container
  • .mw-logo-wordmark
  • .mw-jump-link

Were these intentionally descoped, or simply overlooked?
If some of these are intentionally kept I would suggest adding some inline comments to the template explaining the reasoning.

Just noting that the following classes do appear in our Jest snapshots so we may want to update them at some future date.

@Jdlrobson Ah, some of these were overlooked, some of these I wasnt sure if they were safe to change (i.e. mw-body, mw-header), and some of these were ids used by JS so I wasnt sure if we should bother. To clarify, is there a list of mw- prefixed classes/ids we DO want to keep? That would be easier to work from, and I could put up a followup patch removing all those.

Regarding the stuff used by JS, I think it might make sense to add a temporary feature flag to make handling cached HTML in JS easier. I'm also thinking we could use this flag with other id/classes we want to rename, i.e. the dropdown classes. What do you think about that?

Okay I've opened T322573 to handle the remaining classes.

Regarding the stuff used by JS, I think it might make sense to add a temporary feature flag to make handling cached HTML in JS easier. I'm also thinking we could use this flag with other id/classes we want to rename, i.e. the dropdown classes. What do you think about that?

I think it would be useful to add inline documentation to where they are used that points to T322573. I think it would be fine for you to self merge these patches if that speeds this up.