style: support calc() in color functions. #19457

emilio · 2017-12-01T23:15:52Z

Depends on #19456 and servo/rust-cssparser#207.

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=984021

This change is

highfive · 2017-12-01T23:15:55Z

Heads up! This PR modifies the following files:

@bholley: components/style/Cargo.toml, components/style/values/specified/color.rs, components/selectors/Cargo.toml, components/style/error_reporting.rs, components/style/values/specified/angle.rs and 3 more
@asajeffrey: components/script_layout_interface/Cargo.toml, components/script/Cargo.toml
@canaltinova: components/style/Cargo.toml, components/style/values/specified/color.rs, components/style/error_reporting.rs, components/style/values/specified/angle.rs, components/style/values/specified/calc.rs
@fitzgen: components/script_layout_interface/Cargo.toml, components/script/Cargo.toml
@KiChjang: components/script_layout_interface/Cargo.toml, components/script/Cargo.toml

highfive · 2017-12-01T23:15:56Z

Warning

These commits modify unsafe code. Please review it carefully!

emilio · 2017-12-01T23:16:00Z

r? @SimonSapin

SimonSapin · 2017-12-02T00:04:35Z

ports/geckolib/glue.rs

@@ -4552,7 +4552,16 @@ pub unsafe extern "C" fn Servo_SelectorList_Drop(list: RawServoSelectorListOwned
 fn parse_color(value: &str) -> Result<specified::Color, ()> {


Is every use of this function intended to support full CSS syntax, including calc? I traced one use back to aBGColor in DocumentRendererChild::RenderDocument. Is that <body bgcolor>? https://html.spec.whatwg.org/multipage/#the-page links to https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#rules-for-parsing-a-legacy-colour-value

It is not, that's handled from nsAttrValue::ParseColor (https://searchfox.org/mozilla-central/rev/0b613c3887789f7786cd3131dfe9648398f4a6ac/dom/html/HTMLBodyElement.cpp#58).

That seems to be the default background color to render in a child process if there's no content.

The only web-exposed use of this is CanvasGradient::AddColorStop, and the spec for that is:

https://html.spec.whatwg.org/multipage/#dom-canvasgradient-addcolorstop

Which says to parse as a CSS <color> value, that is, with calc().

bors-servo · 2017-12-02T01:36:11Z

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

emilio · 2017-12-02T02:22:23Z

@bors-servo try

bors-servo · 2017-12-02T02:22:27Z

⌛ Trying commit d39b896 with merge 3c61213...


        Auto merge of #19457 - emilio:color-calc, r=<try>

style: support calc() in color functions. Depends on #19456 and servo/rust-cssparser#207.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19457)

emilio · 2017-12-02T02:33:09Z

@bors-servo try

bors-servo · 2017-12-02T02:33:13Z

⌛ Trying commit ad12a1f with merge 82eeb01...


        Auto merge of #19457 - emilio:color-calc, r=<try>

style: support calc() in color functions. Depends on #19456 and servo/rust-cssparser#207.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19457)

bors-servo · 2017-12-02T04:09:44Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

emilio · 2017-12-02T13:41:55Z

Sigh, I expected tests to be there already... I'll write some.

SimonSapin · 2017-12-04T11:43:33Z

r+ with one last change

Reviewed 14 of 14 files at r1, 1 of 1 files at r2, 1 of 4 files at r3, 7 of 7 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/style/values/specified/color.rs, line 162 at r6 (raw file):

                    CSSParserColor::RGBA(rgba) => Color::Numeric {
                        parsed: rgba,
                        authored: authored.map(|s| s.to_lowercase().into_boxed_str()),

This probably doesn’t make a difference in behavior since all valid color keywords are ASCII, but this should be to_ascii_lowercase.

Comments from Reviewable

emilio · 2017-12-04T11:55:29Z

@bors-servo r=SimonSapin

bors-servo · 2017-12-04T11:55:30Z

📌 Commit a0d7f36 has been approved by SimonSapin

bors-servo · 2017-12-05T03:02:30Z

⌛ Testing commit a0d7f36 with merge 5b9c595...


        Auto merge of #19457 - emilio:color-calc, r=SimonSapin

style: support calc() in color functions. Depends on #19456 and servo/rust-cssparser#207. Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=984021  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19457)

bors-servo · 2017-12-05T03:22:47Z

💔 Test failed - linux-dev

bors-servo · 2017-12-05T06:54:15Z

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


        style: Update cssparser to 0.23.


        style: Formatting nits in color parsing.


        style: Support calc() in color functions.

emilio · 2017-12-05T09:07:38Z

@bors-servo r=SimonSapin p=1

bors-servo · 2017-12-05T09:07:39Z

📌 Commit 23d69ea has been approved by SimonSapin

bors-servo · 2017-12-05T09:37:19Z

⌛ Testing commit 23d69ea with merge 3cef09a...


        Auto merge of #19457 - emilio:color-calc, r=SimonSapin

style: support calc() in color functions. Depends on #19456 and servo/rust-cssparser#207. Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=984021  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19457)

bors-servo · 2017-12-05T10:32:13Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: SimonSapin
Pushing 3cef09a to master...


        style: Support calc() in color functions.

Upstreamed from servo/servo#19457 [ci skip]

highfive assigned pcwalton Dec 1, 2017

highfive added the S-awaiting-review label Dec 1, 2017

highfive assigned SimonSapin and unassigned pcwalton Dec 1, 2017

SimonSapin reviewed Dec 2, 2017

View changes

highfive added the S-needs-rebase label Dec 2, 2017

emilio force-pushed the emilio:color-calc branch 2 times, most recently from 389e8d5 to d39b896 Dec 2, 2017

emilio removed the S-needs-rebase label Dec 2, 2017

emilio force-pushed the emilio:color-calc branch from d39b896 to ad12a1f Dec 2, 2017

emilio force-pushed the emilio:color-calc branch from ad12a1f to 3cd75d1 Dec 2, 2017

emilio mentioned this pull request Dec 2, 2017

style: Update cssparser to 0.23. #19456

Closed

emilio force-pushed the emilio:color-calc branch from 3cd75d1 to a0d7f36 Dec 4, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 4, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 5, 2017

highfive added the S-needs-rebase label Dec 5, 2017

emilio added 3 commits Dec 1, 2017

style: Update cssparser to 0.23.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

ea5efd9

style: Formatting nits in color parsing.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

e91c9ec

style: Support calc() in color functions.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

Loading status checks…

23d69ea

emilio force-pushed the emilio:color-calc branch from a0d7f36 to 23d69ea Dec 5, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Dec 5, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Dec 5, 2017

bors-servo merged commit 23d69ea into servo:master Dec 5, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 5, 2017

This was referenced Dec 5, 2017

Use workspace.default-members to specify default crates for cargo build #19476

Merged

Update WR (AA on transformed box-shadows, resource leak fixes) #19414

Merged

jdm added a commit to web-platform-tests/wpt that referenced this pull request Jan 4, 2018

style: Support calc() in color functions.

a8aedd5

Upstreamed from servo/servo#19457 [ci skip]

servo-wpt-sync mentioned this pull request Jan 4, 2018

style: Support calc() in color functions. web-platform-tests/wpt#8913

Merged

servo / servo

style: support calc() in color functions. #19457

style: support calc() in color functions. #19457

emilio commented Dec 1, 2017 •

edited

highfive commented Dec 1, 2017

highfive commented Dec 1, 2017

emilio commented Dec 1, 2017

This comment has been minimized.

This comment has been minimized.

bors-servo commented Dec 2, 2017

emilio commented Dec 2, 2017

bors-servo commented Dec 2, 2017

emilio commented Dec 2, 2017

bors-servo commented Dec 2, 2017

bors-servo commented Dec 2, 2017

emilio commented Dec 2, 2017

SimonSapin commented Dec 4, 2017

emilio commented Dec 4, 2017

bors-servo commented Dec 4, 2017

bors-servo commented Dec 5, 2017

bors-servo commented Dec 5, 2017

bors-servo commented Dec 5, 2017

emilio commented Dec 5, 2017

bors-servo commented Dec 5, 2017

bors-servo commented Dec 5, 2017

bors-servo commented Dec 5, 2017

		@@ -4552,7 +4552,16 @@ pub unsafe extern "C" fn Servo_SelectorList_Drop(list: RawServoSelectorListOwned
		fn parse_color(value: &str) -> Result<specified::Color, ()> {

servo / servo

Join GitHub today

style: support calc() in color functions. #19457

style: support calc() in color functions. #19457

Conversation

emilio commented Dec 1, 2017 • edited

highfive commented Dec 1, 2017

highfive commented Dec 1, 2017

emilio commented Dec 1, 2017

This comment has been minimized.

SimonSapin Dec 2, 2017

This comment has been minimized.

emilio Dec 2, 2017

bors-servo commented Dec 2, 2017

emilio commented Dec 2, 2017

bors-servo commented Dec 2, 2017

emilio commented Dec 2, 2017

bors-servo commented Dec 2, 2017

bors-servo commented Dec 2, 2017

emilio commented Dec 2, 2017

SimonSapin commented Dec 4, 2017

emilio commented Dec 4, 2017

bors-servo commented Dec 4, 2017

bors-servo commented Dec 5, 2017

bors-servo commented Dec 5, 2017

bors-servo commented Dec 5, 2017

emilio commented Dec 5, 2017

bors-servo commented Dec 5, 2017

bors-servo commented Dec 5, 2017

bors-servo commented Dec 5, 2017

emilio commented Dec 1, 2017 •

edited