Page MenuHomePhabricator

Issues with loading user common/minerva scripts in mobile site
Closed, ResolvedPublic

Description

  • User:<name>/common.css and User:<name>/common.js loading in Minerva skin
  • User:<name>/common.js is loading on the Minerva skin. This shouldn't be the case. Only User:<name>/mobile.js should load.
  • In mobile, Special:MyPage/minerva.js is loading twice. It should only load once.

Possibly related to T147755

Details

Related Gerrit Patches:
mediawiki/extensions/MobileFrontend : masterRemove mobile user modules

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 26 2017, 10:02 PM
ashley added a subscriber: ashley.Jan 26 2017, 10:41 PM

Which one do you mean, common or global CSS & JS? They are two separate concepts, of which the latter is provided by an extension, and I'd argue that it certainly makes sense to load global CSS & JS on all skins, since, well, that's kinda the definition of global.

Nirmos added a subscriber: Nirmos.Jan 27 2017, 6:54 AM

common.js is loaded on mobile. global.js isn't. Related:
T155304
T147755

Which one do you mean, common or global CSS & JS?

Oops. Sorry, I am talking about Common files.

Iniquity renamed this task from Global CSS and JS loading in Minerva skin to Common CSS and JS loading in Minerva skin.Jan 27 2017, 2:30 PM

With respect to the confusion around the name MediaWiki:Common.css - yes it's name would suggest it would load on all skins, but sadly this was written at a time before mobile so many MediaWiki sites have content in here which is not applicable to mobile devices. Hence we created Mobile.css (note Common files will work on the desktop version of the mobile skin).

This seems to be working as expected to me @Iniquity
can you provide more information please?

MediaWiki:Common.css does not load on the Minerva skin for me.
See https://en.m.wikipedia.beta.wmflabs.org/wiki/MediaWiki:Common.css:

.skin-minerva #mw-mf-page-center {
	background-color: red !important;
}

This would apply if it was loaded on the mobile skin.

Likewise a console.log from https://en.m.wikipedia.beta.wmflabs.org/wiki/MediaWiki:Common.js would show in mobile if it was being loaded on mobile.

You are confusing MediaWiki:Common.js with Special:MyPage/common.js

Jdlrobson renamed this task from Common CSS and JS loading in Minerva skin to Special:MyPage Common CSS and JS loading in Minerva skin.Jan 30 2017, 4:40 PM

I dont know what Special MyPage is. What extension provides this? This would be an issue with that.. Not mobilefrontend. We are not purposely disabling it.

Special:MyPage goes to the user's own user page, so Special:MyPage/common.js goes to the user's own common.js subpage, which would be User:Nirmos/common.js for me on enwiki.

Jdlrobson renamed this task from Special:MyPage Common CSS and JS loading in Minerva skin to Issues with loading user scripts/.Feb 14 2017, 5:57 PM
Jdlrobson renamed this task from Issues with loading user scripts/ to Issues with loading user common/minerva scripts in mobile site.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a project: Readers-Web-Backlog.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: matmarex.
ovasileva triaged this task as Normal priority.Feb 22 2017, 7:43 PM

This sounds like a ResourceLoader bug.

@Krinkle any ideas what might be going on here?

I already explained what is happening in the task marked as duplicate: T155304#2953862. Unless this is actually a different issue, it's MobileFrontend not playing well with ResourceLoader, rather than a ResourceLoader bug.

MobileFrontend has its own 'mobile.usermodule', loaded in SkinMinerva, which loads this script. That's the first one you're seeing.
I don't see anything in there to prevent the core 'user' module from being loaded (which loads the User:<skin>.js for the current skin, which is Minerva on mobile). I suspect that results in it being loaded again.

The reason for this is probably change 06ab9c0942f66317b20130ed12e677607783a61e in MediaWiki core, which allowed the 'user' module to load on mobile sites.

