In T280285#7089490, @Jdlrobson wrote:

In T280723#7022456, @Jdlrobson wrote:

Several styles previously found in 'interface' or 'elements' or 'content' or something (I'm not really sure which, as I've never really used any of these to begin with) are now in legacy. This includes the toc-style catlist box and thumbnail icons that now override any skin-specific icons (T280292).

The thumbnail issue looks backwards compatible to me in that the content is perfectly readable. The fact that the default styling has changed is expected. The legacy feature contained a few styles for thumbnails and we decided it was important that we applied those styles consistently. If those are not wanted in Timeless, the usage of the legacy feature should be removed, or more explicit overrides should be provided. I don't see this as a problem for 1.36.

Perfectly readable, and very broken:

@Isarra which skin is this? I'd like to test the patches on this one to see if it goes far enough.

And at this point, I would think no skins should be using these modules, as at very least mediawiki.skinning.interface has been deprecated as of... what, 1.35 at least? And the parsoid one probably shouldn't exist either at this point.

Oh, if you do want to fix it ahead of the time in the gadget, set the 'width' to an actual value. Make this one do width=20 instead of height=20. Mind you, heights actually totally make sense for indicators anyway, so it might make more sense to just leave it and see if all these patches fix it...

This really kind of depends on four different patches. Patch 1 and 2, moving the mw-body-content class to what the skin receives, and removing mw-body-content from anything the skin does itself. Patch 3: removing mw-body-content from the indicators. Patch 4: apparently the only one I actually linked here.

That screenshot of vector also has a missing word. Line should be 'must not be violated'. Vector is missing 'be'; timeless is missing 'not'.

Wait, do we just need to apply some of the weird table js to print, and swap out the css, for this?

Looks resolved, patch went through, apparently forgot to close this. Please reopen if I'm wrong.

Tagged skins may or may not be affected, but I definitely didn't account for this when styling them, so they should be checked. Unsure about any others.

But I figured it out. I can now die satisfied, having wasted all evening on this, and who knows how many other hours of confusion and frustration, all because I was too lazy to set a copyright on a migration test wiki...

Nope, nevermind, it's not actually catching the empty copyright array in any current version.

In T278266#7064225, @Jdlrobson wrote:

The logic check should still happening, it's just been abstracted away so skins don't have to worry about it. Skins should not have any need to validate.
Let me know if you have found a bug, by providing a LocalSettings configuration that doesn't work and I'll take a look.

So that actually would be a good step toward T280723, at least?

In T280292#7064201, @Jdlrobson wrote:

^ Based on your feedback here. @Izno @Isarra does this have the desired effect? If so I can backport to 1.36.

