Implement individual CSS transform properties: translate, rotate, scale #19859

birtles · 2018-01-25T06:11:15Z

These are the servo-side changes for Gecko bug 1207734.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Gecko bug 1207734
There are tests for these changes on the Gecko side including web-platform-tests

This change is

highfive · 2018-01-25T06:11:19Z

Heads up! This PR modifies the following files:

@bholley: components/style/gecko/generated/structs.rs, components/style/gecko/generated/bindings.rs, components/style/values/specified/transform.rs, components/style/values/specified/mod.rs, components/style/properties/gecko.mako.rs and 5 more
@canaltinova: components/style/gecko/generated/structs.rs, components/style/gecko/generated/bindings.rs, components/style/values/specified/transform.rs, components/style/values/specified/mod.rs, components/style/properties/gecko.mako.rs and 5 more
@emilio: components/style/gecko/generated/structs.rs, components/style/gecko/generated/bindings.rs, components/style/values/specified/transform.rs, components/style/values/specified/mod.rs, components/style/properties/gecko.mako.rs and 5 more
@fitzgen: components/script/dom/webidls/CSSStyleDeclaration.webidl
@KiChjang: components/script/dom/webidls/CSSStyleDeclaration.webidl
@asajeffrey: components/script/dom/webidls/CSSStyleDeclaration.webidl

highfive · 2018-01-25T06:11:21Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify style and script code, but no tests are modified. Please consider adding a test!

birtles · 2018-01-25T06:13:10Z

These patches were written by @CJKu. I am just landing them on his behalf. They have been reviewed by @emilio and myself.

I'll wait for the first part of the Gecko-side patches to be landed and merged to autoland before landing this (and I'll update the Gecko bindings at that time). I'm opening this for now so others can have a look and in case the automated tests pick up anything.

birtles · 2018-01-25T06:14:54Z

@bors-servo try

bors-servo · 2018-01-25T06:14:59Z

⌛ Trying commit 1fb6ec1 with merge 094bafb...


        Auto merge of #19859 - birtles:individual-transforms, r=<try>