'mobile.usermodule' (MobileUserModule class) is documented as follows:

/**
 * Alternative of ResourceLoaderUserModule for mobile web.
 * Differs from the user module by not loading common.js,
 * which predate Minerva and may be incompatible.
 */

However, I can see my own common.js being loaded on the mobile site. If my guess about the reason is correct, this has been happening for half a year now, and I haven't heard anyone complaining about this. I think the solution here is simply to remove 'mobile.usermodule' (and 'mobile.usermodule.styles').

Krinkle updated the task description. (Show Details)Apr 14 2017, 8:31 PM

Basically, we have two options:

  1. Restore previous status quo: Rewrite mobile.usermodule to match current core's user and user.styles modules, but omit common.js,css again. And then somehow load these instead of user/user.styles. This would require non-trivial addition of workarounds and would add significant technical debt.
  2. Reduce mobile.usermodule to only providing User "mobile.js" and "mobile.css" and let core load the normal "user" modules (User common.js,css and minerva.js,css). In that case, mobile must also remove their override of site so that MediaWiki:Common.js/css load normally. The absence of that module currently breaks user scripts as a user's common.js may call functions provided by the site's MediaWiki:Common.js - ResourceLoader explicitly provides this guarantee of execution order, despite loading async.

I think it's time to create a clear and simple roadmap that concludes with loading Common.js and Common.css the "normal" way. What are the requirements? Let's identify what actual performance or UX problems loading the "site" module would cause, and address them once and for all.

Their absence continuously adds technical debt and maintenance overhead for both end-users and developers alike. I believe the original justification for this setup was site performance. As member of the Performance Team, I disagree with this approach. It costs too much for too little return.

We can't operate a unique platform like MediaWiki without given our users the trust, respect, and the ability to do it "right". Just like other teams can't curate everything manually for Vector skin users, or users using assistive technology, or users of IE10, Mac users, etc. (See also T127268) We have to trust developers and users to be able to produce code with all these platforms in mind. It requires education, documentation, patience, trust - all the while keeping an eye out for issues and being there to address them.

Change 348480 had a related patch set uploaded (by Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Remove mobile user modules

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

Jdlrobson raised the priority of this task from Normal to High.Apr 17 2017, 3:57 PM
Jdlrobson moved this task from 2014-15 Q4 to Triaged but Future on the Readers-Web-Backlog board.

Thanks for re-sharing that @matmarex I missed it the first time.

@Krinkle Essentially the whole point of MobileFrontend was to provide a different sandbox for desktop/mobile devices and to allow site admins to provide different experiences on the mobile domain. The goal was to appeal to any of the crowd who dislike responsive sites and prefer separate mobile/desktop experiences.
With that in mind, the MobileUserModule was meant to ship mobile.js but not ship the user's common.js or common.css (as common.js was written for desktop users).

With regards to the roadmap, the real question from my perspective is whether there is value in having a "common" js file and whether code sharing could be done in a better way. As far as I can see it exists as a shorthand to apply the same code to Monobook and Vector. I don't see much value in sharing JS/CSS in this way when you throw in a mobile skin. I'd rather see de-duplication via @import statements and remove the common file altogether. To take a concrete example: Hovercards - this is a desktop specific feature. It makes no sense on mobile where user's do not use a mouse. How would we load this via site/user scripts in Vector and Monobook but not Minerva?

With regards to this particular bug, it feels a little like the forced solution is for mobile to use User common.js and drop mobile.js entirely. I don't like this as it causes confusion given the site modules don't behave this way, but I don't see a cleaner solution. There is little value in having both mobile.js and common.js. Patch is above.

Change 348480 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove mobile user modules

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

Jdlrobson moved this task from Needs Analysis to Ready for Signoff on the Reading-Web-Sprint-96 board.

Let me just check this is loading as expected...

Jdlrobson closed this task as Resolved.Apr 24 2017, 11:50 PM

LGTM