Page MenuHomePhabricator

Fix failing less-2.5.3/variables.less test
Closed, ResolvedPublic

Description

The less-2.5.3/variables.less fixture is failing. Most likely one of T352830, T352866 could introduce a regression. Please verify, fix the code and remove the

			'variables' => true, // T352830, T352866

from a list of known failing tests.

Failure:

... parse /Users/pmiazga/projects/wmf/less.php/test/Fixtures/lessjs-2.5.3/less/variables.less Fail
--- actual
+++ /Users/pmiazga/projects/wmf/less.php/test/Fixtures/lessjs-2.5.3/css/variables.css
@@ -17,9 +17,9 @@
   minus-one: -1;
   font-family: 'Trebuchet', 'Trebuchet', 'Trebuchet';
   color: #888 !important;
-  same-color: #888;
+  same-color: #888 !important;
   same-again: #888 !important;
-  multi-important: #888 #888, 2;
+  multi-important: #888 #888, 'Trebuchet' !important;
   multi: something 'A', B, C, 'Trebuchet';
 }
 .variable-names .quoted {
@@ -46,7 +46,7 @@
   pixels: 500px;
   conversion-metric-a: 30mm;
   conversion-metric-b: 3cm;
-  conversion-imperial: 3.00000055in;
+  conversion-imperial: 3in;
   custom-unit: 420octocats;
   custom-unit-cancelling: 18dogs;
   mix-units: 2px;
@@ -56,13 +56,13 @@
   div-px-1: 10px;
   div-px-2: 1px;
   sub-px-1: 12.6px;
-  sub-cm-1: 9.666625420000001cm;
-  mul-px-1: 19.6em;
+  sub-cm-1: 9.666625cm;
+  mul-px-1: 19.6px;
   mul-em-1: 19.6em;
-  mul-em-2: 196cm;
+  mul-em-2: 196em;
   mul-cm-1: 196cm;
   add-px-1: 15.4px;
-  add-px-2: 393.35323207px;
-  mul-px-2: 140cm;
-  mul-px-3: 140cm;
+  add-px-2: 393.35275591px;
+  mul-px-2: 140px;
+  mul-px-3: 140px;
 }

Event Timeline

pmiazga changed the task status from Open to In Progress.Apr 26 2024, 12:09 PM
pmiazga claimed this task.
pmiazga moved this task from Soon to Current Sprint on the MediaWiki-Platform-Team board.

commit that updated !important handling in the upstream: https://github.com/less/less.js/commit/27dea8ed2b54eec89e6173caeb7950831df8c28f

Looks like the remaining issues (at least some of those) are not regression but a missing feature.

Change that improved unit handling (which looks like we already implemented) is https://github.com/less/less.js/commit/c98495a1005115a5b5e6d19be01a087536b59ee9

A nice minimal test case to work on variable scoping and important handling:

@var: 10;
@important-var: @var !important;
.values {
    @var: 12;
    font-size: @important-var;
}

Should output

.values { 
  font-size: 12 !important 
}

Now it does

.values {
  font-size: 10;
}

For future reference - this ticket covers 3 different issues:

  1. Variable scoping -> in Less variables are lazily evaluated, if @var is redefined in a specific block, it should be used when calculating @important-var (see comment below)
  2. The !important statement isn't carried over when calculating variables
-  same-color: #888;
+  same-color: #888 !important;
-  multi-important: #888 #888, 2;
+  multi-important: #888 #888, 'Trebuchet' !important;
  1. The unit value isn't selected same way as in js version - for example the result of operation in lessphp is in px, but the lessjs returns result in cm
-  mul-px-1: 19.6em;
+  mul-px-1: 19.6px;
-  mul-em-2: 196cm;
+  mul-em-2: 196em;
-  mul-px-2: 140cm;
-  mul-px-3: 140cm;
+  mul-px-2: 140px;
+  mul-px-3: 140px;
  1. Sometimes the result of math operation is different than lessjs result:
-  conversion-imperial: 3.00000055in;
+  conversion-imperial: 3in;
-  sub-cm-1: 9.666625420000001cm;
+  sub-cm-1: 9.666625cm;
-  add-px-2: 393.35323207px;
+  add-px-2: 393.35275591px;

Change #1029223 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/libs/less.php@master] Update unit handling

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

Change #1029539 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/libs/less.php@master] Better support of !important variable.

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

I pushed two changes:

First one about unit handling - https://gerrit.wikimedia.org/r/1029223 -> can be safely merged, this one doesn't enable the variables.less fixture yet as it only fixes issue 3 - unit evaluation. I would get that merged.

The second PR - https://gerrit.wikimedia.org/r/1029539 - fixes both issues 1 and 2 -> but it introduces some problems I still need to debug. In the current state

  • one of one PHPUnit tests fail with Exception: getVariables() require Less to be compiled. please use $parser->getCss() before calling getVariables(). This is caused by fact that Value doesn't get fully evaluated and when retrieving variables it still finds the Less_Tree_Operation and throws an exception.
  • in the same test, some variables (if not all) have a trailing spaces, most likely caused by https://github.com/wikimedia/less.php/blob/master/lib/Less/Parser.php#L361

But the !import and variable scope is improved. This change doesn't break any fixtures, only our ParserTest class fails.

This makes only one remaining issue - the precision of some operations is still broken:

-  conversion-imperial: 3in;
+  conversion-imperial: 3.00000055in;

-  sub-cm-1: 9.666625cm;
+  sub-cm-1: 9.666625420000001cm;

-  add-px-2: 393.35275591px;
+  add-px-2: 393.35323207px;

@Hokwelum if you run out of tickets, could you check the precision? @Krinkle do you have any idea why this could happen? I skimmed the code for this part and I don't see anything obvious. I'm wondering if this is anything related to precision handling between PHP and JS. the sub-cm-1 looks like could be precision issue but then add-px-2 feels bit too big (off by ~0.00005) to be precision issue.

Change #1029223 merged by jenkins-bot:

[mediawiki/libs/less.php@master] Fix multiplication of mixed units to preserve the first unit

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

I'm wondering if this is anything related to precision handling between PHP and JS.

Yes. The two handle floats differently.

Two examples (underscores mine):

$ php -a
> print 10/3;
3.3333333333333
> echo 10/2.3;
4.347826086956_5

Compared to browser console, or Node.js:

$ node
> 10/3
3.3333333333333_335
> 10/2.3
4.347826086956_522

We implement fround() and numPrecision to normalize this, which has fixed other test cases in the past. Perhaps that is not being applied in some cases? Or perhaps it needs improvement?

Change #1029539 merged by jenkins-bot:

[mediawiki/libs/less.php@master] Improve support for preserving `!important` via variables

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