Page MenuHomePhabricator

Graphs are broken by MobileFrontend's lazy loading image transformer
Closed, ResolvedPublic3 Estimated Story Points

Description

Certain graphs fail to display at all on mobile as the lazy loading images code breaks them.

For example: https://fr.m.wikipedia.org/wiki/Liste_de_sondages_sur_l'%C3%A9lection_pr%C3%A9sidentielle_fran%C3%A7aise_de_2017#.C3.89volution_des_enqu.C3.AAtes

We're using the graph extension (I think) on a meta page. There are 2 charts that have been generated. Both are too wide for mobile viewing (not responsive) and one of them I can't seem to get to scroll sideways on iPhone 6.

https://meta.wikimedia.org/wiki/New_Readers/Research_and_stats [Link no longer relevant]

Given the audience for this work, we're committed to mobile-first design and display. It would be great to use the extension if it'll work for this. If not, we can create some images and drop them in.

Thanks!


https://fr.m.wikipedia.org/wiki/Liste_de_sondages_sur_l%27élection_présidentielle_française_de_2017

Developer notes

configure

$wgMFContentProviderClass = 'MobileFrontend\ContentProviders\MwApiContentProvider';
$wgMFMwApiContentProviderBaseUri = "https://m.mediawiki.org/w/api.php";

Visit Extension:Graph/Demo locally in mobile.
open dev console and run:

$('.mw-graph-img').each((i, node) => { node.setAttribute('src', 'https://m.mediawiki.org' + node.getAttribute('src')) })

Notice how img doesn't appear.

Apply patch and repeat the steps and confirm they appear

QA steps

QA Results - Beta - 04/05/2020

ACStatusDetails
1T133085#6030671
2T133085#6030671

QA Results - Prod - 04/09/2020

ACStatusDetails
1T133085#6045513
2T133085#6045513

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added subscribers: TerraCodes, Aklapper. · View Herald TranscriptApr 19 2016, 6:32 PM

@Yurik Wanted to check in on this. We'll have to stop using the extension if there's no fix in sight.

@atgo, my apologies, I accidentally had blocked all Phabricator notification emails, and only just reenabled them. Graphs must work for mobile, and if they are not, I have to find a way to fix them. Let me see what i can do there. Sadly MediaWiki-extensions-Graph is still my pet project without any formal WMF support, so I will have to do a bit of priorities juggling :)

@JGirault @Jdlrobson @Esanders know much more about mobile rendering and might give a good suggestion - the first graph has this HTML structure. The image gets right-aligned without being able to scroll horizontally.

<div style="float: right">
 <div class="mw-graph" style="min-width:350px;min-height:250px">
  <img class="mw-graph-img" src="/api/rest_v1/page/graph/png/New_Readers%2FResearch_and_stats/15545294/e7d4be07f364b19148190c3a15f6c162dc44679f.png">
 </div>
</div>

Thanks @Yurik! Really appreciate you looking into it. It's a really handy
feature :)

One thing that might be good, which I know is a bigger undertaking that
would likely require more formal WMF design support, is to restyle the
graphs to be more mobile friendly, too. There's some great examples in this
medium post:
https://medium.com/@markham/don-t-teach-data-journalism-without-teaching-mobile-first-design-2e31a850af6e#.cdk0dwrqn

jrbs added a comment.Apr 25 2016, 5:48 PM

For what it's worth, I have a couple graphs whose mobile functionality could be tested, interactive and static: https://meta.wikimedia.org/wiki/User:JSutherland_(WMF)/sandbox2

Yurik added a comment.Apr 25 2016, 6:27 PM

@atgo, thanks for the great article, and I agree it is a slightly bigger issue - it's a difference between tooling and use of those tools. The <graph> allows you to pick a canvas rectangle (width x height), and draw anything you want on it. It has no notion of mobile vs desktop, just the pixel size (just like an image really). The html generated from it could be a bit more mobile friendly, allowing proper scrolling, etc, but it is not fundamentally changing anything.

On the other hand, we could design much better graph templates and encourage people to use them, creating a guideline on how to use them for each different use case. But yes, that will be a challenge :)

With regards to graphs on mobile there are 2 things to think about

  1. How it looks on a mobile device
  2. Not shipping too much data / bloating the initial page load
  3. Test on a real device.

I haven't looked too much at graphs but you'll want to make sure an width/float css rules are done in stylesheets via classes rather than inline styles.

In terms of not shipping too much data you'll probably want to serve a jpeg by default that when clicked loads the JavaScript needed to run and makes the graph interactive. You wouldn't want to serve the entire library by default. From a quick glance it looks like you'll doing this.

Practically you'll need to ensure any ResourceLoader modules have 'targets'=>['desktop','mobile' ] if they are designed to work on mobile but you must not do this blindly. Be sure to test on a device and review any libraries to check if they are meant to be used on mobile. If you are using a library in core and it is disabled consider not using it and using something else - there may be a good reason it's not enabled!

In terms of #1, also remember that you cannot know the device width up front. You'll probably want to set width: 100%; height: auto;

For responsive divs maintaining aspect ratio you could use the same CSS I've written for maps: https://gerrit.wikimedia.org/r/#/c/285648/

So, what's the status on fixing these?

Yurik added a comment.Aug 3 2016, 6:58 PM

