w3c / css-validator

W3C CSS Validation Service
https://jigsaw.w3.org/css-validator/
Other
208 stars 105 forks source link

Nu validator - Incorrect "One operand must be a number" for CSS variable calculations #355

Closed ahukkanen closed 2 years ago

ahukkanen commented 2 years ago

Problem

The "One operand must be a number" (operandnumber) is incorrectly reported for CSS variable calculations that contain numeric values.

Replicate

  1. Go to https://validator.w3.org/nu/#textarea
  2. Check the "CSS" box
  3. Insert the following CSS into the box
  4. Receive an error "One operand must be a number"

The CSS to insert into the text box:

.emoji-picker {
  --emoji-size: 1em;
  --content-height: calc(var(--emoji-size) * 1em);
}

This is a simplified version of the source CSS from the @joeattardi/emoji-button package on this line: https://github.com/joeattardi/emoji-button/blob/ef8ac84e19ff7f76c955882f7dfe2acc794d1fdb/css/emoji-button.css#L72

Additional details

It seems this has broken rather recently because the same project which is validating HTML through the nu validator has successful builds for the same tests containing the same CSS on the same page 6 days ago (2021-12-03T12:45:40Z).

It also seems the NPM package has not received any updates within those 6 days (latest release 4.6.2 at 2021-11-25T03:57:10.566Z), so the next thing to inspect is the CSS validator which has received recent updates (validator-nu branch).

alecslupu commented 2 years ago

There is no error on https://jigsaw.w3.org/css-validator/#validate_by_input+with_options when running the provided CSS snippet

ahukkanen commented 2 years ago

@alecslupu Yep, it's only the Nu validator (validator-nu branch of this repo) which is used with this: https://validator.w3.org/nu/

sideshowbarker commented 2 years ago

I can reproduce this with the Nu validator but I don’t understand why — because the validator-nu branch has all the changes from the main branch, plus just three other unrelated changes that I am pretty certain would not cause this.

@ylafon Any clue why this might be happening? Could it be due to how the Nu validator is calling into the CSS-checker code?

ahukkanen commented 2 years ago

@sideshowbarker I would suggest trying out the provided example case with each revision starting from commit 301f99ab3479e1a2409876c71286cbb31b9149e1 which is also in the main branch. I would expect it to work with that revision and then work up commit by commit.

I would expect the cause to be found from one of those "unrelated" changes. There might be a regression that's related to one of those commits.

Sorry I'm so new to the validator code myself, so it's a bit difficult for me to go ahead and test that myself. But that's where I would start to look the issue from.

ahukkanen commented 2 years ago

Actually there is an easier way to test this and identify the working version vs broken version through the releases of this repo.

The last release produces the same error while the one before that produces successful validation.

So, the issue might be found between these two releases: https://github.com/w3c/css-validator/compare/922b16c9cc4f0ec705a08d2d5274cadb88155668..92b6fb637c9f0c0538a79b7c355d48217695eeaf

Broken version cssval-20211112 (2021-11-12T21:08:58Z)

$ echo -e ".emoji-picker {\n  --emoji-size: 1em;\n  --content-height: calc(var(--emoji-size) * 1em);\n}" > test.css
$ rm -f css-validator.jar
$ wget https://github.com/w3c/css-validator/releases/download/cssval-20211112/css-validator.jar
$ java -jar css-validator.jar file:test.css

Output:

Sorry! We found the following errors (1)

URI : file:test.css

Line : 3 
       One operand must be a number
       1em)

Valid CSS information

    .emoji-picker
    {
        --emoji-size : 1em ;
    }

Working version cssval-20211001 (2021-10-01T16:51:07Z)

$ echo -e ".emoji-picker {\n  --emoji-size: 1em;\n  --content-height: calc(var(--emoji-size) * 1em);\n}" > test.css
$ rm -f css-validator.jar
$ wget https://github.com/w3c/css-validator/releases/download/cssval-20211001/css-validator.jar
$ java -jar css-validator.jar file:test.css

Output:

Congratulations! No Error Found.

[...]

Valid CSS information

    .emoji-picker
    {
        --emoji-size : 1em ;
        --content-height : calc(var(--emoji-size) * 1em) ;
    }
ahukkanen commented 2 years ago

The funny thing is that if I manually build the JAR file from the last release's commit, it works fine and does not produce the same error as the JAR file associated with the release:

$ git clone https://github.com/w3c/css-validator.git
$ cd css-validator
$ git checkout 922b16c9cc4f0ec705a08d2d5274cadb88155668
$ ant jar
$ echo -e ".emoji-picker {\n  --emoji-size: 1em;\n  --content-height: calc(var(--emoji-size) * 1em);\n}" > test.css
$ java -jar css-validator.jar file:test.css
{output=text, profile=css3svg, vextwarning=true, warning=2, medium=all, lang=en}
W3C CSS Validator results for file:test.css

Congratulations! No Error Found.

[...]

No errors there, it reports the CSS being valid. So maybe the release JAR is just broken or out of sync?

If you repeat the same process with the revision at commit 19fa57aea8ad8b6b8e4c70690b18a1aaea6169ac, you will receive the "One operand must be a number" error:

