I welcome the simplifications drafted in T273505.

I'd like to highlight though, for avoidance of doubt, that the repeated figure selectors are in my view not what stands out in the complexity of content.media.less. The complexity I see there is the use of child positions, nested selectors, and wildcards; which seem difficult to bind to and support in extensions/skins/gadgets.

I suspect a some amount of that might actually be due to needless mirroring of the HTML tree (a common anti-pattern when new to LESS). I haven't look at it very closely yet, but it seems like some of that could can be removed as-is and everything still work the same, similar to how the current production styles are written. In that case the complexity does not represent a fragile contract or need to rethink the selectors, but merely extra specificity in the selector that wasn't needed in the first place.

Change 771695 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/core@master] Add a class on the span representing the broken media element

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

Change 771726 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/services/parsoid@master] Add a class on the span representing the broken media element

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

Change 771726 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Add a class on the span representing the broken media element

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

Change 771695 merged by jenkins-bot:

[mediawiki/core@master] Add a class on the span representing the broken media element

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

Change 772511 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/extensions/VisualEditor@master] Preserve classes on broken media elements

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

Change 772511 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Preserve classes on broken media elements

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

Change 772884 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.16.0-a2

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

Change 772884 merged by jenkins-bot:

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.16.0-a2

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

Change 792739 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/core@master] Get rid of child combinator for figcaptions

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

Change 792739 abandoned by Arlolra:

[mediawiki/core@master] Get rid of child combinator for figcaptions

Reason:

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

Change 792739 restored by Arlolra:

[mediawiki/core@master] Get rid of child combinator for figcaptions

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

Change 812384 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/core@master] [WIP] Add styles for a class on the mw-file-element

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

Change 812384 merged by jenkins-bot:

[mediawiki/core@master] Use a universal selector (*) to match the media element

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

Change 792739 abandoned by Arlolra:

[mediawiki/core@master] [WIP] Get rid of child combinator for figcaptions

Reason:

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

Maybe a stupid question but why do you need to use that much of typeof~=? And on top level of selectors (which makes the CSS bloated when comparing with previous structure and CSS).

That seem rather inefficient and a bit bloated. Seems like something like figure:is(.mw-thumb,.mw-frame) would be shorter at least and probably more future proof (if I'm reading this correctly the styles should be for all thumbs and frames). Or maybe use $= if you need to use attributes for some reason? And maybe not as a top selector...

I mean imagine if someone wants to changes those styles in a gadget or even just try to figure what it does.

Not sure if I was clear about using the top selector. I mean for example lets take this one:

figure[typeof~="mw:File/Thumb"].mw-halign-left, figure[typeof~="mw:File/Frame"].mw-halign-left, figure[typeof~="mw:Image/Thumb"].mw-halign-left, figure[typeof~="mw:Video/Thumb"].mw-halign-left, figure[typeof~="mw:Audio/Thumb"].mw-halign-left, figure[typeof~="mw:Image/Frame"].mw-halign-left, figure[typeof~="mw:Video/Frame"].mw-halign-left, figure[typeof~="mw:Audio/Frame"].mw-halign-left {
  margin: 0.5em 1.4em 1.3em 0;
  clear: left;
  float: left;
}

Not sure if I'm reading this correctly, but it seems to be mostly equivalent to:

figure.mw-halign-left {
  margin: 0.5em 1.4em 1.3em 0;
  clear: left;
  float: left;
}

Or maybe even just .mw-halign-left.

If you need to adjust margins for frameless you could probably use something like this:

.mw-frameless.mw-halign-left {
  margin: 1 2 3 4;
}

Or better yet (rough code, just showing an idea):

:root {
  --mw-margin-tright: 0.5em 1.4em 1.3em 0;
}
.mw-frameless {
  --mw-margin-tright: 1 2 3 4;
}

.mw-halign-left {
  margin: var(--mw-margin-tright);
  clear: left;
  float: left;
}

Some of the redundancy is cleaned up by T273505 but just hasn't been removed yet.

Change 861953 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/core@master] Remove b/c for media typeof

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

Change 862312 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/skins/Timeless@master] Remove b/c for media typeof

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

Change 862313 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/skins/MinervaNeue@master] Remove b/c for media typeof

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

Change 861953 merged by jenkins-bot:

[mediawiki/core@master] Remove b/c for media typeof

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

Change 862312 merged by jenkins-bot:

