1355017 - stylo: background-position should generate valid three-value form when from longhands

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Xidorn Quan [:xidorn] UTC+10]

In Stylo,
> background-position-x: 10%;
> background-position-y: top 3em;
generates
> background-position: 10% top 3em;
which is invalid. The correct form is
> background-position: left 10% top 3em;

The serialization of background-position should probably be implemented as a whole rather than calling to_css on its two subproperties separately.

Comment 1

[Chih-Hsuan Kuo [:tommykuo]]

Hi Xidorn, to change the serialization of background-position, is this the right function[1] to update? And I have some question. What should it be for the following cases?

(1)
> background-position-x: center;
> background-position-y: top 3em;

(2)
> background-position-x: right;
> background-position-y: top 3em;

[1]: https://dxr.mozilla.org/mozilla-central/source/servo/components/style/values/specified/position.rs#33-58

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

You need to read the spec [1]. Basically you need to ensure that the serialization is always a valid input, and in general it should be as simple as possible.


[1] https://drafts.csswg.org/css-backgrounds-3/#the-background-position

Comment 3

[Chih-Hsuan Kuo [:tommykuo]]

I think I got the point! Thx!

Comment 4

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1355017 - Update stylo-failures.md.

If there is any longhand consisting of both keyword and position, we
should attach both keyword and position for both longhands when serialize
them.

To generate a serialized result with keyword, we add a new trait
`ToCssWithKeyword` to finish that. For the longhands which missing
keyword, we use the default keyword for them.
(left for background-position-x and top for background-position-y)

Review commit: https://reviewboard.mozilla.org/r/129898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129898/

Comment 5

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8857874 [details]
Bug 1355017 - Update stylo-failures.md.

Xidorn, can you give me some feedback for this patch? And I have no idea how to test the serialized value, how do you test this and find this issue?

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8857874 [details]
Bug 1355017 - Update stylo-failures.md.

https://reviewboard.mozilla.org/r/129898/#review132468

Looks reasonable. Probably you should also make `mask-position` use this as well.

::: servo/components/style/properties/shorthand/background.mako.rs:231
(Diff revision 1)
> +                let ref x_key = self.background_position_x.0[i].keyword;
> +                let ref x_pos = self.background_position_x.0[i].position;
> +                let ref y_key = self.background_position_y.0[i].keyword;
> +                let ref y_pos = self.background_position_y.0[i].position;

Just use
```rust
let x = &self.background_position_x.0[i];
let y = &self.background_position_y.0[i];
```
should be easier.

::: servo/components/style/properties/shorthand/background.mako.rs:236
(Diff revision 1)
> +                // both keyword and position exist for horizontal element
> +                match (x_key.as_ref(), x_pos.as_ref()) {
> +                    (Some(_), Some(_)) => is_keyword_needed = true,
> +                    _ => {}
> +                };
> +
> +                // both keyword and position exist for vertical element
> +                match (y_key.as_ref(), y_pos.as_ref()) {
> +                    (Some(_), Some(_)) => is_keyword_needed = true,
> +                    _ => {}
> +                };
> +
> +                if is_keyword_needed {

You can just
```rust
(x.keyword.is_some() && x.position.is_some()) ||
(y.keyword.is_some() && y.position.is_some())
```

::: servo/components/style/values/specified/position.rs:33
(Diff revision 1)
> +/// Similar to `ToCss` trait and force to append **keyword** to the result.
> +pub trait ToCssWithKeyword {
> +    /// Serialize `self` in CSS syntax with **keyword**, writing to `dest`.
> +    fn to_css_with_keyword<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write;
> +
> +    /// Serialize `self` in CSS syntax with **keyword** and return a string.
> +    ///
> +    /// (This is a convenience wrapper for `to_css_with_keyword` and probably should not be overridden.)
> +    #[inline]
> +    fn to_css_with_keyword_string(&self) -> String {
> +        let mut s = String::new();
> +        self.to_css_with_keyword(&mut s).unwrap();
> +        s
> +    }
> +}

I don't think it makes much sense to have a new trait for this. Just add a new method for those two types should be enough.

::: servo/components/style/values/specified/position.rs:299
(Diff revision 1)
> +        if let Some(keyword) = self.keyword {
> +            try!(keyword.to_css(dest));
> +        } else {
> +            try!(Keyword::Left.to_css(dest));
> +        }
> +
> +        if let Some(ref position) = self.position {
> +            try!(dest.write_str(" "));
> +            try!(position.to_css(dest));
> +        };

You can use `?` syntax.

::: servo/components/style/values/specified/position.rs:442
(Diff revision 1)
> +        if let Some(keyword) = self.keyword {
> +            try!(keyword.to_css(dest));
> +        } else {
> +            try!(Keyword::Top.to_css(dest));
> +        }
> +
> +        if let Some(ref position) = self.position {
> +            try!(dest.write_str(" "));
> +            try!(position.to_css(dest));
> +        };

ditto.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #5)
> Xidorn, can you give me some feedback for this patch? And I have no idea how
> to test the serialized value, how do you test this and find this issue?

It would be good if you can add some tests to tests/unit/style/properties/serialization.rs for this.

Comment 8

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8857874 [details]
Bug 1355017 - Update stylo-failures.md.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129898/diff/1-2/

Comment 9

[Chih-Hsuan Kuo [:tommykuo]]

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #7)
> (In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #5)
> > Xidorn, can you give me some feedback for this patch? And I have no idea how
> > to test the serialized value, how do you test this and find this issue?
> 
> It would be good if you can add some tests to
> tests/unit/style/properties/serialization.rs for this.

I'll update the test cases later! Thx!

Comment 10

[Chih-Hsuan Kuo [:tommykuo]]

Since it's all servo changes, I create a PR[1] on GitHub and start the review process on it.

[1]: https://github.com/servo/servo/pull/16453

Comment 11

[Chih-Hsuan Kuo [:tommykuo]]

This patch already landed in autoland[1].

[1]: https://hg.mozilla.org/integration/autoland/rev/219973c90b4a

Comment 12

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8857874 [details]
Bug 1355017 - Update stylo-failures.md.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129898/diff/2-3/

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8857874 [details]
Bug 1355017 - Update stylo-failures.md.

https://reviewboard.mozilla.org/r/129898/#review132996

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0cb37bb22087
Update stylo-failures.md. r=aryx

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b997075e6722
Update stylo-failures.md r=me a=me

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1d007c216cc
Update stylo-failures.md r=me a=me

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0cb37bb22087

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b997075e6722
https://hg.mozilla.org/mozilla-central/rev/c1d007c216cc