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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: xidorn, Assigned: kuoe0)

Tracking

(Blocks: 2 bugs)

53 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
(Assignee)

Updated

9 months ago
Assignee: nobody → kuoe0

Updated

9 months ago
Priority: -- → P1
(Assignee)

Comment 1

9 months ago
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
Flags: needinfo?(xidorn+moz)
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
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 3

9 months ago
I think I got the point! Thx!
Comment hidden (mozreview-request)
(Assignee)

Comment 5

9 months ago
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?
Attachment #8857874 - Flags: feedback?(xidorn+moz)
(Reporter)

Comment 6

9 months ago
mozreview-review
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.
Attachment #8857874 - Flags: feedback?(xidorn+moz) → feedback+
(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 hidden (mozreview-request)
(Assignee)

Comment 9

9 months ago
(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!
(Assignee)

Comment 10

9 months ago
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
(Assignee)

Comment 11

9 months ago
This patch already landed in autoland[1].

[1]: https://hg.mozilla.org/integration/autoland/rev/219973c90b4a
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8857874 - Flags: review?(aryx.bugmail)
(Assignee)

Updated

9 months ago
Attachment #8857874 - Flags: review?(xidorn+moz)

Comment 15

9 months ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b997075e6722
Update stylo-failures.md r=me a=me

Comment 16

9 months ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c1d007c216cc
Update stylo-failures.md r=me a=me
https://hg.mozilla.org/mozilla-central/rev/0cb37bb22087
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.