Bug 1583429 - Keep repeat() information for specified/computed value for subgrid. r=layout-reviewers,firefox-style-system-reviewers,emilio

We introduce a different rust type for `<line-name-list>` for subgrid,
which keeps `repeat()` information at computed time. At this moment,
we don't expand `repeat()`.

Also, we precompute the possible expanded list length, so we can expand
`repeat(auto-fill)` in a single iteration in nsGridContainerFrame. If we
don't have this precompution, we have to go through `<line-name-list>`
first to know how many items we have after expanding `repeat(Integer)`.

Differential Revision: https://phabricator.services.mozilla.com/D192876

layout/generic/nsGridContainerFrame.cpp

@@ -76,7 +76,9 @@ GridTemplate::LineNameLists(bool aIsSubgrid) const {
     return AsTrackList()->line_names.AsSpan();
   }
   if (IsSubgrid() && aIsSubgrid) {
-    return AsSubgrid()->names.AsSpan();
+    // For subgrid, we need to resolve <line-name-list> from each
+    // StyleGenericLineNameListValue, so return empty.
+    return {};
   }
   MOZ_ASSERT(IsNone() || IsMasonry() || (IsSubgrid() && !aIsSubgrid));
   return {};
@@ -1453,23 +1455,13 @@ class MOZ_STACK_CLASS nsGridContainerFrame::LineNameMap {
     if (MOZ_UNLIKELY(aRange)) {  // subgrid case
       mClampMinLine = 1;
       mClampMaxLine = 1 + aRange->Extent();
+      MOZ_ASSERT(aTracks.mTemplate.IsSubgrid(), "Should be subgrid type");
+      ExpandRepeatLineNamesForSubgrid(*aTracks.mTemplate.AsSubgrid());
+      // we've expanded all subgrid auto-fill lines in
+      // ExpandRepeatLineNamesForSubgrid()
+      mRepeatAutoStart = 0;
       mRepeatAutoEnd = mRepeatAutoStart;
-      const auto& styleSubgrid = aTracks.mTemplate.AsSubgrid();
-      const auto fillLen = styleSubgrid->fill_len;
-      mHasRepeatAuto = fillLen != 0;
-      if (mHasRepeatAuto) {
-        const auto& lineNameLists = styleSubgrid->names;
-        const int32_t extraAutoFillLineCount =
-            mClampMaxLine - lineNameLists.Length();
-        // Maximum possible number of repeat name lists. This must be reduced
-        // to a whole number of repetitions of the fill length.
-        const uint32_t possibleRepeatLength =
-            std::max<int32_t>(0, extraAutoFillLineCount + fillLen);
-        const uint32_t repeatRemainder = possibleRepeatLength % fillLen;
-        mRepeatAutoStart = styleSubgrid->fill_start;
-        mRepeatAutoEnd =
-            mRepeatAutoStart + possibleRepeatLength - repeatRemainder;
-      }
+      mHasRepeatAuto = false;
     } else {
       mClampMinLine = kMinLine;
       mClampMaxLine = kMaxLine;
@@ -1477,8 +1469,8 @@ class MOZ_STACK_CLASS nsGridContainerFrame::LineNameMap {
         mTrackAutoRepeatLineNames =
             aTracks.mTemplate.GetRepeatAutoValue()->line_names.AsSpan();
       }
+      ExpandRepeatLineNames(aTracks);
     }
-    ExpandRepeatLineNames(!!aRange, aTracks);
     if (mHasRepeatAuto) {
       // We need mTemplateLinesEnd to be after all line names.
       // mExpandedLineNames has one repetition of the repeat(auto-fit/fill)
@@ -1501,48 +1493,17 @@ class MOZ_STACK_CLASS nsGridContainerFrame::LineNameMap {
   }
 
   // Store line names into mExpandedLineNames with `repeat(INTEGER, ...)`
-  // expanded (for non-subgrid), and all `repeat(...)` expanded (for subgrid).
-  void ExpandRepeatLineNames(bool aIsSubgrid,
-                             const TrackSizingFunctions& aTracks) {
-    auto lineNameLists = aTracks.mTemplate.LineNameLists(aIsSubgrid);
+  // expanded for non-subgrid.
+  void ExpandRepeatLineNames(const TrackSizingFunctions& aTracks) {
+    auto lineNameLists = aTracks.mTemplate.LineNameLists(false);
 
     const auto& trackListValues = aTracks.mTrackListValues;
     const NameList* nameListToMerge = nullptr;
     // NOTE(emilio): We rely on std::move clearing out the array.
     SmallPointerArray<const NameList> names;
-    // This adjusts for outputting the repeat auto names in subgrid. In that
-    // case, all of the repeat values are handled in a single iteration.
-    const uint32_t subgridRepeatDelta =
-        (aIsSubgrid && mHasRepeatAuto)
-            ? (aTracks.mTemplate.AsSubgrid()->fill_len - 1)
-            : 0;
-    const uint32_t end = std::min<uint32_t>(
-        lineNameLists.Length() - subgridRepeatDelta, mClampMaxLine + 1);
+    const uint32_t end =
+        std::min<uint32_t>(lineNameLists.Length(), mClampMaxLine + 1);
     for (uint32_t i = 0; i < end; ++i) {
-      if (aIsSubgrid) {
-        if (MOZ_UNLIKELY(mHasRepeatAuto && i == mRepeatAutoStart)) {
-          // XXX expand 'auto-fill' names for subgrid for now since HasNameAt()
-          // only deals with auto-repeat **tracks** currently.
-          const auto& styleSubgrid = aTracks.mTemplate.AsSubgrid();
-          MOZ_ASSERT(styleSubgrid->fill_len > 0);
-          for (auto j = i; j < mRepeatAutoEnd; ++j) {
-            const auto repeatIndex = (j - i) % styleSubgrid->fill_len;
-            names.AppendElement(
-                &lineNameLists[styleSubgrid->fill_start + repeatIndex]);
-            mExpandedLineNames.AppendElement(std::move(names));
-          }
-        } else if (mHasRepeatAuto && i > mRepeatAutoStart) {
-          const auto& styleSubgrid = aTracks.mTemplate.AsSubgrid();
-          names.AppendElement(&lineNameLists[i + styleSubgrid->fill_len - 1]);
-          mExpandedLineNames.AppendElement(std::move(names));
-        } else {
-          names.AppendElement(&lineNameLists[i]);
-          mExpandedLineNames.AppendElement(std::move(names));
-        }
-        // XXX expand repeat(<integer>, ...) line names here (bug 1583429)
-        continue;
-      }
-
       if (nameListToMerge) {
         names.AppendElement(nameListToMerge);
         nameListToMerge = nullptr;
@@ -1595,8 +1556,76 @@ class MOZ_STACK_CLASS nsGridContainerFrame::LineNameMap {
     if (MOZ_UNLIKELY(mExpandedLineNames.Length() > uint32_t(mClampMaxLine))) {
       mExpandedLineNames.TruncateLength(mClampMaxLine);
     }
-    if (MOZ_UNLIKELY(mHasRepeatAuto && aIsSubgrid)) {
-      mHasRepeatAuto = false;  // we've expanded all subgrid auto-fill lines
+  }
+
+  // Store line names into mExpandedLineNames with `repeat(INTEGER, ...)`
+  // expanded, and all `repeat(...)` expanded for subgrid.
+  // https://drafts.csswg.org/css-grid/#resolved-track-list-subgrid
+  void ExpandRepeatLineNamesForSubgrid(
+      const StyleGenericLineNameList<StyleInteger>& aStyleLineNameList) {
+    const auto& lineNameList = aStyleLineNameList.line_names.AsSpan();
+    const uint32_t maxCount = mClampMaxLine + 1;
+    const uint32_t end = lineNameList.Length();
+    for (uint32_t i = 0; i < end && mExpandedLineNames.Length() < maxCount;
+         ++i) {
+      const auto& item = lineNameList[i];
+      if (item.IsLineNames()) {
+        // <line-names> case. Just copy it.
+        SmallPointerArray<const NameList> names;
+        names.AppendElement(&item.AsLineNames());
+        mExpandedLineNames.AppendElement(std::move(names));
+        continue;
+      }
+
+      MOZ_ASSERT(item.IsRepeat());
+      const auto& repeat = item.AsRepeat();
+      const auto repeatLineNames = repeat.line_names.AsSpan();
+
+      if (repeat.count.IsNumber()) {
+        // Clone all <line-names>+ (repeated by N) into
+        // |mExpandedLineNames|.
+        for (uint32_t repeatCount = 0;
+             repeatCount < (uint32_t)repeat.count.AsNumber(); ++repeatCount) {
+          for (const NameList& lineNames : repeatLineNames) {
+            SmallPointerArray<const NameList> names;
+            names.AppendElement(&lineNames);
+            mExpandedLineNames.AppendElement(std::move(names));
+            if (mExpandedLineNames.Length() >= maxCount) {
+              break;
+            }
+          }
+        }
+        continue;
+      }
+
+      MOZ_ASSERT(repeat.count.IsAutoFill(),
+                 "RepeatCount of subgrid is number or auto-fill");
+
+      const size_t fillLen = repeatLineNames.Length();
+      const int32_t extraAutoFillLineCount =
+          mClampMaxLine -
+          (int32_t)aStyleLineNameList.expanded_line_names_length;
+      // Maximum possible number of repeat name lists.
+      // Note: |expanded_line_names_length| doesn't include auto repeat.
+      const uint32_t possibleRepeatLength =
+          std::max<int32_t>(0, extraAutoFillLineCount);
+      const uint32_t repeatRemainder = possibleRepeatLength % fillLen;
+
+      // Note: Expand 'auto-fill' names for subgrid for now since
+      // HasNameAt() only deals with auto-repeat **tracks** currently.
+      const size_t len = possibleRepeatLength - repeatRemainder;
+      for (size_t j = 0; j < len; ++j) {
+        SmallPointerArray<const NameList> names;
+        names.AppendElement(&repeatLineNames[j % fillLen]);
+        mExpandedLineNames.AppendElement(std::move(names));
+        if (mExpandedLineNames.Length() >= maxCount) {
+          break;
+        }
+      }
+    }
+
+    if (MOZ_UNLIKELY(mExpandedLineNames.Length() > uint32_t(mClampMaxLine))) {
+      mExpandedLineNames.TruncateLength(mClampMaxLine);
     }
   }
 
@@ -1876,6 +1905,8 @@ class MOZ_STACK_CLASS nsGridContainerFrame::LineNameMap {
     // The index into mExpandedLineNames to use, if aIndex doesn't point to a
     // name inside of a auto repeat.
     uint32_t repeatAdjustedIndex = aIndex;
+    // Note: For subgrid, |mHasRepeatAuto| is always false because we have
+    // expanded it in the constructor of LineNameMap.
     if (mHasRepeatAuto) {
       // If the index is inside of the auto repeat, use the repeat line
       // names. Otherwise, if the index is past the end of the repeat it must
@@ -1907,6 +1938,8 @@ class MOZ_STACK_CLASS nsGridContainerFrame::LineNameMap {
         repeatAdjustedIndex += mTrackAutoRepeatLineNames.Length() - 2;
       }
     }
+    MOZ_ASSERT(repeatAdjustedIndex < mExpandedLineNames.Length(),
+               "Incorrect repeatedAdjustedIndex");
     MOZ_ASSERT(names.IsEmpty());
     // The index is not inside the repeat tracks, or no repeat tracks exist.
     const auto& nameLists = mExpandedLineNames[repeatAdjustedIndex];
@@ -3924,39 +3957,68 @@ nsContainerFrame* NS_NewGridContainerFrame(PresShell* aPresShell,
   return *cb;
 }
 
+void nsGridContainerFrame::AddImplicitNamedAreasInternal(
+    LineNameList& aNameList, nsGridContainerFrame::ImplicitNamedAreas* aAreas) {
+  for (const auto& nameIdent : aNameList.AsSpan()) {
+    nsAtom* name = nameIdent.AsAtom();
+    uint32_t indexOfSuffix;
+    if (Grid::IsNameWithStartSuffix(name, &indexOfSuffix) ||
+        Grid::IsNameWithEndSuffix(name, &indexOfSuffix)) {
+      // Extract the name that was found earlier.
+      nsDependentSubstring areaName(nsDependentAtomString(name), 0,
+                                    indexOfSuffix);
+
+      // Lazily create the ImplicitNamedAreas.
+      if (!aAreas) {
+        aAreas = new nsGridContainerFrame::ImplicitNamedAreas;
+        SetProperty(nsGridContainerFrame::ImplicitNamedAreasProperty(), aAreas);
+      }
+
+      RefPtr<nsAtom> name = NS_Atomize(areaName);
+      auto addPtr = aAreas->lookupForAdd(name);
+      if (!addPtr) {
+        if (!aAreas->add(addPtr, name,
+                         nsGridContainerFrame::NamedArea{
+                             StyleAtom(do_AddRef(name)), {0, 0}, {0, 0}})) {
+          MOZ_CRASH("OOM while adding grid name lists");
+        }
+      }
+    }
+  }
+}
+
 void nsGridContainerFrame::AddImplicitNamedAreas(
     Span<LineNameList> aLineNameLists) {
   // http://dev.w3.org/csswg/css-grid/#implicit-named-areas
   // Note: recording these names for fast lookup later is just an optimization.
-  const uint32_t len = std::min(aLineNameLists.Length(), size_t(kMaxLine));
-  nsTHashSet<nsString> currentStarts;
   ImplicitNamedAreas* areas = GetImplicitNamedAreas();
+  const uint32_t len = std::min(aLineNameLists.Length(), size_t(kMaxLine));
   for (uint32_t i = 0; i < len; ++i) {
-    for (const auto& nameIdent : aLineNameLists[i].AsSpan()) {
-      nsAtom* name = nameIdent.AsAtom();
-      uint32_t indexOfSuffix;
-      if (Grid::IsNameWithStartSuffix(name, &indexOfSuffix) ||
-          Grid::IsNameWithEndSuffix(name, &indexOfSuffix)) {
-        // Extract the name that was found earlier.
-        nsDependentSubstring areaName(nsDependentAtomString(name), 0,
-                                      indexOfSuffix);
+    AddImplicitNamedAreasInternal(aLineNameLists[i], areas);
+  }
+}
 
-        // Lazily create the ImplicitNamedAreas.
-        if (!areas) {
-          areas = new ImplicitNamedAreas;
-          SetProperty(ImplicitNamedAreasProperty(), areas);
-        }
-
-        RefPtr<nsAtom> name = NS_Atomize(areaName);
-        auto addPtr = areas->lookupForAdd(name);
-        if (!addPtr) {
-          if (!areas->add(
-                  addPtr, name,
-                  NamedArea{StyleAtom(do_AddRef(name)), {0, 0}, {0, 0}})) {
-            MOZ_CRASH("OOM while adding grid name lists");
-          }
-        }
+void nsGridContainerFrame::AddImplicitNamedAreas(
+    Span<StyleLineNameListValue> aLineNameList) {
+  // http://dev.w3.org/csswg/css-grid/#implicit-named-areas
+  // Note: recording these names for fast lookup later is just an optimization.
+  uint32_t count = 0;
+  ImplicitNamedAreas* areas = GetImplicitNamedAreas();
+  for (const auto& nameList : aLineNameList) {
+    if (nameList.IsRepeat()) {
+      for (const auto& repeatNameList :
+           nameList.AsRepeat().line_names.AsSpan()) {
+        AddImplicitNamedAreasInternal(repeatNameList, areas);
+        ++count;
       }
+    } else {
+      MOZ_ASSERT(nameList.IsLineNames());
+      AddImplicitNamedAreasInternal(nameList.AsLineNames(), areas);
+      ++count;
+    }
+
+    if (count >= size_t(kMaxLine)) {
+      break;
     }
   }
 }
@@ -3976,6 +4038,12 @@ void nsGridContainerFrame::InitImplicitNamedAreas(
         AddImplicitNamedAreas(value.AsTrackRepeat().line_names.AsSpan());
       }
     }
+
+    if (aIsSubgrid && aTemplate.IsSubgrid()) {
+      // For subgrid, |aTemplate.LineNameLists(aIsSubgrid)| returns an empty
+      // list so we have to manually add each item.
+      AddImplicitNamedAreas(aTemplate.AsSubgrid()->line_names.AsSpan());
+    }
   };
   Add(aStyle->mGridTemplateColumns, IsSubgrid(eLogicalAxisInline));
   Add(aStyle->mGridTemplateRows, IsSubgrid(eLogicalAxisBlock));

layout/generic/nsGridContainerFrame.h

@@ -338,6 +338,9 @@ class nsGridContainerFrame final : public nsContainerFrame,
   using LineNameList =
       const mozilla::StyleOwnedSlice<mozilla::StyleCustomIdent>;
   void AddImplicitNamedAreas(mozilla::Span<LineNameList>);
+  using StyleLineNameListValue =
+      const mozilla::StyleGenericLineNameListValue<mozilla::StyleInteger>;
+  void AddImplicitNamedAreas(mozilla::Span<StyleLineNameListValue>);
 
   /**
    * Reflow and place our children.
@@ -512,6 +515,10 @@ class nsGridContainerFrame final : public nsContainerFrame,
   void StoreUsedTrackSizes(LogicalAxis aAxis,
                            const nsTArray<TrackSize>& aSizes);
 
+  // The internal implementation for AddImplicitNamedAreas().
+  void AddImplicitNamedAreasInternal(LineNameList& aNameList,
+                                     ImplicitNamedAreas* aAreas);
+
   /**
    * Cached values to optimize GetMinISize/GetPrefISize.
    */

layout/style/test/test_grid_container_shorthands.html

@@ -155,15 +155,14 @@ var grid_template_test_cases = [
     {
         specified: "subgrid [foo] repeat(3, [] [a b] [c]) / subgrid",
         gridTemplateColumns: "subgrid",
-        gridTemplateRows: "subgrid [foo] [] [a b] [c] [] [a b] [c] [] [a b] [c]",
+        gridTemplateRows: "subgrid [foo] repeat(3, [] [a b] [c])",
     },
     {
         // Test that the number of lines is clamped to kMaxLine = 10000.
+        // https://drafts.csswg.org/css-grid/#overlarge-grids
         specified: "subgrid [foo] repeat(999999999, [a]) / subgrid",
         gridTemplateColumns: "subgrid",
-        // Array(n).join(s) is a hack for the non-standard s.repeat(n - 1) .
-        // [foo] + 9999 [a] gives us 10000 lines.
-        gridTemplateRows: "subgrid [foo]" + Array(10000).join(" [a]"),
+        gridTemplateRows: "subgrid [foo] repeat(10000, [a])",
     },
     {
         specified: "subgrid [bar]/ subgrid [] [foo",

servo/components/style/values/generics/grid.rs

@@ -7,7 +7,6 @@
 
 use crate::parser::{Parse, ParserContext};
 use crate::values::specified;
-use crate::values::specified::grid::parse_line_names;
 use crate::values::{CSSFloat, CustomIdent};
 use crate::{Atom, Zero};
 use cssparser::Parser;
@@ -415,7 +414,16 @@ where
 ///
 /// <https://drafts.csswg.org/css-grid/#typedef-track-repeat>
 #[derive(
-    Clone, Copy, Debug, MallocSizeOf, PartialEq, ToComputedValue, ToCss, ToResolvedValue, ToShmem,
+    Clone,
+    Copy,
+    Debug,
+    MallocSizeOf,
+    PartialEq,
+    SpecifiedValueInfo,
+    ToComputedValue,
+    ToCss,
+    ToResolvedValue,
+    ToShmem,
 )]
 #[repr(C, u8)]
 pub enum RepeatCount<Integer> {
@@ -446,9 +454,6 @@ impl Parse for RepeatCount<specified::Integer> {
 }
 
 /// The structure containing `<line-names>` and `<track-size>` values.
-///
-/// It can also hold `repeat()` function parameters, which expands into the respective
-/// values in its computed form.
 #[derive(
     Clone,
     Debug,
@@ -635,11 +640,114 @@ impl<L: ToCss, I: ToCss> ToCss for TrackList<L, I> {
     }
 }
 
+/// The `<name-repeat>` for subgrids.
+///
+/// <name-repeat> = repeat( [ <integer [1,∞]> | auto-fill ], <line-names>+)
+///
+/// https://drafts.csswg.org/css-grid/#typedef-name-repeat
+#[derive(
+    Clone,
+    Debug,
+    MallocSizeOf,
+    PartialEq,
+    SpecifiedValueInfo,
+    ToComputedValue,
+    ToResolvedValue,
+    ToShmem,
+)]
+#[repr(C)]
+pub struct GenericNameRepeat<I> {
+    /// The number of times for the value to be repeated (could also be `auto-fill`).
+    /// Note: `RepeatCount` accepts `auto-fit`, so we should reject it after parsing it.
+    pub count: RepeatCount<I>,
+    /// This represents `<line-names>+`. The length of the outer vector is at least one.
+    pub line_names: crate::OwnedSlice<crate::OwnedSlice<CustomIdent>>,
+}
+
+pub use self::GenericNameRepeat as NameRepeat;
+
+impl<I: ToCss> ToCss for NameRepeat<I> {
+    fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
+    where
+        W: Write,
+    {
+        dest.write_str("repeat(")?;
+        self.count.to_css(dest)?;
+        dest.write_char(',')?;
+
+        for ref names in self.line_names.iter() {
+            if names.is_empty() {
+                // Note: concat_serialize_idents() skip the empty list so we have to handle it
+                // manually for NameRepeat.
+                dest.write_str(" []")?;
+            } else {
+                concat_serialize_idents(" [", "]", names, " ", dest)?;
+            }
+        }
+
+        dest.write_char(')')
+    }
+}
+
+impl<I> NameRepeat<I> {
+    /// Returns true if it is auto-fill.
+    #[inline]
+    pub fn is_auto_fill(&self) -> bool {
+        matches!(self.count, RepeatCount::AutoFill)
+    }
+}
+
+/// A single value for `<line-names>` or `<name-repeat>`.
+#[derive(
+    Clone,
+    Debug,
+    MallocSizeOf,
+    PartialEq,
+    SpecifiedValueInfo,
+    ToComputedValue,
+    ToResolvedValue,
+    ToShmem,
+)]
+#[repr(C, u8)]
+pub enum GenericLineNameListValue<I> {
+    /// `<line-names>`.
+    LineNames(crate::OwnedSlice<CustomIdent>),
+    /// `<name-repeat>`.
+    Repeat(GenericNameRepeat<I>),
+}
+
+pub use self::GenericLineNameListValue as LineNameListValue;
+
+impl<I: ToCss> ToCss for LineNameListValue<I> {
+    fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
+    where
+        W: Write,
+    {
+        match *self {
+            Self::Repeat(ref r) => r.to_css(dest),
+            Self::LineNames(ref names) => {
+                dest.write_char('[')?;
+
+                if let Some((ref first, rest)) = names.split_first() {
+                    first.to_css(dest)?;
+                    for name in rest {
+                        dest.write_char(' ')?;
+                        name.to_css(dest)?;
+                    }
+                }
+
+                dest.write_char(']')
+            },
+        }
+    }
+}
+
 /// The `<line-name-list>` for subgrids.
 ///
-/// `subgrid [ <line-names> | repeat(<positive-integer> | auto-fill, <line-names>+) ]+`
+/// <line-name-list> = [ <line-names> | <name-repeat> ]+
+/// <name-repeat> = repeat( [ <integer [1,∞]> | auto-fill ], <line-names>+)
 ///
-/// https://drafts.csswg.org/css-grid-2/#typedef-line-name-list
+/// https://drafts.csswg.org/css-grid/#typedef-line-name-list
 #[derive(
     Clone,
     Debug,
@@ -652,114 +760,27 @@ impl<L: ToCss, I: ToCss> ToCss for TrackList<L, I> {
     ToShmem,
 )]
 #[repr(C)]
-pub struct LineNameList {
-    /// The optional `<line-name-list>`
-    pub names: crate::OwnedSlice<crate::OwnedSlice<CustomIdent>>,
-    /// Indicates the starting line names that requires `auto-fill`, if in bounds.
-    pub fill_start: usize,
-    /// Indicates the number of line names in the auto-fill
-    pub fill_len: usize,
+pub struct GenericLineNameList<I>{
+    /// The pre-computed length of line_names, without the length of repeat(auto-fill, ...).
+    // We precomputed this at parsing time, so we can avoid an extra loop when expanding
+    // repeat(auto-fill).
+    pub expanded_line_names_length: usize,
+    /// The line name list.
+    pub line_names: crate::OwnedSlice<GenericLineNameListValue<I>>,
 }
 
-impl Parse for LineNameList {
-    fn parse<'i, 't>(
-        context: &ParserContext,
-        input: &mut Parser<'i, 't>,
-    ) -> Result<Self, ParseError<'i>> {
-        input.expect_ident_matching("subgrid")?;
-        let mut line_names = vec![];
-        let mut fill_data = None;
-        // Rather than truncating the result after inserting values, just
-        // have a maximum number of values. This gives us an early out on very
-        // large name lists, but more importantly prevents OOM on huge repeat
-        // expansions. (bug 1583429)
-        let mut max_remaining = MAX_GRID_LINE as usize;
+pub use self::GenericLineNameList as LineNameList;
 
-        loop {
-            let repeat_parse_result = input.try_parse(|input| {
-                input.expect_function_matching("repeat")?;
-                input.parse_nested_block(|input| {
-                    let count = RepeatCount::parse(context, input)?;
-                    input.expect_comma()?;
-                    let mut names_list = vec![];
-                    names_list.push(parse_line_names(input)?); // there should be at least one
-                    while let Ok(names) = input.try_parse(parse_line_names) {
-                        names_list.push(names);
-                    }
-                    Ok((names_list, count))
-                })
-            });
-            if let Ok((names_list, count)) = repeat_parse_result {
-                let mut handle_size = |n| {
-                    let n = cmp::min(n, max_remaining);
-                    max_remaining -= n;
-                    n
-                };
-                match count {
-                    // FIXME(emilio): we shouldn't expand repeat() at
-                    // parse time for subgrid. (bug 1583429)
-                    RepeatCount::Number(num) => {
-                        let n = handle_size(num.value() as usize * names_list.len());
-                        line_names.extend(names_list.iter().cloned().cycle().take(n));
-                    },
-                    RepeatCount::AutoFill if fill_data.is_none() => {
-                        let fill_idx = line_names.len();
-                        let fill_len = names_list.len();
-                        fill_data = Some((fill_idx, fill_len));
-                        let n = handle_size(fill_len);
-                        line_names.extend(names_list.into_iter().take(n));
-                    },
-                    _ => return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError)),
-                }
-            } else if let Ok(names) = input.try_parse(parse_line_names) {
-                if max_remaining > 0 {
-                    line_names.push(names);
-                    max_remaining -= 1;
-                }
-            } else {
-                break;
-            }
-        }
-
-        debug_assert!(line_names.len() <= MAX_GRID_LINE as usize);
-
-        let (fill_start, fill_len) = fill_data.unwrap_or((0, 0));
-
-        Ok(LineNameList {
-            names: line_names.into(),
-            fill_start: fill_start,
-            fill_len: fill_len,
-        })
-    }
-}
-
-impl ToCss for LineNameList {
+impl<I: ToCss> ToCss for LineNameList<I> {
     fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
     where
         W: Write,
     {
         dest.write_str("subgrid")?;
-        let fill_start = self.fill_start;
-        let fill_len = self.fill_len;
-        for (i, names) in self.names.iter().enumerate() {
-            if fill_len > 0 && i == fill_start {
-                dest.write_str(" repeat(auto-fill,")?;
-            }
 
-            dest.write_str(" [")?;
-
-            if let Some((ref first, rest)) = names.split_first() {
-                first.to_css(dest)?;
-                for name in rest {
-                    dest.write_char(' ')?;
-                    name.to_css(dest)?;
-                }
-            }
-
-            dest.write_char(']')?;
-            if fill_len > 0 && i == fill_start + fill_len - 1 {
-                dest.write_char(')')?;
-            }
+        for value in self.line_names.iter() {
+            dest.write_char(' ')?;
+            value.to_css(dest)?;
         }
 
         Ok(())
@@ -795,7 +816,7 @@ pub enum GenericGridTemplateComponent<L, I> {
     /// A `subgrid <line-name-list>?`
     /// TODO: Support animations for this after subgrid is addressed in [grid-2] spec.
     #[animation(error)]
-    Subgrid(Box<LineNameList>),
+    Subgrid(Box<GenericLineNameList<I>>),
     /// `masonry` value.
     /// https://github.com/w3c/csswg-drafts/issues/4650
     Masonry,

servo/components/style/values/specified/grid.rs

@@ -7,8 +7,8 @@
 
 use crate::parser::{Parse, ParserContext};
 use crate::values::generics::grid::{GridTemplateComponent, ImplicitGridTracks, RepeatCount};
-use crate::values::generics::grid::{LineNameList, TrackBreadth, TrackRepeat, TrackSize};
-use crate::values::generics::grid::{TrackList, TrackListValue};
+use crate::values::generics::grid::{LineNameList, LineNameListValue, NameRepeat, TrackBreadth};
+use crate::values::generics::grid::{TrackList, TrackListValue, TrackRepeat, TrackSize};
 use crate::values::specified::{Integer, LengthPercentage};
 use crate::values::{CSSFloat, CustomIdent};
 use cssparser::{Parser, Token};
@@ -344,3 +344,98 @@ impl GridTemplateComponent<LengthPercentage, Integer> {
         Ok(GridTemplateComponent::TrackList(Box::new(track_list)))
     }
 }
+
+impl Parse for NameRepeat<Integer> {
+    fn parse<'i, 't>(
+        context: &ParserContext,
+        input: &mut Parser<'i, 't>,
+    ) -> Result<Self, ParseError<'i>> {
+        input.expect_function_matching("repeat")?;
+        input.parse_nested_block(|i| {
+            let count = RepeatCount::parse(context, i)?;
+            // NameRepeat doesn't accept `auto-fit`
+            // https://drafts.csswg.org/css-grid/#typedef-name-repeat
+            if matches!(count, RepeatCount::AutoFit) {
+                return Err(i.new_custom_error(StyleParseErrorKind::UnspecifiedError));
+            }
+
+            i.expect_comma()?;
+            let mut names_list = vec![];
+            names_list.push(parse_line_names(i)?); // there should be at least one
+            while let Ok(names) = i.try_parse(parse_line_names) {
+                names_list.push(names);
+            }
+
+            Ok(NameRepeat {
+                count,
+                line_names: names_list.into(),
+            })
+        })
+    }
+}
+
+impl Parse for LineNameListValue<Integer> {
+    fn parse<'i, 't>(
+        context: &ParserContext,
+        input: &mut Parser<'i, 't>,
+    ) -> Result<Self, ParseError<'i>> {
+        if let Ok(repeat) = input.try_parse(|i| NameRepeat::parse(context, i)) {
+            return Ok(LineNameListValue::Repeat(repeat));
+        }
+
+        parse_line_names(input).map(LineNameListValue::LineNames)
+    }
+}
+
+impl LineNameListValue<Integer> {
+    /// Returns the length of `<line-names>` after expanding repeat(N, ...). This returns zero for
+    /// repeat(auto-fill, ...).
+    #[inline]
+    pub fn line_names_length(&self) -> usize {
+        match *self {
+            Self::LineNames(..) => 1,
+            Self::Repeat(ref r) => {
+                match r.count {
+                    // Note: RepeatCount is always >= 1.
+                    RepeatCount::Number(v) => r.line_names.len() * v.value() as usize,
+                    _ => 0,
+                }
+            },
+        }
+    }
+}
+
+impl Parse for LineNameList<Integer> {
+    fn parse<'i, 't>(
+        context: &ParserContext,
+        input: &mut Parser<'i, 't>,
+    ) -> Result<Self, ParseError<'i>> {
+        input.expect_ident_matching("subgrid")?;
+
+        let mut auto_repeat = false;
+        let mut expanded_line_names_length = 0;
+        let mut line_names = vec![];
+        while let Ok(value) = input.try_parse(|i| LineNameListValue::parse(context, i)) {
+            match value {
+                LineNameListValue::Repeat(ref r) if r.is_auto_fill() => {
+                    if auto_repeat {
+                        // On a subgridded axis, the auto-fill keyword is only valid once per
+                        // <line-name-list>.
+                        // https://drafts.csswg.org/css-grid/#auto-repeat
+                        return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
+                    }
+                    auto_repeat = true;
+                },
+                _ => (),
+            };
+
+            expanded_line_names_length += value.line_names_length();
+            line_names.push(value);
+        }
+
+        Ok(LineNameList{
+          expanded_line_names_length,
+          line_names : line_names.into(),
+        })
+    }
+}

testing/web-platform/meta/css/css-grid/subgrid/grid-template-computed-nogrid.html.ini

@@ -1,54 +0,0 @@
-[grid-template-computed-nogrid.html]
-  [Property grid-template-columns value 'subgrid [a\] repeat(2, [c\] [d e\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid repeat(1, [\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid repeat(2, [\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid repeat(2, [a\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid repeat(2, [a\] [\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid repeat(2, [\] [a\] [\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid repeat(2, [\] [\] [\]) repeat(auto-fill, [\] [\] [\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid repeat(1, [a b\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid repeat(2, [a b\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid repeat(1, [a\] [b\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid repeat(2, [a\] [b\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid [a\] repeat(2, [b\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid repeat(2, [a\]) [b\]']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid [a\] repeat(2, [b\] [c d\]) [e\]']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid repeat(2, [a b\]) repeat(auto-fill, [c\] [d e\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid repeat(auto-fill, [a\] [b c\]) repeat(2, [d e\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid repeat(2, [a b\]) repeat(auto-fill, [c\] [d e\]) repeat(2, [f g\])']
-    expected: FAIL
-
-  [Property grid-template-columns value 'subgrid [a\] [b c\] repeat(2, [d e\]) [f\] [g h\] repeat(auto-fill, [i\] [j k\]) [l\] repeat(2, [m n\]) [o\]']
-    expected: FAIL

testing/web-platform/meta/css/css-grid/subgrid/grid-template-valid.html.ini

@@ -1,36 +0,0 @@
-[grid-template-valid.html]
-  [e.style['grid-template-rows'\] = "subgrid repeat(2, [a\])" should set the property value]
-    expected: FAIL
-
-  [e.style['grid-template-rows'\] = "subgrid repeat(2, [a\] [b\])" should set the property value]
-    expected: FAIL
-
-  [e.style['grid-template-rows'\] = "subgrid [a\] repeat(2, [b\])" should set the property value]
-    expected: FAIL
-
-  [e.style['grid-template-rows'\] = "subgrid [a\] repeat(2, [b\]) [c\]" should set the property value]
-    expected: FAIL
-
-  [e.style['grid-template-rows'\] = "subgrid [a\] repeat(2, [b\]) repeat(auto-fill, [c\]) [d\]" should set the property value]
-    expected: FAIL
-
-  [e.style['grid-template-columns'\] = "subgrid repeat(2, [a\])" should set the property value]
-    expected: FAIL
-
-  [e.style['grid-template-columns'\] = "subgrid repeat(2, [a\] [b\])" should set the property value]
-    expected: FAIL
-
-  [e.style['grid-template-columns'\] = "subgrid [a\] repeat(2, [b\])" should set the property value]
-    expected: FAIL
-
-  [e.style['grid-template-columns'\] = "subgrid [a\] repeat(2, [b\]) [c\]" should set the property value]
-    expected: FAIL
-
-  [e.style['grid-template-columns'\] = "subgrid [a\] repeat(2, [b\]) repeat(auto-fill, [c\]) [d\]" should set the property value]
-    expected: FAIL
-
-  [e.style['grid-template-rows'\] = "subgrid [\] repeat(2, [\]) [\]" should set the property value]
-    expected: FAIL
-
-  [e.style['grid-template-columns'\] = "subgrid [\] repeat(2, [\]) [\]" should set the property value]
-    expected: FAIL

testing/web-platform/tests/css/css-grid/subgrid/grid-template-invalid.html

@@ -19,6 +19,7 @@ test_invalid_value("grid-template-rows", 'subgrid 1px');
 test_invalid_value("grid-template-rows", 'subgrid [a] 1px');
 test_invalid_value("grid-template-rows", 'subgrid repeat(auto-fill, 1px)');
 test_invalid_value("grid-template-rows", 'subgrid repeat(auto-fill, line)');
+test_invalid_value("grid-template-rows", 'subgrid repeat(auto-fit, [a])');
 test_invalid_value("grid-template-rows", 'subgrid repeat(2, 1px)');
 test_invalid_value("grid-template-rows", 'subgrid repeat(2, line)');
 test_invalid_value("grid-template-rows", 'subgrid repeat(2,');
@@ -30,6 +31,7 @@ test_invalid_value("grid-template-columns", 'subgrid 1px');
 test_invalid_value("grid-template-columns", 'subgrid [a] 1px');
 test_invalid_value("grid-template-columns", 'subgrid repeat(auto-fill, 1px)');
 test_invalid_value("grid-template-columns", 'subgrid repeat(auto-fill, line)');
+test_invalid_value("grid-template-columns", 'subgrid repeat(auto-fit, [a])');
 test_invalid_value("grid-template-columns", 'subgrid repeat(2, 1px)');
 test_invalid_value("grid-template-columns", 'subgrid repeat(2, line)');
 test_invalid_value("grid-template-columns", 'subgrid repeat(2,');