Page MenuHomePhabricator

Kartographer maps with "type": "Feature" and a image in the description is not shown in mobile view
Closed, ResolvedPublicBUG REPORT

Description

At dawiki it was reported that the page https://da.wikipedia.org/w/index.php?title=Oldtidsstien&oldid=9924117 doesn't show the Kartographer map in mobile view.

It seems that it is caused by code like in the example below with an image link in the description of a feature:

{
    "type": "Feature",
    "geometry": { "type": "Point", "coordinates": [10.22919, 56.08545] },
    "properties": {
    "title": "Stenkister",
    "description": "[[File:Stenkiste.jpg|200px]]",
    "marker-symbol": "star",
    "marker-size": "small",
    "marker-color": "0050d0"
  }
  },

If the description is changed to a text without the image link, the map be shown correct in mobile view.

The same can be seen in an example in the help page for Kartographer. The code:

<mapframe text="San Francisco museums" width="350" height="350" zoom="13" latitude="37.81032643553478" longitude="-122.39953994750977">
{
  "type": "Feature",
  "geometry": { "type": "Point", "coordinates": [-122.3988, 37.8013] },
  "properties": {
    "title": "[[wikipedia:Exploratorium|Exploratorium]]",
    "description": "[[File:Giant_Mirror_at_the_Exploratorium.jpeg|200px]]",
    "marker-symbol": "museum",
    "marker-size": "large",
    "marker-color": "0050d0"
  }
}
</mapframe>

is shown in normal view at https://www.mediawiki.org/wiki/Help:Extension:Kartographer#<mapframe>_usage

but it is not shown in mobile view at https://m.mediawiki.org/wiki/Help:Extension:Kartographer#<mapframe>_usage

Developer notes

  1. Ensure MobileFrontend is registered BEFORE Kartographer
  2. Use following config
wfLoadExtension('Kartographer');
$wgKartographerStaticMapframe = true;

Event Timeline

Jdlrobson subscribed.

(Web team do not maintain this extension)

The reason is obvious..

