Page MenuHomePhabricator

ext.uls.interlanguage and ext.uls.pt should set targets to mobile and desktop but not ship on Minerva (and possibly loaded via JS)
Closed, ResolvedPublic

Description

The ext.uls.interlanguage module is a small style module that is added conditionally to all pages but is not used on the Minerva skin. It accounts for the majority of warnings in T235712. Fixing this should be relatively straightforward for someone who understands the code and will lead to a more performant site on the long term.

Acceptance criteria

  • The module should set "targets": [ "desktop", "mobile" ], so that it does not throw the warning described in T235712
  • The module is not loaded on the Minerva skin or ships an empty stylesheet (and since the stylesheet already contains a rule .skin-vector that can be offloaded to Vector specific)

Developer notes

It is added in includes/UniversalLanguageSelectorHooks.php.

if ( $wgULSPosition === 'personal' ) {
          $out->addModuleStyles( 'ext.uls.pt' );
  } else {
          $out->addModuleStyles( 'ext.uls.interlanguage' );
  }

Two possible solutions

  1. The check above should do one of one take into account the skin (which is provided by the addModule hook)
if (  ExtensionRegistry::getInstance()->isLoaded( 'MobileFrontend' ) && MediaWikiServices::getInstance()->getService( 'MobileFrontend.Context' )->shouldDisplayMobileView() ) {
     // add mobile modules or do nothing
} else {
   $out->addModuleStyles( 'ext.uls.interlanguage' );
}
  1. The module should use skinStyles and set Minerva to blank.
		"ext.uls.interlanguage": {
			"targets": [ "desktop", "mobile" ],
                        "skinStyles": {
			      "default": ["css/ext.uls.interlanguage.less"],
 			      "minerva: []
 			      },
			"localBasePath": "resources",
			"remoteExtPath": "UniversalLanguageSelector/resources"
		},

(ULS maintainers should decide which is appropriate)

Event Timeline

Those modules are not marked for mobile because ULS itself does not work well on small screens.

Aside, is there a list of skins which Wikimedia deployed extensions must/should support?

Those modules are not marked for mobile because ULS itself does not work well on small screens.

That's fine, you can still do this, but we are planning to remove the targets system on the long term so the mechanism you use to do this must change - in this particular case I've outlined 2 ways you can achieve the same result in "Developer notes". Is something not clear about that?

Aside, is there a list of skins which Wikimedia deployed extensions must/should support?

Minerva and Vector given they support the majority of Wikimedia's traffic. This particular task is not about skins though - it's an implementation detail - the same warnings are thrown on Vector when run on the mobile domain: https://en.m.wikipedia.org/wiki/Spain?useskin=vector

Jdlrobson updated the task description. (Show Details)Oct 30 2019, 7:20 PM

Thanks. I was asking about skins because we don't regularly test with Minerva, only Vector and occasionally Timeless or Monobook. I think Minerva deserves more attention.

Anyway, I think solution 1 seems better until the future where ULS works nice on small screens.

Jdlrobson updated the task description. (Show Details)Oct 31 2019, 4:07 PM

@Nikerabbit having looked at this some more - it seems that the ext.uls.interlanguage or ext.uls.pt module can be loaded via JavaScript - the uls-settings-trigger element is rendered via JavaScript. I think the only reason it does this is that the style varies based on the value of $wgULSPosition - in which case merging these two modules into a single ResourceLoaderLessVarFileModule that varies on the wgULSPosition value is probably the approach here. Am I missing something?

Jdlrobson renamed this task from ext.uls.interlanguage should set targets to mobile and desktop but not ship on Minerva to ext.uls.interlanguage and ext.uls.pt should set targets to mobile and desktop but not ship on Minerva (and possibly loaded via JS).Oct 31 2019, 10:08 PM

What do you mean with "can be loaded via JavaScript"? Wouldn't that make FOUC worse?

ResourceLoaderLessVarFileModule didn't exist when this code was written, and this extension has been on passive maintenance for years.

How urgent/impactful is this task?

Reading web will likely working on this as part of desktop refresh given we will be adjusting the position of the language switcher.

Looking at the code it doesn't seem to prevent any flashes of unstyled content as the thing it styles is created via Javascript (at least that was my read of the code),

I believe fixing the ResourceLoader issue is also a goal of the performance team for this quarter.

You are correct it won't prevent it, though it tries to make it as short as possible. I can help with code review, but we don't have any ULS work planned, so can't help much beyond that.

Jdlrobson triaged this task as High priority.Nov 13 2019, 6:57 PM

Marking as high given the number of warnings as a reminder that this will be a priority when we begin working on the language switcher work.

Change 604184 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/UniversalLanguageSelector@master] Define unsupported skins

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

Jdlrobson closed this task as Resolved.Jul 28 2020, 4:06 PM
Jdlrobson claimed this task.

https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page

mw.loader.getState('ext.uls.interlanguage') === 'registered'
true

LGTM!