Custom properties, take 2 #7555

SimonSapin · 2015-09-07T16:01:24Z

Support var() in shorthand properties, and fix various bugs.

bors-servo · 2015-09-08T12:08:19Z

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

bors-servo · 2015-09-09T11:14:32Z

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

SimonSapin · 2015-09-09T12:47:56Z

Rebased.

SimonSapin · 2015-09-09T21:02:48Z

The remaining failing css-variables tests are:

Those with supports in the name fail because we don’t implement @support
w3c/csswg-test#853 should fix those with font-face in the name
test_variable_legal_values.htm fails because we don’t support stylesheet updates. This PR adds a copy that uses a style attribute instead and passes.

SimonSapin · 2015-09-09T21:07:10Z

Depends on ~~servo/rust-cssparser#89~~

mbrubeck · 2015-09-16T18:42:16Z

Reviewed 4 of 4 files at r1, 10 of 10 files at r2, 8 of 8 files at r3, 2 of 2 files at r4.
Review status: 15 of 38 files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io

mbrubeck · 2015-09-16T19:43:37Z

Reviewed 2 of 2 files at r5, 4 of 4 files at r6, 2 of 2 files at r7, 7 of 7 files at r8.
Review status: 25 of 38 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

^{tests/wpt/mozilla/tests/css/test_variable_legal_values.html, line 1 [r7] (raw file):}
Should we change this filename to be different from the original?

Comments from the review on Reviewable.io

SimonSapin · 2015-09-16T20:13:38Z

Review status: 25 of 38 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

^{tests/wpt/mozilla/tests/css/test_variable_legal_values.html, line 1 [r7] (raw file):}
I don’t know. It does it differently, but it still tests legal values for variables.

Comments from the review on Reviewable.io

mbrubeck · 2015-09-17T06:01:27Z

Reviewed 6 of 6 files at r9, 9 of 9 files at r10.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

^{tests/wpt/mozilla/tests/css/test_variable_serialization_specified.html, line 44 [r10] (raw file):}
Is this serialization required by the spec, or is it implementation-defined?

Comments from the review on Reviewable.io

SimonSapin · 2015-09-17T12:30:58Z

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

^{tests/wpt/mozilla/tests/css/test_variable_serialization_specified.html, line 44 [r10] (raw file):}
https://drafts.csswg.org/css2/syndata.html#unexpected-eof says “User agents must close all open constructs […] at the end of the style sheet.” (In CSS Syntax Level 3 this is not as succinct, but it’s a consequence of the consume a token and consume a component value algorithms.)

In this example:

<p style="background: var(--b) green; --a: url(foo.png); --b: var(--a /* unclosed comment">

… you want green to be parsed as a color after the foo.png image, not be part of the comment or the var function.

But yeah, the css-variables spec could probably specify this a bit more.

Comments from the review on Reviewable.io

mbrubeck · 2015-09-17T12:47:50Z

@bors-servo r+

bors-servo · 2015-09-17T12:47:51Z

📌 Commit 7569b49 has been approved by mbrubeck

bors-servo · 2015-09-17T12:47:53Z

🔒 Merge conflict


        Support var() in shorthands.


        Fix !important in custom properties.

Test changes are also in https://github.com/w3c/csswg-test/pull/847/files


        Allow whitespace-only custom property values.


        The function name var() is case-insensitive.


        Don’t ignore input at the end of a declaration with var().


        Look for var() in a non-custom property declaration after a parse error.


        Add a copy of a test that uses a style attribute instead of a stylesh…

…eet. `tests/wpt/css-tests/css-variables-1_dev/html/test_variable_legal_values.htm` relies on setting `.data` on a text node inside a `<style>` element to update a stylesheet, but this doesn’t work on Servo yet. Add a copy of this file that uses a style attribute instead.


        Record first and last token type of custom property values.


        var() substitution: insert /**/ between tokens that would be parse as…

… one token.


        Initial support for custom properties in CSSStyleDeclaration

mbrubeck · 2015-09-17T12:50:40Z

Reviewed 2 of 2 files at r12.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

Comments from the review on Reviewable.io

mbrubeck · 2015-09-17T12:50:56Z

@bors-servo r+

bors-servo · 2015-09-17T12:50:56Z

📌 Commit feaf6f4 has been approved by mbrubeck

bors-servo · 2015-09-17T12:50:58Z

⌛ Testing commit feaf6f4 with merge 1b6d4da...


        Auto merge of #7555 - SimonSapin:custom-properties++, r=mbrubeck

@pcwalton

