Page MenuHomePhabricator

ResourceLoaderSkinModule: Improve the content-parser-output module
Open, HighPublic3 Estimated Story Points

Description

How content generated by users is cleared and floated is poorly defined, and not shared by all skins. To solve this, the ResourceLoaderSkinModule enables styles by default that adds a pseudo-element to the end of mw-parser-output that clears all floats on the page.

However, this pseudo-element however should apply to the parent element based on the learnings in T279008.

Acceptance criteria

  • The rule inside the content-parser-output feature applies to the parent element (#bodyContent) using a CSS selector to make overriding by skins and site styles easier.
  • The content-parser-output feature has been renamed to reflect its new purpose and existing callers have been updated.
  • SkinTemplate::wrapHTML is renamed to something more meaningful that reflects the new name and all callers in core are updated. wrapHTML method is kept for backwards compability.
  • Drop the Vector overrides. e.g. revert https://gerrit.wikimedia.org/r/677038 and I47d16eb8186efa83e158713d852b443bce9aee1c

Background

Historically all Wikimedia-deployed skin have an element at the bottom of the page that clears any content before it. In CologneBlue and Modern this is the footer (#footer, .mw-footer). In Minerva and Monobook it was an element above the footer (#page-secondary-actions in Minerva and .visualClear in Monobook). In Vector, there was a clear: both on the #bodyContent. In all cases, all of these clears carried no documentation to why they existed, except a vague notion that "they were for user-generated content".

In T277218 we learned that editors expect their floats cleared.

When auditing the skins on skins.wmflabs.org/ I (@Jdlrobson) found that many of these skins, did not clear floats, and those that did, did so with visualClear like elements which led to a desire to move this definition into core and simplify the lives of skin developers.

Much of MediaWiki is more complicated than it needs to be, and constraining in this way makes the UI much more predictable to skin developers.

The misunderstanding

The method OutputPage::addHTML allows the addition of any HTML and means that .mw-parser-output can be followed by any arbitrary element. In practice, however, the only time when this ever happens in practice is in category pages. File pages, are a special case -but the parser output is always embedded inside an element #shared-image-desc and none of its sibling elements can be floated by edits to the wikitext https://en.wikipedia.org/wiki/File:Amtrak_Cardinal.svg

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.

Some pages do contain an mw-parser-output element but as a child element of something else (e.g. edit pages) so this is not a problem.

The long term solution

It's clear by the regression on Commons Wikimedia that the definition is wrong. The clear should return to the parent element of .mw-parser-output, however, this has implications on architecture that I don't want to rush into.

The parent element with id="mw-content-text" does not have a consistent class name. It's either mw-content-ltr or mw-content-rtl. We should introduce a class that is always present to identify this element. e.g. mw-content. The method that wraps this is called SkinTemplate::wrapHTML - in the process of doing this it would be good to name that something more meaningful and reflects this name e.g. SkinTemplate::createContentHTML. There are lots of things labelled content so I don't want to rush into naming here to make that matter worse.

In ResourceLoaderSkinModule the feature that provides these styles is called content-parser-output and requires that all CSS styling inside it is scoped to mw-parser-output (see note [1]). We should rename this feature to something like content-<new name> and expand its responsibilities to content created outside the parser. e.g. anything that can be a child of that element created by the wrapHTML method. Again, I don't want to rush that.

[1] https://gerrit.wikimedia.org/g/mediawiki/core/+/a87fa828745a7cde05751a91048623e72ea147b9/resources/src/mediawiki.skinning/content.parser-output.less#6

Related Objects

Mentioned In
T282063: ResourceLoaderSkinModule: `center` and `small` rules in the `elements` behaviour is not consistent across skins due to inclusion in an optional `elements` feature
T281540: External links icons are missing on special pages and various secondary content when using ResourceLoaderSkinModule
T280766: Phase out legacy error, warning and success classes for usage outside the parser
T280260: mw-empty-elt element is not hidden in old Vector
T279008: mw-parser-output now clears after block, which is a noticeable change for Category pages where infoboxes used to render floated alongside the automatic content
Mentioned Here
T280401: Centred `thumb` images should not have centred captions
T119430: External link node doesn't render in reference context
T160269: "edit" link for page lead shows in different font in Vector skin
T281540: External links icons are missing on special pages and various secondary content when using ResourceLoaderSkinModule
T280609: Determine future of BaseTemplate::getClear
T280260: mw-empty-elt element is not hidden in old Vector
T280259: Special:Book interface does not clear itself before footer
T155863: MediaWiki has "content" styles applying only inside .mw-body, making them not reusable
T188443: Wrap indicators in mw-parser-output class
T230471: TMH audio player missing for clips inside <indicator>
T277218: ResourceLoaderSkinModule style should be added to ensure user generated content is cleared
T279008: mw-parser-output now clears after block, which is a noticeable change for Category pages where infoboxes used to render floated alongside the automatic content

Event Timeline

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

[mediawiki/core@master] WIP: Encapsulate content body rules

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

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/676602

Previously it was assumed that all skins cleared any content above the footer. While this was true for all Wikimedia deployed skins it
was not true for non-wikimedia deployed skins.

Which skins are that? Would those not be broken in various ways due to parser features, extensions, and gadgets that expect mw-context-text or one of its parents (like bodyContent) to clear a float? I'm not aware of any such third-party skin, but I guess a third-party skin could choose not to support some of those features.

(For those reading along, this new tasks' description answers this:)

Previously the clearing in Vector was completely undocumented, […]

Task description:

When auditing the skins on skins.wmflabs.org/ I (@Jdlrobson) found that many of these skins, did not clear floats, and those that did, did so with visualClear like elements which led to a desire to move this definition into core and simplify the lives of skin developers.

I think this last part means you understand this is not about an undocumented Vector or Wikimedia-specific aspect, but rather that this is a long-standing general expectation of the MediaWiki ecosystem. One that most skins, both WMF and third-party, have always followed. There are a dozen third-party skins on Codesearch alone that not only clear the content float, but also specifically use visualClear class name. (This is not unlike emptyPortlet in terms of well-known behaviours implicitly responsible to a skin.)

But, to help a few third-party skins that seemingly missed this, and to help future skin developers, you want to move this one line of CSS to core in a discoverable place (perhaps even enabled by default for SkinModule?), and in such a way that the rule applies to a class name related to MW skins that skin developers are likely to already have in their layout – if they follow our skin best practices guide. That makes perfect sense to me, and I support that fully.

Task description:

The parent element with id="mw-content-text" does not have a consistent class name.

Try one parent further (in Vector). That leads to #bodyContent and .mw-body-content. Going one futher still, leads to #content and .mw-body. There are no elements following the content within either of these parents, so those can be considered as well.

mw-body is one of the standardised class names we came up with a number of years ago, and its closing tag in most skins closely resembles where <div class="visualClear"></div> is traditionally placed.

I think originally the way it was intended when we discussed this with Gabriel (for Parsoid) and Trevor/Volker (for Vector and skin standardisation) was that mw-body would be the skin's preferred wrapper for the page flow / main column, and mw-body-content anologous to the main content (e.g. as produced by special pages, page actions, or parser output) thus not including sitenotice/heading/indicators etc.

However, we never found a purpose for a standardised wrapper afaik (wrapping seems very much a skin-specific concept that is hard to generalise any expectations about), and it seems in the past few months we have (possibly by accident) introduced additional uses of mw-body-content for unrelated purposes, possibly to work around styling problems in Vector. I'm not exactly sure why. It seems like either those should have been given mw-parser-output (and we still need that, for T188443, T230471 etc.), or if they were about skin styles, then the relevant skin styles should perhaps have been changed to target mw-body instead of mw-body-content. Vector now basically has every child of mw-body marked with mw-body-content which certainly seems like it wants those styles to be written against mw-body instead. Looking through the Git history, this can be traced to T155863 and styling of content in a "current skin"-like way within OOUI modal dialogs. That's an interesting use case indeed, and probably calls for something that is semantic and not layout-driven. I guess that ship has sailed and we can't now standardise mw-body-content as a layout element.

Anyway, it seems like mw-body has not been repurposed and could still be a viable option here. It doesn't really matter in practice since there is nothing in-between them, but mw-body-content seemed more ideal in my opinion, as that has less historical baggage and is closer to what we care about and less likely to break if things are appended to mw-body by extensions or gadgets. But, given a repurposed mw-body-content is hard to reverse now, perhaps mw-body makes for a good second choice?

I don't see how that justifies adding this responsibility to individual features like RCFilters though. […]

Task description:

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 […] I wouldn't use a component from a component library that floated itself and relied on other components for its layout.

That's a fair general principle, and I do agree. However, it is a moot point becuase there is no distinction between the content area for user-generated content and the content area for special pages. They are one and the same. No matter which way we slice it, the various fixes such as for RCFilters will end up redundant.

We've been moving away from <div class="visualClear"></div> in skins. I believe @Volker_E was a big part of that. <div class="visualClear"></div> is a skin construct, and many skins do not include it. If we decided to restore that, it would make sense for it to be part of the output of the new function.

I think originally the way it was intended when we discussed this with Gabriel (for Parsoid) and Trevor/Volker (for Vector and skin standardisation) was that mw-body would be the skin's preferred wrapper for the page flow / main column, and mw-body-content anologous to the main content (e.g. as produced by special pages, page actions, or parser output) thus not including sitenotice/heading/indicators etc.

Thanks that's useful context. The reason I've been wary of rushing this change is there is a lot of confusion in the naming. The challenge with making mw-body the preferred wrapper is that it has different meaning in https://github.com/wikimedia/Vector/blob/master/includes/templates/skin.mustache#L50 and other skins.

mw-body-content is also used in Vector to wrap tagline, subtitles, user messages, undelete links and a class provided by the skin itself. It has different meanings in other skins https://codesearch.wmcloud.org/skins/?q=mw-body-content&i=nope&files=&excludeFiles=&repos=

If we were to standardize on the mw-body-content classes, we'd want to move that into the SkinTemplate class and possibly reimagine it - for example would the tagline be part of the new block of HTML or would that change its position in the DOM hierarchy.

But, given a repurposed mw-body-content is hard to reverse now, perhaps mw-body makes for a good second choice?

I certainly don't to rush a decision here. It may be worth the effort of repurposing mw-body-content once we come up with the correct definition. Personally, I like the idea of tieing it to the OutputPage class, to make it clearer where it comes from. mw-body-output might also make sense.

Is https://en.wikipedia.org/wiki/Special:Book being broken part of this problem ?

Yeh, I've captured that separately so we don't forget about it. Thanks for the report! T280259

This regressed again per T280260 so let's get this dealt with one and for all.

This problem has already been solved. Can we fix the misuse of mw-body-content instead introducing a confusingly similar an entirely new and mw-content-body class and thus make skin developers learning curve greater?

Just an update here to say I'm exploring this possibility. It's possible, but will require some work in multiple skin repos and communication which is fine but needs an action plan which I'm currently working on (targetting mid next week).

The challenge is the children of that element vary depending on the skin. For the sake of 1.36, we could modify the module to apply its rule to mw-body-content instead of mw-parser-output and then work on making sure skins don't have any traces of this class in them during 1.37, making sure it's provided as date by SkinTemplate or BaseTemplate e.g. make sure this returns zero results:
https://codesearch.wmcloud.org/skins/?q=mw-body-content&i=nope&files=.*%5C.(mustache%7Cphp)&excludeFiles=&repos=

This might look like replacing the SkinMustache html-body-content template variable with an object called data-body-content with several keys including id, class and html. The skin templates would then render what it's given. 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).

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.

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

[mediawiki/core@master] Reclaim the `mw-body-content` class for result of SkinTemplate::wrapHTML

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

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

[mediawiki/skins/Vector@master] Remove mw-body-content from HTML that is not the article body

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

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

[mediawiki/skins/Vector@master] Remove mw-body-content from HTML that is not the article body

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

I'll try not be as late this time, and raise something you might already know or would have found out on your own: Per T279388#6974561, what is the plan for general selectors?

(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).

(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).

Also special pages, file pages, other content models, anything that may still be using parsed messages from the MediaWiki namespace or outputting content where we still expect consistent content styles to apply.

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.

That was .visualClear. So while this should be safe for that (skins not using visualClear probably would be far more likely to be clearing specific things by id), unfortunately the float-clearing rule that was shipped with core has already been removed...

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

How content generated by users is cleared and floated is poorly defined, and not shared by all skins.

It was before the .visualClear styles were unexpectedly removed from core, despite being there since mw 1.10.

To solve this, the ResourceLoaderSkinModule enables styles by default that adds a pseudo-element to the end of mw-parser-output that clears all floats on the page.

That is like the least of what .visualClear was for... and also should really be up to the skin anyway. They may still have other stuff after the main output(s) that still expects content floats to apply, such as metadata, external content, extension content, or other content model content that is still a part of the overall content and should be treated as such...

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.

This premise is wrong. Components should have levels, where those at the same level work in isolation, but also where nesting is expected of different levels within each other, per how HTML and CSS work in general.

Regarding special pages in particular, special pages are page content and should be styled consistently with the rest of the content. This is important both in terms of visuals and UX, as these seemingly random discrepencies (to users) at best lose functionality or effective layout (things clearing differently, too-wide tables not scrolling), and at worst serve to outright confuse as they defy established expectation from the parsed wiki content (such as missing external link styles resulting in what looks like a local link unexpectedly taking users to another site entirely).

Expecting individual special pages to implement their own styles to handle their own 'content' styles, not just floats, but table styles, links, etc, is not just unnecessary, but also just plain poor practice. At best, we wind up with unnecessary code duplication. More likely, the code and user experience alike wind up diverging, both making considerably more work for developers and maintainers down the road, and also for users to make sense of what they're, well, using.

(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

This module is not for those kind of things. In this team from a product perspective we tend to think of the article body as a black box, that shouldn't be tinkered with inside skins.
Styling of things inside that box is handled by separate content features. We have content.links, content.externallinks. This feature should be considered an internal feature, that hides away implementation details (e.g. the mw-empty-elt tidy migration), bugs that need CSS workarounds (e.g. a.external.autonumber) and expected behaviours from editors like the floating.

(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).

The above content. features, may be scoped to .mw-parser-output or may not be (and use general selectors) depending on where the classes come from. For extensions, assuming they are using the Parser or API to get parser output those will remain styles as the API will output the parser-output class. They are not impacted by this change. The only impact of this change will be if .mw-body-content is relied on for styling of something outside the article body. I'm not seeing any red flags when I do a codesearch. The most likely impact is skins may need to rethink how they style the heading, which is not part of the article body content (e.g..mw-body-content h1) but they can do that at their leisure

I am not aware of anything that will break here with the changes proposed. Do you have any specific examples?

There are a dozen third-party skins on Codesearch alone that not only clear the content float, but also specifically use visualClear class name. (This is not unlike emptyPortlet in terms of well-known behaviours implicitly responsible to a skin.)

This is captured in the discussion in T280609 so am ignoring the discussion on visualClear as I'd rather not go off-topic here. It doesn't make sense for that to beinside this module, as visualClear is often used outside the article body and this module is specifically for article body which has now been redefined from "output of the parser". Happy to continue the conversation on that ticket. Timeline is much longer - perhaps a year or 2.

(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

This module is not for those kind of things. […]

I wasn't talking about adding (or keeping) such styles in any SkinModule, content-parser-output or otherwise.

I'm referring to the fact that 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" content (as opposed to layout elements or markup rendered in parts of the skin layout that are neither within the content area, nor look like content that is within the content area). Headings, thumbnails, and external link styles are examples of this. Modal dialogs are an example of applying these styles, without there being a skin-owned layout element (e.g. no width, border, margin, or other styles directly against this class; only semantic styles for children within).

Please explain your strategy for how this, and other examples I mentioned, will continue to work.

I'm referring to the fact that 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" content

Which skins ? Is this purely hypothetical ? The class .mw-body-content currently only exists inside skins. Adding this in core, will mean skins need to use a new class or more appropriate class. FWIW am not concerned with hypothetical right now. If not hypothetical could you given an explicit example of something you are concerned about here, as I've taken another look and I'm clearly not seeing what you are seeing.

Previously, I carefully audited all skins on code search and found that this would not impact any known skin in this way. When I looked at extensions using this class, the mw-body-content is added or coupled with a mw-parser-output and usually mimicking the output of a skin.

I'm referring to the fact that 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" content

I am not aware of this pattern being used anywhere. These should be and in practice are using the mw-parser-output class. Wiki content (externalinks) comes from the parser. mw-body-content is used for layout. If you are simulating wiki content, it can be used there. We do this on the mobile editor preview for example.

In terms of the migration, it's important to note, I'll be making sure we correct all the known skins prior to making the change proposed in https://gerrit.wikimedia.org/r/c/682773 and the timeline for this is several months. Right now the main concern is moving the float rule in https://gerrit.wikimedia.org/r/c/677041 - I expect when the markup changes are made to Monobook, CologneBlue, Modern we will tease out any problems if there are any.

If you are concerned about due-diligence here for any of the above, I can tidy up my notes regarding impact to skins and post it publically here.

I'm referring to the fact that 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" content

Is this purely hypothetical?

No, I've mentioned numerous examples. Did you look into those? What did you find?

I'm referring to the fact that 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" content

Is this purely hypothetical?

No, I've mentioned numerous examples.

No, you have not mentioned any non-hypothetical examples here.

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.

Can you be more specific and give tangible examples e.g. links to code you are concerned that is going to break?

Did you look into those? What did you find?

This is a large technical change that impacts all skins so of course. Here are my private notes. https://gist.github.com/jdlrobson/1cc0d7d3f767e4e66c0b0e9ef6c543ec

Change 677041 abandoned by Jdlrobson:

[mediawiki/core@master] Rename content-parser-output to `content-body`

Reason:

Merged into https://gerrit.wikimedia.org/r/c/mediawiki/core/ /682773/. It seems safer (From a revert perspective) to do these together.

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

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

[mediawiki/skins/CologneBlue@master] Remove mw-body-content from indicators

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

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

[mediawiki/skins/Modern@master] Remove mw-body-content from indicators

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

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

[mediawiki/skins/Timeless@master] Drop mw-body-content class from #bodyContent

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

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

[mediawiki/skins/MonoBook@master] Drop mw-body-content class from Monobook in preparation for glorious future

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

...

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.

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. Links or tables on pages like [[Special:Version]] behaving differently than on [[Highway XYZ]] is just weird, from a UX standpoint. Parser output isn't even remotely the same as page content, it's specifically what the parser itself spit out...

(Unless it's done via message()->parse or ->parseasblock or the like. We can't even safely assume that interface content that has been run through the parser will be classed with mw-parser-output, as it historically never has been - T281540)

Can you be more specific and give tangible examples e.g. links to code you are concerned that is going to break?

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...

  • Extension:ContentTranslation
  • Skin:Mask
  • Skin:BlueSky
  • Skin:Material

And it might be worth looking into on-wiki scripts and styles as well given the universality of this class...

Change 685095 had a related patch set uploaded (by Isarra; author: Isarra):

[mediawiki/core@master] Remove mw-body-content class from BaseTemplate::getIndicators

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

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.

I covered these in https://gist.github.com/jdlrobson/1cc0d7d3f767e4e66c0b0e9ef6c543ec

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.

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.

Which sometimes includes things that do not play nice with duplication, like paddings, margins, and borders. JS selectors are also a concern.

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.

Which is why overall I'm actually all for this (I think). Just that the ramifications of the change itself need to be fully considered.

Is T160269: "edit" link for page lead shows in different font in Vector skin being affected by these changes?

I was auditing .mw-body-content usages for MediaWiki-skins-Mirage with https://gist.github.com/jdlrobson/1cc0d7d3f767e4e66c0b0e9ef6c543ec and noticed that .mw-editsection are allowed outside .mw-body-content. Is that still the case?

Also, I noticed that Mirage (not sure about other skins) applies this class to the site notice too. Is that still necessary (or supported?)

I'm referring to the fact that 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" content

Is this purely hypothetical?

No, I've mentioned numerous examples.

No, you have not mentioned any non-hypothetical examples here.

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, […]

Explain how skins are supposed to declare styles for wiki content in a way that applies to content-area parser output and indicators, but also interface messages such as edit notices and block messages, content-like special pages that are not user-generated, OOUI modal dialogs with on-brand markup for the current skin/theme, etc. I keep repeating the same list, but you haven't really responsed. Do you feel you understand why I think these examples relate to mw-body-content? Do you understand how these various examples are working today, or have recently broken, and whether this change might affect that?

As for direct pointers, you submitted five dependencies along with your core patch, which seem like quite clear evidence to start with. The patches lack a commit message explaining why or when these classes were added, what problem they solve then and today, and why they are safe to remove now.

Since I mentioned external links as an example, and you don't appear to have researched that yet despite my mentioning it multiple times and having done half the investigation already, I suppose I can do the other half as well. Picking up where I left off in in T279388#697456, when I linked to T155863.

… so, In T155863 and change 341073 the external link styles were changed from targetting .mw-body (which was and is a skin layout construct) to targetting .mw-body-content (which was and still is for the time being, a semantic construct for generic content styles). This was motivated by needing to allow VisualEditor and OOUI to let the skin apply these generic content styles to its modal dialogs, and in its "edit notice" popovers etc. (It couldn't apply mw-body as that would turn random parts of the UI into a vector skin column). Shortly after this change, we found that for regular page views, it was not exactly a no-op as intended. While there was little to nothing between "mw-body-content" child and "mw-body" parent in most skins, Vector and others do place sitenotice and page indicators there. Hence mw-body-content was applied there. See also T119430 and various other resolved tasks from back then.

How do you envision this to work after your proposed change? Is the new strategy to use completely generic selectors such as ul, a.external, and h2, etc. and then potentially fight these within the skin stylesheets with undoes/overrides for anything in non-content placements? It seems preferable to use .mw-body-content h2 in a skin like Vector, as opposed to targetting h2 and then have to fight/undo its border and margin everywhere that isn't generic content such as in the skin header, skin sidebar, WVUI library components, etc.

I see that more or less the same problem is being hit in T281540, so I suppose we can continue for now.

Explain how skins are supposed to declare styles for wiki content in a way that applies to content-area parser output and indicators, but also interface messages such as edit notices and block messages, content-like special pages that are not user-generated, OOUI modal dialogs with on-brand markup for the current skin/theme

This is out of scope for this task. Right now the focus for this task is for skins to be reliably able to target anything generated inside the OutputPage class by a call to getHTML. Nothing else. Issues outside that will be evaluated on a case by case basis. At time of writing there are no magic styles provided by skins or mediawiki software that are restricted to elements with mw-body-content as shown with my audit in https://gist.github.com/jdlrobson/1cc0d7d3f767e4e66c0b0e9ef6c543ec.

How do you envision this to work after your proposed change? Is the new strategy to use completely generic selectors such as ul, a.external, and h2

No. The hope is to use .mw-body-content instead of .mw-parser-output or generic selectors. Provided OutputPage::getHTML is being used those styles will work. If OutputPage is not being used, presumably that thing may need to apply the mw-body-content class or use OutputPage internally. We'll deal with those on a case by case basis.

However, that will not work until mw-body-content is applied consistently across skins, and most notably the Minerva skin. The reason we have the bugs we do is because of workarounds/hacks to target both Minerva and Vector.

The motivation here is we've have had various problems due to inconsistent application of styling rules across different experiences, such as floating rules (the issue which led you to all these tickets), missing external links (T281540,) and issues with centering (T280401). The root cause of these problems is due to inconsistencies with markup across skins. It's clear we need to first provide consistent markup across multiple skins to be able to provide consistent styling. If you would like to be more involved in these discussions I am happy to involve you in the planning/discussion process within the team or bring this topic up in frontend developer meetup. Just let me know.