Page MenuHomePhabricator

Remove $wgResourceLoaderLESSImportPaths
Closed, ResolvedPublic

Description

Similar to T140804, this leads to unmaintainable code. Different extension stylesheets should not be sharing code in this fragile manner. However, there are imho two main use cases for this that we do want to keep:

  • Using mixins provided by MediaWiki core (mediawiki.mixins).
  • Creating interfaces based on the current skin or OOjs UI theme. We would define a standard set of variables and functions (with defaults) that skins and themes can override. Importing this theme.less or something, would automatically import the appropiate definition based on the skin for the current module context. Tracked as T112747.

These two use cases can be realised by providing those paths directly as import paths (not kept in a global configuration variable).

There are currently two uses of this variable:

  • extensions/Flow (flow.less): Imports their own paths via the global import path (filed as T140806). Callers can be updated to point to the relevant directory directly.
  • skins/MinervaNeue (minerva.less): Mostly the same as Flow, except there are a few callers in other repositories. Should be fixed by using ResourceModuleSkinStyles, or by using core's less variables instead (e.g. @width-breakpoint-tablet etc.), or, if skin-dependant, with T112747:
    • MobileApp: /styles/links.less (2016)
    • Popups: /resources/renderer/LinkPreview.less (2017)
    • CookieWarning: ext.CookieWarning.mobile.less (2018)
    • MobileFrontend: skinStyles/{mobile.editor.ve,mobile.notifications.overlay} (2017)

See also:

Event Timeline

Krinkle created this task.Jul 19 2016, 7:25 PM
Restricted Application added subscribers: Zppix, Aklapper. · View Herald TranscriptJul 19 2016, 7:25 PM
Krinkle changed the task status from Open to Stalled.Apr 11 2017, 12:18 AM
Krinkle triaged this task as Normal priority.
Restricted Application added a project: Performance-Team. · View Herald TranscriptMar 30 2018, 1:17 AM
Krinkle updated the task description. (Show Details)Apr 28 2018, 2:26 AM
Krinkle updated the task description. (Show Details)

@Jdlrobson Good news and bad news. Good news: All listed uses were fixed. Bad news, a new one popped up in CookieWarning. It imports both @import 'minerva.variables' and @import 'minerva.mixins'. Looks like this may predate the Minerva/MobileFrontend split. So where previously it would only run on target=mobile with Minerva, looks like in theory the extension could now break if only MobileFrontend is installed. (Unknown import from globals exported by Minerva.)

Its import for minerva.mixins seems unused. The import for Minerva variables seem mostly be for device widths which might be satisfied by core's mediawiki.ui/variables. There's one other variable it uses (@contentPaddingTablet). I don't know if that has a core alternative, but if not, then we may need to hardcode it, or if its preferred to be specific to Minerva perhaps in Minerva's skinStyles?. It's currently loaded for target=mobile.

Krinkle changed the task status from Stalled to Open.Apr 28 2018, 2:38 AM

Change 433941 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/CookieWarning@master] Drop Minerva LESS dependency

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

Florian updated the task description. (Show Details)May 19 2018, 10:03 AM

Change 433941 merged by jenkins-bot:
[mediawiki/extensions/CookieWarning@master] Drop Minerva LESS dependency

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

Change 434041 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/skins/MinervaNeue@master] [WIP] Remove minerva.less from global import path

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

Krinkle updated the task description. (Show Details)May 19 2018, 6:21 PM

@Jdlrobson Looks like another use got introduced in MobileFrontend after the deprecation started: skinStyles/mobile.editor.ve and skinStyles/mobile.notifications.overlay. Looks like these should probably be moved to Minerva?

Change 434045 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] [WIP] resourceloader: Add test case for ResourceLoader::getLessCompiler

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

Change 434046 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] [WIP] Remove deprecated wgResourceLoaderLESSImportPaths

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

Change 434045 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Add test case for ResourceLoader::getLessCompiler

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

Change 434166 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/skins/MinervaNeue@master] Move remaining Minerva skinStyles from MobileFrontend

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

Change 434167 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/MobileFrontend@master] Remove CSS duplicated in Minerva

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

Change 434321 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/MobileFrontend@master] Remove mobile.less from global import path

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

Change 434166 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Move remaining Minerva skinStyles from MobileFrontend

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

Change 434167 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove CSS duplicated in Minerva

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

Change 434041 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Remove minerva.less from global import path

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

Change 434321 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove mobile.less from global import path

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

Change 434046 merged by jenkins-bot:
[mediawiki/core@master] Remove deprecated wgResourceLoaderLESSImportPaths

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

@Krinkle It really doesn't make sense to me that VisualEditorOverlay.js is in MobileFrontend, but its stylesheet mobile.editor.ve/minerva.less is in MinervaNeue. This means any change to the styling of that overlay spans two repos.

@Esanders Aye, yes, that makes sense!

I moved that file as part me helping Reading-Web finish their move of Minerva-specific styles from MobileFrontend to Minerva. I understood that Reading-Web prefers to keep these customisations centralised in the Minerva repository – which also allows them to re-use code by sharing a Less-variables import between them.

The main motivation behind this move was 1) Reading's goal of MobileFrontend not depending on Minerva (and vice-versa), and 2) Performance's goal of removing use of the deprecated hook that allowed that dependency to exist in an implicit way.

This file could certainly be moved back to MobileFrontend, in one of two ways: 1) Change the stylesheet to not use Minerva's shared import, or 2) By making the module in MobileFrontend explicitly depend on Minerva through a FileModule subclass that exists in Minerva. Whichever direction you choose, I'd recommend discussing that further with Reading-Web, as I can't decide this :)

TBH in the long term, I'd like to see all the VisualEditor Mobile code moved to VisualEditor and integrated with MobileFrontend. Likewise I'd like to see all the Echo code move to Echo. I think this is important as that's where your code lives and ideally MobileFrontend integration can happen inside one repo.

I think it's fine to move these skin styles to MobileFrontend if possible. I seem to recall there were issues adding styles here for some reason due to how skin/extension registration works but I forget what, so be careful if doing this, that the styles still apply.