I realised that such global variable is meant to be used in the future. The problem is shifted to includes/modules/MobileSiteModule.php, where I think getDependencies isn't called (while getPages is), although I can't figure out why.

Is it related that ResourceLoaderWikiModule, unlike similar classes, doesn't have a getDependencies method?

Sounds like sometimes the CSS is loaded, sometimes not and sometimes only some rules are loaded. This may lead to corrupted mobile visualization. Adding as subscribers people from previous task.

What is the problem? Its not clear from the description what is happening.

@Jdlrobson the main problem is that mobile style, i.e. Mediawiki:Mobile.css isn't loaded, thus breaking page style.

I'll look into this Monday, but yes it seems like the dependencies inside https://gerrit.wikimedia.org/r/#/c/420405/4/includes/modules/MobileSiteModule.php,unified are not being honoured for some reason.

As a workaround you can put this code into MediaWiki:Mobile.js to get this to work:

mw.loader.using('mobile.site.styles')

@Jdlrobson Thanks for the hint, I applied the fix.

@Jdlrobson I looked at this over the weekend. I believe the culprit is the isKnownEmpty() logic.

The wiki previously did not have a Common.js or Mobile.js page. As such, their mobile.site was previously loading with an empty script, and the CSS only. That's fine.

When we split the modules into mobile.site and mobile.site.styles (with the former depending on the latter), the former became empty. ResourceLoader would normally load both modules at once in the same batch just like before (given dependency resolution happens ahead of time). But this isn't working because WikiModule, unlike FileModule, has an isKnownEmpty() method that makes the module skip itself if none of its associated pages exist on the wiki.

This makes it so that modules like site.styles and mobile.site.styles, which require loading in a separate http request, only happen from the HTML if said feature is enabled (wgUseSiteCSS) and used (the on-wiki page exists).

The problem is that, while this works fine for style modules, it poses a problem for JS modules, as skipping them means we also don't look at the dependencies anymore.

As far as I can tell, the fix would be to not apply isKnownEmpty to dynamic loading, but only to hardcoded http links. I recall this being what it was designed for, so I'm unsure of as to why and how it got to apply to dynamic loading. But should be a simple fix.

Description edited - hopefully this is now more discoverable by keywords like 'Mobile.css'.

Thanks!

@Krinkle the mobile.site module in extension.json (https://gerrit.wikimedia.org/r/#/c/420405/4/extension.json) appears to be specifying its own dependencies which clobber the default module dependencies.

In T191596#4120670, @Jdlrobson wrote:

@Krinkle the mobile.site module in extension.json (https://gerrit.wikimedia.org/r/#/c/420405/4/extension.json) appears to be specifying its own dependencies which clobber the default module dependencies.

Yeah, that's a separate issue. That dependency should probably be moved to the MobileSiteModule class. However, note that the code being referred to here already didn't work. Even before this commit, mobile.site was using class MobileSiteModule, which extends WikiModule without implementing an dependencies option, and without a getDependencies implementation. So as far as I can tell, this has not been working for quite some time. It may be safest to simply remove it.

// Location: https://en.m.wikipedia.org/wiki/Main_Page

mw.loader.moduleRegistry['mobile.site']
//> {module: {…}, version: "1g4dpfx", dependencies: Array(1), group: null, source: "local", …}

.dependencies
//> ["mobile.site.styles"]

Change 425570 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Do not rely on dependencies to load mobile.site/mobile.site.styles

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

I've intentionally kept the fix for this as simple as possible ^

@Jdlrobson, it looks like there's an open question in code review as to how to proceed. Should we split this task so that the original tracks the revert and the split tracks the proper fix?

I'll fix the bug in core which is about as much effort as a revert, but without all the regressions.

But I'd like to have a failing test in RLClientHtml to work from. That should be easy to set-up, and I can help with that too (ping me on IRC).

Change 428545 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] isKnownEmpty modules with dependencies cannot be empty

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

Change 428545 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Consider having dependencies as non-empty in WikiModule::isKnownEmpty

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

Minor follow up in https://gerrit.wikimedia.org/r/425570 needs to be merged. Once that's done we can move this to QA.

Change 425570 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] mobile.site should not have dependencies

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

@Jdlrobson @Krinkle et al., thanks for fixing. However there's a little problem: the first patch was deployed with wmf.1, the other one will be in wmf.2. In the meanwhile, keeping the mobile.js as suggested above will load mobile style also in desktop version, while removing the manual RL call we still have the initial problem reported in this task. Could someone please take a look and deploy the last patch on wmf.1 if necessary?

@Daimona the first patch won't do anything. The second patch is the one that's needed.
The suggestion in https://phabricator.wikimedia.org/T191596#4114628 will load the styles in the mean time on the mobile. It will not load the styles in desktop - MediaWiki:Mobile.js is only loaded for mobile devices. You can use that workaround safely with or without the second patch. If you are seeing that JS running on desktop we have a bigger problem here! :)

@Jdlrobson I had applied the suggestion to manually call RL, but after yesterday's deployment of wmf.1 many users started complaining for wrong graphics. It turned out that the mobile style was loaded on desktop and deleting Mobile.js brought everything back to normal. Of course, without that manual call, mobile users still don't have the CSS. So, I'm afraid we actually have another problem here, although I'm not sure about how big it could be :-)

@Daimona Hmmm.. I'm not seeing how this could happen. I just verified now that it's impossible for the mobile script to load on desktop so I'm not sure what could have caused this.

If the Mobile.js page is restored and the issue returns I'll be happy to debug that and work out what's going on for you but it should be safe to restore that page. If not, we should identify the bug here and get that fixed immediately.

@Jdlrobson Huh, I restored Mobile.js and the problem is gone, both for desktop and mobile. Maybe it's been some cache-ish problem, but at least now everything's fine :-)