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

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM: CSS Object Model
P3
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: xidorn, Assigned: Conache Cristian, Mentored)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [good first bug][lang=c++])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

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.

Updated

6 months ago
Priority: -- → P3
Mentor: xidorn+moz@upsuper.org
Whiteboard: [good first bug][lang=c++]

Comment 1

6 months ago
Hi Xidorn,

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

Thanks,
Sam
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

5 months ago
Hi Xidorn Quan,

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

Cheers,
Prasad

CC: wajekarprasad@gmail.com
Flags: needinfo?(xidorn+moz)
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.
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 5

5 months ago
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
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 hidden (mozreview-request)
Assignee: nobody → cconache
(Reporter)

Comment 8

5 months ago
mozreview-review
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.
Attachment #8812550 - Flags: review?(xidorn+moz)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

5 months ago
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.
(Reporter)

Comment 11

5 months ago
mozreview-review
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.
Attachment #8812550 - Flags: review?(xidorn+moz)
Comment hidden (mozreview-request)
(Reporter)

Comment 13

5 months ago
mozreview-review
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!
Attachment #8812550 - Flags: review?(xidorn+moz) → review+

Comment 14

5 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3d97584afe38
no whitespace between "!" and "important" in Declaration::AppendPropertyAndValueToString and Declaration::AppendVariableAndValueToStr, r=xidorn
Created attachment 8813000 [details] [diff] [review]
followup patch

Comment 16

5 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e4a9c304b478
followup - Fix mochitest failure and lint error.

Comment 17

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3d97584afe38
https://hg.mozilla.org/mozilla-central/rev/e4a9c304b478
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Updated

5 months ago
Keywords: checkin-needed
(Assignee)

Updated

5 months ago
Keywords: checkin-needed
Will ride the train with 53
status-firefox52: affected → ---
You need to log in before you can comment on or make changes to this bug.