Page MenuHomePhabricator

Graphs create img tags without width and height
Open, Needs TriagePublic

Description

Graphs using the <graph>-extension only show empty background in mobile view (both in Minerva and Vector). This seems to be limited to the article namespace, they do show in sandboxes and template pages. This is seen in at least svwp, enwp and dewp.

An example from the svwp article Nordbalt in mobile view shows <img width="0" height="0" ...:

<div class="mw-graph" style="min-width:600px;min-height:400px"><noscript>&lt;img class="mw-graph-img" src="/api/rest_v1/page/graph/png/Nordbalt/0/cadd362816f69c5f3d109bc9fe9a43a7120b0061.png"&gt;</noscript><img width="0" height="0" class="mw-graph-img image-lazy-loaded" alt="" src="/api/rest_v1/page/graph/png/Nordbalt/0/cadd362816f69c5f3d109bc9fe9a43a7120b0061.png" srcset=""></div>

The width and height properties should be copied to the image. Either that fails or they are replaced by "0".

Acceptance criteria

  • An explicit width needs to be added on the graph img tag.

e.g

<div class="mw-graph" style="min-width:600px;min-height:400px"><img class="mw-graph-img" src="/api/rest_v1/page/graph/png/Nordbalt/0/cadd362816f69c5f3d109bc9fe9a43a7120b0061.png"></div>

becomes

<div class="mw-graph" style="min-width:600px;min-height:400px"><img class="mw-graph-img" width=600 height=400 src="/api/rest_v1/page/graph/png/Nordbalt/0/cadd362816f69c5f3d109bc9fe9a43a7120b0061.png"></div>

Developer notes

The img tag does not define width or height attributes
These are important to avoid reflows and to add compatibility with MobileFrontend's lazy image loading.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 18 2019, 6:15 PM
JohanahoJ updated the task description. (Show Details)Feb 18 2019, 6:17 PM
Larske added a subscriber: Larske.Feb 18 2019, 7:05 PM
Larske added a comment.EditedFeb 18 2019, 7:16 PM

This sounds similar to phab:T216318

JohanahoJ updated the task description. (Show Details)Feb 19 2019, 1:11 PM
JohanahoJ reopened this task as Open.Feb 19 2019, 6:16 PM
JohanahoJ added a comment.EditedFeb 19 2019, 6:19 PM

Reopened this task, because T216318 now again focuses on EasyTimeline.

JohanahoJ renamed this task from Graphs in article namespace do not show in mobile view to Graphs fail with MobileFrontend lazy loading.Feb 19 2019, 6:33 PM
Jdlrobson reopened this task as Open.Feb 19 2019, 9:34 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson renamed this task from Graphs fail with MobileFrontend lazy loading to Graphs create img tags without width and height.Feb 19 2019, 9:36 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Needs triage to Triaged on the Mobile board.Feb 22 2019, 12:07 AM

