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.

In content.media-common.less, the following:

figure[ typeof~='mw:File/Thumb' ],
figure[ typeof~='mw:File/Frame' ] {

    /* ... */

    > :not( figcaption ) {
        .mw-file-element {
            margin: 3px;
        }

        /**
         * Broken media get a span instead.
         */
        .mw-broken-media {
            display: inline-block;
            line-height: 1.6em;
            /* stylelint-disable-next-line declaration-property-value-keyword-no-deprecated */
            word-break: break-word;  // Same as in the figcaption
            /* This is hardcoded in Linker::makeThumbLink2 for broken media */
            width: 180px;
            /* Styles the text of broken media */
            font-size: @font-size-thumbinner-screen;
        }
    }

is transpiled into:

figure[typeof~='mw:File/Thumb'] > :not(figcaption),
figure[typeof~='mw:File/Frame'] > :not(figcaption) {
}
figure[typeof~='mw:File/Thumb'] > :not(figcaption) .mw-file-element,
figure[typeof~='mw:File/Frame'] > :not(figcaption) .mw-file-element {
    margin: 3px
}
figure[typeof~='mw:File/Thumb'] > :not(figcaption) .mw-broken-media,
figure[typeof~='mw:File/Frame'] > :not(figcaption) .mw-broken-media {
    display: inline-block;
    line-height: 1.6em;
    word-break: break-word;
    width: 180px;
    font-size: 94%
}

Notice the :not() at the rightmost position, in the first ruleset. During a performance analysis in Chrome, it was reported as the selector with the most attempts ("Top selector match attempt: …").

Also notice that the ruleset is actually empty.

That's an artifact from Less, which apparently isn't smart enough to clean up such leftovers from an intermediary container…

As a workaround, we may consider:

figure[ typeof~='mw:File/Thumb' ],
figure[ typeof~='mw:File/Frame' ] {

    /* ... */

    /**
     * Note: these two rules intentionally repeat `> :not( figcaption )`
     * instead of nesting them under a shared selector. Nesting would compile
     * to an extra, empty CSS ruleset whose key selector is `:not()` — a
     * selector with no class/id/tag to bucket on, forcing the browser to test
     * it against almost every element in the DOM on every style recalc.
     */

    > :not( figcaption ) .mw-file-element {
        margin: 3px;
    }

    /**
     * Broken media get a span instead.
     */
    > :not( figcaption ) .mw-broken-media {
        display: inline-block;
        line-height: 1.6em;
        /* stylelint-disable-next-line declaration-property-value-keyword-no-deprecated */
        word-break: break-word;  // Same as in the figcaption
        /* This is hardcoded in Linker::makeThumbLink2 for broken media */
        width: 180px;
        /* Styles the text of broken media */
        font-size: @font-size-thumbinner-screen;
    }

Quick measurement on this page, using Chrome's Performance > Selector stats — they are the top 2 costliest selectors:

This Less artifact alone accounts for 4% of the total CSS selector cost, costing 3.27 ms.

@Od1n Thanks for the analysis, the workaround seems reasonable. Do you want push a patch to gerrit?

Just to confirm: this is the lowest-hanging fruit (and a bit of an unexpected one), since it's both the costliest and the most directly fixable.

@ABreault-WMF, feel free to take the patch from here, if you're up for it.

Note that it would just need a code comment (for reference, there is already one in my code snippet), to prevent this Less artifact from being reintroduced later.

In T270150#12126845, @Od1n wrote:

[…] That's an artifact from Less, […]

Upstream issue:

• https://github.com/less/less.js/issues/2477
• https://github.com/less/less.js/issues/4276

In T270150#12131174, @Od1n wrote:

@ABreault-WMF, feel free to take the patch from here, if you're up for it.

Will do. Is it ok if I set you as the author of the patch (git log --author "Od1n" turns up some results) since I want you to have credit for it?

@Krinkle Thanks for pointing these out. That explains a lot — it's the CSS comment that's actually causing this. As similarly shown in the second screenshot in the OP of #4276, the docblock comment should ideally be preserved, positioned right before the ruleset it describes. But since this is an upstream issue, all we can do on our end is sidestep it for now.

@ABreault-WMF I don't care about the credit, really — I'd even prefer not to be credited.

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

[mediawiki/core@master] Fix costly :not in rightmost position

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

In T270150#12133667, @Od1n wrote:

@ABreault-WMF I don't care about the credit, really — I'd even prefer not to be credited.

No problem, I put up a patch at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1312622 and verified locally that it was sufficient to drop the empty ruleset. Hopefully you agree

Change #1312622 merged by jenkins-bot:

[mediawiki/core@master] Fix costly :not in rightmost position

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

Given that following my initial report it was revealed that the issue comes precisely from CSS comments, yes, switching to Less comments is also a viable alternative.

And thanks to @Arlolra for the subsequent changes.

I went to check Jenkins' "performance checks" reports, unfortunately nothing stands out there, it stays buried in the noise of the measurements, but well, we know it's there (and it's measurable locally).