Page MenuHomePhabricator

visualdiff: Kartographer overlay colour on maps has different tone
Closed, ResolvedPublicBUG REPORT

Event Timeline

After a bit of debugging this looks like its the issue:

image.png (196×1 px, 32 KB)

image.png (124×1 px, 19 KB)

The svg renders 2 elements for parsoid but one with legacy parser output.

thiemowmde changed the subtype of this task from "Task" to "Bug Report".Apr 17 2024, 11:43 AM
thiemowmde added a project: Regression.
thiemowmde added subscribers: ihurbain, thiemowmde.

The layer with the fill="#555555" fill-opacity="0.5" is just duplicated, one on top of the other. That's the reason why it appears so much darker.

And that's not a rendering issue. The mask actually appears twice in mw.config.get( 'wgKartographerLiveData' ).mask. It's like some code (maybe processKartographerNode, @ihurbain?) is executed twice and stores the GeoJSON twice in the parser cache.

Yeah, that's the current thought (that somehow it's added twice) - I intend to look into that in the next week, ish

I also noticed that the page actually contains the same (well, almost same) map twice. The only difference is that one of the maps sets the coordinates manually, while the other centers automatically on the content. The data is apparently the same. What probably happens is that the second <mapframe> adds the same data a second time.

This is very annoying to reproduce, because it seems that it depends on ExternalData rendering in an annoying way. In particular, copy-pasting the page into a sandbox (like https://en.wikivoyage.org/wiki/User:IHurbainPalatin_(WMF)/sandbox?useparsoid=1) does reproduce the double-shading... but it also does so in the legacy parser. And, the only difference between that sandbox and the page is that I passed the wikidata QID explicitly in the mapshape template :/

This pattern of repeating the mapframe and the mapshape is also visible here: https://en.wikivoyage.org/wiki/Albufeira with the same effect.

I also can't say that I'm super convinced that the legacy parser is doing the right thing on these pages.

We're displaying the same mapframe twice (second time with 0 parameter), and we're adding the mapshape twice to that map. My very quick tests on my non-reproducing samples seem to point in the direction that a single mapshape is enough for this overlay to be displayed.

I would REALLY LIKE to understand *why* it's only rendered once on legacy, and only if not relying the QID associated to the page, but I'm starting to be out of ideas.

Not sure if my partly uninformed poking in the dark is helpful. Let's see.

I find it important to note that this happens only on Wikivoyage where $wgKartographerWikivoyageMode is enabled and users can specify group="…" and show="…" attributes. Note how the code allows groups with the same name to be merged. Without this array_merge there would be no duplicates. But this array_merge is intentional and correct. Maps on Wikivoyage are supposed to behave like that.

I also found https://gerrit.wikimedia.org/r/735446 where we added a fail-safe that makes this impossible on non-Wikivoyage wikis. See T147608. Simply having the same map twice on a page apparently caused the same dark shading we see here.

My current suspicion is that the behavior we see in the new Parsoid code is actually the correct one (well, at least for this specific example page), and instead it's the old, legacy code that's behaving bad. Could that be? 😵‍💫️

I tend to agree on "the legacy code is the one behaving weirdly", mostly because it seems to only trigger in very, very specific instances. I'll poke again a bit because I would really like to understand why :D

Well this was a fun rabbit hole to fall into. Parsoid indeed does the right thing here, and the legacy output is incomplete. But the issue is not in Kartographer per se, it's in the interplay between templates and Kartographer.

When {{mapshape}} is called without template arguments, its call is cached, and retrieved in PPTemplateFrame_Hash::cachedExpand (in core). This means that the extension code is actually not called the second time it is called on the page - and hence, Kartographer\State::addData(from the legacy implementation of Kartographer) is not called.
In contrast, during a Parsoid parse, the template call is also resolved from the cache, BUT the filling in of the extensionData is done as a post-processing pass, from the data that is generated during the template extension (which, in Parsoid's case, contains all the data required to be added to the page; the fact that it gets cached does not change that fact).

When calling {{mapshape|Q261577}}, the template result is not pulled from the template cache, it is recomputed, and the shape is added twice in the legacy parser too.

Incidentally, I finally reproduced it on my local wiki, while still avoiding Wikibase shenanigans, by modifying my local Template:Mapshape to provide a default QID for the map, which enabled me to pass the template without arguments.

Some investigation about the expected number of affected pages, on wikis that have an equivalent to this specific template:

  • bnwikivoyage 0 page
  • dewikivoyage 0 page
  • enwikivoyage 7 pages (Albufeira, Bristol, Colorado Springs, Erongo, Glacier Bay National Park, Olney, Wikivoyage:Portugal Expedition)
  • eswikivoyage 0 page
  • fawikivoyage 0 page
  • fiwikivoyage 0 page
  • frwikivoyage 0 page
  • hewikivoyage 0 page
  • hiwikivoyage 0 page
  • itwikivoyage 0 page
  • jawikivoyage 5 pages (岐阜県, インド, 京都市, 生駒市, タリン)
  • ptwikivoyage 0 page
  • trwikivoyage 0 page

This investigation was done on wikivoyage wikitext dumps with

$ cat ../<wikivoyagedump>.xml | node index.js -i "{{mapshape}}(?:.|\n)*{{mapshape}}" -l

and the equivalent for translated template names.

Affected pages were modified. Closing the issue.