Not much yet -- could use some help, and I will need to look at it some more once back from vacation.

Yurik added a comment.Dec 21 2016, 6:52 AM

@Esanders @Jdlrobson could you take a look at T147768 to see if the frame implementation would work on a mobile?

TheDJ added a subscriber: TheDJ.Jan 12 2017, 8:33 PM

Some of these problems seem easily fixable with something along the lines of:

@media only screen and (max-width:500px) {
	@supports (object-fit: contain) {
		div.mw-graph {
			width: 100% !important;
			min-width: auto !important;
			min-height: auto !important;
		}
		div.mw-graph canvas,
		div.mw-graph .mw-graph-img {
			width: 100% !important;
			height: 100% !important;
			object-fit: contain;
			max-height: 50vh;
			max-width: 100%;
		}
	}
}

Although it doesn't work for interactive graphs, since the mouse position for these doesn't seem to take into account the computed width and height, but is looking directly at an attribute or initialization value or something (probably requires fixing the vega JS library).

I'm wondering about those min-width's on mw-graph. Why were they added ?

TheDJ removed Yurik as the assignee of this task.May 17 2017, 11:28 AM
TheDJ added a subscriber: Yurik.

unassigning from yurik, don't think he is actively working no this.

Mhurd added a comment.EditedJun 6 2017, 1:56 AM

@TheDJ

I'm wondering about those min-width's on mw-graph. Why were they added ?

Ya on the ticket I just closed as a dupe we were wonder about that and the graph line widths too.

Jdlrobson moved this task from Needs triage to Triaged on the Mobile board.Jul 19 2017, 1:17 AM
Jdlrobson updated the task description. (Show Details)Apr 1 2020, 9:42 PM
Jdlrobson renamed this task from Graphs are not mobile friendly to Graphs are broken by MobileFrontend's lazy loading image transformer.Apr 1 2020, 9:55 PM
Jdlrobson edited projects, added MobileFrontend; removed MediaWiki-extensions-Graph.

I've looked into this and it appears the lazy loading images is the problem here.

T133085
The offending code is https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/src/mobile.startup/lazyImages/lazyImageLoader.js#L37 which sets width and height to 0. We shouldn't be setting those if that's the case.
That fix would make the graphs show which is something (and a super trivial change). The resulting graph won't be mobile friendly but the fixes for that are already captured in T207929 and T197188.

@ovasileva this seems like an extremely small and impactful change that we could make.

Jdlrobson updated the task description. (Show Details)Apr 1 2020, 10:01 PM
ovasileva triaged this task as High priority.Apr 2 2020, 8:51 AM

I've looked into this and it appears the lazy loading images is the problem here.

T133085
The offending code is https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/src/mobile.startup/lazyImages/lazyImageLoader.js#L37 which sets width and height to 0. We shouldn't be setting those if that's the case.
That fix would make the graphs show which is something (and a super trivial change). The resulting graph won't be mobile friendly but the fixes for that are already captured in T207929 and T197188.

@ovasileva this seems like an extremely small and impactful change that we could make.

Agreed - let's take a look later today

TheDJ awarded a token.Apr 2 2020, 9:28 AM

Change 585539 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Do not run the lazy image formatter on images which don't have dimensions

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

Change 585539 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix graphs: Do not run the lazy image formatter on images which don't have dimensions

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

Jdlrobson assigned this task to Edtadros.Apr 3 2020, 8:47 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: polishdeveloper.

(Thanks for the merge @polishdeveloper !)

Edtadros reassigned this task from Edtadros to Jdlrobson.Apr 6 2020, 1:17 AM
Edtadros added a subscriber: Edtadros.

Test Result

Status:
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPhoneX

Test Artifact(s):

QA Steps

✅ AC1: Visit https://m.mediawiki.org/wiki/Extension:Graph/Demo on a mobile device and confirm graphs are not loading.

❓ AC2: Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Extension:Graph/Demo on a mobile device and confirm the graphs are showing (ignore red links - this doesn't mean the graph isn't showing)

The following graphs are broken.

Edtadros updated the task description. (Show Details)Apr 6 2020, 1:18 AM
Jdlrobson added a project: User-notice.

Also broken on desktop (https://en.wikipedia.beta.wmflabs.org/wiki/Extension:Graph/Demo#Using_RESTBase_API) so I think this is unrelated. Looks good to me.

Seems suitable for tech news as editors should know their graphs will now work in mobile and weren't before.

ovasileva set the point value for this task to 3.Apr 6 2020, 5:12 PM
ovasileva moved this task from Incoming to Product Doing on the covid-19 board.Apr 7 2020, 3:21 PM
ovasileva reassigned this task from Jdlrobson to Edtadros.Apr 7 2020, 5:09 PM

Test Result

Status:
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPhoneX

Test Artifact(s):

QA Steps

✅ AC1: Visit https://m.mediawiki.org/wiki/Extension:Graph/Demo on a mobile device and confirm graphs are not loading.

Edtadros reassigned this task from Edtadros to ovasileva.Apr 10 2020, 5:33 AM
Edtadros updated the task description. (Show Details)
ovasileva closed this task as Resolved.Apr 10 2020, 9:32 AM

Looks good!

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptApr 10 2020, 9:32 AM
Ainali added a subscriber: Ainali.Apr 27 2020, 8:26 PM