Page MenuHomePhabricator

Enable strict units in LESS
Open, Needs TriagePublic

Description

We should enable strict units in LESS.

You can do math in LESS code, which is nice. What is not so nice is that you can do math even if the units on the values are incompatible: LESS will happily calculate 20em + 100px and return 120em as a result. This has caused problems (T265560: Text on UploadWizard's "Describe" page is too narrow (due to .mwe-upwiz-data defining "width: calc(-132.5%)"); also some issues caught at the last minute in code review of https://gerrit.wikimedia.org/r/c/oojs/ui/+/514970).

There is an option called strictUnits, which causes it to refuse to compile that. We should enable it. However, since it actually fails the compilation (like a syntax error), it can break some LESS code that has previously worked fine (one example I've seen was a variable ending up with the correct value, but in units of 1/em, due to some fuzzy math).

Here's a task so that folks can complain if that happens.

Event Timeline

matmarex created this task.Oct 15 2020, 6:46 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 15 2020, 6:46 PM

Change 634315 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] resourceloader: Set strictUnits to true for LESS compilation

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

The patch definitely causes some issues. Most things I checked were fine (e.g. core, Vector, VisualEditor), but Minerva styles failed. I'm not planning to track this down right now, but maybe someone will.

Apparently there is a LessFileCompilationTest, which will fail if the LESS compilation fails for any module, so you just need to run the MediaWiki unit tests for your skin/extension and check the results. This makes it much easier to find the problematic code.

Change 634600 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] Fix units of copy-pasted @ooui-font-size-base LESS variable

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

Change 634620 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] rcfilters: Fix units of LESS variables

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

Change 634600 merged by jenkins-bot:
[mediawiki/core@master] Fix units of copy-pasted @ooui-font-size-base LESS variable

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

Change 634620 merged by jenkins-bot:
[mediawiki/core@master] rcfilters: Fix units of LESS variables

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

Change 634656 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] DateTimeInputWidget: Fix incorrect value due to mixed LESS units

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

Change 634658 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/Vector@master] Set strictUnits to true for LESS, and fix units in the code

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

That makes core tests pass (and Vector – apparently it wasn't fine, no idea how I missed it).

Jenkins reports the following failures in skins and extensions (it does not, unfortunately, give line numbers):

  • skins/MinervaNeue
    • 29 different failures, but they are probably all caused by something in minerva.less/minerva.variables.less
      • "Incompatible units. Change the units or use the unit function. Bad units: 'em' and /em'."
  • extensions/CheckUser
    • modules/ext.checkUser.styles/investigate.less in the "ext.checkUser.styles" module
      • Less_Exception_Compiler: Multiple units in dimension. Correct the units or use the unit function. Bad unit: /em
    • modules/ext.checkUser.styles/investigateblock.less in the "ext.checkUser.styles" module
      • Less_Exception_Compiler: Incompatible units. Change the units or use the unit function. Bad units: 'em' and /em'.
  • extensions/Echo
    • modules/styles/mw.echo.ui.overlay.minerva.less in the "ext.echo.ui" module
      • Multiple units in dimension. Correct the units or use the unit function. Bad unit: /em
  • extensions/Flow
    • modules/styles/board/navigation.less in the "ext.flow.board.styles" module
      • Multiple units in dimension. Correct the units or use the unit function. Bad unit: /em
  • extensions/Kartographer
    • modules/dialog/dialog.less in the "ext.kartographer.dialog" module
      • Multiple units in dimension. Correct the units or use the unit function. Bad unit: /em
  • extensions/UniversalLanguageSelector
    • resources/css/ext.uls.pt.less in the "ext.uls.pt" module
      • Multiple units in dimension. Correct the units or use the unit function. Bad unit: /em
  • extensions/WikibaseMediaInfo
    • resources/mediasearch-vue/components/base/Button.less in the "wikibase.mediainfo.mediasearch.vue.styles" module
      • Multiple units in dimension. Correct the units or use the unit function. Bad unit: /em

I'm not sure how the CI for mediawiki/core works, but I think it does not run actually everything, so we might find more issues still.

I think most of the above are caused by a single common pattern. For example, in Kartographer, this line causes it: padding-top: 4 / 14em;.

The intent is to define a value that computes to 4px padding (assuming a font size that computes to 14px, like in Vector), but defined in em, so that it can scale with user-defined font size. Currently it works correctly (the result is 0.28571429em), but with strictUnits, the division results in a unit of /em (reciprocal em, or: the inverse of em), which can't be output to CSS, as CSS has no such unit.

The following syntaxes can be used to achieve the same instead:

padding-top: 4em / 14; (it looks a bit silly when written like this, but it might be the most convenient if one of the values is defined in another variable)
padding-top: ( 4 / 14 ) * 1em;
padding-top: unit( 4 / 14, em );

The following, sadly, does not work – it outputs a space before the unit. Meh.
padding-top: ( 4 / 14 )em;

I'm not planning to track this down any further (for real this time), but I hope this helps.

Change 634658 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Set strictUnits to true for LESS, and fix units in the code

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

Change 634656 merged by jenkins-bot:
[mediawiki/core@master] DateTimeInputWidget: Fix incorrect value due to mixed LESS units

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

I'm not sure how the CI for mediawiki/core works, but I think it does not run actually everything, so we might find more issues still.

Yeah, core only runs with the Wikimedia gate set (defined here). But it's a start.