Page MenuHomePhabricator

Popups should use standard thumbnail sizes
Closed, ResolvedPublic5 Estimated Story Points

Description

Currently, Popups is causing a lot of requests to non-standard sizes. We are planning to start rate-limiting hits to those (T408062: FY 25/26 WE 5.4.7 Standardize thumbnail sizes and T402792: Consider rate limiting non-standard thumbnail sizes) urgently for various reasons. So it would break Popups images now.
It currently takes the thumbnail url, extracts the thumbsize and changes it on the fly:

function generateThumbnailData( thumbnail, original, thumbSize ) {
	const parts = thumbnail.source.split( '/' ),
		lastPart = parts[ parts.length - 1 ],
		originalIsSafe = isSafeImgFormat( original.source ) || undefined;
	// The last part, the thumbnail's full filename, is in the following form:
	// ${width}px-${filename}.${extension}. Splitting the thumbnail's filename
	// makes this function resilient to the thumbnail not having the same
	// extension as the original image, which is definitely the case for SVG's
	// where the thumbnail's extension is .svg.png.
	const filenamePxIndex = lastPart.indexOf( 'px-' );
	if ( filenamePxIndex === -1 ) {
		// The thumbnail size is not customizable. Presumably, RESTBase requested a
		// width greater than the original and so MediaWiki returned the original's
		// URL instead of a thumbnail compatible URL. An original URL does not have
		// a "thumb" path, e.g.:
		//
		//   https://upload.wikimedia.org/wikipedia/commons/a/aa/Red_Giant_Earth_warm.jpg
		//
		// Instead of:
		//
		//   https://upload.wikimedia.org/wikipedia/commons/thumb/a/aa/Red_Giant_Earth_warm.jpg/512px-Red_Giant_Earth_warm.jpg
		//
		// Use the original if it's a supported image format.
		return originalIsSafe && original;
	}
	const filename = lastPart.slice( filenamePxIndex + 3 );
	// Scale the thumbnail's largest dimension.
	let width, height;
	if ( thumbnail.width > thumbnail.height ) {
		width = thumbSize;
		height = Math.floor( ( thumbSize / thumbnail.width ) * thumbnail.height );
	} else {
		width = Math.floor( ( thumbSize / thumbnail.height ) * thumbnail.width );
		height = thumbSize;
	}
	// If the image isn't an SVG, then it shouldn't be scaled past its original
	// dimensions.
	if ( width >= original.width && filename.indexOf( '.svg' ) === -1 ) {
		// if the image format is not supported, it shouldn't be rendered.
		return originalIsSafe && original;
	}
	parts[ parts.length - 1 ] = `${ width }px-${ filename }`;
	return {
		source: parts.join( '/' ),
		width,
		height
	};
}

This should at least use the standard sizes (e.g. build the width, the jump to the next larger one). In the longer term, it probably shouldn't change the url to the thumbnail on the fly.

Event Timeline

This problem is blocking KR work for WE5, responsible content reuse. Please treat the problem with some urgency, this code is preventing us from rate-limiting severely non-standard thumbnail size generation, which is one of the pillars of our work.

bvibber subscribed.

Taking this as I've touched relevant other thumbnail bits recently and should be able to figure this out :)

Quick peek at the situation:

  • prod config has $wgThumbnailSteps = [ 20, 40, 60, 120, 250, 330, 500, 960 ];
  • for suitably aspect-ratio'd images it seems to ask for a 640px image on my 2x display, 480px on a 1.5x display, and presumably would a ask for 320px on a 1x display
    • for taller images it may pick a non-standard width (untested so far but looks like it from the code)
  • we want for the JS to apply the same logic as File::adjustThumbWidthForSteps() in the PHP side
    • if the requested width is smaller than the smallest step, use the smallest step
    • if the requested width is exactly a step, use the step
    • if the requested width is between steps, round one step up
    • if the requested width is greater than the highest step, use the requested width

I think the right thing to do is apply this logic in a common place like core's mw.utils and call that from Popups, since other extensions doing runtime thumbnail link generation will have the same issue. I'll try to whip up a proof of concept real quick and see what that looks like.

Change #1211230 had a related patch set uploaded (by Bvibber; author: Bvibber):

[mediawiki/core@master] Common JS logic for thumbnail steps in mw.util

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

Change #1211231 had a related patch set uploaded (by Bvibber; author: Bvibber):

[mediawiki/extensions/Popups@master] Respect wgThumbnailSteps when generating thumbs

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

It would be great if we can get the simple patch deployed in the meantime so we can start working on rate limiting non-standard sizes

If there's any objection to this core patch I can move the logic into Popups, I just..... really think it belongs in core. :)

@Ladsgroup any help with core code review would be welcome:

Then we can merge and deploy it and the fix:

