(Still reproducible on 1.33-wmf.21. Tagging per mw:Maintainers.)

Change 498445 had a related patch set uploaded (by Aezell; owner: Aezell):
[mediawiki/extensions/ImageMap@master] Default image map coords to zero

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

That patch fails a CI security check with this error: Calling method \Xml::element() in \ImageMap::render that outputs using tainted argument $attribs. (Caused by: Builtin-\Xml::element) (Caused by: ./includes/ImageMap.php +249; ./includes/ImageMap.php +189; ./includes/ImageMap.php +252; ./includes/ImageMap.php +263)

I'll look into it. At first glance, it appears to have nothing to do with my change. Maybe I can clean it up.

Upon further examination, it's possible this bug only presents for poly maps. There is code to protect against non-numeric coords for rect and circle and default maps.

The fix may be similar but should probably happen elsewhere in the code. This might also help clean up the error above.

I pushed a new patch that works much better and passes Jenkins.

Now, instead of defaulting to zero, we warn the user that there is an invalid coordinate. I like that solution much better.

Change 498445 merged by jenkins-bot:
[mediawiki/extensions/ImageMap@master] Warn user about invalid coordinates

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

Thanks for cleaning this up!

However, this change caused all articles about major cities on the German-language Wikipedia to display a large, layout-breaking error message instead of the usual map right at the top of the page. A heads up to the communities would have been much appreciated.

@aezell was the marking of floats as invalid an intentional decision ?

Because it seems that this broke poly's that were using float numbers. I think float values for imagemaps are pretty much undocumented, but they were (and for other numbers ARE) taken into account when applying scale for instance, before being cast to integers.

According to the <area> coords spec of HTML, which our image maps are targeting, they are documented as Length's, which are either percentage values or integer Pixel values. Percentages were however never properly supported by browsers. Thus in HTML 5 it was explicitly redefined to be a list of comma separated integers.

Because of that, I think it's ok to have these invalid indeed, but it is a change in behaviour.

@Cirdan I apologize for the breakage. There was discussion here about the fact that the old code simply dumped the error into the log and that behavior might be preferable. It seems in the case you mention that it is.

As @TheDJ points out, I was basing my work on the HTML5 specs. All of the values are converted to integers so any floats, percentages, etc. get scaled naively. This was true of the previous code as well so I didn't change that behavior.

I think a possible solution here is to use is_numeric instead of ctype_digit in the check. It would permit floats even though they'll be cast to integers as the code does naive scaling.

Agree that floats should be permitted, and any type conversion only done after scaling, to avoid loss of precision. At https://en.wikipedia.org/w/index.php?title=Template:Indian_states_and_territories_image_map&diff=889959581&oldid=889956524 I was forced to convert a whole heap of poly co-ordinates to integer in order to get the template to render at all. Fortunately I could do this by using a perl command on a text file of the wikitext - "perl -i -pe 's/(\d*\.\d*)/int($1+0.5)/ge' filename" otherwise manually rounding hundreds of co-ordinates would have been quite a task. The floats had existed in the code for years without throwing this error, so it certainly was a change in behaviour.

Change 500159 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/ImageMap@master] Revert "Warn user about invalid coordinates"

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

Change 500160 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/ImageMap@wmf/1.33.0-wmf.23] Revert "Warn user about invalid coordinates"

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

Reverted in-branch to unbreak prod. Still broken in master, to be fixed (or also reverted) before the next train.

Change 500160 merged by jenkins-bot:
[mediawiki/extensions/ImageMap@wmf/1.33.0-wmf.23] Revert "Warn user about invalid coordinates"

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

In T217087#5071494, @Krinkle wrote:

This is breaking real content that has worked for years.

https://en.wikipedia.org/w/index.php?title=Template:Indian_states_and_territories_image_map&oldid=889956524

Now renders an image again, and with clickable countries:

Mentioned in SAL (#wikimedia-operations) [2019-03-30T03:39:06Z] <krinkle@deploy1001> Synchronized php-1.33.0-wmf.23/extensions/ImageMap/includes/ImageMap.php: I1387825f25e / T217087 (duration: 00m 52s)

In T217087#5071532, @Krinkle wrote:

In T217087#5071494, @Krinkle wrote:

This is breaking real content that has worked for years.

https://en.wikipedia.org/w/index.php?title=Template:Indian_states_and_territories_image_map&oldid=889956524

Now renders an image again, and with clickable countries:

Thank you for your quick action. Much appreciated.

Change 500184 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/ImageMap@master] Use is_numeric instead of ctype_digit

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

Change 500184 merged by jenkins-bot:
[mediawiki/extensions/ImageMap@master] Use is_numeric instead of ctype_digit for 'poly' validation

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

