[css-logical] Implement the padding-block/inline shorthands

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P3
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

({dev-doc-complete})

Trunk
mozilla66
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment)

Comment on attachment 9036320 [details] [diff] [review]
[css-logical] Implement the padding-block/inline shorthands

Review of attachment 9036320 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, please do send an intent to implement / ship to dev-platform.

Thanks! (And sorry for the lag with the other reviews, I need to find some time to go through all of them :/)

::: servo/components/style/properties/shorthands/padding.mako.rs
@@ +27,5 @@
> +            input: &mut Parser<'i, 't>,
> +        ) -> Result<Longhands, ParseError<'i>> {
> +            let start_value = NonNegativeLengthPercentage::parse(context, input)?;
> +            let end_value =
> +                input.try(|input| NonNegativeLengthPercentage::parse(context, input)).unwrap_or(start_value.clone());

nit: unwrap_or_else(|| start_value.clone()) avoids the copy if you specify both values.

@@ +38,5 @@
> +
> +        impl<'a> ToCss for LonghandsToSerialize<'a>  {
> +            fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result where W: fmt::Write {
> +                self.padding_${axis}_start.to_css(dest)?;
> +        

nit: Please remove the trailing whitespace.

@@ +43,5 @@
> +                if self.padding_${axis}_end != self.padding_${axis}_start {
> +                    dest.write_str(" ")?;
> +                    self.padding_${axis}_end.to_css(dest)?;
> +                }
> +        

nit: And here.

::: testing/web-platform/meta/css/css-logical/logical-box-padding.html.ini
@@ +2,1 @@
>    [Test that padding shorthand sets longhands and serializes correctly.]

Why are these still failing? Do they test the computed style serialization of the shorthands or something?

If so, it'd be good to add `bug: https://bugzilla.mozilla.org/show_bug.cgi?id=137688` to these lines.
Attachment #9036320 - Flags: review?(emilio) → review+
Assignee

Comment 4

4 months ago

.unwrap_or_else(|| start_value.clone());
^^^^^^^^^^^^^^ -- takes 0 arguments
|
expected closure that takes 1 argument

So, is |_| the way to solve that?

Flags: needinfo?(emilio)

Yup, sorry. unwrap_or_else on Option<T> takes no arguments, on Result<T, E> takes the error, which in this case we ignore, so _ is the right thing to signal that.

Flags: needinfo?(emilio)

Comment 6

4 months ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c15d04c80f5b
[css-logical] Implement the padding-block/inline shorthands.  r=emilio

Comment 7

4 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Updated

4 months ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.