Fix serialization of timing-function #14899

canova · 2017-01-06T20:45:54Z

transition-timing-function and animation-timing-function properties corrected to reflect the spec. It was converting function keywords to cubic-bezier or steps functions directly. But we needed to store it to use in the serialization step.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14822 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-01-06T20:45:57Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/box.mako.rs, components/style/properties/shorthand/box.mako.rs
@emilio: components/style/properties/longhand/box.mako.rs, components/style/properties/shorthand/box.mako.rs

highfive · 2017-01-06T20:45:58Z

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

canova · 2017-01-06T20:46:23Z

r? @Manishearth

Manishearth · 2017-01-06T20:46:54Z

@bors-servo try

bors-servo · 2017-01-06T20:46:57Z

⌛ Trying commit 22b7b9b with merge 7c1f6c9...


        Auto merge of #14899 - canaltinova:transition-timing, r=<try>

Fix serialization of timing-function  `transition-timing-function` and `animation-timing-function` properties corrected to reflect [the spec](https://drafts.csswg.org/css-transitions/#serializing-a-timing-function). It was converting function keywords to `cubic-bezier` or `steps` functions directly. But we needed to store it to use in the serialization step. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14822 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14899)

Manishearth · 2017-01-06T20:47:39Z

(please add a test if try doesn't turn up anything)

You can add it to WPT or add it to CSS (if CSS, add it under mozilla and submit upstream directly)

Manishearth

r=me with nits

Manishearth · 2017-01-06T21:35:36Z

components/style/properties/longhand/box.mako.rs

-                    "step-end" => Ok(super::STEP_END),
-                    _ => Err(())
-                }
+        impl TransitionTimingFunction {


This impl need not exist anymore. computed values don't need parse functions

Manishearth · 2017-01-06T21:35:36Z

components/style/properties/longhand/box.mako.rs

-                        try!(dest.write_str(", "));
-                        try!(start_end.to_css(dest));
-                        dest.write_str(")")
+                        let is_start = if let StartEnd::Start = start_end { true } else { false };


let is_start = StartEnd::Start == start_end.

Though you can just pass start_end directly to serialize_steps and deal with it there.

Manishearth · 2017-01-06T21:35:36Z

components/style/properties/longhand/box.mako.rs

+        #[inline]
+        fn to_computed_value(&self, context: &Context) -> computed_value::T {
+            let mut computed = vec![];
+            for timing_function in self.0.iter() {


computed_value::T(self.0.iter().map(|f| f.to_computed_value(context)).collect())

Any reason why we can't use vector_longhand here (with allow_empty=True)? Then you only need to write code dealing with TTF, not with the vec of TTF.

I don't think there is a reason not to. But there are another properties to change too. Do you prefer to doing it in this pr or another one?

canova · 2017-01-06T22:09:35Z

@bors-servo try

bors-servo · 2017-01-06T22:09:37Z

⌛ Trying commit 106b61d with merge a190401...


        Auto merge of #14899 - canaltinova:transition-timing, r=<try>

Fix serialization of timing-function  `transition-timing-function` and `animation-timing-function` properties corrected to reflect [the spec](https://drafts.csswg.org/css-transitions/#serializing-a-timing-function). It was converting function keywords to `cubic-bezier` or `steps` functions directly. But we needed to store it to use in the serialization step. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14822 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14899)

bors-servo · 2017-01-06T22:14:55Z

💔 Test failed - linux-dev

canova · 2017-01-06T22:20:26Z

@bors-servo try
(oops, I forgot to push last changes)

bors-servo · 2017-01-06T22:20:29Z

⌛ Trying commit 9f0d065 with merge 6d295ab...


        Auto merge of #14899 - canaltinova:transition-timing, r=<try>

Fix serialization of timing-function  `transition-timing-function` and `animation-timing-function` properties corrected to reflect [the spec](https://drafts.csswg.org/css-transitions/#serializing-a-timing-function). It was converting function keywords to `cubic-bezier` or `steps` functions directly. But we needed to store it to use in the serialization step. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14822 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14899)

bors-servo · 2017-01-06T22:37:16Z

💔 Test failed - linux-dev

Manishearth · 2017-01-06T22:38:27Z

./mach test-unit -p style needs fixing

   Compiling style v0.0.1 (file:///home/servo/buildbot/slave/linux-dev/build/components/style)
   Compiling style_tests v0.0.1 (file:///home/servo/buildbot/slave/linux-dev/build/tests/unit/style)
error[E0308]: mismatched types
   --> /home/servo/buildbot/slave/linux-dev/build/tests/unit/style/properties/serialization.rs:586:71
    |
586 |         properties.push(PropertyDeclaration::TransitionTimingFunction(timing_function));
    |                                                                       ^^^^^^^^^^^^^^^ expected struct `style::properties::longhands::transition_timing_function::SpecifiedValue`, found struct `style::computed_values::computed_value::T`
    |
    = note: expected type `style::properties::DeclaredValue<style::properties::longhands::transition_timing_function::SpecifiedValue>`
    = note:    found type `style::properties::DeclaredValue<style::computed_values::computed_value::T>`

error: aborting due to previous erro

Manishearth · 2017-01-06T22:38:53Z

ongoing wpt / css builds are at http://build.servo.org/builders/linux-rel-wpt/builds/1752 http://build.servo.org/builders/linux-rel-css/builds/1744

canova · 2017-01-07T14:02:09Z

Some tests were wrong and updated the test expectations as fail until they're merged into servo (See here). Also added a new test to cover all cases.

canova · 2017-01-07T14:04:06Z

I don't now the best place for the new test so I placed it in cssom folder again :) I'm open to suggestions.

bors-servo · 2017-01-07T20:36:56Z

📌 Commit 0d07980 has been approved by Manishearth

bors-servo · 2017-01-07T21:14:45Z

⌛ Testing commit 0d07980 with merge 365c067...


        Auto merge of #14899 - canaltinova:transition-timing, r=Manishearth

Fix serialization of timing-function  `transition-timing-function` and `animation-timing-function` properties corrected to reflect [the spec](https://drafts.csswg.org/css-transitions/#serializing-a-timing-function). It was converting function keywords to `cubic-bezier` or `steps` functions directly. But we needed to store it to use in the serialization step. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14822 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14899)

bors-servo · 2017-01-07T21:56:17Z

💔 Test failed - android

canova · 2017-01-07T22:08:43Z

@bors-servo retry

Infra

bors-servo · 2017-01-07T23:15:09Z

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


        Fix serialization of transition-timing-function


        Fix try_parse_one macro and parse function of TransitionTimingFunction


        Convert computed_value::T to SpecifiedValue in serialization.rs


        Update transition-timing-function css test

canova · 2017-01-07T23:28:55Z

@bors-servo r=Manishearth

bors-servo · 2017-01-07T23:28:56Z

📌 Commit 8d90aa0 has been approved by Manishearth

bors-servo · 2017-01-08T00:50:29Z

⌛ Testing commit 8d90aa0 with merge 496447a...


        Auto merge of #14899 - canaltinova:transition-timing, r=Manishearth

Fix serialization of timing-function  `transition-timing-function` and `animation-timing-function` properties corrected to reflect [the spec](https://drafts.csswg.org/css-transitions/#serializing-a-timing-function). It was converting function keywords to `cubic-bezier` or `steps` functions directly. But we needed to store it to use in the serialization step. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14822 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14899)

bors-servo · 2017-01-08T01:56:13Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev

highfive assigned glennw Jan 6, 2017

highfive added the S-awaiting-review label Jan 6, 2017

highfive assigned Manishearth and unassigned glennw Jan 6, 2017

Manishearth approved these changes Jan 6, 2017

View changes

canova force-pushed the canova:transition-timing branch from cc064ad to 106b61d Jan 6, 2017

highfive added the S-tests-failed label Jan 6, 2017

canova force-pushed the canova:transition-timing branch from 106b61d to 9f0d065 Jan 6, 2017

highfive removed the S-tests-failed label Jan 6, 2017

highfive added the S-tests-failed label Jan 6, 2017

highfive removed the S-tests-failed label Jan 7, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 7, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 7, 2017

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

canova added 4 commits Jan 6, 2017

Fix serialization of transition-timing-function

e2f94d1

Fix try_parse_one macro and parse function of TransitionTimingFunction

96cf81a

Convert computed_value::T to SpecifiedValue in serialization.rs

533ac6b

Update transition-timing-function css test

Loading status checks…

8d90aa0

canova force-pushed the canova:transition-timing branch from 0d07980 to 8d90aa0 Jan 7, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Jan 7, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jan 7, 2017

bors-servo merged commit 8d90aa0 into servo:master Jan 8, 2017
2 of 3 checks passed

2 of 3 checks passed

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

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jan 8, 2017

canova deleted the canova:transition-timing branch Jan 10, 2017

wafflespeanut mentioned this pull request Feb 7, 2017

Remove an invalid FIXME #15426

Closed

servo / servo

Fix serialization of timing-function #14899

Fix serialization of timing-function #14899

canova commented Jan 6, 2017 •

edited by larsbergstrom

highfive commented Jan 6, 2017

highfive commented Jan 6, 2017

canova commented Jan 6, 2017

Manishearth commented Jan 6, 2017

bors-servo commented Jan 6, 2017

Manishearth commented Jan 6, 2017 •

edited

Manishearth left a comment

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.

canova commented Jan 6, 2017

bors-servo commented Jan 6, 2017

bors-servo commented Jan 6, 2017

canova commented Jan 6, 2017

bors-servo commented Jan 6, 2017

bors-servo commented Jan 6, 2017

Manishearth commented Jan 6, 2017

Manishearth commented Jan 6, 2017

canova commented Jan 7, 2017

canova commented Jan 7, 2017

bors-servo commented Jan 7, 2017

bors-servo commented Jan 7, 2017

bors-servo commented Jan 7, 2017

canova commented Jan 7, 2017

bors-servo commented Jan 7, 2017

canova commented Jan 7, 2017

bors-servo commented Jan 7, 2017

bors-servo commented Jan 8, 2017

bors-servo commented Jan 8, 2017

servo / servo

Join GitHub today

Fix serialization of timing-function #14899

Fix serialization of timing-function #14899

Conversation

canova commented Jan 6, 2017 • edited by larsbergstrom

highfive commented Jan 6, 2017

highfive commented Jan 6, 2017

canova commented Jan 6, 2017

Manishearth commented Jan 6, 2017

bors-servo commented Jan 6, 2017

Manishearth commented Jan 6, 2017 • edited

Manishearth left a comment

This comment has been minimized.

Manishearth Jan 6, 2017

This comment has been minimized.

Manishearth Jan 6, 2017

This comment has been minimized.

Manishearth Jan 6, 2017

This comment has been minimized.

Manishearth Jan 6, 2017

This comment has been minimized.

canova Jan 6, 2017

canova commented Jan 6, 2017

bors-servo commented Jan 6, 2017

bors-servo commented Jan 6, 2017

canova commented Jan 6, 2017

bors-servo commented Jan 6, 2017

bors-servo commented Jan 6, 2017

Manishearth commented Jan 6, 2017

Manishearth commented Jan 6, 2017

canova commented Jan 7, 2017

canova commented Jan 7, 2017

bors-servo commented Jan 7, 2017

bors-servo commented Jan 7, 2017

bors-servo commented Jan 7, 2017

canova commented Jan 7, 2017

bors-servo commented Jan 7, 2017

canova commented Jan 7, 2017

bors-servo commented Jan 7, 2017

bors-servo commented Jan 8, 2017

bors-servo commented Jan 8, 2017

canova commented Jan 6, 2017 •

edited by larsbergstrom

Manishearth commented Jan 6, 2017 •

edited