(Note the constants patch does not look generally correct, I won't merge that.)

For the record, Popups indeed picks problematic sizes for tall images, such as 479px and 436px.

Even before the recent introduction of step sizes (wgThumbnailSteps), these were already considered non-standard and subject to tight rate limits and those requests may fail (wgThumbLimits, wgImageLimit, and renderfile-nonstandard rate limit).

Today's Main Page features https://en.wikipedia.org/wiki/Philosophy and https://en.wikipedia.org/wiki/Sheikh_Hasina and Popups is embedding:

  • https://upload.wikimedia.org/wikipedia/commons/thumb/a/a4/Le_Penseur_by_Rodin_%28Kunsthalle_Bielefeld%29_2014-04-10.JPG/479px-Le_Penseur_by_Rodin_%28Kunsthalle_Bielefeld%29_2014-04-10.JPG
  • https://upload.wikimedia.org/wikipedia/commons/thumb/2/21/Prime_Minister_Sheikh_Hasina_Attends_Business_Advisory_Committee_Meeting_2024-05-02_%28PID-0008829%29_%28portrait_cropped%29.jpg/436px-Prime_Minister_Sheikh_Hasina_Attends_Business_Advisory_Committee_Meeting_2024-05-02_%28PID-0008829%29_%28portrait_cropped%29.jpg

For the record, Popups indeed picks problematic sizes for tall images, such as 479px and 436px.

I'm happy to report that the patch fixes these, normalizing them both to 500px on my 2x display on local testing :D

Change #1211265 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] mediawiki.util,FileRepo: Improve adjustThumbWidthForSteps test coverage

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

Change #1211230 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.util: Add adjustThumbWidthForSteps for step sizing in JS

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

Change #1211265 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.util,FileRepo: Improve adjustThumbWidthForSteps test coverage

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

Change #1211231 merged by jenkins-bot:

[mediawiki/extensions/Popups@master] Respect wgThumbnailSteps when generating thumbs

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

Change #1211277 had a related patch set uploaded (by Bvibber; author: Bvibber):

[mediawiki/core@wmf/1.46.0-wmf.3] mediawiki.util: Add adjustThumbWidthForSteps for step sizing in JS

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

Change #1211278 had a related patch set uploaded (by Bvibber; author: Bvibber):

[mediawiki/core@wmf/1.46.0-wmf.4] mediawiki.util: Add adjustThumbWidthForSteps for step sizing in JS

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

Change #1211279 had a related patch set uploaded (by Bvibber; author: Bvibber):

[mediawiki/extensions/Popups@wmf/1.46.0-wmf.3] Respect wgThumbnailSteps when generating thumbs

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

Change #1211280 had a related patch set uploaded (by Bvibber; author: Bvibber):

[mediawiki/extensions/Popups@wmf/1.46.0-wmf.4] Respect wgThumbnailSteps when generating thumbs

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

Scheduled for deploy this coming morning UTC (yes I know it's late my time):

Change #1211277 merged by jenkins-bot:

[mediawiki/core@wmf/1.46.0-wmf.3] mediawiki.util: Add adjustThumbWidthForSteps for step sizing in JS

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

Change #1211278 merged by jenkins-bot:

[mediawiki/core@wmf/1.46.0-wmf.4] mediawiki.util: Add adjustThumbWidthForSteps for step sizing in JS

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

Change #1211280 merged by jenkins-bot:

[mediawiki/extensions/Popups@wmf/1.46.0-wmf.4] Respect wgThumbnailSteps when generating thumbs

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

Change #1211279 merged by jenkins-bot:

[mediawiki/extensions/Popups@wmf/1.46.0-wmf.3] Respect wgThumbnailSteps when generating thumbs

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

Mentioned in SAL (#wikimedia-operations) [2025-11-26T08:08:23Z] <bvibber@deploy2002> Started scap sync-world: Backport for [[gerrit:1211277|mediawiki.util: Add adjustThumbWidthForSteps for step sizing in JS (T411013)]], [[gerrit:1211279|Respect wgThumbnailSteps when generating thumbs (T411013)]], [[gerrit:1211278|mediawiki.util: Add adjustThumbWidthForSteps for step sizing in JS (T411013)]], [[gerrit:1211280|Respect wgThumbnailSteps when generating thumbs (T411013)]]

Mentioned in SAL (#wikimedia-operations) [2025-11-26T08:10:46Z] <bvibber@deploy2002> bvibber: Backport for [[gerrit:1211277|mediawiki.util: Add adjustThumbWidthForSteps for step sizing in JS (T411013)]], [[gerrit:1211279|Respect wgThumbnailSteps when generating thumbs (T411013)]], [[gerrit:1211278|mediawiki.util: Add adjustThumbWidthForSteps for step sizing in JS (T411013)]], [[gerrit:1211280|Respect wgThumbnailSteps when generating thumbs (T411013)]] synced to the testservers (see

Mentioned in SAL (#wikimedia-operations) [2025-11-26T08:15:30Z] <bvibber@deploy2002> Finished scap sync-world: Backport for [[gerrit:1211277|mediawiki.util: Add adjustThumbWidthForSteps for step sizing in JS (T411013)]], [[gerrit:1211279|Respect wgThumbnailSteps when generating thumbs (T411013)]], [[gerrit:1211278|mediawiki.util: Add adjustThumbWidthForSteps for step sizing in JS (T411013)]], [[gerrit:1211280|Respect wgThumbnailSteps when generating thumbs (T411013)]] (duration: 07

matthiasmullie subscribed.

Seems to work.

The popup image for provincia del Québec at https://it.wikipedia.org/wiki/Montr%C3%A9al seems to load at 500px-wide (which it then displays at 320px)
It gets the thumbnail info from API, where a 330px-wide thumb (another one at good step width) is returned, which the code then decides to scale up to 480px (320 * 1.5 ). In the end it ends up getting a 500px image, because that’s the next step the new code scales back up to.

HSwan-WMF set the point value for this task to 3.Nov 26 2025, 5:06 PM
HSwan-WMF changed the point value for this task from 3 to 5.

We're going to split out a task for follow-up: T411125

That covers possibly changing the common PHP/JS thumbnail steps logic for the case where it asks for something between the last step and the file's original width (eg, should it then return the original size instead of the requested size?)

which means I think we're good to close this one out. :D