The issue is that previously skins using BaseTemplate::getFooterIcons could assume that what they got back was validated in that it was only icons, and that no empty or non-icon items would be returned. Given the mw installer/standard LocalSettings config also expect this and indirectly set unused default icons to empty arrays (so it's not exactly like sysadmins are likely to figure out some way to work around it on their end, either), this results in problems for any skins styling the footer icons in any way that assumes there actually be an icon there (for example padding, borders, spacers).

...yeah there's no way to validate the icons in skin.php without actually moving the whole getFooterIcons function into that instead. 'get' definitely can't do this.

So either we need to merge the above (and a followup patch) into 1.35, or we gotta go back and redo all the skins that assumed the footerIcons would actually be validated after getting them (which BaseTemplate did, but 'get' does not)...

Welp, looks like this was resolved in core by toccolours being set in the toc feature, which means all my skins are going to have to override that back to something that matches too... :P

In T280723#7022456, @Jdlrobson wrote:

Several styles previously found in 'interface' or 'elements' or 'content' or something (I'm not really sure which, as I've never really used any of these to begin with) are now in legacy. This includes the toc-style catlist box and thumbnail icons that now override any skin-specific icons (T280292).

The thumbnail issue looks backwards compatible to me in that the content is perfectly readable. The fact that the default styling has changed is expected. The legacy feature contained a few styles for thumbnails and we decided it was important that we applied those styles consistently. If those are not wanted in Timeless, the usage of the legacy feature should be removed, or more explicit overrides should be provided. I don't see this as a problem for 1.36.

In T279388#7059219, @Jdlrobson wrote:

Which if anything has been a bit of a regression where it's been implemented, as generally it's better to keep content styles consistent between all pages, whether they're parser output or special pages or whatever.

Agreed, but that's a regression that predates this change, and this change allows us to keep content styles consistent across skins by making mw-body-content a first class citizen provided by core rather than a class that skins must remember to add.

In T279388#7059217, @Jdlrobson wrote:

Well, based on a very brief look at https://codesearch.wmcloud.org/search/?q=%5C.mw-body-content&i=nope&files=&excludeFiles=&repos= it looks like all of the following will likely be impacted...

These are not impacted. While they use the class, they will not be impacted by the change here as they have no styling rules that will be broken. Skins that are using the class will continue to use that class and the styling rules in each of them will also apply.

In T279388#7054191, @Jdlrobson wrote:

...

You said: skins (also) use .mw-body-content as the generic parent for selectors that implement general styles for chunks of HTML content that should be styled as "wiki" .
I don't think this is true, and haven't seen evidence for this, but it's possible you have information I don't.

I think this is based on how skins used to do this, but I don't have any evidence suggesting this statement this true. It might have been true in 2015, but over the last few years skins have being using the mw-parser-output class for such things.

Based on how Vector seems to have previously been/normally just has all the text be that tiny at such resolutions, I suspect chrome might be trying to work around whyever that is (likely legacy Vector not being marked as responsive and not having the viewport etc stuff set?) and make it actually legible. And it's just not applying to the toc itself for some reason. So the issue here is probably just a case of 'skin is old and not responsive to begin with'.

Specifically, how we can avoid triggering this:

Dumb question, but is there any way to make this replacement work in 1.35 without doing a version compare first?

I think I'm just going to start including a fork of the 1.34 shared.css with my skins instead.

...anything run through some sort of getMessage( 'msg', [params] )->parse()? Lots of special pages begin with some sort of customisable blurb that does that. Other extensions do that various places as well, inside or outside of the content block. And parse() sends it through the parser, so it's basically just parser output without the css class.

The problem is that there is nowhere obvious for the .visualClear CSS rule to live.

But seriously I think maybe we should just start wrapping all parser output in mw-parser-output? This would also allow for other non-content blocks of normally parsed wikitext that extensions may add, such as to the sidebar or footer, to maintain consistent UX with regular content, and consistent UX is, uh, important. Good. Optimal? Maybe.

This whole thing feels like a regression. I wouldn't even know where to start filing tasks, as the whole approach just seems wrong. I can try to spell it out, but please, please, get an actual designer on this. Learn how to cascade your features, like how css cascades styles. You're separating out particular content components into their own features when you should be separating out particular levels of styles. Every skin should float trights right, and tlefts left, because that's a basic assumption of them being what they are. Every skin has editsection links, and probably wants the sizes standardised regardless of header size. Every skin with a category block in the content block probably wants the category lists inline. Users expect things like .center and .visualClear and .noprint and .nomobile to work regardless of skin, because why in the world would basic content styles and affordances be specific to a particular skin? These are all first-level features, and shouldn't even be separate features at all, aside from the category one, because not only are they common to all skins, aside from the categories, none of them are even placed by the skin.

In T280609#7039423, @ashley wrote:

I don't think it matters how you render <div class="visualClear"></div>, whether manually, via BaseTemplate#getClear or some other way, but the class (definition) definitely needs to stay.

Er, should these even be used inside the parser? Like what are they even for that a successbox wouldn't cover? Why have this variation?

Some pages do not contain an mw-parser-output element, notably special pages, but work on the invalid assumption that the skin will always clear any floats inside them. This was the case with the RCFilters page. In these cases, I think it certainly justifies adding the responsibility on the special page itself, so @Krinkle I respectfully disagree with you on that. I wouldn't use a component from a component library that floated itself and relied on other components for its layout. Components should work in isolation. I believe we should fix each of these so that they are future-proofed.

To add on to what I just said, the task description is just wrong.

In T279388#7026285, @Krinkle wrote:

In T279388#7026179, @Jdlrobson wrote:

This would potentially break floating rules in skins not using mw-body-content but I think that's okay, with the expectation they're likely using something else (likely .visualClear).

Afaik there has never been a float clearing rule that shipped with core for this, so even skins not using mw-body-content would presumably be unaffected by this feature shipping.

(e.g. external link styles and other "wiki" styles that we sometimes apply to small chunks of user content rendered outside the proper content block, in other words content in all the various places you'll be removing this class from; places that should not themselves be the target of a selector and not have layout, but used for styling their children; Also thinking about rendering a small previews of something within an OOUI/WVUI dialog).

In T24660#7031826, @Kozuch wrote:

WTF is going on here? WMF not able to add one line of code to show the categories for last 10 years since this bug was filled? This really pisses me off...

Okay, this might be the only one of these skin features issues that can actually be fixed in Timeless without just totally reimplementing everything... and it's like the least impactful anyway. And some of the others even made it into 1.36, whereas this doesn't appear to have.

But you should really just be starting with only the stuff that skins probably won't have any visual stylistic reason to override, and should always apply. For the page content, this is stuff like:

• General positioning - trights float right, tlefts float left, centered stuff is centered
• Unsetting the default header styles from editlinks so they're not all gigantified (specific skins may need to unset more, but at least the hard work should be done at this point)
• Hiding stuff like the 'retrieved from' line from screen view
• Things like .plainlinks, .noprint, .nomobile, .visualClear, and other classes users may expect to be able to apply to control output
• No bullets on the toc items, why would anyone want that
• Generic, consistent handling for mobile, if there's some way to actually tell it what the hell the skin's mobile cutoff is - stuff like thumbs going full-width, table overflow handling, collapsible sections, etc. A lot of what MF does, I suppose?

Help changing the defaults to more sensible ones is appreciated. The use of % font-sizes for example seems a little odd. Perhaps explicit font-sizes would be better. You can disable the feature if the CSS rules are not helping (that's what Minerva does)

The thumbnail issue looks backwards compatible to me in that the content is perfectly readable.

In T280609#7021994, @Isarra wrote:

This isn't for user content. This is for clearing floats within the skin itself?

This was wrong. It's for everything. I just... slightly broke my brain when I first saw this (and a related task that mentioned user use specifically).

Extensions also use the visualClear class. Yes, they could also define their own versions, but... why?

But why remove it from core? If skins aren't using it, then this doesn't affect them. If skins are using it, they're often using a combination of getClears and applying the class to specific elements where a full separate clear isn't needed.

In T89981#7014852, @Jdlrobson wrote:

Given a few of the issues we've been having I suggest we move all deployed Wikimedia skins away from legacy during 1.37 and deprecate it in that release rather than rush it now.

Skins should instead copy across the method to their own skin or use the LESS mixin instead

Or... what? I don't understand. You still need to make an element as a spacer for a lot of this... yes, flexbox is a thing, but we're not gonna all of a sudden convert every skin to it now, why would we do that?

This isn't for user content. This is for clearing floats within the skin itself?

Possibly slightly better tag? Not actually an issue with RL in general.

The problem here appears to be that various things have been added to the 'legacy' feature that were previously part of 'interface' or 'elements', or some other feature that was not previously used. This is a breaking change for all skins that were previously only using legacy and not these other features.

Timeless was not using core styles at all for a lot of this stuff... hopefully it's just specific features we need to (explicitly?) disable here, because if not, overriding this could get very annoying. But if it is, then maybe we can also disable some other things that we were overriding previously? Make things a bit neater overall?

Jon mentioned this on https://gerrit.wikimedia.org/r/c/mediawiki/skins/Timeless/+/678785/:

According to Timo master versions should only be compatible with the /next/ release and only tarballs should be considered stable. See https://gerrit.wikimedia.org/r/c/mediawiki/core/+/673320 and https://phabricator.wikimedia.org/T271441#6764043 which proposes making skins fatal when they use ResourceLoaderSkinModule with non-existent features. I don't think that's great from a skin perspective as I worry that will discourage skin developers from adopting new features that make their lives easier, but feedback from yourself and Jack on your own experiences with skin development there would be helpful.

Yeah, no, timeless is breaking it, you're spot on.

Ooooooh this is the image overflow stuff crap.

Timeless assumes topicon sizes are set appropriately. Maybe this is a stupid assumption, but are you sure you can't just... do that? Set a specific default size in the script?

In T279645#6990123, @Legoktm wrote:

I'll defer to Isarra and co. on the correct way to solve this...it's unclear to me why $wgLogos['icon'] is a single png and not an array with 1x/1.5x/2x/svg options like the main logo does.

Like why would any project still be using rasters in this day and age?!

It's because of T273250: $wgLogos['icon'] notext logo set for newvector is too small to use in Timeless, except we went ahead and used it anyway out of pure spite. Or, more likely, laziness/lack of resources?

Technically Timeless has already switched over, just has the old logos as a fallback. Also needs to deprecate its own custom handling, but that's a separate problem entirely...

T273250 is also going to pose a problem here for any sites using png notext icons. (Mostly wikipedia, I think?)

I've held off on a proof of concept patch for MonoBook for the time being since the WMF config was just nowhere near there yet in terms of having the logos/wordmarks consistently available for all the sites, but once that's a little closer, we should absolutely get on migrating this. (WikiVoyage is a great example why we can't just do it now, though, as some may actually want only a notext logo and thus will only set that, and some do want a wordmark but just don't have one, presently...)

This is funny because I don't even know what this means.

Resolved with some bad css at some point...

Sorry about that, it turns out some of us are just drunk and sleep deprived and that doesn't result in the most meaningful testing...

Duuur I'm an idiot, I just need to copy the 1.35 fix out of timeless, huh.

In T269891#6684977, @Jdlrobson wrote:

FYI In Vector and Minerva we use a checkbox hack for the menus. We've been meaning to consolidate the associated CSS code as a core module.

In T274199#6927223, @Legoktm wrote:

Thanks for putting up that patch but am I correct in understanding if there's no default icon set then there just won't be any logo shown? What is the correct procedure for putting back the already existing logos? That's really what I don't get.

It sounds like you're saying the fix is to not use the new $wgLogos system in Timeless because it's not ready yet - is that correct?

No that's not what I'm saying. I'm saying that every logo needs to be manually created, derived from the old logo and certain projects don't have that yet. I'm saying Timeless can workaround this issue by accumulating technical debt if it wishes.

I think @Isarra's take above is right. Let's do this right for every skin rather than applying more duct tape :).

In T274199#6926997, @Jdlrobson wrote:

...I still don't like the colours. HMM.

Not removing this from Anisa yet after all, as the replacement apparently isn't available in 1.35, and Anisa is still targeting 1.35 and not 1.36... is this still only deprecated as of an unreleased version, or something?