My apologies to all for this breakage. Thank you to @Krinkle for cleaning up my mess.

And thanks to @Esanders for the new patch.

Change 500159 abandoned by Krinkle:
Revert "Warn user about invalid coordinates"

Reason:
Thx

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

Is this still relevant for a Tech News update? In that case, how do we best phrase it?

(I.e. do we still need to give warning about upcoming changes that might affect content?)

@Johan I think the new patch from @Esanders fixes the bug without changing the expected use cases. I don't think this new fix will affect the users in any way.

We should probably update parser tests for this though, considering we are deviating from HTML5 expectations, this issue could easily be reintroduced...

legit maps on many wikis are still broken. i believe this story should be reopened - i could not understand the status of the patch, but in production sites (wikipedias of all languages), this is still borked.

also, i may have missed something, but i saw only "float" values discussed, while in the real world, some of the issues are not with float values, but rather with _negative_ values.

as far as i know, -1 (even though it lies outside the image borders) is a legit coordinate, which previously worked with no complaints, and currently display error message, instead of the map.

(shameless plug): this case demonstrates again that adding tracking categories to imagemap errors is not a "luxury". please resolve T106075.

i'll reopen this story - not 100% sure this is the right thing to do, but my grandma told me - "when in doubt, reopen".

peace.

@Cirdan Do you have any examples? I'm looking at https://de.wikipedia.org/wiki/Baden-W%C3%BCrttemberg and https://de.wikipedia.org/wiki/Bayern and https://de.wikipedia.org/wiki/Berlin which seem fine.

Also, just for clarity's sake, this wasn't "redeployed." There was a second patch made which is deployed as this while loop in the master branch of the extension indicates: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/ImageMap/+/refs/heads/master/includes/ImageMap.php#209

In it, you can see that it excludes negative coordinates as @Kipod's post comments on.

Here's a link to a broken page:
https://en.wikipedia.org/w/index.php?title=Template:Imagemap_Germany_district_AW&oldid=566491718

A couple of minor enhancement requests:

1. Add a tracking category, e.g. Category:Imagemap with invalid coordinates, so that gnomes can find and fix these errors instead of having to encounter them randomly.
2. Tweak the error text, which complains about the input not being a "number", even when the input is a number that happens to be negative.
3. Best option: If negative numbers are truly invalid, deprecate the use of negative numbers but continue the old behavior of replacing the negative number with a zero, but add a tracking category so that gnomes can fix these templates. Give the job queue some time to populate the error category (see also T157670, which means that it may take many months or years for the error category to populate fully), then remove support for negative numbers once all pages are null-edited.

Negative numbers were explicitly excluded it seems. The HTML 5 spec however notes that negative numbers are allowed in the syntax for imagemap poly's. Seems like something that should be fixed yes.. Also parsertests ;)

Based on the patch and its commit message, I believe the author was following the same logic as is used for the coordinates in a circle or rectangle map. Whatever the spec, I think we can agree that negative coordinates aren't logical in the coordinate system used for polygonal image maps.

However, we have a lot of polygon image maps in our content that are breaking on this. So, this seems to be a case where being technically correct isn't the best approach.

This should be an easy change. I'll see if I can prepare a patch.

Change 502888 had a related patch set uploaded (by Aezell; owner: Aezell):
[mediawiki/extensions/ImageMap@master] Allow negative coordinates in poly image maps

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

In T217087#5102461, @aezell wrote:

....., I think we can agree that negative coordinates aren't logical in the coordinate system used for polygonal image maps.

no, we can't agree.
it is legit to have a poly area, where one of the vertices is outside the edge of the image.
it actually makes sense: basically, the part of the polygon intersecting the image itself is "click sensitive".

also, the test itself is inconsistent: it allows poly points outside the image to the right and bottom, but not to the left or top.

of course, the winning argument here is that there _are_ such imagemaps all over the place, which until recently worked just fine, thank you, and you do not introduce a breaking change based on "we can agree".

looked at https://gerrit.wikimedia.org/r/502888 , and i don't like it.
it has "if (coord < 0) coord = 0". this perverts the shape of the polygon as set by the user.
html5 does not preclude negative coords, and you should not pervert the shape based on a superstition. if you get a negative coord, please be a good citizen and pass it along.

peace.

In T217087#5101989, @Jonesey95 wrote:

Here's a link to a broken page:
https://en.wikipedia.org/w/index.php?title=Template:Imagemap_Germany_district_AW&oldid=566491718

A couple of minor enhancement requests:

