1363875 - [css-align] Rename `auto` to `legacy` for `justify-items`

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Daniel Holbert [:dholbert] (PTO until 2/24)]

The CSSWG just resolved to rename the justify-items "auto" value to be called "legacy" instead.

IRC discussion is quoted on the github issue for this change:
https://github.com/w3c/csswg-drafts/issues/1318#issuecomment-300543630

...and there's some background information there, too.

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-align] Rename 'auto' to 'legacy' for 'justify-items' (in Gecko).

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-align] Rename 'auto' to 'legacy' for 'justify-items' (in Stylo).

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-align] Update the devtools properties file after renaming 'auto' to 'legacy' for 'justify-items' (automated change from running 'mach devtools-css-db').

Comment 4

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=062de7b9beebe2ce8b66affd99179bec32724a94

Comment 5

[Cameron McCormack (:heycam)]

Should we wait until we get a response to the questions you raise in https://github.com/w3c/csswg-drafts/issues/1318#issuecomment-301659397 and the following comment?

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8868019 [details] [diff] [review]
part 1 - [css-align] Rename 'auto' to 'legacy' for 'justify-items' (in Gecko).

Cancelling review for now.  Please re-request if/once you think the spec issues are resolved sufficiently.

Comment 7

[Javier Fernandez]

I think Mats' concerns on  https://github.com/w3c/csswg-drafts/issues/1318#issuecomment-301659397 have been addressed already:

  * Content Distribution properties (align-content, justify-content) don't allow <content-position> fallback in current syntax, so the ambiguities in the place-content shorthand that these fallabck were generating have gone now.
  * The spec editors stated that place-items shorthand doesn't accept 'legacy' keywords, so this source of ambiguity is also gone.

I'm planning to work on this for both Blink [1] and WebKit [2], so I guess Firefox could do the same and ensure we have compatible implementations for these Alignment properties.

1- https://crbug.com/726147 and https://crbug.com/726148 
2- https://webkit.org/b/172711 and https://webkit.org/b/172712

Comment 8

[Emilio Cobos Álvarez (:emilio)]

I'll fix this after bug 1430817.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1363875: [css-align]: Rename justify-items: auto to legacy.

Review commit: https://reviewboard.mozilla.org/r/238062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238062/

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8969329 [details]
Bug 1363875: [css-align]: Rename justify-items: auto to legacy.

https://reviewboard.mozilla.org/r/238062/#review243838

Looks great, thanks for bringing this up-to-date with the spec.

::: servo/components/style/values/computed/align.rs:38
(Diff revision 1)
> -    /// The specified value for the property. Can contain `auto`.
> +    /// The specified value for the property. Can contain `legacy`.
>      #[css(skip)]
>      pub specified: specified::JustifyItems,
> -    /// The computed value for the property. Cannot contain `auto`.
> +    /// The computed value for the property. Cannot contain `legacy`.
>      pub computed: specified::JustifyItems,

I think this makes the code comments ambiguous.  I'm assuming you mean "cannot contain a bare-keyword 'legacy' value", but the way it's written it's very likely to be read as "cannot contain any value that includes the 'legacy' keyword".  Can you update the comments to make them clearer please?
(note that the old comments are clear because 'auto' cannot be combined with any other keywords, unlike 'legacy')

::: servo/components/style/values/specified/align.rs:94
(Diff revision 1)
>              AlignFlags::SAFE => dest.write_str("safe ")?,
>              // Don't serialize "unsafe", since it's the default.
>              _ => {},

nit: it would be nice to have an assertion here that 'extra_flags' is zero, to guarantee that the match expressions above it handled all possible combinations of flag bits.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8969329 [details]
Bug 1363875: [css-align]: Rename justify-items: auto to legacy.

https://reviewboard.mozilla.org/r/238062/#review243958

::: servo/components/style/values/specified/align.rs:96
(Diff revision 1)
> +                }
> +                dest.write_char(' ')?;
> +            },
>              AlignFlags::SAFE => dest.write_str("safe ")?,
>              // Don't serialize "unsafe", since it's the default.
>              _ => {},

I agree that there should probably be an assertion that this cannot contain a combination of the flags.

::: servo/components/style/values/specified/align.rs:578
(Diff revision 1)
> +    Ok(try_match_ident_ignore_ascii_case! { input,
> +        "legacy" => {
> +            match input.try(parse_left_right_center) {
> +                Ok(flags) => AlignFlags::LEGACY | flags,
> +                Err(..) => AlignFlags::LEGACY,
> +            }
> +        }
> +        "left" => {
> +            input.expect_ident_matching("legacy")?;
> +            AlignFlags::LEGACY | AlignFlags::LEFT
> +        }
> +        "right" => {
> +            input.expect_ident_matching("legacy")?;
> +            AlignFlags::LEGACY | AlignFlags::RIGHT
> +        }
> +        "center" => {
> +            input.expect_ident_matching("legacy")?;
> +            AlignFlags::LEGACY | AlignFlags::CENTER
> +        }
> +    })