Custom properties, take 2 Support `var()` in shorthand properties, and fix various bugs. r? @pcwalton  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7555)

bors-servo · 2015-09-17T13:27:49Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Sep 7, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Sep 8, 2015

SimonSapin force-pushed the SimonSapin:custom-properties++ branch from a13b28f to f5999b7 Sep 8, 2015

SimonSapin removed the S-needs-rebase label Sep 8, 2015

metajack assigned pcwalton Sep 8, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Sep 9, 2015

SimonSapin force-pushed the SimonSapin:custom-properties++ branch from f5999b7 to a88dc2e Sep 9, 2015

jdm removed the S-needs-rebase label Sep 9, 2015

SimonSapin force-pushed the SimonSapin:custom-properties++ branch from a88dc2e to a41cbed Sep 9, 2015

SimonSapin force-pushed the SimonSapin:custom-properties++ branch from a41cbed to eef83fc Sep 9, 2015

SimonSapin force-pushed the SimonSapin:custom-properties++ branch 2 times, most recently from 4bcbbfb to 8cfdabb Sep 16, 2015

mbrubeck added S-awaiting-answer and removed S-awaiting-review labels Sep 17, 2015

mbrubeck assigned mbrubeck and unassigned pcwalton Sep 17, 2015

mbrubeck added A-content/css I-enhancement labels Sep 17, 2015

highfive added S-awaiting-merge and removed S-awaiting-review S-awaiting-answer labels Sep 17, 2015

SimonSapin added 10 commits Sep 3, 2015

Support var() in shorthands.

4abe7cd

Fix !important in custom properties.

c7622bf

Test changes are also in https://github.com/w3c/csswg-test/pull/847/files

Allow whitespace-only custom property values.

ab9e1af

The function name var() is case-insensitive.

54ef805

Don’t ignore input at the end of a declaration with var().

70ea5f6

Look for var() in a non-custom property declaration after a parse error.

389d537

Record first and last token type of custom property values.

81dd1ab

var() substitution: insert /**/ between tokens that would be parse as…

d56ea10

… one token.

Initial support for custom properties in CSSStyleDeclaration

Loading status checks…

feaf6f4

SimonSapin force-pushed the SimonSapin:custom-properties++ branch from 7569b49 to feaf6f4 Sep 17, 2015

highfive added S-awaiting-review and removed S-awaiting-merge labels Sep 17, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 17, 2015

bors-servo mentioned this pull request Sep 17, 2015

Adds support for input element's maxlength attr #7323

Merged

bors-servo merged commit feaf6f4 into servo:master Sep 17, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

SimonSapin deleted the SimonSapin:custom-properties++ branch Sep 17, 2015

servo / servo

Custom properties, take 2 #7555

Custom properties, take 2 #7555

SimonSapin commented Sep 7, 2015

bors-servo commented Sep 8, 2015

bors-servo commented Sep 9, 2015

SimonSapin commented Sep 9, 2015

SimonSapin commented Sep 9, 2015

SimonSapin commented Sep 9, 2015

mbrubeck commented Sep 16, 2015

mbrubeck commented Sep 16, 2015

SimonSapin commented Sep 16, 2015

mbrubeck commented Sep 17, 2015

SimonSapin commented Sep 17, 2015

mbrubeck commented Sep 17, 2015

bors-servo commented Sep 17, 2015

bors-servo commented Sep 17, 2015

mbrubeck commented Sep 17, 2015

mbrubeck commented Sep 17, 2015

bors-servo commented Sep 17, 2015

bors-servo commented Sep 17, 2015

bors-servo commented Sep 17, 2015

servo / servo

Join GitHub today

Custom properties, take 2 #7555

Custom properties, take 2 #7555

Conversation

SimonSapin commented Sep 7, 2015

bors-servo commented Sep 8, 2015

bors-servo commented Sep 9, 2015

SimonSapin commented Sep 9, 2015

SimonSapin commented Sep 9, 2015

SimonSapin commented Sep 9, 2015

mbrubeck commented Sep 16, 2015

mbrubeck commented Sep 16, 2015

SimonSapin commented Sep 16, 2015

mbrubeck commented Sep 17, 2015

SimonSapin commented Sep 17, 2015

mbrubeck commented Sep 17, 2015

bors-servo commented Sep 17, 2015

bors-servo commented Sep 17, 2015

mbrubeck commented Sep 17, 2015

mbrubeck commented Sep 17, 2015

bors-servo commented Sep 17, 2015

bors-servo commented Sep 17, 2015

bors-servo commented Sep 17, 2015