Closed Bug 1363875 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: dholbert, Assigned: emilio)

References

(Depends on 1 open bug)

Details

(Keywords: site-compat)

Attachments

(4 files)

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.
Depends on: 1176782
Assignee: nobody → mats
Depends on: css-align-3
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Rename `auto` to `legacy` for `justify-items` → [css-align] Rename `auto` to `legacy` for `justify-items`
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?
Flags: needinfo?(mats)
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.
Attachment #8868019 - Flags: review?(cam)
Attachment #8868020 - Flags: review?(cam)
Attachment #8868021 - Flags: review?(cam)
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
I'll fix this after bug 1430817.
Depends on: 1430817
Flags: needinfo?(emilio)
Assignee: mats → emilio
Flags: needinfo?(emilio)
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.
Attachment #8969329 - Flags: review?(mats) → review+
Flags: needinfo?(mats)
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
```
Attachment #8969329 - Flags: review?(xidorn+moz) → review+
(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.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd4ccf11c07
[css-align]: Rename justify-items: auto to legacy. r=mats,xidorn
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.
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10543 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/ecd4ccf11c07
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.