[mediawiki/skins/Timeless@master] Remove b/c for media typeof

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

Change 862313 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Remove b/c for media typeof

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

In T270150#8426746, @Nux wrote:

Maybe a stupid question but why do you need to use that much of typeof~=? And on top level of selectors (which makes the CSS bloated when comparing with previous structure and CSS).

Not a stupid question by any means. I've removed much of the bloat in the above merged patches (ex. https://gerrit.wikimedia.org/r/c/mediawiki/core/+/861953) but perhaps a more satisfying answer is in T314318#8434270. Please have a look.

Thanks for the explanation. Current code looks much better in terms of top selector and readability. I see that JR was the one to propose not adding classes. I thinks it makes things harder and you will need to re-write lots of stuff. And in the end the community with their templates mimicking images will be angry (assuming you will remove old CSS; judging from T318433). I'm not sure if doing migrations in so many communities is even realistic... But I honestly wish you luck and I hope I'm wrong :-)

Taking a page from the discussion at https://en.wikipedia.org/w/index.php?title=MediaWiki_talk:Common.css&oldid=1133053251#Parsoid_specific_CSS_needed_for_Cite

It definitely seems like using a wildcard selector in the rightmost position from T314097 wasn't an ideal choice for performance, not to mention the brittleness of the direct descendents and first-childs that led to T320285.

For example, looking at,

figure[typeof~='mw:File/Thumb'] > a:first-child > *:first-child,
figure[typeof~='mw:File/Frame'] > a:first-child > *:first-child,
figure[typeof~='mw:File/Thumb'] > span:first-child > *:first-child,
figure[typeof~='mw:File/Frame'] > span:first-child > *:first-child {
  margin: 3px;
}

We can use a snippet like,

Array.from( document.querySelectorAll(
	"[typeof*='mw:File'] > *:first-child > *:first-child"
) ).forEach( function ( el ) {
	el.classList.add( 'mw-file-element' );
} );

var selectors = [
	"figure[typeof~='mw:File/Thumb'] > a:first-child > *:first-child",
	"figure[typeof~='mw:File/Thumb'] > a:first-child > .mw-file-element",
	"figure[typeof~='mw:File/Thumb'] :not( figcaption ) .mw-file-element"
];

for ( var i = 0; i < selectors.length; i++ ) {
	var t = Date.now();
	for ( var j = 0; j < 1000; j++ ) {
		document.querySelectorAll( selectors[ i ] );
	}
	console.log( selectors[ i ] + ': ' + ( Date.now() - t ) + ' ms' );
}

on https://en.wikipedia.org/api/rest_v1/page/html/San_Francisco and in Chrome get results like,

figure[typeof~='mw:File/Thumb'] > a:first-child > *:first-child: 337 ms
figure[typeof~='mw:File/Thumb'] > a:first-child > .mw-file-element: 190 ms
figure[typeof~='mw:File/Thumb'] > :not( figcaption ) .mw-file-element: 198 ms

There are several other rules that would stand to benefit from the mw-file-element class, since the vertical alignment, image border, class media option classes all apply to the wrapper element now but the styles need to be applied to the media elements. As is evident in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/812384

Maybe the bloat that was a concern of T314097 / T297984 is justifiable.

Change 901726 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/core@master] [WIP] Add classes

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

Change 902121 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/services/parsoid@master] [WIP] Add classes

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

Change 902203 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/core@master] [WIP] Update styles with classes

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

Change 901726 merged by jenkins-bot:

[mediawiki/core@master] Add classes on elements inside the media structure

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

In T270150#8808624, @gerritbot wrote:

Change 901726 merged by jenkins-bot:

[mediawiki/core@master] Add classes on elements inside the media structure

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

@ssastry (Replying here to the ping on IRC). Thanks for asking me to take another look, I appreciate it. I think this is mostly going in the right direction, the use of a class name as the right-most segment should help a lot in bringing the cost down. I'm playing around with a speed-test at change 912366 to try and quantify the week-over-week impact of this change.

I'll be using devtools to measure the actual style calculation cost of the pageview. I think the previous micro-benchmarking that was done based on querySelector() might not be representative as that mainly exposes internal caches, as well as exposes browser logic relating to finding individual elements. As I understand it, style calculation isn't really done that way in browsers. That is, they don't iterate the stylesheet to find matching elements and cascadingly assign the right styles. It's more the other way around, as the DOM is parsed, each element needs to get rendered on-screen. It's the element that effectively looks up what its matching CSS rules are. More "get selector rules by element" than "get elements by selector".