1. Add a tracking category, e.g. Category:Imagemap with invalid coordinates, so that gnomes can find and fix these errors instead of having to encounter them randomly.
2. Tweak the error text, which complains about the input not being a "number", even when the input is a number that happens to be negative.
3. Best option: If negative numbers are truly invalid, deprecate the use of negative numbers but continue the old behavior of replacing the negative number with a zero, but add a tracking category so that gnomes can fix these templates. Give the job queue some time to populate the error category (see also T157670, which means that it may take many months or years for the error category to populate fully), then remove support for negative numbers once all pages are null-edited.

+1

• regarding "tracking" - let me rephrase: please resolve T106075
• as to the error message: let me expand: the recent incident taught me something: with imagemaps, where you have very very very long lines, which sometime wraps 4 or even more lines in editor, it would be very helpful to include the offending string that could not be parsed (sanitize it, if you have to) as part of the error message. otherwise it can be a real challenge to locate the problem. imagine O instead of 0 in one of the numbers, or l/I instead of 1.
• as to "if negative numers are truly invalid" - i do not believe they are valid.

peace

In T217087#5102814, @Kipod wrote:

it is legit to have a poly area, where one of the vertices is outside the edge of the image.

That's true. To the end user, it will look like two vertices that intersect with the edge. I was confused about how the browser would render that.

of course, the winning argument here is that there _are_ such imagemaps all over the place, which until recently worked just fine, thank you, and you do not introduce a breaking change based on "we can agree".

Exactly as I said in my response from which you are quoting. As you're agreeing with me here, I don't understand why your tone is so condescending. I didn't introduce the breaking change regarding the negative coordinates. The break is already in production. I was suggesting a patch that might help resolve it. You are implying that I have an agenda that is something other than fixing the breakage that is already in production and that implication is incorrect.

looked at https://gerrit.wikimedia.org/r/502888 , and i don't like it.
it has "if (coord < 0) coord = 0". this perverts the shape of the polygon as set by the user.
html5 does not preclude negative coords, and you should not pervert the shape based on a superstition. if you get a negative coord, please be a good citizen and pass it along.

I agree with your technical points and am amending my patch to allow for negative coordinates.

Your tone in this response is highly critical of me personally. It's uncalled for and diminishes your otherwise helpful collaboration.

I've amended https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ImageMap/+/502888 to completely allow negative coordinates.

In T217087#5103885, @aezell wrote:

Your tone in this response is highly critical of me personally. It's uncalled for and diminishes your otherwise helpful collaboration.

this was not intentional. i did not mean anything personal in my comment (when i say "you" in phabricator, i usually take it to mean "mediawiki software", as i have no personal acquaintance with any developer, and i usually do not check to see who wrote what ("blame" in git-ish) )

please accept my sincere apology - again, no personal slight was meant. my style tends to be somewhat informal, and sometimes i do use "graphic" terms.
in this context, "superstition" meant to imply that since such limitation ("all poly area vertices must reside within the map") is not documented, it must have come from someone's belief...

again, please accept my apology. i will do my best to avoid phrases that can be perceived as offensive in the future, but my track record in this area is not stellar.....

peace.

In T217087#5104227, @Kipod wrote:

again, please accept my apology.

Thank you for this apology. It is well received.

And thanks for helping me understand the negative coordinates better. I appreciate your help.

@aezell To confirm, this patch makes it behave the same as it did previously? Or is turning negative into positive different from how it was tolerated by the parser in the past?

For what it's worth, if previous behaviour was to skip and render only the remaining coords, that seems perfectly fine as well. The main issue is to have the rest still render. (eg. the bad coord could be skipped or re-read as a copy of a preceding/next coord instead to make a mostly usable output still.)

@Krinkle This patch should ensure that the coordinates are numeric, either positive or negative. The only change here, from production, is to allow negative coordinates. This modifies Ed's patch which came after your revert.

So, it should preserve the previous parsing behavior while also avoiding the log errors that were occurring which got this whole thing started.

@aezell Yeah, it changes what we have in prod now, but I mean the behaviour of "negative becomes positive" is what we had up until recently, right? Or were they previously tolerated in a different way?

Change 503473 had a related patch set uploaded (by Krinkle; owner: Aezell):
[mediawiki/extensions/ImageMap@wmf/1.33.0-wmf.25] Allow negative coordinates in poly image maps

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

Change 503473 merged by jenkins-bot:
[mediawiki/extensions/ImageMap@wmf/1.33.0-wmf.25] Allow negative coordinates in poly image maps

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

Change 502888 merged by jenkins-bot:
[mediawiki/extensions/ImageMap@master] Allow negative coordinates in poly image maps

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

Mentioned in SAL (#wikimedia-operations) [2019-04-12T21:10:51Z] <krinkle@deploy1001> Synchronized php-1.33.0-wmf.25/extensions/ImageMap/includes/ImageMap.php: I0ee84f059da / T217087 (duration: 05m 12s)