Added image-orientation property #14498

dpyro · 2016-12-08T19:46:21Z

Implemented as per the MDN documentation—I could not find a current CSS draft. I am not sure if this is the complete correct metadata for the longhand helper:

<%helpers:longhand name="image-orientation”
                   experimental=“True”
                   animatable=“False”>

Also I am not sure how to test this and would appreciate help in creating a testing strategy.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #13877.

There are tests for these changes

This change is

Manishearth

Needs tests and should not be enabled in anything but testing mode.

Manishearth · 2016-12-08T19:50:08Z

components/script/dom/webidls/CSSStyleDeclaration.webidl

@@ -359,6 +359,8 @@ partial interface CSSStyleDeclaration {

  [SetterThrows, TreatNullAs=EmptyString] attribute DOMString imageRendering;
  [SetterThrows, TreatNullAs=EmptyString] attribute DOMString image-rendering;
+  [SetterThrows, TreatNullAs=EmptyString] attribute DOMString imageOrientation;


Please remove these

Manishearth · 2016-12-08T19:50:08Z

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

@@ -50,6 +50,131 @@ ${helpers.single_keyword("image-rendering",
                         custom_consts=image_rendering_custom_consts,
                         animatable=False)}

+// Image Orientation
+// https://developer.mozilla.org/en-US/docs/Web/CSS/image-orientation
+<%helpers:longhand name="image-orientation"


Please add products="none" here, and add parse tests for it (see https://github.com/servo/servo/wiki/Property-hacking-guide#testing). Use ./mach test-unit -p style to test.

wafflespeanut · 2016-12-08T20:31:30Z

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

+        }
+    }
+
+    pub fn parse(_context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {


Nit: Since we're using the context inside, we should probably rename this to context?

I used the parameter names from the property hacking guide. Will change.

wafflespeanut · 2016-12-08T20:31:30Z

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

+        } else {
+            // Handle flip
+            input.expect_ident_matching("flip")
+            .map(|()| SpecifiedValue { angle: Some(Angle::from_radians(0f32)), flipped: true })


Nit: Should we have f32 in the suffix here? (and everywhere?) Doesn't rustc figure this out?

We can write 0. or 0.0 as an alternative to this. Rust considers it as an integer otherwise.

I will double-check but it would not take a plain 0 by itself.

wafflespeanut · 2016-12-08T20:31:30Z

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

+    fn normalize_angle(angle: &Angle) -> Angle {
+        let radians = angle.radians();
+        let normalized_radians = ((radians % TWO_PI) + TWO_PI) % TWO_PI;
+        Angle::from_radians(normalized_radians)


Does this really work as expected? The spec says that this should rounded to the nearest "90 deg"

Yes, this is a careless error. I will be testing to make sure I have the correct implementation.

Manishearth · 2016-12-09T00:23:44Z

r? @canaltinova

KiChjang · 2016-12-09T01:38:52Z

@bors-servo delegate=canaltinova

bors-servo · 2016-12-09T01:38:52Z

✌️ @canaltinova can now approve this pull request

canova

This looks good! Some comments:

canova · 2016-12-09T15:01:14Z

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

+// Image Orientation
+// https://developer.mozilla.org/en-US/docs/Web/CSS/image-orientation
+<%helpers:longhand name="image-orientation"
+                   experimental="True"


You can remove this line I think. This property is not used by servo yet.

canova · 2016-12-09T15:01:14Z

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

@@ -50,6 +50,132 @@ ${helpers.single_keyword("image-rendering",
                         custom_consts=image_rendering_custom_consts,
                         animatable=False)}

+// Image Orientation
+// https://developer.mozilla.org/en-US/docs/Web/CSS/image-orientation


You should write spec link(https://drafts.csswg.org/css-images/#the-image-orientation) instead of mdn link here

canova · 2016-12-09T15:01:14Z

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

+            // Handle flip
+            input.expect_ident_matching("flip")
+            .map(|()| SpecifiedValue { angle: Some(INITIAL_ANGLE), flipped: true })
+        }


I think you can unify this else if and else parts like this:

} else { let angle = input.try(|i| Angle::parse(context, i)).or(Some(INITIAL_ANGLE)); let flipped = input.try(|input| input.expect_ident_matching("flip")).is_ok(); Ok(SpecifiedValue { angle: angle, flipped: flipped }) }

Should we be able to return a parse error or just silently ignore? Ex. 0deg fl

We don't need to return a parse error for this. It will parse it as 0deg. After this parsing, fl will be unconsumed from parser and will try to consume ; but it'll find fl instead and fail.

canova · 2016-12-09T15:01:14Z

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

+        #[derive(Clone, PartialEq, Copy, Debug)]
+        #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
+        pub enum T {
+            FromImage,


Computed value: an , rounded and normalized (see text), plus optionally a flip keyword

According to spec, FromImage should be converted to Angle in computed value. To do that, we need to access image metadata. But I don't think we can do that here. Maybe this process needs to be done in glue/layout parts? What do you think @Manishearth?

Thanks for the spec link. Should the computed value v be bounded to -180deg <= v < 180deg or 0 <= v < 360deg?

The computed value of the property is calculated by rounding the specified angle to the nearest quarter-turn (90deg, 100grad, .25turn, etc.), rounding away from 0 (that is, 45deg is rounded to 90deg, while -45deg is rounded to -90deg), then moduloing the value by 1 turn (360deg, 400grad, etc.).

It says 0-360 degrees. Btw, normalize_angle function seems wrong. We should round radians to nearest quarter. But currently adding a quarter instead of rounding. You might want to look how gecko does:
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h#2165-2171

Oops, sorry I didn't see the last commit about normalize_angle. Will look at it.

So while it does say that the computed value is based on the image, we can defer that to layout (which is what Gecko does). Keep the computed value as is for now.

canova · 2016-12-09T15:01:14Z

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

+        computed_value::T::AngleWithFlipped(INITIAL_ANGLE, false)
+    }
+
+    use values::CSSFloat;


nit: These declarations can be moved to top, where other use declarations are.

canova

It looks good to me! Could you squash your commits?

canova · 2016-12-09T23:01:15Z

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

+            rounded_quarter_turns % 4.0
+        };
+        let normalized_radians = normalized_quarter_turns/4.0 * TWO_PI;
+        Angle::from_radians(normalized_radians)


Well, I didn't quite understand this function but I tried with some numbers and it looks working 😄

dpyro · 2016-12-10T01:40:47Z

@canaltinova How would I go about squashing the commits?

canova · 2016-12-10T12:24:53Z

@dpyro You can see how to squash here: https://github.com/servo/servo/wiki/Beginner's-guide-to-rebasing-and-squashing#squashing


        Added image-orientation property

canova · 2016-12-10T21:52:20Z

@bors-servo r+
Thanks for doing this!

bors-servo · 2016-12-10T21:52:20Z

📌 Commit faddb0c has been approved by canaltinova

bors-servo · 2016-12-10T21:52:22Z

⌛ Testing commit faddb0c with merge 5c2d401...


        Auto merge of #14498 - dpyro:image-orientation, r=canaltinova

Added image-orientation property  Implemented as per the MDN documentation—I could not find a current CSS draft. I am not sure if this is the complete correct metadata for the longhand helper: ``` <%helpers:longhand name="image-orientation” experimental=“True” animatable=“False”> ``` Also I am not sure how to test this and would appreciate help in creating a testing strategy. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13877.  - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14498)

