1315403 - Serialization of CSS declaration should not have whitespace between "!" and "important"

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P3)

Description

[Xidorn Quan [:xidorn] UTC+10]

If we have document:
> <div style="display: inline !important"></div>
and we get
> document.querySelector('div').style.cssText

On Firefox it returns:
> display: inline ! important;
while on other browser it returns:
> display: inline !important;

Per spec, there shouldn't be whitespace between "!" and "important", so it is clear that we are wrong here.

This should be a simple fix to Declaration::AppendPropertyAndValueToString and Declaration::AppendVariableAndValueToString.

Comment 1

[Sam Clusker]

Hi Xidorn,

I would be willing to look at this bug, any further instructions that I would need to proceed?

Thanks,
Sam

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Sure. You need to be able to build Firefox first. These instructions should be helpful: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions

Then you can look into layout/style/Declaration.cpp and do the fix as I suggested in comment 0. And it's worth adding a test in layout/style/test for ensuring the fix is correct.

Comment 3

[wajekarprasad]

Hi Xidorn Quan,

I am ready to work on this bug, any further suggestions?

Cheers,
Prasad

CC: [e-mail]

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

See comment 2 for how to build Firefox. When you are ready, just do the change I mentioned in comment 0, and then add a test for this issue. That test should ideally be a web-platform test in testing/web-platform/tests/cssom.

Comment 5

[Conache Cristian]

Hi Xidorn,

I would like to fix this bug as my first bug. Also, I want to ask you if we should create the "cssom" directory in testing/web-platform/tests, because I can find only "cssom-view" there.

Thanks,
Cristian

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Yeah, we should create the "cssom" directory there. Actually this directory already exists in the upstream repo. We just haven't synced with that.

Comment 7

[Conache Cristian]

Attachment: Bug 1315403-no whitespace between "!" and "important" in Declaration::AppendPropertyAndValueToString and Declaration::AppendVariableAndValueToStr,

Review commit: https://reviewboard.mozilla.org/r/94254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94254/

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8812550 [details]
Bug 1315403-no whitespace between "!" and "important" in Declaration::AppendPropertyAndValueToString and Declaration::AppendVariableAndValueToStr,

https://reviewboard.mozilla.org/r/94254/#review94398

Thanks for the patch!

There are several issues below. Please address them and update the patch.

::: testing/web-platform/meta/MANIFEST.json:14903
(Diff revision 1)
> +        "path": "cssom/CSSserialization-noWhitespace.html",
> +        "url": "/cssom/CSSserialization-noWhitespace.html"

This doesn't seem to be a good name of this test. It is testing the serialization of a CSS declaration with important flag. Not having a whitespace between "!" and "important" is just part of a correct serialization.

Probably rename it to something like "serialization-CSSDeclaration-with-important".

::: testing/web-platform/tests/cssom/CSSserialization-noWhitespace.html:3
(Diff revision 1)
> +<title>cssom - Serialization of CSS declaration not having whitespace between 
> +"!" and "important"</title>

Same as above, I think the title just needs to be something like "Serialization of CSS declaration with important flag".

::: testing/web-platform/tests/cssom/CSSserialization-noWhitespace.html:9
(Diff revision 1)
> +"!" and "important"</title>
> +<script src=/resources/testharness.js></script>
> +<script src=/resources/testharnessreport.js></script>
> +<div style="display: inline !important"></div>
> +<script>
> +var css_style = document.querySelector('div').style.cssText

There should be a semicolon at the end of this line.

::: testing/web-platform/tests/cssom/CSSserialization-noWhitespace.html:10
(Diff revision 1)
> +	test(function () {
> +		assert_equals(css_style,"display: inline !important;", 
> +    	"the exclamation mark should be directly followed by 'important'");
> +   });

The indention looks weird, probably because of the lack of semicolon above. Please fix it. Note that, it should use whitespaces rather than tabs for indention.

Please add a test that, when the declaration itself has a whitespace between "!" and "important", verify that the serialization removes that whitespace. This can probably be in the same element, using a different property.

It's probably also worth adding another test which tests the serialization result when the style is set dynamically via cssText.

::: testing/web-platform/tests/cssom/CSSserialization-noWhitespace.html:11
(Diff revision 1)
> +<script src=/resources/testharnessreport.js></script>
> +<div style="display: inline !important"></div>
> +<script>
> +var css_style = document.querySelector('div').style.cssText
> +	test(function () {
> +		assert_equals(css_style,"display: inline !important;", 

Please remove the tailing whitespace.

::: testing/web-platform/tests/cssom/CSSserialization-noWhitespace.html:12
(Diff revision 1)
> +<div style="display: inline !important"></div>
> +<script>
> +var css_style = document.querySelector('div').style.cssText
> +	test(function () {
> +		assert_equals(css_style,"display: inline !important;", 
> +    	"the exclamation mark should be directly followed by 'important'");

I think you can remove the description of "assert_equals" because it's simply testing the whole serialization result.

Comment 9

[Conache Cristian]

Comment on attachment 8812550 [details]
Bug 1315403-no whitespace between "!" and "important" in Declaration::AppendPropertyAndValueToString and Declaration::AppendVariableAndValueToStr,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94254/diff/1-2/

Comment 10

[Conache Cristian]

Hi Xidorn, 

Thank you for reviewing my code! I tried to fix the issues and I updated the patch. Please, let me know if it's all ok.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8812550 [details]
Bug 1315403-no whitespace between "!" and "important" in Declaration::AppendPropertyAndValueToString and Declaration::AppendVariableAndValueToStr,

https://reviewboard.mozilla.org/r/94254/#review94768

There are some small style issues below, but it's almost there!

::: testing/web-platform/tests/cssom/serialization-CSSDeclaration-with-important.html:12
(Diff revision 2)
> +<div id="whitespace" style="background-color: blue !important; color: red ! important;"></div>
> +<div id="dinamicallyStyle"></div>
> +<script>
> +    test(function () {
> +        var css_style = document.querySelector('#noWhitespace').style.cssText;
> +        assert_equals(css_style,"display: inline !important;");

A small style nit: please add a whitespace after the comma here and elsewhere in the file.

::: testing/web-platform/tests/cssom/serialization-CSSDeclaration-with-important.html:18
(Diff revision 2)
> +    },"Inline style declaration without whitespace between '!' and 'important'.");
> +
> +    test(function () {
> +        var css_style = document.querySelector('#whitespace').style.cssText;
> +        assert_equals(css_style,"background-color: blue !important; color: red !important;");
> +   },"Inline style declaration with whitespace between '!' and 'important'.");

Another small nit: this line seems to indent by three whitespaces, while it should indent by four. This also applies to the following two tests.

Comment 12

[Conache Cristian]

Comment on attachment 8812550 [details]
Bug 1315403-no whitespace between "!" and "important" in Declaration::AppendPropertyAndValueToString and Declaration::AppendVariableAndValueToStr,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94254/diff/2-3/

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8812550 [details]
Bug 1315403-no whitespace between "!" and "important" in Declaration::AppendPropertyAndValueToString and Declaration::AppendVariableAndValueToStr,

https://reviewboard.mozilla.org/r/94254/#review94794

Looks good now, thanks!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d97584afe38
no whitespace between "!" and "important" in Declaration::AppendPropertyAndValueToString and Declaration::AppendVariableAndValueToStr, r=xidorn

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Attachment: followup patch

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4a9c304b478
followup - Fix mochitest failure and lint error.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3d97584afe38
https://hg.mozilla.org/mozilla-central/rev/e4a9c304b478

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Will ride the train with 53