Shorthand with variable reference should not have extra whitespace after colon for serialization #15422

mdevlamynck · 2017-02-07T12:24:43Z

Fixes #15295. Tell me if anything is wrong with the tests, I'm not sure I created them correctly.

./mach build -d does not report any errors
./mach test-tidy does not report any errors

These changes fix #15295.
There are tests for these changes

This change is

highfive · 2017-02-07T12:24:48Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/declaration_block.rs
@emilio: components/style/properties/declaration_block.rs

highfive · 2017-02-07T12:24:49Z

Warning

These commits include an empty title element (<title></title>). Consider adding appropriate metadata.

bors-servo · 2017-02-07T12:29:34Z

☔ The latest upstream changes (presumably #15405) made this pull request unmergeable. Please resolve the merge conflicts.

emilio · 2017-02-07T12:31:38Z

components/style/properties/declaration_block.rs

        &AppendableValue::Declaration(decl) => {
            if !decl.value_is_unparsed() {
                // for normal parsed values, add a space between key: and value
                try!(write!(dest, " "));
            }
         },
-         &AppendableValue::DeclarationsForShorthand(..) => try!(write!(dest, " "))
+         &AppendableValue::DeclarationsForShorthand(..) => try!(write!(dest, " ")),
+        _ => {},


Instead of a wildcard, let's do &AppendableValue::Css(..) => {}, so if new AppendableValue kinds are added it's not forgotten here.

emilio

Looks good! Mostly nits and needs a rebase, thanks for working on this! :)

emilio · 2017-02-07T12:32:59Z

tests/wpt/web-platform-tests/cssom/serialize-variable-reference.html

+	test(function() {
+		var elem = document.getElementById('longhand-whitespace');
+
+	    assert_equals(elem.style.cssText, 'font-size: var(--a);');


Please use spaces instead of tabs for indentation.

emilio · 2017-02-07T12:32:59Z

tests/wpt/web-platform-tests/cssom/serialize-variable-reference.html

@@ -0,0 +1,34 @@
+<!doctype html>
+<meta charset="utf-8">
+<title></title>


Let's add a descriptive title here.

mdevlamynck · 2017-02-08T18:47:09Z

tests/wpt/mozilla/meta/MANIFEST.json

@@ -9804,6 +9804,11 @@
     {}
    ]
   ],
+   "mozilla/referrer-policy/generic/subresource/mozresource.pyc": [


Are those changes normal after a ./mach update-manifest? They seem to make the travis build fail.

mdevlamynck · 2017-02-08T18:49:59Z

Fixed nits and rebase done ;)

I'm just not sure of the changes to tests/wpt/mozilla/meta/MANIFEST.json, they appeared after a ./mach update-manifest I used to solve conflicts in tests/wpt/metadata/MANIFEST.json during the rebase.

jdm · 2017-02-08T19:10:11Z

That looks like a new problem; go ahead and remove the pyc entries manually and we'll sort that out separately.

canova · 2017-02-11T23:04:46Z

r? @emilio

canova · 2017-02-11T23:07:24Z

tests/wpt/web-platform-tests/cssom/serialize-variable-reference.html

@@ -0,0 +1,34 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>CSSO - Serialize is consistent with whitespace.</title>


little nit: this should be CSSOM :)

Yes, please fix this.

emilio · 2017-02-12T00:00:04Z

tests/wpt/web-platform-tests/cssom/serialize-variable-reference.html

+        var elem = document.getElementById('longhand');
+
+        assert_equals(elem.style.cssText, 'font-size:var(--a);');
+    }, "Longhand without withspace doesn't had one")


Let's write this as "Longhand with variables preserves original serialization: without whitespace", and so on.

You have a few typos here too, please replace "withspace" with "whitespace".

Thanks!

emilio · 2017-02-12T00:00:49Z

components/style/properties/declaration_block.rs

