Fix parsing behavior of text-shadow property#16754
Conversation
|
|
emilio
left a comment
emilio
left a comment
There was a problem hiding this comment.
Looks reasonable to me, if you double-check they use the exact same parsing rules. If so, r=me with or without the nit below.
| let mut iter = self.0.iter(); | ||
| if let Some(shadow) = iter.next() { | ||
| try!(shadow.to_css(dest)); | ||
| shadow.to_css(dest)?; |
There was a problem hiding this comment.
Given you're refactoring all this, why not:
match iter.next() {
Some(shadow) => shadow.to_css(dest)?,
None => return dest.write_str("none"),
}
// ...There was a problem hiding this comment.
Yeah, that looks better.
| } | ||
|
|
||
| impl From<Shadow> for SpecifiedTextShadow { | ||
| fn from(shadow: Shadow) -> SpecifiedTextShadow { |
There was a problem hiding this comment.
Given it's pretty much the same type, and they share parsing, the only thing that differs is the to_computed_value impl, right?
There was a problem hiding this comment.
Yes, also Shadow has 2 additional fields but they are not getting parsed if disable_spread_and_inset argument is true in parse method(which is true in that property).
|
I double-checked parsing methods and they are nearly same except third value rejects negative values in @bors-servo r=emilio |
|
📌 Commit c072f3a has been approved by |
highfive
added
S-awaiting-merge
Fix parsing behavior of text-shadow property Blur radius should reject negative values. There was already `Shadow` struct for properties like this. `filter: drop-shadow` also has the same syntax with that property. I thought sharing the same code would be better and used Shadow struct for parsing. Converted to SpecifiedTextShadow to get rid of unneccessary fields and to be able to convert its computed value. Maybe it would be better to avoid using `Shadow` or just using `Shadow` but sharing code and avoiding unneccessary fields seems better. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16754) <!-- Reviewable:end -->
|
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev |
highfive
removed
the
S-awaiting-merge
4 participants
Blur radius should reject negative values. There was already
Shadowstruct for properties like this.filter: drop-shadowalso has the same syntax with that property. I thought sharing the same code would be better and used Shadow struct for parsing. Converted to SpecifiedTextShadow to get rid of unneccessary fields and to be able to convert its computed value. Maybe it would be better to avoid usingShadowor just usingShadowbut sharing code and avoiding unneccessary fields seems better../mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is