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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Mats Palmgren (inactive)]

https://drafts.csswg.org/css-logical/#propdef-padding-inline

Comment 1

[Mats Palmgren (inactive)]

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 3

[Mats Palmgren (inactive)]

Intent to ship post:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/gpubhXTxI6o

Comment 4

[Mats Palmgren (inactive)]

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

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

Comment 5

[Emilio Cobos Álvarez (: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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c15d04c80f5b
[css-logical] Implement the padding-block/inline shorthands.  r=emilio

Comment 7

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/c15d04c80f5b

Comment 8

[Chris Mills [:cmills]]

Docs completed; Rachel Andrew created the docs a while ago, and recently updated the compat data too:

https://developer.mozilla.org/en-US/docs/Web/CSS/padding-block#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/CSS/padding-inline#Browser_compatibility

I also added a note to the Fx66 rel notes:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/66#CSS