Page MenuHomePhabricator

Lazy loading code should be skin agnostic
Closed, ResolvedPublic

Description

Currently skins need to implement the JS for lazy loading themselves.
e.g. Minerva :
https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/resources/mobile.startup/Skin.js#L81

As a result images do not load in other skins when they run in mobile mode
e.g. Vector
https://en.m.wikipedia.org/wiki/Jennifer_Lawrence?useskin=vector

The HTML markup comes from MobileFrontend and MobileFormatter.

To get lazy loading images working a skin needs to run the following code:

mw.loader.using('mobile.startup').then(()=> {
  var M = mw.mobileFrontend;
  var Page = M.require( 'mobile.startup/Page');
  var Skin = M.require('mobile.startup/Skin');
  new Skin( { el: 'body', page: new Page( { namespaceNumber: mw.config.get( 'wgNamespaceNumber' ) } ), tabletModules: [] } );
})

This is highly unreasonable.

Proposed solution

The MobileFrontend extension will register a lightweight mobile.init module which runs script for all skins operating in mobile mode. It will provide an instance of a skin and will provide the method mw.mobileFrontend.getCurrentPage that is currently inside MinervaNeue.

MinervaNeue will no longer create its own instance of a skin, instead making use of the skin created inside the new mobile.init module.

Open questions

While working on this I hit one snag: https://gerrit.wikimedia.org/r/376553

  • When we port this over we'll need to make a decision about the tabletModules. Currently the Skin object is responsible for loading certain modules on demand. If the Skin is defined inside MobileFrontend, then Minerva has no way to communicate what modules it needs to load. We'll need to create that mechanism using mw.config or mw.hook. I personally think this code is a burden that provides very little value and I propose we remove it and merge the skins.minerva.tablet.scripts with skins.minerva.scripts per T167713 to make this problem go away.

Acceptance criteria

Related Objects

StatusSubtypeAssignedTask
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJdlrobson
DeclinedNone
ResolvedJdlrobson
Resolvedphuedx
ResolvedJdlrobson
ResolvedJdlrobson
DuplicateSpikeNone
ResolvedJdrewniak
DeclinedNone
ResolvedJdlrobson
Resolved Jhernandez
ResolvedJdrewniak
Resolvedpmiazga
ResolvedJdrewniak
ResolvedJdlrobson
ResolvedJdrewniak

Event Timeline

Change 376557 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Load mobile.init module for all mobile skins

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

Jdlrobson changed the task status from Open to Stalled.Sep 7 2017, 6:35 PM

I've written a proof of concept patch, but while doing this I uncovered the fact that this is blocked by T167713.
I have updated the description with a proposed solution.
I think this is ready to discuss/estimate at our convenience.

Change 376555 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Hygiene: Minerva uses skin instance defined by MobileFrontend

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

Change 383720 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Minerva is responsible for loading tablet modules in tablet mode

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

Change 376553 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] M.getCurrentPage and skin should be initialised by MobileFrontend

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

Change 383724 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Tablet modules no longer managed by MobileFrontend

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

Change 383720 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Minerva is responsible for loading tablet modules in tablet mode

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

Change 383724 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Tablet modules no longer managed by MobileFrontend

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

Change 376553 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] M.getCurrentPage and skin should be initialised by MobileFrontend

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

Change 376555 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: Minerva uses skin instance defined by MobileFrontend

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

Change 376557 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Load mobile.init module for all mobile skins

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