stylo: Implement -moz-transform property #16231

canova · 2017-04-02T13:35:38Z

-moz-transform property is an alias shorthand property. It has transform as sub-property but their parsing is different in some cases(matrix/matrix3d). -moz-transform also accepts LengthOrPercentage in the nondiagonal homogeneous components of matrix and matrix3d.

It looks like length and percentage values are getting computed to number directly. For example 120px converts into 120 and calc(120px + 20%) converts into 140. So I did like this. But we need to check this behavior again to be sure, I guess.

Also I spent half day investigating why matrices are not serializing after this change and found out their ToCss functions is not completely implemented yet 😄 I was going to implement them but there is an easy issue about it(#15194) so I left it that way. Probably all tests won't pass without it.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Support parsing moz-specific matrix functions in prefixed transform property #16003 and Bug 1351356

This change is

canova · 2017-04-02T13:36:48Z

r? @Manishearth @emilio @wafflespeanut

canova · 2017-04-02T13:41:15Z

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

                        let matrix = SpecifiedMatrix {
                            m11: values[0],        m12: values[1],        m13: Number::new(0.0), m14: Number::new(0.0),
                            m21: values[2],        m22: values[3],        m23: Number::new(0.0), m24: Number::new(0.0),
                            m31: Number::new(0.0), m32: Number::new(0.0), m33: Number::new(1.0), m34: Number::new(0.0),
-                            m41: values[4],        m42: values[5],        m43: Number::new(0.0), m44: Number::new(1.0),
+                            m41: lengths[0].clone(), m42: lengths[1].clone(),


I'm not happy with these indentations but I couldn't find a better way to indent this :(

No need to stress too much on it. NBD really :)

canova · 2017-04-02T14:42:23Z

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dda1b604bbfda11d29b22a262b63397b383fccc
Try shows lots of passes in reftests but some mochitest failures increased probably due to serialization. I'll look closely.

Manishearth · 2017-04-02T15:47:31Z

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

-                m41: self.m41.to_computed_value(context),
-                m42: self.m42.to_computed_value(context),
-                m43: self.m43.to_computed_value(context),
+                m41: length_to_computed_number(self.m41.to_computed_value(context)),


No, we need to keep these as LoPoN, no?

Why? That's the computed value form.

Yeah, length values are getting normalized into numbers during computed value conversion. See:

data:text/html,<script>var div = document.createElement('div');div.style.MozTransform = "matrix(1, 1, 1, 1, 40px, 120px)";alert("Specified: " + div.style.MozTransform);alert("Computed: " + window.getComputedStyle(div).MozTransform);</script>

Otherwise interpolation algorithm would fail.

We already allow percentages in translate; these are resolved against the refbox in layout time. Interpolation fails for us here, and gecko uses interpolatematrix to handle these -- it defers interpolation till layout time. This is an independent issue we need to fix.

Yes, lengths can map to numbers by pixel value here, but percentages do not map.

(I think I've mentioned it before -- I'm quite annoyed that the spec doesn't really specify units here, but to be 100% correct, the homogeneous coordinates in the matrix should be in units of pixel, and the rest are unitless ratios, but it implicitly expects us to convert the ratio to pixels of the same numeric value)

Manishearth · 2017-04-02T15:50:17Z

components/style/properties/properties.mako.rs

@@ -577,6 +573,12 @@ impl ShorthandId {
            None => return None
        };

+        // Alias shorthand properties like -moz-transform should not be
+        // serialized as shothand. They will be serialized as londhand.


spelling of shorthand and longhand

Oops, will fix that :)

Manishearth · 2017-04-02T15:51:42Z

components/style/properties/properties.mako.rs

+        // Alias shorthand properties like -moz-transform should not be
+        // serialized as shothand. They will be serialized as londhand.
+        if self.flags().contains(ALIAS_PROPERTY) {
+            return None;


Note that you still want it to work if someone asks for -moz-transform to be serialized. I suspect this is the cause of most of the test failures.

Manishearth · 2017-04-02T15:53:10Z

Where are the LoPoN values computed to numbers? In gecko, specified and computed transform are the same.

emilio · 2017-04-02T16:08:51Z

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

+            Either::First(length) => {
+                match length {
+                    computed::LengthOrPercentage::Length(len) => (len.0 / 60) as CSSFloat,
+                    computed::LengthOrPercentage::Percentage(percentage) => percentage,


That being said, I don't think this is right, is it? What are percentages resolved against?

It looks like gecko is convert to number while computing but doesn't serialize it.
See with and without percentage value in this example, it's same:

<!DOCTYPE html> <style> div { width: 100px; height: 100px; background-color: red; -moz-transform: matrix(1, 0.3, 0, 1, 500%, 0); } </style> <div></div>

Probably this is a gecko bug.

That looks indeed wrong, perhaps @dholbert or @heycam can confirm it? If we do that, let's just add a comment here or something.

I suspect that's a serialization bug, not how it's actually treated. again, translate takes percentages, and they get resolved at use time, which leads to a bunch of interpolation headaches.

emilio · 2017-04-02T16:09:37Z

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

+        match value {
+            Either::First(length) => {
+                match length {
+                    computed::LengthOrPercentage::Length(len) => (len.0 / 60) as CSSFloat,


There's Au::to_f32_px iirc, or at least the AU_PER_PX constant that you can use instead of the magic value.

Yeah, that's better. I'll change that. Thanks!

emilio · 2017-04-02T16:09:47Z

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

+                    computed::LengthOrPercentage::Length(len) => (len.0 / 60) as CSSFloat,
+                    computed::LengthOrPercentage::Percentage(percentage) => percentage,
+                    computed::LengthOrPercentage::Calc(calc) => {
+                        (calc.length.0 / 60) as CSSFloat + calc.percentage.unwrap_or(0.0)


Same here re. 60, and percentage resolution doesn't really seem right.

wafflespeanut · 2017-04-02T16:28:52Z

components/style/properties/data.py

-        self.creates_stacking_context = arg_to_bool(creates_stacking_context)
-        self.fixpos_cb = arg_to_bool(fixpos_cb)
-        self.abspos_cb = arg_to_bool(abspos_cb)
+        self.flags = flags.split() if flags else []


Nit: flags = [] in the function definition?

(I'm not sure why we have this tradition of default'ing all the arguments to None in the function definition and change them here)

Well, when I try to default flags argument to [], python gives an AttributeError that tells split is not an attribute of list.

wafflespeanut · 2017-04-02T16:31:21Z

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

@@ -1619,6 +1673,17 @@ ${helpers.predefined_type("scroll-snap-coordinate",
        }
    }

+    /// Parses `transform` property.
+    pub fn parse(context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue,()> {


inline? (here and below)

bors-servo · 2017-04-04T21:15:52Z

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

canova · 2017-04-08T16:30:44Z

Updated the PR, now it also stores the percentage forms in computed value and passes to gecko side. But still I need to test this and do another pass on the code to see if I can change some parts with a better way.

Also I had to create a new amended commit instead of fixup commits because of huge conflicts. That will make hard to review, sorry about that :/

Btw, do we want -moz-transform in Servo?

Manishearth · 2017-04-09T01:08:45Z

I am ambivalent on -moz-transform being in Servo, do whichever is easier. It is probably harder to add servo support since this needs extra layout tweaks (not much though).

Manishearth

So overall this approach is on the right track, but we have some issues which I hadn't realized initially.

Firstly, we got the parsing wrong. Probably my fault, I think I misread the original code. For prefixed matrix, the last two elements can be length, percentage, number, or calc. For prefixed matrix3d, fourth from last and third from last is l/p/n/calc. Second from last is length/number/calc, not percentage (it's in the z-direction, percentage can't apply in that direction because there's no z-direction to the reference box).

We can resolve lengths to their Au values as Numbers whilst computing (or leave them as Au). Percentages must stay put; .value() is not something we should be calling here. During layout time, the first percentage will be resolved against width, and the second against height -- see what Translate does in fragment.rs. I am not sure how to deal with calcs, we can leave them as calcs I guess and let layout resolution deal with it. I suspect the best approach here is to compute the LoNs (parsed as LoNoC) as L (Au), and the LoPoNs (parsed as LoPoN) as LoP. However, this may need special handling to handle unitless calcs. This can be a followup, I don't think we really care about unitless calcs in -moz-transform that much. I can expand on what the issue here is if you want, though.

I think we need to stop using ComputedMatrix for computed prefixed matrices. Ignore animation for now, let animation fail for these -- the interpolatematrix approach is what we need to use here.

Instead, have a separate PrefixedMatrix variant that uses a struct similar to ComputedMatrix (ComputedMatrixWithPercents?) that holds number or percentage in the xy homogeneous slots. This can be resolved to a ComputedMatrix given a refbox width and height, and use that in layout. Again, we're ignoring animation for now, prefixed matrices can't be animated without interpolatematrix (which I can help you work on next if you have time, see bug 1335998!)

It is also okay to use the same matrix variant for prefixed and uprefixed, and have them all use the same ComputedMatrixWithPercents type. This just means animation code has to be tweaked a bit to attempt to convert the matrix into one with pure numbers provided it doesn't contain percents. Use whichever approach you prefer!

Manishearth · 2017-04-09T01:45:48Z

components/style/properties/gecko.mako.rs

-                )};
+                % if item == "number_percentage":
+                    ${css_value_setters[item] % (
+                        "bindings::Gecko_CSSValue_GetArrayItem(gecko_value, %d)" % (index + 1),


It should be number OR percentage, not AND, no?

bors-servo · 2017-04-10T03:24:45Z

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

bors-servo · 2017-04-12T11:25:56Z

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

canova · 2017-04-13T21:21:19Z

It works in stylo currently but I have problem with serialization. It serializes transform property as -moz-transform. I need to find a way to workaround that.

Manishearth · 2017-04-13T23:55:13Z

You'll need to special case serialization here.

Manishearth

This looks correct. I think you still need to fix the case where we explicitly ask for serialization.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24a503ecb5367d0c693312d7d2909f8b08ecdde3

Manishearth · 2017-04-14T06:06:20Z

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

+                        //
+                        // -moz-transform accepts LengthOrPercentageOrNumber in the
+                        //  nondiagonal homogeneous components. transform accepts only number.
+                        let mut values = vec![];


use Vec::with_capacity(4) and 2

canova · 2017-04-14T10:32:55Z

Oops, rebase fail. Will look at serialization soon.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb1257d23b860442c3e91071de9e4c99ec4e596f

canova · 2017-04-14T18:17:29Z

Err.. https://treeherder.mozilla.org/#/jobs?repo=try&revision=31b04c6bc0b9f4423edbc7149016e8885d7db875

canova · 2017-04-15T17:22:47Z

Ok, solved the serialization problem with that fixup commit. I wanted to solve it with mako code generation but couldn't. That works, but maybe we can do better.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=380b80b0dcfac49f639a50d0b79fdf719cbe64f1

bors-servo · 2017-04-16T05:56:44Z

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

Manishearth

r=me

canova · 2017-04-19T22:15:35Z

@bors-servo r=Manishearth

bors-servo · 2017-04-19T22:15:36Z

📌 Commit f9225d8 has been approved by Manishearth

bors-servo · 2017-04-19T22:37:09Z

⌛ Testing commit f9225d8 with merge b8b4ec9...

stylo: Implement -moz-transform property -moz-transform property is an alias shorthand property. It has transform as sub-property but their parsing is different in some cases(matrix/matrix3d). -moz-transform also accepts LengthOrPercentage in the nondiagonal homogeneous components of matrix and matrix3d. It looks like length and percentage values are getting computed to number directly. For example 120px converts into 120 and calc(120px + 20%) converts into 140. So I did like this. But we need to check this behavior again to be sure, I guess. Also I spent half day investigating why matrices are not serializing after this change and found out their ToCss functions is not completely implemented yet 😄 I was going to implement them but there is an easy issue about it(#15194) so I left it that way. Probably all tests won't pass without it. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #16003 and [Bug 1351356](https://bugzilla.mozilla.org/show_bug.cgi?id=1351356)    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16231)

bors-servo · 2017-04-19T23:13:41Z

☀️ 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-msvc-dev
Approved by: Manishearth
Pushing b8b4ec9 to master...

highfive assigned Manishearth Apr 2, 2017

canova commented Apr 2, 2017

View reviewed changes

Manishearth reviewed Apr 2, 2017

View reviewed changes

emilio reviewed Apr 2, 2017

View reviewed changes

wafflespeanut reviewed Apr 2, 2017

View reviewed changes

canova force-pushed the moz-transform branch from e134581 to 8cc110d Compare April 2, 2017 17:24

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 2, 2017

canova force-pushed the moz-transform branch from 8cc110d to 5ddf959 Compare April 2, 2017 17:29

emilio mentioned this pull request Apr 3, 2017

Implement serialization for transform functions. #16242

Merged

5 tasks

highfive added the S-needs-rebase There are merge conflict errors. label Apr 4, 2017

canova force-pushed the moz-transform branch from 5ddf959 to 68d02fb Compare April 8, 2017 16:28

canova force-pushed the moz-transform branch from 68d02fb to 34c69f0 Compare April 8, 2017 16:47

Manishearth requested changes Apr 9, 2017

View reviewed changes

highfive added the S-needs-rebase There are merge conflict errors. label Apr 10, 2017

canova force-pushed the moz-transform branch 3 times, most recently from 867d0d7 to 3bd8523 Compare April 11, 2017 23:40

highfive added the S-needs-rebase There are merge conflict errors. label Apr 12, 2017

canova force-pushed the moz-transform branch from 3bd8523 to 4624aba Compare April 13, 2017 21:08

Manishearth approved these changes Apr 14, 2017

View reviewed changes

canova force-pushed the moz-transform branch from 4624aba to 29c9de6 Compare April 14, 2017 10:23

canova force-pushed the moz-transform branch from 29c9de6 to 96b3299 Compare April 14, 2017 18:01

highfive added the S-needs-rebase There are merge conflict errors. label Apr 16, 2017

canova added 2 commits April 19, 2017 22:08

Create a generic flag system for properties

67799f9

Implement moz-transform property

f9225d8

canova force-pushed the moz-transform branch from d8e7be3 to f9225d8 Compare April 19, 2017 21:32

Manishearth approved these changes Apr 19, 2017

View reviewed changes

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Apr 19, 2017

bors-servo merged commit f9225d8 into servo:master Apr 19, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 19, 2017

bors-servo mentioned this pull request Apr 19, 2017

Cleanup various modules and introduce generic types #16444

Merged

canova deleted the moz-transform branch April 25, 2017 20:08

stylo: Implement -moz-transform property #16231

stylo: Implement -moz-transform property #16231

Conversation

canova commented Apr 2, 2017 • edited

canova commented Apr 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canova commented Apr 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canova Apr 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Apr 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wafflespeanut Apr 2, 2017 • edited

Choose a reason for hiding this comment

bors-servo commented Apr 4, 2017

canova commented Apr 8, 2017 • edited

Manishearth commented Apr 9, 2017

Manishearth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Apr 10, 2017

bors-servo commented Apr 12, 2017

canova commented Apr 13, 2017

Manishearth commented Apr 13, 2017

Manishearth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canova commented Apr 14, 2017

canova commented Apr 14, 2017

canova commented Apr 15, 2017

bors-servo commented Apr 16, 2017

Manishearth left a comment

Choose a reason for hiding this comment

canova commented Apr 19, 2017

bors-servo commented Apr 19, 2017

bors-servo commented Apr 19, 2017

bors-servo commented Apr 19, 2017

canova commented Apr 2, 2017 •

edited

canova Apr 2, 2017 •

edited

wafflespeanut Apr 2, 2017 •

edited

canova commented Apr 8, 2017 •

edited