-            }
-         },
-         &AppendableValue::DeclarationsForShorthand(..) => try!(write!(dest, " "))
+           if !decl.value_is_unparsed() {


Please indent this to four spaces as it was.

bors-servo · 2017-02-24T05:00:49Z

☔ The latest upstream changes (presumably #15685) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2017-02-24T15:51:34Z

@emilio Review ping.


        Shorthand with variable reference should not have extra whitespace af…

…ter colon for serialization

jdm · 2017-03-24T17:22:06Z

@emilio This has been sitting for almost a month :(

emilio · 2017-03-25T12:06:11Z

Ouch, sorry for the lag here :(.

Looks good, though it needs a manifest update. I'll take care of it landing it as soon as I have a more stable internet connection if @mdevlamynck doesn't get to it first.

Thanks for the patch @mdevlamynck, sorry again :/

jdm · 2017-03-25T12:37:16Z

Note: the manifest update can be accomplished with ./mach update-manifest, then committing and squashing the result.

emilio · 2017-03-26T09:35:57Z

Closed in favor of #16135, thanks for doing this!


        Auto merge of #16135 - emilio:shorthand-reference-variable, r=emilio

Shorthand with variable reference should not have extra whitespace after colon for serialization. This is #15422 + a manifest update. Fixes #15295.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16135)


        Auto merge of #16135 - emilio:shorthand-reference-variable, r=<try>

Shorthand with variable reference should not have extra whitespace after colon for serialization. This is #15422 + a manifest update. Fixes #15295.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16135)


        Auto merge of #16135 - emilio:shorthand-reference-variable, r=<try>

Shorthand with variable reference should not have extra whitespace after colon for serialization. This is #15422 + a manifest update. Fixes #15295.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16135)


        Auto merge of #16135 - emilio:shorthand-reference-variable, r=upsuper

Shorthand with variable reference should not have extra whitespace after colon for serialization. This is #15422 + a manifest update. Fixes #15295.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16135)


        servo: Merge #16135 - Shorthand with variable reference should not ha…

…ve extra whitespace after colon for serialization (from emilio:shorthand-reference-variable); r=upsuper This is servo/servo#15422 + a manifest update. Fixes #15295. Source-Repo: https://github.com/servo/servo Source-Revision: 3a3f258f33d7f1c18645df95d3ca72879a770266 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : ce5ef6e91c6f473be4e90d149f33a17757d49d06


        servo: Merge #16135 - Shorthand with variable reference should not ha…

…ve extra whitespace after colon for serialization (from emilio:shorthand-reference-variable); r=upsuper This is servo/servo#15422 + a manifest update. Fixes #15295. Source-Repo: https://github.com/servo/servo Source-Revision: 3a3f258f33d7f1c18645df95d3ca72879a770266


        servo: Merge #16135 - Shorthand with variable reference should not ha…

…ve extra whitespace after colon for serialization (from emilio:shorthand-reference-variable); r=upsuper This is servo/servo#15422 + a manifest update. Fixes #15295. Source-Repo: https://github.com/servo/servo Source-Revision: 3a3f258f33d7f1c18645df95d3ca72879a770266 UltraBlame original commit: d5e22d9308739ad39e78908a09c68cf9812ad1c1


        servo: Merge #16135 - Shorthand with variable reference should not ha…

…ve extra whitespace after colon for serialization (from emilio:shorthand-reference-variable); r=upsuper This is servo/servo#15422 + a manifest update. Fixes #15295. Source-Repo: https://github.com/servo/servo Source-Revision: 3a3f258f33d7f1c18645df95d3ca72879a770266 UltraBlame original commit: d5e22d9308739ad39e78908a09c68cf9812ad1c1


        servo: Merge #16135 - Shorthand with variable reference should not ha…

…ve extra whitespace after colon for serialization (from emilio:shorthand-reference-variable); r=upsuper This is servo/servo#15422 + a manifest update. Fixes #15295. Source-Repo: https://github.com/servo/servo Source-Revision: 3a3f258f33d7f1c18645df95d3ca72879a770266 UltraBlame original commit: d5e22d9308739ad39e78908a09c68cf9812ad1c1

highfive assigned metajack Feb 7, 2017

highfive added the S-awaiting-review label Feb 7, 2017

highfive added the S-needs-rebase label Feb 7, 2017

emilio reviewed Feb 7, 2017

View changes

mdevlamynck reviewed Feb 8, 2017

View changes

highfive assigned emilio and unassigned metajack Feb 11, 2017

canova reviewed Feb 11, 2017

View changes

emilio reviewed Feb 12, 2017

View changes

components/style/properties/declaration_block.rs Outdated

}

},

&AppendableValue::DeclarationsForShorthand(..) => try!(write!(dest, " "))

if !decl.value_is_unparsed() {

This comment has been minimized.

Sign in to view

emilio Feb 12, 2017
Member

Please indent this to four spaces as it was.

highfive added the S-needs-rebase label Feb 24, 2017

Shorthand with variable reference should not have extra whitespace af…

Loading status checks…

529c7a5

…ter colon for serialization

emilio mentioned this pull request Mar 26, 2017

Shorthand with variable reference should not have extra whitespace after colon for serialization. #16135

Merged

emilio closed this Mar 26, 2017

servo / servo

Shorthand with variable reference should not have extra whitespace after colon for serialization #15422

Shorthand with variable reference should not have extra whitespace after colon for serialization #15422

mdevlamynck commented Feb 7, 2017 •

edited by emilio

highfive commented Feb 7, 2017

highfive commented Feb 7, 2017

bors-servo commented Feb 7, 2017

This comment has been minimized.

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mdevlamynck commented Feb 8, 2017

jdm commented Feb 8, 2017

canova commented Feb 11, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Feb 24, 2017

jdm commented Feb 24, 2017

jdm commented Mar 24, 2017

emilio commented Mar 25, 2017 •

edited

jdm commented Mar 25, 2017

emilio commented Mar 26, 2017

servo / servo

Join GitHub today

Shorthand with variable reference should not have extra whitespace after colon for serialization #15422

Shorthand with variable reference should not have extra whitespace after colon for serialization #15422

Conversation

mdevlamynck commented Feb 7, 2017 • edited by emilio

highfive commented Feb 7, 2017

highfive commented Feb 7, 2017

bors-servo commented Feb 7, 2017

This comment has been minimized.

emilio Feb 7, 2017 • edited

emilio left a comment

This comment has been minimized.

emilio Feb 7, 2017

This comment has been minimized.

emilio Feb 7, 2017

This comment has been minimized.

mdevlamynck Feb 8, 2017

mdevlamynck commented Feb 8, 2017

jdm commented Feb 8, 2017

canova commented Feb 11, 2017

This comment has been minimized.

canova Feb 11, 2017

This comment has been minimized.

emilio Feb 11, 2017

This comment has been minimized.

emilio Feb 12, 2017

This comment has been minimized.

emilio Feb 12, 2017

bors-servo commented Feb 24, 2017

jdm commented Feb 24, 2017

jdm commented Mar 24, 2017

emilio commented Mar 25, 2017 • edited

jdm commented Mar 25, 2017

emilio commented Mar 26, 2017

mdevlamynck commented Feb 7, 2017 •

edited by emilio

emilio Feb 7, 2017 •

edited

emilio commented Mar 25, 2017 •

edited