id == sha1( FormatJson::encode( $this->geometries, false, FormatJson::ALL_OK )

where geometries is the return value of the SimpleStyleParser, which interprets the results of the sanitized version of the features, contains rendered output, which differs between mobile and desktop target:

mobile:

<a href="https://en.wikipedia.org/wiki/Exploratorium" class="extiw" title="wikipedia:Exploratorium">Exploratorium</a>
<a href="/wiki/File:Giant_Mirror_at_the_Exploratorium.jpeg" class="image"><noscript>&lt;img alt="Giant Mirror at the Exploratorium.jpeg" src="//upload.wikimedia.org/wikipedia/commons/thumb/2/21/Giant_Mirror_at_the_Exploratorium.jpeg/200px-Giant_Mirror_at_the_Exploratorium.jpeg" decoding="async" width="200" height="226" data-file-width="960" data-file-height="1083"&gt;</noscript><img width="200" height="226" class="image-lazy-loaded" alt="Giant Mirror at the Exploratorium.jpeg" src="//upload.wikimedia.org/wikipedia/commons/thumb/2/21/Giant_Mirror_at_the_Exploratorium.jpeg/200px-Giant_Mirror_at_the_Exploratorium.jpeg" srcset="" style="width: 200px; height: 226px;"></a>

desktop:

<a href="https://en.wikipedia.org/wiki/Exploratorium" class="extiw" title="wikipedia:Exploratorium">Exploratorium</a>
<a href="/wiki/File:Giant_Mirror_at_the_Exploratorium.jpeg" class="image" title=""><img alt="Giant Mirror at the Exploratorium.jpeg" src="//upload.wikimedia.org/wikipedia/commons/thumb/2/21/Giant_Mirror_at_the_Exploratorium.jpeg/200px-Giant_Mirror_at_the_Exploratorium.jpeg" decoding="async" width="200" height="226" srcset="//upload.wikimedia.org/wikipedia/commons/thumb/2/21/Giant_Mirror_at_the_Exploratorium.jpeg/300px-Giant_Mirror_at_the_Exploratorium.jpeg 1.5x, //upload.wikimedia.org/wikipedia/commons/thumb/2/21/Giant_Mirror_at_the_Exploratorium.jpeg/400px-Giant_Mirror_at_the_Exploratorium.jpeg 2x" data-file-width="960" data-file-height="1083"></a>

I think this can be fixed by using a new parseroptions when rendering the content (so it doesn't use mobile) in SimpleStyleRenderer... That's what things like expandtemplates and apiparse do if I remember correctly.

TheDJ changed the subtype of this task from "Task" to "Bug Report".Mar 17 2020, 2:54 PM
TheDJ added a project: User-TheDJ.

Is it possible one of the issues in T229472 is breaking this?

Well its definitely related of course. Not sure id call it a bug. Mobile simply generates different output and Kartographer doesnt take that into account.

Change 585314 had a related patch set uploaded (by TheDJ; owner: TheDJ):
[mediawiki/extensions/Kartographer@master] Bypass mobilefrontend when parsing images

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

Hmm. This is harder than I figured..

First of all, I suspect MobileFrontend should be rewritten to stop using onPageRenderingHash and instead use ParserOptionsRegister (introduced in 1.30). Because now I depend on MobileFrontend and it's context to manipulate the output, whereas I should be able to just use ParserOptions directly.

But even then, that doesn't solve T246314: Map cannot be displayed when Simplified Chinese characters are present of course.. the

@TheDJ I dont' think this has anything to do with MobileFrontend.

I'm seeing srcset on both desktop and mobile HTML. My read of MobileFrontend's code is that this should not impact the map in any way. Srcset is only stripped for images with associated File pages. The only problem I'm seeing is that the map on the mobile domain is generating an invalid URI.

If I can change domain=da.wikipedia.org to domain=da.m.wikipedia.org in both the srcset on mobile and desktop I see the static image on mobile.
So I think something is wrong in the group/domain parameter logic here.

Mobile:

<img src="https://maps.wikimedia.org/img/osm-intl,13,56.0857,10.2351,222x200.png?lang=da&amp;domain=da.wikipedia.org&amp;title=Oldtidsstien&amp;groups=_d168f3a5c8d35d64693d372dac998c9b8b97ddc1" alt="" decoding="async" srcset="https://maps.wikimedia.org/img/osm-intl,13,56.0857,10.2351,222x200@2x.png?lang=da&amp;domain=da.wikipedia.org&amp;title=Oldtidsstien&amp;groups=_d168f3a5c8d35d64693d372dac998c9b8b97ddc1 2x" width="222" height="200">

Desktop:

<img src="https://maps.wikimedia.org/img/osm-intl,13,56.0857,10.2351,222x200.png?lang=da&amp;domain=da.wikipedia.org&amp;title=Oldtidsstien&amp;groups=_c4dfb5f90a7ac55e4a918ad43fa7cde19ee57c11" alt="" decoding="async" srcset="https://maps.wikimedia.org/img/osm-intl,13,56.0857,10.2351,222x200@2x.png?lang=da&amp;domain=da.wikipedia.org&amp;title=Oldtidsstien&amp;groups=_c4dfb5f90a7ac55e4a918ad43fa7cde19ee57c11 2x" width="222" height="200">

Note the difference in groups values but how domain is the same. Mobile should spit out the same group or update domain (not sure if that's possible with the parser).

Something like this might help?

$domain = $wgServerName;
+               if ( Extension::isLoaded( 'MobileFrontend' ) ) {
+                       $services = MediaWikiServices::getInstance();
+                       $ctx = $services->getService( 'MobileFrontend.Context' );
+                       if ( $ctx->shouldDisplayMobileView() ) {
+                               $domain = $ctx->getMobileUrl( $domain );
+                       }
+               }

See also T171398

@Jdlrobson This is not about the srcset that Kartographer adds for the static image. The srcset in question is of the image inside the description. That image, description and the entire json blob are what gets hashed with sha1 which is used as the groupid and why the groupids don't match on the two platforms.

I know, it's a shitty solution, we should have never allowed parsed wikicontent inside the graph to begin with, but since i don't have enough time to do an entire redesign of kartographer :(

Another idea I could explore is to sha1 hash the raw input or even the PST of the parser.

TheDJ ah okay. IS the bug for the static image captured anywhere? As that seems more of a pressing issue to me.

I'm actually unable to replicate this particular issue. Descriptions are showing up fine for me (this is on mobile).

Screen Shot 2020-04-02 at 8.25.57 AM.png (594×432 px, 218 KB)

In the mediawiki.org example when I view source I see the image here without srcset set but it doesn't seem to be impacted by the lazy loaded image code:

title=\"wikipedia:Exploratorium\"\u003EExploratorium\u003C/a\u003E","description":"\u003Ca href=\"/wiki/File:Giant_Mirror_at_the_Exploratorium.jpeg\" class=\"image\"\u003E\u003Cimg alt=\"Giant Mirror at the Exploratorium.jpeg\" src=\"//upload.wikimedia.org/wikipedia/commons/thumb/2/21/Giant_Mirror_at_the_Exploratorium.jpeg/200px-Giant_Mirror_at_the_Exploratorium.jpeg\" decoding=\"async\

What am I doing wrong? Can you help me replicate this locally so I can help you fix it?

Well the domain issue is of course broken due to T195494: Handle mobile domains in core. Actually if that could use m.mediawiki.org, it would actually work indeed as you noticed.

Kartotherian when retrieving the staticmap, uses the MediaWiki generated ID in the img src, and then makes an api mapdata call to retrieve the parsed result from MediaWiki, and uses that to render (and cache) the image requested. See:

https://m.mediawiki.org/w/api.php?action=query&prop=mapdata&titles=Help:Extension:Kartographer vs
https://www.mediawiki.org/w/api.php?action=query&prop=mapdata&titles=Help:Extension:Kartographer

As you can see this contains parsed output (but Kartographer will always use the desktop api call). The first entry in the result is from the mentioned fragment and you'll see that the difference is srcset. Initially (before I found the api result, and used Special:ExpandTemplates) I figured that the lazyload code was also involved, but it turns out it is not. This is actually not so strange, as the lazyloading code is a post rendering transformation of the HTML (and Kartographer uses 'blob' parsing, it doesn't do a full page parsing), but the srcset code is actually part of the parsing run (which is also why it set a flag on the hashkey of the parsed result) and DOES influence the outcome.

This might get 'fixed' if the current suggestion for T326147 is merged.

@thiemowmde and @Krinkle

I know of three ways that kartographer is currently broken, which get back to the parser output

  1. using [[File:|thumb]] in a description causes mobilefrontend's srcset to introduce variance
  2. language variants introduce character variance which we do not handle right now
  3. Using [[File:|thumb]], when you have a non-standard thumbsize introduces variance

Although I haven't tested all these 3 situations in a while (editors tend to remove the map when these problems are presented), it seems that at least the mobilefrontend problem still exists.

From all I read here it appears like there is a very simple solution: Make sure the rendering of descriptions (done in SimpleStyleParser) is always the same, ignoring skin, user language, and any other per-user setting. Why not just do that?

Another option is to change the sha1 hash calculation to be done on a more canonical representation of the GeoJSON, e.g. after most normalizations but before expanding any wikitext. But I'm not sure if we can make this consistently work and not run into different hashes exactly because of that.

Change 868429 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Kartographer@master] Add missing language support when loading .map pages from Commons

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

Change 887805 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Kartographer@master] [POC] Hash calculated from canonical, partly normalized GeoJSON

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

Copying here to merge conversations:

I'm failing to understand why this bug happens. While MF is doing something unusual here that would ideally be fixed to use modern ParserOptions or be removed (T326147), in general it seems fair that ParserOptions as a general concept exists and thus that the same page title doesn't contain HTML-identical content for everyone always.

If the data is stored agnostic of ParserOutput (e.g in page_props, like TemplateData and I think some of Maps maybe as well still?) then it should presumably be accessed agnostic of ParserOutput as well. It would be stored during the post-save canonical parser options links update and then reliably accessible as such (but indeed not match the options from the current request, but that's a functional trade-off).

If the data is stored specific to ParserOptions, then it needs to be accessed as such as well, e.g. in the same request using embedded mw.config data or some such (if needed by JS) or in an API request that carries the same request context but that's near impossible to do reliably across the board given (domain, session cookies, uselang/useskin) given e.g. MF having custom params like useformat= to complicate matters (and even with domain consolitation and UA-sniff, would /w/api.php vary by this? Or only /w/index|/wiki?). Something similar was attempted a few years ago with lazy-loading citations which was undeployed eventually given it didn't work or scale well in practice. Another aspect is revision ID, which can change between CDN caching the page and serving it to a user and the user then interacting with the maps to trigger it, and parsing old revisions in a deterministic way isn't something we support or would want to rely upon.

But.. curious, since it seems to not work in prod even for the ~50% of simple non-logged-in, no params, non-recently-edited article, where is this currently getting mixed up? The task suggests it uses the desktop domain - where is that logic? Couldn't that be a path URL instead?

Spot-checking the error log, this is a major cause of users seeing broken maps (there isn't enough identifying metadata on the log messages to give a concrete percentage however). The conversation in this task agrees with my understanding of the problem, that the mobile rendering of popup images results in different HTML which evaluates to a different hash, but the maps thumbnail snapshot server and mapdata API currently do not know about mobile rendering so are lacking a parameter to forward the variation.

Forwarding a "mobile" parameter to those two components would solve the problem here, while still allowing variation. I'll leave the decision to others: we should forward the variation if this is important for mobile users; and we should force non-mobile rendering at all times if the distinction is unimportant.

Make sure the rendering of descriptions (done in SimpleStyleParser) is always the same, ignoring skin, user language, and any other per-user setting. Why not just do that?

Some of these variations could possibly be suppressed, but others need to be forwarded and respected throughout the rendering pipeline. For example, language variant can render text in a totally different script (eg. Kurdish using Latin vs. Arabic letters) which will be unintelligible to many readers.

The suggestion by TheDJ and Thiemo to calculate a map group ID using a more stable version of the source text is promising, but let's move that part of the discussion to T329170.

Reading through T326147, I think this is going to happen--it would eliminate the variation in parser output. I'll set this task to block on that one.

Change 889948 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Kartographer@master] [POC] No per-user thumbsize when parsing wikitext in GeoJSON

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

Change 585314 abandoned by TheDJ:

[mediawiki/extensions/Kartographer@master] Bypass mobilefrontend when parsing images

Reason:

no longer required because of T326147

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

Reading through T326147, I think this is going to happen--it would eliminate the variation in parser output. I'll set this task to block on that one.

That is done, is this problem resolved now?

Reading through T326147, I think this is going to happen--it would eliminate the variation in parser output. I'll set this task to block on that one.

That is done, is this problem resolved now?

The example I created in one of the duplicates merged into this ticket seems to be working now. See desktop version vs mobile version from T329091: Mobile view seems to request wrong group ids in some cases. They show the same result now. So I guess the issue is resolved.

Edit:
Also the example at the very top of the ticket now works on desktop and mobile. And same goes for the example help page.

TheDJ claimed this task.

I confirm as well. Fixed as far as this issue.

Change 935699 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Kartographer@master] Add test case documenting current thumb size behavior

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

Change 935699 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Add test case documenting current thumb size behavior

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