Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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
9 months ago
6 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)

(Reporter)

Description

9 months ago
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.
Priority: -- → P3
(Reporter)

Updated

8 months ago
Mentor: xidorn+moz@upsuper.org
Whiteboard: [good first bug][lang=c++]

Comment 1

8 months ago
Hi Xidorn,

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

Thanks,
Sam
(Reporter)

Comment 2

8 months ago
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

8 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)
(Reporter)

Comment 4

8 months ago
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

8 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
(Reporter)

Comment 6

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

Updated

8 months ago
Assignee: nobody → cconache
(Reporter)

Comment 8

8 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

8 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

8 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

8 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

8 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
(Reporter)

Comment 15

8 months ago
Created attachment 8813000 [details] [diff] [review]
followup patch

Comment 16

8 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e4a9c304b478
followup - Fix mochitest failure and lint error.
https://hg.mozilla.org/mozilla-central/rev/3d97584afe38
https://hg.mozilla.org/mozilla-central/rev/e4a9c304b478
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Updated

8 months ago
Keywords: checkin-needed
(Assignee)

Updated

8 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.