Bug 1363875: [css-align]: Rename justify-items: auto to legacy. r=mats,xidorn
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 19 Apr 2018 17:50:32 +0200
changeset 468252 ecd4ccf11c070978542a43aab32fbe558f703f51
parent 468251 be575e9c66e18816eb90884b718342906c00f548
child 468253 bc8bc00108b74a90e03db5e68033ff55fb218009
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats, xidorn
bugs1363875
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1363875: [css-align]: Rename justify-items: auto to legacy. r=mats,xidorn MozReview-Commit-ID: Jfwib2XDmSw
layout/style/nsCSSValue.cpp
layout/style/nsStyleStruct.cpp
layout/style/test/property_database.js
servo/components/style/properties/gecko.mako.rs
servo/components/style/properties/longhand/position.mako.rs
servo/components/style/style_adjuster.rs
servo/components/style/values/computed/align.rs
servo/components/style/values/specified/align.rs
testing/web-platform/meta/css/css-align/default-alignment/parse-justify-items-004.html.ini
testing/web-platform/tests/css/css-align/default-alignment/shorthand-serialization-001.html
--- a/layout/style/nsCSSValue.cpp
+++ b/layout/style/nsCSSValue.cpp
@@ -780,17 +780,21 @@ nsCSSValue::AtomizeIdentValue()
 }
 
 /* static */ void
 nsCSSValue::AppendAlignJustifyValueToString(int32_t aValue, nsAString& aResult)
 {
   auto legacy = aValue & NS_STYLE_ALIGN_LEGACY;
   if (legacy) {
     aValue &= ~legacy;
-    aResult.AppendLiteral("legacy ");
+    aResult.AppendLiteral("legacy");
+    if (!aValue) {
+      return;
+    }
+    aResult.AppendLiteral(" ");
   }
   // Don't serialize the 'unsafe' keyword; it's the default.
   auto overflowPos = aValue & (NS_STYLE_ALIGN_SAFE | NS_STYLE_ALIGN_UNSAFE);
   if (MOZ_UNLIKELY(overflowPos == NS_STYLE_ALIGN_SAFE)) {
     aResult.AppendLiteral("safe ");
   }
   aValue &= ~overflowPos;
   MOZ_ASSERT(!(aValue & NS_STYLE_ALIGN_FLAG_BITS),
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -1508,17 +1508,17 @@ nsStylePosition::nsStylePosition(const n
   , mGridAutoRowsMin(eStyleUnit_Auto)
   , mGridAutoRowsMax(eStyleUnit_Auto)
   , mGridAutoFlow(NS_STYLE_GRID_AUTO_FLOW_ROW)
   , mBoxSizing(StyleBoxSizing::Content)
   , mAlignContent(NS_STYLE_ALIGN_NORMAL)
   , mAlignItems(NS_STYLE_ALIGN_NORMAL)
   , mAlignSelf(NS_STYLE_ALIGN_AUTO)
   , mJustifyContent(NS_STYLE_JUSTIFY_NORMAL)
-  , mSpecifiedJustifyItems(NS_STYLE_JUSTIFY_AUTO)
+  , mSpecifiedJustifyItems(NS_STYLE_JUSTIFY_LEGACY)
   , mJustifyItems(NS_STYLE_JUSTIFY_NORMAL)
   , mJustifySelf(NS_STYLE_JUSTIFY_AUTO)
   , mFlexDirection(NS_STYLE_FLEX_DIRECTION_ROW)
   , mFlexWrap(NS_STYLE_FLEX_WRAP_NOWRAP)
   , mObjectFit(NS_STYLE_OBJECT_FIT_FILL)
   , mOrder(NS_STYLE_ORDER_INITIAL)
   , mFlexGrow(0.0f)
   , mFlexShrink(1.0f)
--- a/layout/style/test/property_database.js
+++ b/layout/style/test/property_database.js
@@ -5165,23 +5165,23 @@ var gCSSProperties = {
                       "safe end unsafe start", "safe end unsafe", "normal safe start",
                       "unsafe end start", "end start safe", "space-around unsafe",
                       "safe stretch", "auto", "first", "last" ]
   },
   "justify-items": {
     domProp: "justifyItems",
     inherited: false,
     type: CSS_TYPE_LONGHAND,
-    initial_values: [ "auto", "normal" ],
+    initial_values: [ "legacy", "normal" ],
     other_values: [ "end", "flex-start", "flex-end", "self-start", "self-end",
                     "center", "left", "right", "stretch", "start",
                     "legacy left", "right legacy", "legacy center",
                     "unsafe right", "unsafe left", "safe right",
                     "safe center" ],
-    invalid_values: [ "space-between", "abc", "30px", "legacy", "legacy start",
+    invalid_values: [ "auto", "space-between", "abc", "30px", "legacy start",
                       "end legacy", "legacy baseline", "legacy legacy", "unsafe",
                       "safe legacy left", "legacy left safe", "legacy safe left",
                       "safe left legacy", "legacy left legacy", "baseline unsafe",
                       "safe unsafe", "safe left unsafe", "safe stretch", "last" ]
   },
   "justify-self": {
     domProp: "justifySelf",
     inherited: false,
@@ -5207,17 +5207,17 @@ var gCSSProperties = {
     invalid_values: [ "none", "center safe", "right / end" ]
   },
   "place-items": {
     domProp: "placeItems",
     inherited: false,
     type: CSS_TYPE_TRUE_SHORTHAND,
     subproperties: [ "align-items", "justify-items" ],
     initial_values: [ "normal" ],
-    other_values: [ "normal center", "baseline end", "end auto",
+    other_values: [ "normal center", "baseline end", "end legacy",
                     "end", "flex-end left", "last baseline start", "stretch",
                     "safe center", "end legacy left" ],
     invalid_values: [ "space-between", "start space-evenly", "none", "end/end",
                       "center safe", "auto start" ]
   },
   "place-self": {
     domProp: "placeSelf",
     inherited: false,
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -1801,17 +1801,17 @@ fn static_assert() {
     ${impl_simple_type_with_conversion("align_items")}
 
     pub fn set_justify_items(&mut self, v: longhands::justify_items::computed_value::T) {
         self.gecko.mSpecifiedJustifyItems = v.specified.into();
         self.set_computed_justify_items(v.computed);
     }
 
     pub fn set_computed_justify_items(&mut self, v: values::specified::JustifyItems) {
-        debug_assert_ne!(v.0, ::values::specified::align::AlignFlags::AUTO);
+        debug_assert_ne!(v.0, ::values::specified::align::AlignFlags::LEGACY);
         self.gecko.mJustifyItems = v.into();
     }
 
     pub fn reset_justify_items(&mut self, reset_style: &Self) {
         self.gecko.mJustifyItems = reset_style.gecko.mJustifyItems;
         self.gecko.mSpecifiedJustifyItems = reset_style.gecko.mSpecifiedJustifyItems;
     }
 
--- a/servo/components/style/properties/longhand/position.mako.rs
+++ b/servo/components/style/properties/longhand/position.mako.rs
@@ -113,21 +113,23 @@ macro_rules! impl_align_conversions {
                               spec="https://drafts.csswg.org/css-align/#propdef-align-items",
                               extra_prefixes="webkit",
                               animation_value_type="discrete",
                               servo_restyle_damage = "reflow")}
 
     #[cfg(feature = "gecko")]
     impl_align_conversions!(::values::specified::align::AlignItems);
 
-    ${helpers.predefined_type(name="justify-items",
-                              type="JustifyItems",
-                              initial_value="computed::JustifyItems::auto()",
-                              spec="https://drafts.csswg.org/css-align/#propdef-justify-items",
-                              animation_value_type="discrete")}
+    ${helpers.predefined_type(
+        name="justify-items",
+        type="JustifyItems",
+        initial_value="computed::JustifyItems::legacy()",
+        spec="https://drafts.csswg.org/css-align/#propdef-justify-items",
+        animation_value_type="discrete",
+    )}
 
     #[cfg(feature = "gecko")]
     impl_align_conversions!(::values::specified::align::JustifyItems);
 % endif
 
 // Flex item properties
 ${helpers.predefined_type("flex-grow", "NonNegativeNumber",
                           "From::from(0.0)",
--- a/servo/components/style/style_adjuster.rs
+++ b/servo/components/style/style_adjuster.rs
@@ -668,27 +668,25 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
         } else {
             // Need to remove to handle unvisited link inside visited.
             self.style
                 .flags
                 .remove(ComputedValueFlags::IS_RELEVANT_LINK_VISITED);
         }
     }
 
-    /// Resolves "justify-items: auto" based on the inherited style if needed to
-    /// comply with:
+    /// Resolves "justify-items: legacy" based on the inherited style if needed
+    /// to comply with:
     ///
     /// <https://drafts.csswg.org/css-align/#valdef-justify-items-legacy>
-    ///
-    /// (Note that "auto" is being renamed to "legacy")
     #[cfg(feature = "gecko")]
     fn adjust_for_justify_items(&mut self) {
         use values::specified::align;
         let justify_items = self.style.get_position().clone_justify_items();
-        if justify_items.specified.0 != align::AlignFlags::AUTO {
+        if justify_items.specified.0 != align::AlignFlags::LEGACY {
             return;
         }
 
         let parent_justify_items = self.style.get_parent_position().clone_justify_items();
 
         if !parent_justify_items
             .computed
             .0
--- a/servo/components/style/values/computed/align.rs
+++ b/servo/components/style/values/computed/align.rs
@@ -15,62 +15,65 @@ pub use super::specified::{AlignSelf, Ju
 /// The computed value for the `justify-items` property.
 ///
 /// Need to carry around both the specified and computed value to handle the
 /// special legacy keyword without destroying style sharing.
 ///
 /// In particular, `justify-items` is a reset property, so we ought to be able
 /// to share its computed representation across elements as long as they match
 /// the same rules. Except that it's not true if the specified value for
-/// `justify-items` is `auto` and the computed value of the parent has the
+/// `justify-items` is `legacy` and the computed value of the parent has the
 /// `legacy` modifier.
 ///
-/// So instead of computing `auto` "normally" looking at get_parent_position(),
+/// So instead of computing `legacy` "normally" looking at get_parent_position(),
 /// marking it as uncacheable, we carry the specified value around and handle
 /// the special case in `StyleAdjuster` instead, only when the result of the
 /// computation would vary.
 ///
 /// Note that we also need to special-case this property in matching.rs, in
 /// order to properly handle changes to the legacy keyword... This all kinda
 /// sucks :(.
 ///
 /// See the discussion in https://bugzil.la/1384542.
 #[derive(Clone, Copy, Debug, Eq, PartialEq, ToCss)]
 pub struct JustifyItems {
-    /// The specified value for the property. Can contain `auto`.
+    /// The specified value for the property. Can contain the bare `legacy`
+    /// keyword.
     #[css(skip)]
     pub specified: specified::JustifyItems,
-    /// The computed value for the property. Cannot contain `auto`.
+    /// The computed value for the property. Cannot contain the bare `legacy`
+    /// keyword, but note that it could contain it in combination with other
+    /// keywords like `left`, `right` or `center`.
     pub computed: specified::JustifyItems,
 }
 
 impl JustifyItems {
-    /// Returns the `auto` value.
-    pub fn auto() -> Self {
+    /// Returns the `legacy` value.
+    pub fn legacy() -> Self {
         Self {
-            specified: specified::JustifyItems::auto(),
+            specified: specified::JustifyItems::legacy(),
             computed: specified::JustifyItems::normal(),
         }
     }
 }
 
 impl ToComputedValue for specified::JustifyItems {
     type ComputedValue = JustifyItems;
 
     /// <https://drafts.csswg.org/css-align/#valdef-justify-items-legacy>
     fn to_computed_value(&self, _context: &Context) -> JustifyItems {
         use values::specified::align;
         let specified = *self;
-        let computed = if self.0 != align::AlignFlags::AUTO {
+        let computed = if self.0 != align::AlignFlags::LEGACY {
             *self
         } else {
             // If the inherited value of `justify-items` includes the
-            // `legacy` keyword, `auto` computes to the inherited value,
-            // but we assume it computes to `normal`, and handle that
-            // special-case in StyleAdjuster.
+            // `legacy` keyword, `legacy` computes to the inherited value, but
+            // we assume it computes to `normal`, and handle that special-case
+            // in StyleAdjuster.
             Self::normal()
         };
 
         JustifyItems {
             specified,
             computed,
         }
     }
--- a/servo/components/style/values/specified/align.rs
+++ b/servo/components/style/values/specified/align.rs
@@ -4,19 +4,18 @@
 
 //! Values for CSS Box Alignment properties
 //!
 //! https://drafts.csswg.org/css-align/
 
 use cssparser::Parser;
 use gecko_bindings::structs;
 use parser::{Parse, ParserContext};
-use selectors::parser::SelectorParseErrorKind;
 use std::fmt::{self, Write};
-use style_traits::{CssWriter, ParseError, StyleParseErrorKind, ToCss};
+use style_traits::{CssWriter, ParseError, ToCss};
 
 bitflags! {
     /// Constants shared by multiple CSS Box Alignment properties
     ///
     /// These constants match Gecko's `NS_STYLE_ALIGN_*` constants.
     #[derive(MallocSizeOf, ToComputedValue)]
     pub struct AlignFlags: u8 {
         // Enumeration stored in the lower 5 bits:
@@ -76,24 +75,36 @@ impl AlignFlags {
     }
 }
 
 impl ToCss for AlignFlags {
     fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
     where
         W: Write,
     {
-        match *self & AlignFlags::FLAG_BITS {
-            AlignFlags::LEGACY => dest.write_str("legacy ")?,
+        let extra_flags = *self & AlignFlags::FLAG_BITS;
+        let value = self.value();
+
+        match extra_flags {
+            AlignFlags::LEGACY => {
+                dest.write_str("legacy")?;
+                if value.is_empty() {
+                    return Ok(());
+                }
+                dest.write_char(' ')?;
+            },
             AlignFlags::SAFE => dest.write_str("safe ")?,
             // Don't serialize "unsafe", since it's the default.
-            _ => {},
+            AlignFlags::UNSAFE => {},
+            _ => {
+                debug_assert_eq!(extra_flags, AlignFlags::empty());
+            },
         }
 
-        dest.write_str(match self.value() {
+        dest.write_str(match value {
             AlignFlags::AUTO => "auto",
             AlignFlags::NORMAL => "normal",
             AlignFlags::START => "start",
             AlignFlags::END => "end",
             AlignFlags::FLEX_START => "flex-start",
             AlignFlags::FLEX_END => "flex-end",
             AlignFlags::CENTER => "center",
             AlignFlags::LEFT => "left",
@@ -431,20 +442,20 @@ impl Parse for AlignItems {
 
 /// Value of the `justify-items` property
 ///
 /// <https://drafts.csswg.org/css-align/#justify-items-property>
 #[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, ToCss)]
 pub struct JustifyItems(pub AlignFlags);
 
 impl JustifyItems {
-    /// The initial value 'auto'
+    /// The initial value 'legacy'
     #[inline]
-    pub fn auto() -> Self {
-        JustifyItems(AlignFlags::AUTO)
+    pub fn legacy() -> Self {
+        JustifyItems(AlignFlags::LEGACY)
     }
 
     /// The value 'normal'
     #[inline]
     pub fn normal() -> Self {
         JustifyItems(AlignFlags::NORMAL)
     }
 }
@@ -457,34 +468,22 @@ impl Parse for JustifyItems {
         // <baseline-position>
         //
         // It's weird that this accepts <baseline-position>, but not
         // justify-content...
         if let Ok(baseline) = input.try(parse_baseline) {
             return Ok(JustifyItems(baseline));
         }
 
-        // auto | normal | stretch
-        //
-        // FIXME(emilio): auto is no longer a keyword in the current spec, and
-        // has been renamed to legacy, but that needs different changes because
-        // right now it's the initial value for both style systems, and has that
-        // weird behavior of "inheriting" into descendants.
-        //
-        // Fix this in both.
-        //
-        // See also:
-        //   https://bugs.webkit.org/show_bug.cgi?id=172711
-        //   https://bugs.chromium.org/p/chromium/issues/detail?id=726148
-        //
-        if let Ok(value) = input.try(parse_auto_normal_stretch) {
+        // normal | stretch
+        if let Ok(value) = input.try(parse_normal_stretch) {
             return Ok(JustifyItems(value));
         }
 
-        // [ legacy || [ left | right | center ] ]
+        // legacy | [ legacy && [ left | right | center ] ]
         if let Ok(value) = input.try(parse_legacy) {
             return Ok(JustifyItems(value));
         }
 
         // <overflow-position>? <self-position>
         let overflow = input
             .try(parse_overflow_position)
             .unwrap_or(AlignFlags::empty());
@@ -562,34 +561,35 @@ fn parse_self_position<'i, 't>(
         "center" => AlignFlags::CENTER,
         "self-start" => AlignFlags::SELF_START,
         "self-end" => AlignFlags::SELF_END,
         "left" if axis == AxisDirection::Inline => AlignFlags::LEFT,
         "right" if axis == AxisDirection::Inline => AlignFlags::RIGHT,
     })
 }
 
-// [ legacy && [ left | right | center ] ]
+fn parse_left_right_center<'i, 't>(
+    input: &mut Parser<'i, 't>,
+) -> Result<AlignFlags, ParseError<'i>> {
+    Ok(try_match_ident_ignore_ascii_case! { input,
+        "left" => AlignFlags::LEFT,
+        "right" => AlignFlags::RIGHT,
+        "center" => AlignFlags::CENTER,
+    })
+}
+
+// legacy | [ legacy && [ left | right | center ] ]
 fn parse_legacy<'i, 't>(input: &mut Parser<'i, 't>) -> Result<AlignFlags, ParseError<'i>> {
-    let a_location = input.current_source_location();
-    let a = input.expect_ident()?.clone();
-    let b_location = input.current_source_location();
-    let b = input.expect_ident()?;
-    if a.eq_ignore_ascii_case("legacy") {
-        (match_ignore_ascii_case! { &b,
-            "left" => Ok(AlignFlags::LEGACY | AlignFlags::LEFT),
-            "right" => Ok(AlignFlags::LEGACY | AlignFlags::RIGHT),
-            "center" => Ok(AlignFlags::LEGACY | AlignFlags::CENTER),
-            _ => Err(())
-        }).map_err(|()| {
-            b_location.new_custom_error(SelectorParseErrorKind::UnexpectedIdent(b.clone()))
-        })
-    } else if b.eq_ignore_ascii_case("legacy") {
-        (match_ignore_ascii_case! { &a,
-            "left" => Ok(AlignFlags::LEGACY | AlignFlags::LEFT),
-            "right" => Ok(AlignFlags::LEGACY | AlignFlags::RIGHT),
-            "center" => Ok(AlignFlags::LEGACY | AlignFlags::CENTER),
-            _ => Err(())
-        }).map_err(|()| a_location.new_custom_error(SelectorParseErrorKind::UnexpectedIdent(a)))
-    } else {
-        Err(a_location.new_custom_error(StyleParseErrorKind::UnspecifiedError))
-    }
+    let flags = try_match_ident_ignore_ascii_case! { input,
+        "legacy" => {
+            let flags = input.try(parse_left_right_center)
+                .unwrap_or(AlignFlags::empty());
+
+            return Ok(AlignFlags::LEGACY | flags)
+        }
+        "left" => AlignFlags::LEFT,
+        "right" => AlignFlags::RIGHT,
+        "center" => AlignFlags::CENTER,
+    };
+
+    input.expect_ident_matching("legacy")?;
+    Ok(AlignFlags::LEGACY | flags)
 }
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-align/default-alignment/parse-justify-items-004.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[parse-justify-items-004.html]
-  [Checking invalid combination - justify-items: auto]
-    expected: FAIL
-
--- a/testing/web-platform/tests/css/css-align/default-alignment/shorthand-serialization-001.html
+++ b/testing/web-platform/tests/css/css-align/default-alignment/shorthand-serialization-001.html
@@ -12,17 +12,17 @@
 
 <script>
 
 var initial_values = {
     alignContent: "normal",
     alignItems: "normal",
     alignSelf: "auto",
     justifyContent: "normal",
-    justifyItems: "auto",
+    justifyItems: "legacy",
     justifySelf: "auto",
 };
 
 var place_content_test_cases = [
     {
         alignContent: "center",
         shorthand: "center normal",
     },
@@ -47,21 +47,21 @@ var place_content_test_cases = [
         justifyContent: "end",
         shorthand: "start end",
     },
 ];
 
 var place_items_test_cases = [
     {
         alignItems: "center",
-        shorthand: "center auto",
+        shorthand: "center legacy",
     },
     {
         alignItems: "baseline",
-        shorthand: "baseline auto",
+        shorthand: "baseline legacy",
     },
     {
         justifyItems: "safe start",
         shorthand: "normal safe start",
     },
     {
         justifyItems: "unsafe start",
         shorthand: ["normal start", "normal unsafe start"],