Page MenuHomePhabricator

Improper scaling of images sized with "pxpx"
Closed, ResolvedPublic

Description

Author: jonnymt

Description:
A number of users are reporting issues with images displaying at a very large size (appears to be their default size) when the sizing is set using the "px" parameter. There is currently a handful of discussions at the technical village pump [http://en.wikipedia.org/wiki/Wikipedia:Village pump (technical)] indicating that the removal of the "px" parameter seems to be a workaround. I've personally tested this on Firefox/WinXP and Camino/OS X 10.5, and others have reported the same problem on IE/WinXP.


Version: unspecified
Severity: minor
URL: http://en.wikipedia.org/wiki/Wikipedia:Village_pump_%28technical%29#Barnstars_format

Details

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:04 PM
bzimport set Reference to bz13500.
bzimport added a subscriber: Unknown Object (MLST).

The problem (in all test cases I've investigated so far) is images sized with pxpx, not px. This was recently changed so as not to work. See bug 13436.

This regression was made and undone previously wrt bug 8335.

ayg wrote:

It seems like it would be better to let the "pxpx" variant die, at least. It's not really a particularly sane thing to permit. Existing usages can be fixed easily enough.

One general problem here is that [[Image:Foo.jpg|some erroneous option|caption]] will silently eat the first option with no explanation as to what went wrong, and similarly for all options. Perhaps noisier failure modes would be useful to allow people to debug this stuff. That would definitely be a compatibility problem, but it might be a good idea looking forward.

ayg wrote:

Okay, this is causing a bit too much havoc. Fixed in r32504.

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

[mediawiki/core@master] Add `double-px-category` tracking category for deprecated image size syntax

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

Change #1064409 merged by jenkins-bot:

[mediawiki/core@master] Add `double-px-category` tracking category for deprecated image size syntax

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

Pppery subscribed.

Suggested wording for Tech News:

A new tracking category "Pages with image sizes containing extra px" has been added, listing images whose size is specified with multiple "px" units.

It looks like this isn't available until the deployment train, so I'll hesitantly postpone the entry until the following week (so that folks can immediately look at the contents), unless you think it is urgent that editors be warned about it's arrival beforehand. Also, I see in the blue notice-box at https://www.mediawiki.org/wiki/Help:Tracking_categories that the category is likely to be empty/very incomplete at the start, because tracking categories aren't all instantly filled, and have to wait for each page to be re-parsed.
The text for Tech News could be something like:

Editors who help to cleanup broken wikitext can now use a new automatic [[Special:TrackingCategories|tracking category]] named "[[:Category:{{int:Double-px-category}}]]". Pages in this category contain images that specify multiple "px" values instead of just one. [ 1 ]

Why was this change deployed? I don't see a link to an affected article or page. These errors were already tracked via the Linter "bogus file options" flag, as far as I know. I fixed hundreds or thousands of them at en.WP.

If this was really a problem, please link to an affected page. If not it seems like it would have been better to work on an active image-related bug like T275074.

Feel free to ping me for help with old Linter-related issues like this. Sometimes bugs that are very stale have been fixed or otherwise dealt with already.

I just noticed that pages are categorized when using this syntax:

<gallery mode="packed" heights="200px">

Though it is documented: see here and here.

Currently, "gallery packed" allows:

  • 200
  • 200px, but it's categorized as "double px"

Compare with regular image tags:

  • 200px
  • 200pxpx, but it's categorized as "double px"

See code: https://gerrit.wikimedia.org/g/mediawiki/core/+/3d2c3032fd8d1514451322376eaf9228f6e7a497/includes/parser/Parser.php#6380. Notice the regular "px" is not there; I guess it is matched beforehand.

We're now late enough that this isn't worth announcing. Sorry that we failed here.

This change should be reverted entirely, as far as I can tell. Special:LintErrors/bogus-image-options already detects this condition. Why have a tracking category for this one condition and not for all of the Linter issues, as has been requested many times?

Please back out this new code, or link to an affected page that is not detected by the existing Linter code.

The fix (T374311) is going out on this week's train.

Just to put on the record why this tracking category was added:

  1. Parsoid had an issue with localized 'px' on eowiki, where it should be 're'.
  2. It turns out that the original codepath wasn't so much working around 'pxpx' in human-authored wikitext, it was a crappy workaround for the fact that Gallery wasn't doing proper localization of the 'px' string, and so in order to make '150px' work for gallery widths, the original code authors (inadvertently) had to make '150repx' (!) work for non-gallery files. (T374311)
  3. So fixing the localization of eowiki for parsoid, revealed a gallery localization bug in core, which wouldn't have shown up except for the fact that this core tracking category was being generated.
  4. End goal was to actually remove the hack in core; tracking category in core is the usual mechanism to do that. Third party wikis probably do not (almost certainly do not) have Linter installed. So while it is good that the Parsoid-side lint has resulted in most actual instances on WMF wikis being fixed, in order to actually clean up the code we need to notify third party wikis users as well, and the core tracking category allows us to do that.
  5. Hopefully sometime before the next LTS we can actually remove the pxpx hack in both core and Parsoid, contributing to greater maintainability of our codebases.

Thank you for the explanation. It would have been helpful to have this explanation earlier. Now that the category is mostly cleared out on en.WP, it has revealed at least one Linter bug; I filed T376943. So this code is useful on en.WP after all!

I have fully emptied the tracking category on frwiki (back from something like 4,000 pages).

  • Most of the cases have been fixed by using this (small, but Lua-based) template: {{Taille px pour image}} (adds "px" suffix if and only if missing)
  • Much more rarely (for #expr calculations), the opposite may be needed:
    • replace "42px" with "42" (and don't modify other inputs): {{#invoke:String|replace|source={{{foobar}}}|pattern=^(%d+) *px$|replace=%1|plain=false}}
    • also replacing "42x43px" or "x43px" to respectively "42x43" or "x43" (pattern ^(%d*x?%d+) *px$) would make no sense, if we need the result to be an integer.