Page MenuHomePhabricator

MobileFrontend: webapp-manifest is missing icon size
Closed, ResolvedPublic2 Estimated Story Points

Description

Running Lighthouse on https://en.wikipedia.org/wiki/Main_Page showed the following warning:

c.png (318×814 px, 21 KB)

Closer inspection shows that the "size" attribute is actually missing entirely.

https://en.m.wikipedia.org/w/api.php?action=webapp-manifest

manifest.json
{"name":"Wikipedia","orientation":"portrait","dir":"ltr","lang":"en","display":"browser","theme_color":"#252525","background_color":"#FFFFFF","start_url":"/wiki/Main_Page","icons":[{"src":"/static/apple-touch/wikipedia.png"}]}

I ran a quick test by manually stepping through the PHP code that creates this manifest in production and indeed, it does not work. The problem is that the url is expanded to HTTP instead of HTTPS, and (by default) no redirects are followed.

MobileFrontend/ApiWebappManifest actually doesn't expand the url properly in the first place, it uses PROTO_RELATIVE - which is invalid given that we're making a request from PHP (protocol-relative urls only make sense when used in by browsing contexts), there is no relative context for PHP. MWHttpRequest tolerates this error by re-expanding the url a second time using PROTO_HTTP as deterministic fallback.

As such, the request is a "success" but without any body content (it got a HTTP 301 redirect response).

$appleTouchIcon = $wgAppleTouchIcon;
// From ApiWebappManifest.php
$appleTouchIconUrl = wfExpandUrl( $appleTouchIcon, PROTO_RELATIVE );
$request = MWHttpRequest::factory( $appleTouchIconUrl ); $status = $request->execute(); $appleTouchIconContent = $request->getContent();
return [ $status->isOK(), strlen($appleTouchIconContent) ];
/* array(2) {
  [0]=>
  bool(true)
  [1]=>
  int(0)
} */

// HTTP (same result but explicitly, instead of indirectly via MWHttpRequest)
$appleTouchIconUrl = wfExpandUrl( $appleTouchIcon, PROTO_HTTP );
$request = MWHttpRequest::factory( $appleTouchIconUrl ); $status = $request->execute(); $appleTouchIconContent = $request->getContent();
return [ $status->isOK(), strlen($appleTouchIconContent) ];
/* array(2) {
  [0]=>
  bool(true)
  [1]=>
  int(0)
} */

// HTTPS (using PROTO_CANONICAL or PROTO_HTTPS)
$appleTouchIconUrl = wfExpandUrl( $appleTouchIcon, PROTO_CANONICAL );
$request = MWHttpRequest::factory( $appleTouchIconUrl ); $status = $request->execute(); $appleTouchIconContent = $request->getContent();
return [ $status->isOK(), strlen($appleTouchIconContent) ];
/* array(2) {
  [0]=>
  bool(true)
  [1]=>
  int(3248)
} */

It was last changed in 676b75aacea for (T153250), but most likely it didn't work prior to this, either.

Event Timeline

ovasileva added a project: Readers-Web-Backlog.
ovasileva moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
ovasileva set the point value for this task to 2.Feb 2 2017, 5:52 PM

Bump. Icon size is still missing and failing on the Lighthouse report.

I want to start with this Can you give me the details how to proceed

^ /cc @Jdlrobson – not because you're "the manifest person" but because you're the TL and might be able to give a few pointers.

Moving this to Triaged but Future as it's been in Sprint +1 for over two months.

Hi @Rammanojpotla sorry I somehow missed this ping. Are you still keen to work on this? I'm not sure how to expand on this.. the description from @Krinkle is pretty complete but the code in question can be found here: https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/includes/api/ApiWebappManifest.php - understanding what's happening there would be a good starting point!

@Jdlrobson thanks for asking but presently I am not working on this issue

Jdlrobson lowered the priority of this task from High to Medium.Apr 13 2017, 7:04 PM
Jdlrobson renamed this task from MobileFrontend: webbapp-manifest is missing icon size to MobileFrontend: webapp-manifest is missing icon size.Aug 15 2017, 5:10 PM

Change 372167 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Use proto current not proto relative

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

Seems to be a problem with the URL being passed to getimagesizefromstring not resolving. https://gerrit.wikimedia.org/r/372167 should fix that.

Change 372167 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Use proto current not proto relative

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

Width/height still showing on https://en.m.wikipedia.beta.wmflabs.org/w/api.php?action=webapp-manifest
Not showing on ehttps://www.mediawiki.org/w/api.php?action=webapp-manifest
We'll have to wait for that change to go out or swat it to check if it fixes the problem!

Sign off is blocked until tomorrow at which point we can check https://www.mediawiki.org/w/api.php?action=webapp-manifest to verify the fix.

Fix has been verified:
https://www.mediawiki.org/w/api.php?action=webapp-manifest now shows the sizes of the image.

Didn't @pmiazga say that he was going to sign this off? 😉