Implement individual CSS transform properties: translate, rotate, scale These are the servo-side changes for [Gecko bug 1207734](https://bugzilla.mozilla.org/show_bug.cgi?id=1207734). - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [Gecko bug 1207734](https://bugzilla.mozilla.org/show_bug.cgi?id=1207734) - [X] There are tests for these changes on the Gecko side including web-platform-tests  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19859)

bors-servo · 2018-01-25T06:36:34Z

💔 Test failed - arm32

birtles · 2018-01-25T06:41:01Z

Failure was:

error: failed to execute compile
caused by: error reading compile response from server
caused by: Failed to read response header
caused by: failed to fill whole buffer
error: Could not compile script.

birtles · 2018-01-25T06:42:02Z

@bors-servo retry

infra


        Auto merge of #19859 - birtles:individual-transforms, r=<try>

Implement individual CSS transform properties: translate, rotate, scale These are the servo-side changes for [Gecko bug 1207734](https://bugzilla.mozilla.org/show_bug.cgi?id=1207734). - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [Gecko bug 1207734](https://bugzilla.mozilla.org/show_bug.cgi?id=1207734) - [X] There are tests for these changes on the Gecko side including web-platform-tests  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19859)

bors-servo · 2018-01-25T06:42:07Z

⌛ Trying commit 1fb6ec1 with merge 5d7c24f...

birtles · 2018-01-25T06:45:39Z

As for the Taskcluster failure, the log just has:

It looks like rustup is not installed. See instructions at https://github.com/servo/servo/#setting-up-your-environment

?

bors-servo · 2018-01-25T06:58:02Z

💔 Test failed - arm32

emilio · 2018-01-25T14:22:56Z

On 01/25/2018 07:45 AM, Brian Birtles wrote: As for the Taskcluster failure, the log <https://public-artifacts.taskcluster.net/EP2crE3RRIadSc_5scuT5A/0/public/logs/live_backing.log> just has: It looks like rustup is not installed. See instructions at https://github.com/servo/servo/#setting-up-your-environment ?

Yeah, taskcluster doesn't block landing yet, just ignore that. See: https://groups.google.com/d/msg/mozilla.dev.servo/lttaOHEMzes/SYTcTk88AAAJ

jdm · 2018-01-25T16:33:11Z

@bors-servo retry

bors-servo · 2018-01-25T16:33:15Z

⌛ Trying commit 1fb6ec1 with merge f3c049c...


        Auto merge of #19859 - birtles:individual-transforms, r=<try>

Implement individual CSS transform properties: translate, rotate, scale These are the servo-side changes for [Gecko bug 1207734](https://bugzilla.mozilla.org/show_bug.cgi?id=1207734). - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [Gecko bug 1207734](https://bugzilla.mozilla.org/show_bug.cgi?id=1207734) - [X] There are tests for these changes on the Gecko side including web-platform-tests  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19859)

bors-servo · 2018-01-25T17:05:37Z

💔 Test failed - linux-rel-wpt

jdm · 2018-01-25T18:17:00Z

{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "e.style['translate'] = \"100deg\" should not set the property value", "test": "/css/css-transforms/parsing/translate-parsing-invalid.html", "line": 127937, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "e.style['translate'] = \"100px 200px 300%\" should not set the property value", "test": "/css/css-transforms/parsing/translate-parsing-invalid.html", "line": 127938, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "e.style['translate'] = \"100px 200px calc(30px + 30%)\" should not set the property value", "test": "/css/css-transforms/parsing/translate-parsing-invalid.html", "line": 127939, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "e.style['scale'] = \"100px\" should not set the property value", "test": "/css/css-transforms/parsing/scale-parsing-invalid.html", "line": 135858, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "e.style['scale'] = \"100 200 300 400\" should not set the property value", "test": "/css/css-transforms/parsing/scale-parsing-invalid.html", "line": 135859, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "\"100deg\" and \"180deg\" are valid rotate values", "test": "/css/css-transforms/animation/rotate-interpolation.html", "line": 139961, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "\"45deg\" and \"-1 1 0 60deg\" are valid rotate values", "test": "/css/css-transforms/animation/rotate-interpolation.html", "line": 139968, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "\"none\" and \"7 -8 9 400grad\" are valid rotate values", "test": "/css/css-transforms/animation/rotate-interpolation.html", "line": 139975, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "\"none\" and \"none\" are valid rotate values", "test": "/css/css-transforms/animation/rotate-interpolation.html", "line": 139982, "action": "test_result", "expected": "FAIL"}
{"status": "FAIL", "group": "default", "message": "assert_equals: expected \"calc(10px + 10%) calc(20px + 20%) calc(30px + 30em)\" but got \"calc(10% + 10px) calc(20% + 20px) calc(30em + 30px)\"", "stack": "test_valid_value/<@http://web-platform.test:8000/css/css-transforms/parsing/resources/parsing-testcommon.js:19:9\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1494:20\ntest@http://web-platform.test:8000/resources/testharness.js:511:9\ntest_valid_value@http://web-platform.test:8000/css/css-transforms/parsing/resources/parsing-testcommon.js:15:5\n@http://web-platform.test:8000/css/css-transforms/parsing/translate-parsing-valid.html:26:1\n", "subtest": "Serialization should round-trip after setting e.style['translate'] = \"calc(10px + 10%) calc(20px + 20%) calc(30px + 30em)\"", "test": "/css/css-transforms/parsing/translate-parsing-valid.html", "line": 140039, "action": "test_result", "expected": "PASS"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "Serialization should round-trip after setting e.style['translate'] = \"0\"", "test": "/css/css-transforms/parsing/translate-parsing-valid.html", "line": 140041, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "Serialization should round-trip after setting e.style['translate'] = \"1px 2px 0\"", "test": "/css/css-transforms/parsing/translate-parsing-valid.html", "line": 140043, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "e.style['rotate'] = \"100px\" should not set the property value", "test": "/css/css-transforms/parsing/rotate-parsing-invalid.html", "line": 143951, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "e.style['rotate'] = \"100 400deg\" should not set the property value", "test": "/css/css-transforms/parsing/rotate-parsing-invalid.html", "line": 143952, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "e.style['rotate'] = \"100 200 400deg\" should not set the property value", "test": "/css/css-transforms/parsing/rotate-parsing-invalid.html", "line": 143953, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "e.style['rotate'] = \"100 200 300 500 400deg\" should not set the property value", "test": "/css/css-transforms/parsing/rotate-parsing-invalid.html", "line": 143954, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "\"2 30 400\" and \"10 110 1200\" are valid scale values", "test": "/css/css-transforms/animation/scale-interpolation.html", "line": 149210, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "\"26 17 9\" and \"2\" are valid scale values", "test": "/css/css-transforms/animation/scale-interpolation.html", "line": 149217, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "\"none\" and \"4 3 2\" are valid scale values", "test": "/css/css-transforms/animation/scale-interpolation.html", "line": 149224, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "\"none\" and \"none\" are valid scale values", "test": "/css/css-transforms/animation/scale-interpolation.html", "line": 149231, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "\"220px 240px 260px\" and \"300px 400px 500px\" are valid translate values", "test": "/css/css-transforms/animation/translate-interpolation.html", "line": 149256, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "\"480px 400px 320px\" and \"240% 160%\" are valid translate values", "test": "/css/css-transforms/animation/translate-interpolation.html", "line": 149263, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "\"none\" and \"8px 80% 800px\" are valid translate values", "test": "/css/css-transforms/animation/translate-interpolation.html", "line": 149270, "action": "test_result", "expected": "FAIL"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "\"none\" and \"none\" are valid translate values", "test": "/css/css-transforms/animation/translate-interpolation.html", "line": 149277, "action": "test_result", "expected": "FAIL"}

nox · 2018-01-26T09:26:09Z

components/style/properties/helpers/animated_properties.mako.rs

+        sum.scale = self.scale.animate(&other.scale, procedure)?;
+        sum.skew = self.skew.animate(&other.skew, procedure)?;
+        sum.perspective = self.perspective.animate(&other.perspective, procedure)?;
+        sum.quaternion = MatrixDecomposed3D::slerp(&self.quaternion,


Why is this call not like the others? Can't the type of self.quaternion implement Animate and use MatrixDecomposed3D::slerp there? That would make this implementation for MatrixDecomposed3D derivable.

nox · 2018-01-26T09:26:09Z

components/style/properties/helpers/animated_properties.mako.rs

+/// <https://drafts.csswg.org/css-transforms/#interpolation-of-transforms>
+impl ComputedRotate {
+    fn fill_unspecified(rotate: &ComputedRotate)
+        -> Result<(Number, Number, Number, Angle), ()>{


Indentation is broken. Spacing is broken.

nox · 2018-01-26T09:26:09Z

components/style/properties/gecko.mako.rs

+    pub fn set_${ident}(&mut self, other: values::computed::${type}) {
+        unsafe { self.gecko.${gecko_ffi_name}.clear() };
+
+        match other.to_transform_operation() {


Nit: if let makes for more readable code here.

nox · 2018-01-26T09:26:09Z

components/style/properties/helpers/animated_properties.mako.rs

-    /// <https://drafts.csswg.org/css-transforms/#interpolation-of-decomposed-3d-matrix-values>
-    fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
+impl MatrixDecomposed3D {
+    fn slerp(from: &Quaternion, to: &Quaternion, procedure: Procedure) -> Result<Quaternion, ()> {


Make this an implementation of Animatable for Quaternion instead of keeping it on MatrixDecomposed3D .

nox · 2018-01-26T09:26:09Z

components/style/properties/helpers/animated_properties.mako.rs

@@ -2280,6 +2290,130 @@ impl Matrix3D {
    }
 }

+/// <https://drafts.csswg.org/css-transforms/#interpolation-of-transforms>
+impl ComputedRotate {
+    fn fill_unspecified(rotate: &ComputedRotate)


I don't understand the relation between that method name and the spec link just above.

nox · 2018-01-26T09:26:09Z

components/style/properties/helpers/animated_properties.mako.rs

+        //
+        // Unspecified translations default to 0px
+        match translate {
+            &Translate::None => {


Nit: it's more readable to instead dereference the matched value *translate and not prefix every pattern with &.

birtles · 2018-01-26T10:54:45Z

Thanks for the review!
Please bear in mind these are not my patches -- I'm just landing them so I can't answer all the questions. I'll make requested changes next week though.

nox · 2018-01-26T11:08:02Z

@birtles ACK, didn't realise that! I think we can do follow-up issues if you don't have the bandwidth to make the changes.

birtles · 2018-01-26T11:12:56Z

Thanks! I'll try to get to it. I think there were some test failures that I need to look into anyway.

bors-servo · 2018-01-31T21:10:54Z

⌛ Testing commit 4bcf284 with merge 319707a...


        Auto merge of #19859 - birtles:individual-transforms, r=emilio

Implement individual CSS transform properties: translate, rotate, scale These are the servo-side changes for [Gecko bug 1207734](https://bugzilla.mozilla.org/show_bug.cgi?id=1207734). - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [Gecko bug 1207734](https://bugzilla.mozilla.org/show_bug.cgi?id=1207734) - [X] There are tests for these changes on the Gecko side including web-platform-tests  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19859)

bors-servo · 2018-01-31T21:41:34Z

💔 Test failed - linux-rel-wpt

birtles · 2018-01-31T21:43:49Z

Oh, I didn't realize the PASSes that @jdm originally quoted were actually failures!
I guess there is an expectations file somewhere I need to update.


        Implement rotate property styling


        Implement translate property styling


        Implement scale property styling


        Implement interpolation of Quaternions

So that we can reuse the code in ComputedRotate.


        Implement Animate trait for individual transforms

birtles · 2018-01-31T22:16:22Z

@bors-servo r=emilio

bors-servo · 2018-01-31T22:16:22Z

📌 Commit 8a4661b has been approved by emilio

bors-servo · 2018-01-31T22:16:26Z

⌛ Testing commit 8a4661b with merge 3e27459...


        Auto merge of #19859 - birtles:individual-transforms, r=emilio

Implement individual CSS transform properties: translate, rotate, scale These are the servo-side changes for [Gecko bug 1207734](https://bugzilla.mozilla.org/show_bug.cgi?id=1207734). - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [Gecko bug 1207734](https://bugzilla.mozilla.org/show_bug.cgi?id=1207734) - [X] There are tests for these changes on the Gecko side including web-platform-tests  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19859)

bors-servo · 2018-01-31T23:07:11Z

☀️ 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: emilio
Pushing 3e27459 to master...

highfive assigned wafflespeanut Jan 25, 2018

highfive added the S-awaiting-review label Jan 25, 2018

highfive added the S-tests-failed label Jan 25, 2018

birtles force-pushed the birtles:individual-transforms branch from 1fb6ec1 to 8711efe Jan 26, 2018

highfive removed the S-tests-failed label Jan 26, 2018

nox requested changes Jan 26, 2018

View changes

highfive assigned emilio and unassigned wafflespeanut Jan 31, 2018

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jan 31, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 31, 2018

CJKu added 5 commits Jan 25, 2018

Implement rotate property styling

62c0c6f

Implement translate property styling

de3e8c9

Implement scale property styling

174f5f7

Implement interpolation of Quaternions

9a62c0b

So that we can reuse the code in ComputedRotate.

Implement Animate trait for individual transforms

Loading status checks…

8a4661b

birtles force-pushed the birtles:individual-transforms branch from 4bcf284 to 8a4661b Jan 31, 2018

highfive added S-awaiting-review and removed S-tests-failed labels Jan 31, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 31, 2018

bors-servo merged commit 8a4661b into servo:master Jan 31, 2018
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 Jan 31, 2018

bors-servo mentioned this pull request Jan 31, 2018

Derive more Parse implementations (fixes #19827) #19903

Merged

birtles deleted the birtles:individual-transforms branch Feb 1, 2018

servo / servo

Implement individual CSS transform properties: translate, rotate, scale #19859

Implement individual CSS transform properties: translate, rotate, scale #19859

birtles commented Jan 25, 2018 •

edited by SimonSapin

highfive commented Jan 25, 2018

highfive commented Jan 25, 2018

birtles commented Jan 25, 2018

birtles commented Jan 25, 2018

bors-servo commented Jan 25, 2018

bors-servo commented Jan 25, 2018

birtles commented Jan 25, 2018

birtles commented Jan 25, 2018

bors-servo commented Jan 25, 2018

birtles commented Jan 25, 2018

bors-servo commented Jan 25, 2018

emilio commented Jan 25, 2018

jdm commented Jan 25, 2018

bors-servo commented Jan 25, 2018

bors-servo commented Jan 25, 2018

jdm commented Jan 25, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

birtles commented Jan 26, 2018

nox commented Jan 26, 2018

birtles commented Jan 26, 2018 •

edited

bors-servo commented Jan 31, 2018

bors-servo commented Jan 31, 2018

birtles commented Jan 31, 2018

birtles commented Jan 31, 2018

bors-servo commented Jan 31, 2018

bors-servo commented Jan 31, 2018

bors-servo commented Jan 31, 2018

servo / servo

Join GitHub today

Implement individual CSS transform properties: translate, rotate, scale #19859

Implement individual CSS transform properties: translate, rotate, scale #19859

Conversation

birtles commented Jan 25, 2018 • edited by SimonSapin

highfive commented Jan 25, 2018

highfive commented Jan 25, 2018

birtles commented Jan 25, 2018

birtles commented Jan 25, 2018

bors-servo commented Jan 25, 2018

bors-servo commented Jan 25, 2018

birtles commented Jan 25, 2018

birtles commented Jan 25, 2018

bors-servo commented Jan 25, 2018

birtles commented Jan 25, 2018

bors-servo commented Jan 25, 2018

emilio commented Jan 25, 2018

jdm commented Jan 25, 2018

bors-servo commented Jan 25, 2018

bors-servo commented Jan 25, 2018

jdm commented Jan 25, 2018

This comment has been minimized.

nox Jan 26, 2018

This comment has been minimized.

nox Jan 26, 2018

This comment has been minimized.

nox Jan 26, 2018

This comment has been minimized.

nox Jan 26, 2018

This comment has been minimized.

nox Jan 26, 2018

This comment has been minimized.

nox Jan 26, 2018

birtles commented Jan 26, 2018

nox commented Jan 26, 2018

birtles commented Jan 26, 2018 • edited

bors-servo commented Jan 31, 2018

bors-servo commented Jan 31, 2018

birtles commented Jan 31, 2018

birtles commented Jan 31, 2018

bors-servo commented Jan 31, 2018

bors-servo commented Jan 31, 2018

bors-servo commented Jan 31, 2018

birtles commented Jan 25, 2018 •

edited by SimonSapin

birtles commented Jan 26, 2018 •

edited