$ git checkout 19fa57aea8ad8b6b8e4c70690b18a1aaea6169ac
$ ant jar
$ echo -e ".emoji-picker {\n  --emoji-size: 1em;\n  --content-height: calc(var(--emoji-size) * 1em);\n}" > test.css
$ java -jar css-validator.jar file:test.css
{output=text, profile=css3svg, vextwarning=true, warning=2, medium=all, lang=en}
W3C CSS Validator results for file:test.css

Sorry! We found the following errors (1)

URI : file:test.css

Line : 3 
       One operand must be a number
       1em)

Valid CSS information

    .emoji-picker
    {
        --emoji-size : 1em ;
    }

So I believe this issue has already been fixed because it works fine with any revision after that commit, e.g. the next one at: https://github.com/w3c/css-validator/commit/1900d1d2f0707caef7135248fc5476f3c99b3afd

To fix the online validator, it just needs the latest revision and potentially a new release because the last release is broken although manually building that version from the release's tagged commit, it works fine.

Hard to say what's wrong exactly, maybe something wrong with the release process?

ahukkanen commented 2 years ago

It also seems that at the validator repo (Nu HTML validator), the css-validator submodule has been last time updated 2 months ago (exactly at 2021-10-27T06:30:46Z): image

At that point the CSS validator repository has lived at commit: 19fa57aea8ad8b6b8e4c70690b18a1aaea6169ac

This seems to be exactly the first broken revision in the example shown above. Any revision after that works fine.

ylafon commented 2 years ago

The test seems wrong, but I can explain the behaviour difference. First, you are trying to multiply units, so the unit you will have is a surface and not a length, the error is a valid one.

Why you don't always have an error? Because in some case, the ordering in which variable declaration is done is such that reusing it in the same property won't be seen, in that case, as the var() is of an unknown (yet) value, it is not checked as it can be anything.

ahukkanen commented 2 years ago

@ylafon The error is not there after 19fa57aea8ad8b6b8e4c70690b18a1aaea6169ac.

If I repeat the same test at 19fa57aea8ad8b6b8e4c70690b18a1aaea6169ac without the unit multiplication, there is still an error but just a different one:

$ git checkout 19fa57aea8ad8b6b8e4c70690b18a1aaea6169ac
$ ant jar
$ echo -e ".emoji-picker {\n  --emoji-size: 1em;\n  --content-height: calc(var(--emoji-size) * 1);\n}" > test.css
$ java -jar css-validator.jar file:test.css
{output=text, profile=css3svg, vextwarning=true, warning=2, medium=all, lang=en}
W3C CSS Validator results for file:test.css

Sorry! We found the following errors (1)

URI : file:test.css

Line : 3 
       Invalid type: “var(--emoji-size) * 1”
       1)

Valid CSS information

    .emoji-picker
    {
        --emoji-size : 1em ;
    }

Any revision after that commit won't produce this error and will report "Congratulations! No Error Found.".


EDIT: The same thing applies for the release version cssval-20211112:

$ rm -f css-validator.jar
$ wget https://github.com/w3c/css-validator/releases/download/cssval-20211112/css-validator.jar
$ echo -e ".emoji-picker {\n  --emoji-size: 1em;\n  --content-height: calc(var(--emoji-size) * 1);\n}" > test.css
$ java -jar css-validator.jar file:test.css
[...]
Sorry! We found the following errors (1)

URI : file:test.css

Line : 3 
       Invalid type: “var(--emoji-size) * 1”
       1)
[...]
ahukkanen commented 2 years ago

Here's another example that fails at the Nu CSS validator:

:root {
  --emoji-size: 2rem;
  --emoji-size-multiplier: 1.3;
  --emojis-per-row: 8;
  --emoji-area-height: calc((var(--row-count) * var(--emoji-size) * var(--emoji-size-multiplier)) + var(--category-name-height));
  --emoji-preview-size: 2.75em;
  --emoji-preview-height: calc(var(--emoji-preview-size) + 1em + 1px);
  --emoji-preview-margin: 4px;
  --emoji-preview-height-full: calc(var(--emoji-preview-height) + var(--emoji-preview-margin));
}
:root {
  --row-count: 6;
  --category-name-height: 2rem;
  --content-height: var(--emoji-area-height);
  --category-tabs-height: calc(1.5em + 9px);
  --category-tabs-offset: 8px;
  --picker-width: calc(var(--emojis-per-row) * var(--emoji-size) * var(--emoji-size-multiplier) + 2.75rem);
  --search-height: 2em;
  --search-margin: .5em;
  --search-margin-bottom: 4px;
  --search-height-full: calc(var(--search-height) + var(--search-margin) + var(--search-margin-bottom));
}
.test {
  height: calc(var(--content-height) + var(--category-tabs-height) + var(--category-tabs-offset));
}
.test2 {
  min-height: calc(var(--emoji-size) * var(--emoji-size-multiplier));
  grid-template-columns: repeat(var(--emojis-per-row), calc(var(--emoji-size) * var(--emoji-size-multiplier)));
  grid-auto-rows: calc(var(--emoji-size) * var(--emoji-size-multiplier));
}

The exact same CSS passes fine if you paste it at the jigsaw CSS validator: https://jigsaw.w3.org/css-validator/validator.html.en#validate_by_input

ylafon commented 2 years ago

There was an issue with the deployment of the nu-validator making it not update a few classes. @sideshowbarker and I fixed this and it should work as expected now.