Handle 3- and 4- valued <position>s in style #13042

canova · 2016-08-25T21:15:01Z

This is first part of fix #12690 and covers just specified style part for now.

These changes fix #12690 (github issue number if applicable).

This change is


        Handle 3- and 4- valued <position>s in specified style

highfive · 2016-08-25T21:15:05Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/background.mako.rs, components/style/values/specified/basic_shape.rs, components/style/values/specified/position.rs

highfive · 2016-08-25T21:15:06Z

Warning

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

canova · 2016-08-25T21:16:48Z

@Manishearth Sorry accidentally deleted the branch and it closed the other pr :( (-d instead of -f) Updated the commit could you look at it when you are available?

This is not building yet. to_computed_value function needs to change because horiz_position and vert_position need unwrapping. But I wanted to wait until this function change.
Destructing the Option values messed the code :(

izgzhen · 2016-08-25T23:18:16Z

components/style/values/specified/position.rs

+            keyword == Keyword::Top => LengthOrPercentage::Percentage(Percentage(0.0)),
+            PositionComponent::Keyword(keyword) if keyword == Keyword::Right ||
+            keyword == Keyword::Bottom => LengthOrPercentage::Percentage(Percentage(1.0)),
+            PositionComponent::Keyword(_) => unimplemented!(), // TODO: All keywords are covered but rust forcing me to add this too?


Maybe match further on keyword looks better and eliminate the compiler's complain?

You are right. Probably this is better solution. I'll update soon, thanks!

Manishearth · 2016-08-26T06:21:43Z

components/style/values/specified/basic_shape.rs

@@ -265,8 +265,10 @@ impl Circle {
        } else {
            // Defaults to origin
            Position {
-                horizontal: LengthOrPercentage::Percentage(Percentage(0.5)),


Store it as center center

Manishearth · 2016-08-26T06:24:13Z

components/style/values/specified/position.rs

 }

 impl ToCss for Position {
    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
-        try!(self.horizontal.to_css(dest));
+        // TODO: canaltinova: We should add keywords, probably?


Yes, this could also be just a keyword. Print the components in order, with spaces between them if they exist.

Actually if background position is center bottom 10px, Position::parse function sends center as value and it stores as 50% bottom 10px. Keywords are storing if second keyword is explicitly defined. Otherwise it's assuming LengthOrPercentage. Is it a bad behaviour? If so, we have to check PositionComponents at Position::parse whether if they are keyword or length.

Yes, this is not the right behavior. This was done because basic-shape has different requirements for Position serialization, which I now know to not generalize to all positions.

We should store keywords as-is, and only reorder. The ToCss impl should be relatively simple, so that we can add any additional complexity to the basic-shape side. It's fine if you break basic-shape for now, I'll fix it later.

Manishearth · 2016-08-26T06:24:50Z

components/style/values/specified/position.rs

            -> Result<Position, ()> {
-        let (horiz, vert) = match (category(first), category(second)) {
+        // Check firts and second positions, this is more like for 2 value backgrounds.


comment spelling

I'll fix thanks!

Manishearth · 2016-08-26T06:27:45Z

Could you write tests for these in tests/unit/style/parsing/position.rs (by using the examples from the spec)?

Your changes may make the basic shape tests fail. That is okay, comment them out for now, I'll fix them later. basic-shape serializes positions differently.

Manishearth · 2016-08-26T06:36:59Z

components/style/values/specified/position.rs

+        if let Ok(third) = input.try(PositionComponent::parse) {
+            if let Ok(fourth) = input.try(PositionComponent::parse) {
+                // Handle 4 value background position
+                Position::new(Some(second), Some(fourth), Some(first), Some(third))


4-value can also be specified with the horizontal ones first.

Oh, nvm, Position::new solves that.

Manishearth · 2016-08-26T06:39:00Z

components/style/values/specified/position.rs

+impl ToCss for Keyword {
+    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+        match *self {
+            Keyword::Right => try!(dest.write_str("right ")),


left, top, and center also serialize.

I didn't write them because of comment above. Just explicit keywords are storing.

This is also wrong, my bad: we want to store explicit keywords for background-positions and general positions. basic-shape needs special treatment whilst serializing.

Manishearth · 2016-08-26T06:42:03Z

components/style/values/specified/position.rs

@@ -107,16 +233,16 @@ impl ToComputedValue for Position {
    #[inline]
    fn to_computed_value(&self, context: &Context) -> computed_position::Position {
        computed_position::Position {
-            horizontal: self.horizontal.to_computed_value(context),


This shouldn't be too hard to implement now. You can convert each position to a pair of Options for length and percentage, and then construct a calc value out of them. In case the keyword is right or bottom, you make the len/percentage negative and add 100%. For center, you add 50% but don't make it negative.

canova · 2016-08-27T18:54:41Z

@Manishearth Made a new commit can you review when you have time?

I made a new commit instead of squashing first, so you can review more easily. After last changes I can squash them.

canova · 2016-08-27T18:59:28Z

tests/unit/style/parsing/position.rs

+    assert_roundtrip!(Position::parse, "right 10px bottom", "right 10px bottom");
+    assert_roundtrip!(Position::parse, "bottom right 10px", "right 10px bottom");
+    assert_roundtrip!(Position::parse, "center right 10px", "right 10px center");
+    assert_roundtrip!(Position::parse, "center bottom 10px", "center bottom 10px");


Added some additional example apart from specs to cover most of cases.

canova · 2016-08-27T19:00:10Z

tests/unit/style/parsing/basic_shape.rs

@@ -77,6 +77,7 @@ fn test_border_radius() {

 #[test]
 fn test_circle() {
+    /*


These tests were failing as you said before. Commented them.

Open an issue and leave a comment linking to them 😄

(or just link to a comment on this issue)

Opened an issue: #13083

canova · 2016-08-27T19:02:22Z

tests/unit/style/parsing/position.rs

+    assert_roundtrip!(Position::parse, "center bottom", "center bottom");
+    assert_roundtrip!(Position::parse, "center 10px", "center 10px");
+    assert_roundtrip!(Position::parse, "center 10%", "center 10%");
+    assert_roundtrip!(Position::parse, "right 10%", "right 10%");


After new behavior these tests needed to change too.

Manishearth · 2016-08-27T19:13:01Z

components/style/values/specified/position.rs

+                }
+            },
+            Keyword::Center => {
+                horiz_keyword.to_length_or_percentage().to_computed_value(context)


center is 50% + length, so you need to take the length, and add 50% to its calc value percentage

I wrote in irc but I'm writing here just in case :)
By 50% + length you mean we can write positions like this center 10px center 10px? I tried it in firefox and chrome but it seems invalid. And spec is also agree:

[ center | [ left | right ] ? ] &&
[ center | [ top | bottom ] ? ]

But I might misunderstood your comment I'm not sure :)

No, you are right. Make sure you error if the length is anything but none here.

Manishearth · 2016-08-27T19:19:24Z

Looking good, with a few small issues.

Manishearth · 2016-08-27T19:20:19Z

components/style/values/specified/position.rs

        };
+
+        // Unwrap positions from PositionComponent and wrap with Option
+        let (horiz_position, vert_position) = if let Some(PositionComponent::Length(horiz_pos)) = first_position {


You can use match first_position, second_position) here. But this is ok too.

Fixed it thanks!


        Change parsing behavior and implement to_computed_value function

Manishearth · 2016-08-28T01:26:18Z

@bors-servo try

bors-servo · 2016-08-28T01:26:21Z

⌛ Trying commit 8372f29 with merge 255da48...


        Auto merge of #13042 - canaltinova:position, r=<try>

@Manishearth

Handle 3- and 4- valued <position>s in style  This is first part of fix #12690 and covers just specified style part for now. r? @Manishearth --- - [X] These changes fix #12690 (github issue number if applicable).   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13042)

bors-servo · 2016-08-28T02:14:06Z

💔 Test failed - linux-rel

highfive · 2016-08-28T02:14:09Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-table-007.htm
  └   → /css-transforms-1_dev/html/transform-table-007.htm a5c014b20ef1363bea6f24eda28c7efb7c45698a
/css-transforms-1_dev/html/reference/transform-blank-ref.htm fa6407b1acbbfea27e27061e7d1bdeca98e4a728
Testing a5c014b20ef1363bea6f24eda28c7efb7c45698a == fa6407b1acbbfea27e27061e7d1bdeca98e4a728

Manishearth · 2016-08-28T02:19:39Z

@bors-servo retry

#11574

bors-servo · 2016-08-28T02:19:42Z

⌛ Trying commit 8372f29 with merge 832d01f...


        Auto merge of #13042 - canaltinova:position, r=<try>

@Manishearth

Handle 3- and 4- valued <position>s in style  This is first part of fix #12690 and covers just specified style part for now. r? @Manishearth --- - [X] These changes fix #12690 (github issue number if applicable).   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13042)

bors-servo · 2016-08-28T02:20:36Z

⌛ Trying commit 8372f29 with merge 3c995ab...


        Auto merge of #13042 - canaltinova:position, r=<try>

@Manishearth

Handle 3- and 4- valued <position>s in style  This is first part of fix #12690 and covers just specified style part for now. r? @Manishearth --- - [X] These changes fix #12690 (github issue number if applicable).   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13042)

bors-servo · 2016-08-28T03:58:33Z

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

Manishearth · 2016-08-28T04:07:30Z

Needs to error on center NotKeyword and we're good

Manishearth · 2016-08-28T04:08:06Z

Oh, it already does, just not there.

@bors-servo r+

bors-servo · 2016-08-28T04:08:07Z

📌 Commit 8372f29 has been approved by Manishearth

canova · 2016-08-29T13:41:56Z

@Manishearth Is something wrong with bors? :)

Manishearth · 2016-08-29T13:48:54Z

@bors-servo r+ try- retry

it doesn't forget the try state.

bors-servo · 2016-08-29T13:48:55Z

💡 This pull request was already approved, no need to approve it again.

bors-servo · 2016-08-29T13:48:56Z

📌 Commit 8372f29 has been approved by Manishearth

bors-servo · 2016-08-29T13:48:57Z

⌛ Testing commit 8372f29 with merge b8d725d...


        Auto merge of #13042 - canaltinova:position, r=Manishearth

@Manishearth

Handle 3- and 4- valued <position>s in style  This is first part of fix #12690 and covers just specified style part for now. r? @Manishearth --- - [X] These changes fix #12690 (github issue number if applicable).   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13042)

bors-servo · 2016-08-29T14:45:54Z

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

Manishearth · 2016-08-29T14:51:38Z

Thanks!

canova · 2016-08-29T15:21:04Z

@Manishearth Thank you also for mentoring me on this :) I learned so much and you helped me a lot. I'm sorry if I took a lot of your time :)

Manishearth · 2016-08-29T15:33:41Z

Not much of my time -- most of the time went into figuring out what we were supposed to do, which I would have had to do anyway if I'd have done it myself.

And mentoring is fun and it's great to see new contributors work on this project, so it's definitely not a waste of time 😄


        Auto merge of #13122 - Manishearth:basic-shape-position-redux, r=Simo…

@SimonSapin

…nSapin Handle specialized serialization of <position> in basic shapes Fixes #13083 We temporarily broke basic-shape serialization in #13042 when 4-value positions were implemented, since I didn't want to increase the scope of that PR too much. This fixes it. r? @SimonSapin cc @canaltinova  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13122)

Handle 3- and 4- valued <position>s in specified style

Loading status checks…

4eb93c8

highfive assigned Manishearth Aug 25, 2016

highfive added the S-awaiting-review label Aug 25, 2016

izgzhen reviewed Aug 25, 2016
View changes

Manishearth reviewed Aug 26, 2016
View changes

canova reviewed Aug 27, 2016
View changes

Manishearth reviewed Aug 27, 2016
View changes

Change parsing behavior and implement to_computed_value function

Loading status checks…

8372f29

canova force-pushed the canova:position branch from 27fb9e6 to 8372f29 Aug 27, 2016

canova mentioned this pull request Aug 27, 2016

Fix basic shape's position serialization #13083

Closed

highfive added the S-tests-failed label Aug 28, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-tests-failed labels Aug 28, 2016

bors-servo merged commit 8372f29 into servo:master Aug 29, 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

highfive removed the S-awaiting-merge label Aug 29, 2016

canova deleted the canova:position branch Aug 29, 2016

Manishearth mentioned this pull request Aug 30, 2016

Handle specialized serialization of <position> in basic shapes #13122

Merged

servo / servo

Join GitHub today

Handle 3- and 4- valued <position>s in style #13042

Handle 3- and 4- valued <position>s in style #13042

Conversation

canova commented Aug 25, 2016 • edited by larsbergstrom

highfive commented Aug 25, 2016

highfive commented Aug 25, 2016

canova commented Aug 25, 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.

canova Aug 26, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Aug 26, 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 commented Aug 27, 2016

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 Aug 27, 2016

This comment has been minimized.

This comment has been minimized.

Manishearth commented Aug 28, 2016

bors-servo commented Aug 28, 2016

bors-servo commented Aug 28, 2016

highfive commented Aug 28, 2016

Manishearth commented Aug 28, 2016 • edited

bors-servo commented Aug 28, 2016

bors-servo commented Aug 28, 2016

bors-servo commented Aug 28, 2016

Manishearth commented Aug 28, 2016

Manishearth commented Aug 28, 2016

bors-servo commented Aug 28, 2016

canova commented Aug 29, 2016

Manishearth commented Aug 29, 2016

bors-servo commented Aug 29, 2016

bors-servo commented Aug 29, 2016

bors-servo commented Aug 29, 2016

bors-servo commented Aug 29, 2016

Manishearth commented Aug 29, 2016

canova commented Aug 29, 2016

Manishearth commented Aug 29, 2016

canova commented Aug 25, 2016 •

edited by larsbergstrom

canova commented Aug 25, 2016 •

edited

canova Aug 26, 2016 •

edited

Manishearth commented Aug 26, 2016 •

edited

Manishearth commented Aug 28, 2016 •

edited