Maybe less redundant this way?
```rust
let flags = input.try(parse_left_right_center);
input.expect_ident_matching("legacy")?;
let flags = flags
    .or_else(|| input.try(parse_left_right_center))
    .unwrap_or(AlignFlags::empty());
AlignFlags::LEGACY | flags
```

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #10)
> Comment on attachment 8969329 [details]
> Bug 1363875: [css-align]: Rename justify-items: auto to legacy.
> 
> https://reviewboard.mozilla.org/r/238062/#review243838
> 
> Looks great, thanks for bringing this up-to-date with the spec.
> 
> ::: servo/components/style/values/computed/align.rs:38
> (Diff revision 1)
> > -    /// The specified value for the property. Can contain `auto`.
> > +    /// The specified value for the property. Can contain `legacy`.
> >      #[css(skip)]
> >      pub specified: specified::JustifyItems,
> > -    /// The computed value for the property. Cannot contain `auto`.
> > +    /// The computed value for the property. Cannot contain `legacy`.
> >      pub computed: specified::JustifyItems,
> 
> I think this makes the code comments ambiguous.  I'm assuming you mean
> "cannot contain a bare-keyword 'legacy' value", but the way it's written
> it's very likely to be read as "cannot contain any value that includes the
> 'legacy' keyword".  Can you update the comments to make them clearer please?
> (note that the old comments are clear because 'auto' cannot be combined with
> any other keywords, unlike 'legacy')

Yeah, fair enough, updated :)

> ::: servo/components/style/values/specified/align.rs:94
> (Diff revision 1)
> >              AlignFlags::SAFE => dest.write_str("safe ")?,
> >              // Don't serialize "unsafe", since it's the default.
> >              _ => {},
> 
> nit: it would be nice to have an assertion here that 'extra_flags' is zero,
> to guarantee that the match expressions above it handled all possible
> combinations of flag bits.

Added.


(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11)
> Comment on attachment 8969329 [details]
> Bug 1363875: [css-align]: Rename justify-items: auto to legacy.
> 
> https://reviewboard.mozilla.org/r/238062/#review243958
> 
> ::: servo/components/style/values/specified/align.rs:96
> (Diff revision 1)
> > +                }
> > +                dest.write_char(' ')?;
> > +            },
> >              AlignFlags::SAFE => dest.write_str("safe ")?,
> >              // Don't serialize "unsafe", since it's the default.
> >              _ => {},
> 
> I agree that there should probably be an assertion that this cannot contain
> a combination of the flags.
> 
> ::: servo/components/style/values/specified/align.rs:578
> (Diff revision 1)
> > +    Ok(try_match_ident_ignore_ascii_case! { input,
> > +        "legacy" => {
> > +            match input.try(parse_left_right_center) {
> > +                Ok(flags) => AlignFlags::LEGACY | flags,
> > +                Err(..) => AlignFlags::LEGACY,
> > +            }
> > +        }
> > +        "left" => {
> > +            input.expect_ident_matching("legacy")?;
> > +            AlignFlags::LEGACY | AlignFlags::LEFT
> > +        }
> > +        "right" => {
> > +            input.expect_ident_matching("legacy")?;
> > +            AlignFlags::LEGACY | AlignFlags::RIGHT
> > +        }
> > +        "center" => {
> > +            input.expect_ident_matching("legacy")?;
> > +            AlignFlags::LEGACY | AlignFlags::CENTER
> > +        }
> > +    })
> 
> Maybe less redundant this way?
> ```rust
> let flags = input.try(parse_left_right_center);
> input.expect_ident_matching("legacy")?;
> let flags = flags
>     .or_else(|| input.try(parse_left_right_center))
>     .unwrap_or(AlignFlags::empty());
> AlignFlags::LEGACY | flags
> ```

Hmm, I agree it's more compact, but I find a bit harder to follow. Will think a bit about it... I guess it avoids having to repeat the flags, which is nice.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd4ccf11c07
[css-align]: Rename justify-items: auto to legacy. r=mats,xidorn

Comment 14

[Emilio Cobos Álvarez (:emilio)]

I landed the WPT test change even though https://github.com/w3c/web-platform-tests/pull/10502 has also modified it. It's the exact same change so hopefully there should be no conflict.

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10543 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 17

[Dorel Luca [:dluca]]

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

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 19

[Kohei Yoshino]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/justify-items-auto-has-been-renamed-to-legacy/