Closed
Bug 1355017
Opened 7 years ago
Closed 7 years ago
stylo: background-position should generate valid three-value form when from longhands
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: xidorn, Assigned: kuoe0.tw)
References
Details
Attachments
(1 file)
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•7 years ago
|
Assignee: nobody → kuoe0
Updated•7 years ago
|
Priority: -- → P1
Assignee | ||
Comment 1•7 years 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)
Reporter | ||
Comment 2•7 years ago
|
||
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•7 years ago
|
||
I think I got the point! Thx!
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years 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•7 years 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.
Reporter | ||
Updated•7 years ago
|
Attachment #8857874 -
Flags: feedback?(xidorn+moz) → feedback+
Reporter | ||
Comment 7•7 years ago
|
||
(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•7 years 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•7 years 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•7 years ago
|
||
This patch already landed in autoland[1]. [1]: https://hg.mozilla.org/integration/autoland/rev/219973c90b4a
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8857874 -
Flags: review?(aryx.bugmail)
Assignee | ||
Updated•7 years ago
|
Attachment #8857874 -
Flags: review?(xidorn+moz)
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8857874 [details] Bug 1355017 - Update stylo-failures.md. https://reviewboard.mozilla.org/r/129898/#review132996
Attachment #8857874 -
Flags: review?(aryx.bugmail) → review+
Comment 14•7 years ago
|
||
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/0cb37bb22087 Update stylo-failures.md. r=aryx
Comment 15•7 years ago
|
||
Pushed by kwierso@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b997075e6722 Update stylo-failures.md r=me a=me
Comment 16•7 years ago
|
||
Pushed by kwierso@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c1d007c216cc Update stylo-failures.md r=me a=me
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0cb37bb22087
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b997075e6722 https://hg.mozilla.org/mozilla-central/rev/c1d007c216cc
You need to log in
before you can comment on or make changes to this bug.
Description
•