bors-servo · 2016-12-10T21:52:55Z

💔 Test failed - mac-rel-wpt2

canova · 2016-12-10T21:57:12Z

@bors-servo retry

network error

bors-servo · 2016-12-10T21:57:14Z

⌛ Testing commit faddb0c with merge 37c4051...


        Auto merge of #14498 - dpyro:image-orientation, r=canaltinova

Added image-orientation property  Implemented as per the MDN documentation—I could not find a current CSS draft. I am not sure if this is the complete correct metadata for the longhand helper: ``` <%helpers:longhand name="image-orientation” experimental=“True” animatable=“False”> ``` Also I am not sure how to test this and would appreciate help in creating a testing strategy. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13877.  - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14498)

bors-servo · 2016-12-10T23:00:07Z

💔 Test failed - linux-rel-css

KiChjang · 2016-12-11T08:33:39Z

@bors-servo retry

#13919

bors-servo · 2016-12-11T08:33:41Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-css...

bors-servo · 2016-12-11T08:40:25Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-css...

Manishearth · 2016-12-11T08:40:46Z

This is intermittent, updated #13919

bors-servo · 2016-12-11T09:20:10Z

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

Manishearth reviewed Dec 8, 2016

View changes

wafflespeanut reviewed Dec 8, 2016

View changes

jdm assigned Manishearth Dec 8, 2016

highfive assigned canova and unassigned Manishearth Dec 9, 2016

highfive added the S-awaiting-review label Dec 9, 2016

canova reviewed Dec 9, 2016

View changes

Added image-orientation property

Loading status checks…

faddb0c

dpyro force-pushed the dpyro:image-orientation branch from 81bfff1 to faddb0c Dec 10, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 10, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 10, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 10, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 10, 2016

bors-servo merged commit faddb0c into servo:master Dec 11, 2016
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

servo / servo

Join GitHub today

Added image-orientation property #14498

Added image-orientation property #14498

Conversation

dpyro commented Dec 8, 2016 • 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Dec 9, 2016

KiChjang commented Dec 9, 2016

bors-servo commented Dec 9, 2016

canova left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

canova Dec 9, 2016 • edited

This comment has been minimized.

This comment has been minimized.

canova Dec 9, 2016 • edited

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.

canova left a comment

This comment has been minimized.

dpyro commented Dec 10, 2016

canova commented Dec 10, 2016

canova commented Dec 10, 2016

bors-servo commented Dec 10, 2016

bors-servo commented Dec 10, 2016

bors-servo commented Dec 10, 2016

canova commented Dec 10, 2016

bors-servo commented Dec 10, 2016

bors-servo commented Dec 10, 2016

KiChjang commented Dec 11, 2016 • edited by Manishearth

bors-servo commented Dec 11, 2016

bors-servo commented Dec 11, 2016

Manishearth commented Dec 11, 2016

bors-servo commented Dec 11, 2016

dpyro commented Dec 8, 2016 •

edited

canova Dec 9, 2016 •

edited

canova Dec 9, 2016 •

edited

KiChjang commented Dec 11, 2016 •

edited by Manishearth