Is there any progress on this? It blanks out all lazy-loaded maps dependent on 'Graph:street map with marks', as well as other graph images. I think the lazy image loader may also be knocking out the GeoJSON markers on some <maplink> maps that include a 'marker-image', although the en.template version seems to be not affected. (Examples of various affected maps are at https://en.m.wikipedia.org/wiki/User:RobinLeicester/sandbox).

Apparently adding any Graph:Chart template in the article "fixes" the issue. I put {{Graph:Chart||type=pie||width=0||height=0||x=0||y=0}} in a random place in the article and the map (even OSM map had this problem) displays correctly. The template can be put anywhere in the article, be it in a section or not. I'm not an expert though, so I don't know why it is working.

Awesome. I have incorporated it below the map on OSM Location map, so it will be there on any page needed. Its a fudge, but a pretty good one! I am guessing Graph:Chart found a way to turn off the lazy loader.

Jc86035 added a subscriber: Jc86035.May 5 2019, 9:52 AM

The reason that it works is that Template:Graph:Chart/styles.css contains a CSS fix (thanks to @Yair_rand). It would be sufficient to include only the TemplateStyles CSS instead of the whole template.

.mw-graph-img {
	width: inherit;
	height: inherit;
}

Thanks, the CSS fix should also be added to {{Graph:Street map with marks}} (and any other similar map templates).

I am working in a bit of a fog of ignorance here, so will describe what I have done and if someone thinks it doesn't make sense, please sort it, or point me in a better direction:
The en.wikipedia.org/wiki/Template:Graph:Chart/styles.css had some additional css as well as the lines above, so I copied all of that into new styles.css files for both en: and mediawiki versions of {{Graph:Street map with marks}}. I then copied - again from en.wikipedia.org/wiki/Template:Graph:Chart - the instruction {{safesubst:#tag:templatestyles||src="Graph:Street map with marks/styles.css"}}, to invoke it. (no idea about safesubst but I assume it makes sense).
I am not familiar with how the mediawiki version gets applied across other languages, so have not done anything about that. Nor am I familiar with how to find which other Graph templates might need the fix (as opposed to ones that call other graph templates).

Lofhi added a subscriber: Lofhi.Jun 14 2019, 9:43 PM
stjn added subscribers: Yurik, stjn.Jun 15 2019, 9:19 AM

Judging by (git) blame, the code for width/height was added a long time ago by @Yurik. Maybe he can fill in about situations where min-width / min-height are preferable to explicit specification on <img>?

To fix this issue, one needs to do the following:

  1. Add width/height attributes to img in Graph extension (easy). This will make the images from Graphoid visible on mobile for every project, but they will usually not fit into the viewport.
  2. Figure out how graphs should look in MobileFrontend. Should they resize to fit the viewport and become less legible or should they be scrollable (achievable with a wrapper around .mw-graph and some basic styles)?

I would like to do a patch for fixing the first issue, since it affects many projects that don’t know about TemplateStyles hack, and hear the thoughts on the latter from MobileFrontend team and Yurik.

Example of article affected by this (since some links appear with hacks fixing the issue):
https://ru.m.wikipedia.org/wiki/Несвиж#Население

Change 517163 had a related patch set uploaded (by Saint Johann; owner: Saint Johann):
[mediawiki/extensions/Graph@master] Fix img attributes in Graphoid output

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

Yurik added a comment.EditedJun 15 2019, 2:27 PM

@stjn this is a fairly complex issue. I have done a lot of work with the Vega lib since then, integrating it as part of Kibana (Elasticsearch), so I can elaborate on a "better" way to address it. I will try to be brief :) Also, take a look here on how I did it in Kibana.

First, a bit of a background:

  • Vega by default uses width/height json params as "non-final" -- instead they are used as a guide for the size of the initial drawing canvas, but they get extended by the size of the axis labels, padding, legend boxes, etc. It is not possible to compute the size of the final graph by looking at the initial json (unless you re-implement all of the internal Vega logic). Using width/height as the image size will make the graph look very strange, unless used only for "minwidth/minheight".
  • Vega supports a different layout model - autosize: { type: 'fit', contains: 'padding' }. In this model Vega will fit everything inside the given width/height, reducing the effective drawing area until all labels, axis, and legends fit.

Preferred solution:
For the next version of Vega (3+), I would recommend to adapt the same code as I did for Kibana. Note that it is not possible to do it for the current Vega version because it would break the existing graphs.

  • Both graphoid and graph ext will automatically add the autosize: { type: 'fit', contains: 'padding' } to the vega json unless it already has autosize param
  • if autosize is not set or set to the above, use width/height json values to generate HTML <img> tag width/height. If the autosize model is different, then min-width / min-height should be used instead.

Hope this helps.

stjn added a comment.Jun 15 2019, 2:58 PM

Ah, you’re right. Noticed through the testing on desktop that width and height of <img> and container are different even for simplest images. To be honest, it’s a really strange technical decision that, I guess, invalidates my patch.

  • if autosize is not set or set to the above, use width/height json values to generate HTML <img> tag width/height. If the autosize model is different, then min-width / min-height should be used instead.

Is there some way to know and set correct width and height in any circumstance, even if autosize will be different? Relying on min-width / min-height wouldn’t be enough for mobile even with the hack mentioned above (width: inherit one), since it will introduce the same issue you have laid out in your comment, but on mobile.

Yurik added a comment.Jun 15 2019, 3:35 PM

@stjn I don't think there is any reliable way to compute the image height/width ahead of time without going through the full graph rendering. PHP could call out to Graphoid during the post-save, but that may require even more work than simply updating to Vega 3+ and implementing the above logic (defaulting to autosize=fit model).