There's a recent talk by Nolan that talks about some of this: Nolan Lawson's talk on CSS runtime performance (2023). That might be of interest to gain a bit more exposure to this.

Actually, rather than evaluate the week-over-week impact on how the new CSS renders with the new HTML - perhaps we can step back and look at the wider picture: compare the legacy HTML and the legacy CSS with the figure HTML and the figure stylesheets, and measure those as actual pageviews side-by-side. That's precisely the change we're making and how it impacts real users.

I've looped in Roan who may be interested and available to work more closely on perhaps one more round of refinements based on these findings. In particular, to think more holistically about the trade-offs (render speed, support-ability of selectors as an API to skins/gadgets, duplication of logic VisualEditor JS, and a bit HTML size). I'm hoping there may be an optimum that will present itself when the problem space is exposed to front-end devs like @Catrope and @AnneT.

perhaps we can step back and look at the wider picture: compare the legacy HTML and the legacy CSS with the figure HTML and the figure stylesheets, and measure those as actual pageviews side-by-side.

You can maybe get a sense of that from the page drilldown dashboard pre- / post- deploy to cawiki, T297984#8525146

Am I understanding correctly that part of the problem is that we could potentially add more CSS classes to the parser-generated markup, but this would not cover templates that mimic the parser output? If that's the case, I wonder if some version of "CSS-only components" could help here. This could look like creating semantic CSS class names via BEM and documenting the intended markup structure and classes, which template authors could copy. We're documenting CSS-only Codex components on the Codex docs site (example); something similar could be done on-wiki for this.

(This is one suggestion after 5 minutes of reviewing Phab tasks and comments; I'd be interested to learn more about the problem.)

Change 918615 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/extensions/VisualEditor@master] Preserve classes on all file elements, not just broken images

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

Change 918615 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Preserve classes on all file elements, not just broken images

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

Change 902121 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Add classes on elements inside the media structure

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

Change 919886 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/vendor@master] Bump parsoid to 0.18.0-a10

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

Change 919886 merged by jenkins-bot:

[mediawiki/vendor@master] Bump parsoid to 0.18.0-a10

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

Change 902203 merged by jenkins-bot:

[mediawiki/core@master] Remove unneeded wildcard selectors

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

Change 922613 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/core@master] Don't restrict border to :not(figcaption)

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

Change 922614 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/core@master] Remove special casing for broken media in styling

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

Change 922613 merged by jenkins-bot:

[mediawiki/core@master] Don't restrict border to :not(figcaption)

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

Change 922614 merged by jenkins-bot:

[mediawiki/core@master] Remove special casing for broken media in styling

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

Change 922944 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/skins/Timeless@master] Copy upstream media styling changes

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

Change 922944 merged by jenkins-bot:

[mediawiki/skins/Timeless@master] Copy upstream media styling changes

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

The legacy parser always used to set a horizontal alignment for thumbnails,
https://github.com/wikimedia/mediawiki/blob/master/includes/linker/Linker.php#L433-L446
https://github.com/wikimedia/mediawiki/blob/master/includes/linker/Linker.php#L665-L670

However, in Parsoid, it's only set if the media option is explicit in the source and otherwise relies on page content language class on the content container. The omission is done for clean roundtrips of the media options but leads to a bunch of unnecessary duplication in css and is making it cumbersome to replicate the styling in, https://en.wikipedia.org/wiki/MediaWiki:Gadget-responsiveContentBase.css

https://github.com/wikimedia/mediawiki/blob/master/resources/src/mediawiki.skinning/content.media-common.less#L72-L117

	// Default where page content language is not set
	// Allow to flip
	margin: @margin-tright;
	clear: right;
	float: right;

	// Defaults for page content language

	.mw-content-ltr & {
		/* @noflip */
		margin: @margin-tright;
		/* @noflip */
		clear: right;
		/* @noflip */
		float: right;
	}

	...

	// Override defaults when explicitly set
	// Order of application is important, so don't combine with the defaults

	&.mw-halign-right {
		/* @noflip */
		margin: @margin-tright;
		/* @noflip */
		clear: right;
		/* @noflip */
		float: right;
	}

Always setting a class in Parsoid could be done at the expense of normalizing an explicit media option that matches the page content language, and otherwise rely on selser to preserve it.