Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-grid] Should grid-template-areas accept empty strings? #5110

Closed
Loirooriol opened this issue May 25, 2020 · 17 comments
Closed

[css-grid] Should grid-template-areas accept empty strings? #5110

Loirooriol opened this issue May 25, 2020 · 17 comments
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-grid-1 Tested Memory aid - issue has WPT tests

Comments

@Loirooriol
Copy link
Contributor

CSS.supports("grid-template-areas", `""`)
CSS.supports("grid-template-areas", `"" ""`)

They are both true in Firefox and false in Chromium and old Edge. Looking at https://drafts.csswg.org/css-grid/#propdef-grid-template-areas,

  • There is no trash token.
  • All strings have the same number of columns (0).
  • There's in no non-rectangular named grid area (since there's no named grid area).

So it seems valid per the spec. But also pretty pointless, so possibly it was not deliberate. Given that neither Chromium nor old Edge support that, should we really allow it?

@Loirooriol
Copy link
Contributor Author

I guess the utility of this is something like

#grid {
  display: inline-grid;
  grid-auto-columns: 50px 100px;
  grid-auto-rows: 50px 100px;
  grid-template-areas: "";
  border: solid;
}

and if the grid is empty, you get a 50px tall and 0px wide grid. If you used grid-template-areas: "." you would get a 50x50 square. If you used grid-template-rows: 50px then you would have to reorder grid-auto-rows: 100px 50px in case there are two items, but this would also affect leading implicit tracks. Maybe grid-template-rows: 50px repeat(auto-fit, 100px) could be an alternative?

Anyways I don't know why somebody would want to do this, and allowing it for rows but not for columns is not much consistent either.

@fantasai
Copy link
Collaborator

CC @rachelandrew and @jensimmons for an opinion...

@rachelandrew
Copy link
Contributor

@fantasai I am struggling to think of a use case, so don't have a strong feeling either way, other than we should probably clarify it and raise the issue with the relevant browser.

@tabatkins
Copy link
Member

It definitely wasn't deliberate, but there's also no use-case for it, so whatever's easiest to spec/implement seems reasonable. I have no preference one way or the other.

@Loirooriol
Copy link
Contributor Author

Supporting this in Chromium would be simple (just removing some checks).
diff --git a/third_party/blink/renderer/core/css/css_grid_template_areas_value.cc b/third_party/blink/renderer/core/css/css_grid_template_areas_value.cc
index 57a7d1e672d..a9b7b2f7393 100644
--- a/third_party/blink/renderer/core/css/css_grid_template_areas_value.cc
+++ b/third_party/blink/renderer/core/css/css_grid_template_areas_value.cc
@@ -44,7 +44,6 @@ CSSGridTemplateAreasValue::CSSGridTemplateAreasValue(
       row_count_(row_count),
       column_count_(column_count) {
   DCHECK(row_count_);
-  DCHECK(column_count_);
 }
 
 static String StringForPosition(const NamedGridAreaMap& grid_area_map,
diff --git a/third_party/blink/renderer/core/css/properties/css_parsing_utils.cc b/third_party/blink/renderer/core/css/properties/css_parsing_utils.cc
index 7e66f00f7f8..501e222ae75 100644
--- a/third_party/blink/renderer/core/css/properties/css_parsing_utils.cc
+++ b/third_party/blink/renderer/core/css/properties/css_parsing_utils.cc
@@ -1638,7 +1638,6 @@ CSSIdentifierValue* ConsumeFontVariantCSS21(CSSParserTokenRange& range) {
 }
 
 Vector<String> ParseGridTemplateAreasColumnNames(const String& grid_row_names) {
-  DCHECK(!grid_row_names.IsEmpty());
   Vector<String> column_names;
   // Using StringImpl to avoid checks and indirection in every call to
   // String::operator[].
@@ -2053,15 +2052,10 @@ bool ParseGridTemplateAreasRow(const String& grid_row_names,
                                NamedGridAreaMap& grid_area_map,
                                const size_t row_count,
                                size_t& column_count) {
-  if (grid_row_names.ContainsOnlyWhitespaceOrEmpty())
-    return false;
-
   Vector<String> column_names =
       ParseGridTemplateAreasColumnNames(grid_row_names);
   if (row_count == 0) {
     column_count = column_names.size();
-    if (column_count == 0)
-      return false;
   } else if (column_count != column_names.size()) {
     // The declaration is invalid if all the rows don't have the number of
     // columns.
diff --git a/third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc b/third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
index 39164982348..80a5a649fca 100644
--- a/third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
+++ b/third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
@@ -3082,7 +3082,6 @@ const CSSValue* GridTemplateAreas::ParseSingleValue(
 
   if (row_count == 0)
     return nullptr;
-  DCHECK(column_count);
   return MakeGarbageCollected<cssvalue::CSSGridTemplateAreasValue>(
       grid_area_map, row_count, column_count);
 }

I'd be fine with that, though not a big fan of its asymmetry.

@mrego
Copy link
Member

mrego commented May 29, 2020

if the grid is empty, you get a 50px tall and 0px wide grid. If you used grid-template-areas: "." you would get a 50x50 square.

I'm a bit lost about why you think that should work like that. In your example if you have no items the grid container is 0x0 in Chromium/WebKit and 0x50 in Firefox (independently if you set grid-template-areas to "" or to "."). So it seems what you describe there is not happening in any browser right now.

@Loirooriol
Copy link
Contributor Author

@mrego See https://software.hixie.ch/utilities/js/live-dom-viewer/saved/8150
grid-template-areas: "" is 0x50 in Firefox and 0x0 in Chromium.
grid-template-areas: "." is 50x50 everywhere, since it forces the explicit grid to have at least 1 column and 1 row, even if there is no item.

@tabatkins
Copy link
Member

Since nobody has a strong opinion either way, and nobody has use-cases for an explicitly empty g-t-a, I'm inclined to go with the majority implementation here, and rule that it's disallowed, so g-t-a must define at least one cell to be valid.

@emilio @MatsPalmgren any objection to this?

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-grid] Should grid-template-areas accept empty strings?, and agreed to the following:

  • RESOLVED: grid-template-areas should not accept empty strings
The full IRC log of that discussion <dael> Topic: [css-grid] Should grid-template-areas accept empty strings?
<dael> github: https://github.com//issues/5110
<dael> TabAtkins: Oriol raised issue that tech spec does not disallow empty strings in grid-template-areas
<dael> TabAtkins: No reason to do it either way but no one wants to put empty string in. Both Chrome and Edge consider it to be invalid
<dael> TabAtkins: As long as no objections we're going to say we require one cell to be expressed or it's invalid
<dael> Rossen_: Other opinions?
<dael> plinss: Req at least one cell with a name?
<dael> TabAtkins: A cell. It can be empty.
<dael> plinss: If you define an area with empty string for name what does it mean?
<dael> TabAtkins: It's got a track but no alignment
<dael> plinss: Same as dot?
<dael> florian: [missed] parser
<dael> TabAtkins: We don't give strings per cell name. String is name of all cells in row
<dael> plinss: Right
<florian> s/[missed] parser/no, empty string would be parse error/
<dael> oriol: Difference between empty stirng and string with dot is it forces it to have spec number of column and rows. If allow empty strings grid would force 5 rows and no column
<dael> oriol: Strings with dots you could have 1 column. That's difference
<dael> TabAtkins: Given no use case for rows with 0 columns and it doesn't go the other way around I'm inclined to call invalid matching Chrome and Edge
<dael> florian: Doesn't seem to be a use case to allow. It's a weird error and we can align on the market share of browsers
<dael> Rossen_: I can see it spit out by preprocessors or tools. Having spec is good
<dael> Rossen_: I don't hear obj or pushback.
<dael> Rossen_: Other questions or reasons why we shouldn't?
<florian> s/on the market share of browsers/, the market share of browsers who do this already prove it's possible/
<dael> dholbert: Sounds fine to me/Mozilla
<dael> RESOLVED: grid-template-areas should not accept empty strings

@MatsPalmgren
Copy link

I don't feel strongly either way, so this seems fine to me. I'm assuming the resolution is not just about empty strings, but also that strings containing only whitespace are also invalid. IOW, a string that doesn't produce any null or named cell tokens is invalid.

@tabatkins
Copy link
Member

Yes, it's actually a requirement that g-t-a must define at least one cell. This'll be clear when I make the edits.

@dholbert
Copy link
Member

dholbert commented Jul 21, 2020

@tabatkins : just to be sure we're understanding...

Can you clarify which of these values should be considered invalid:

grid-template-areas: "";
grid-template-areas: "" "";
grid-template-areas: "foo" "";

Are all three invalid? (I think that would make sense, and it's how Mats has implemented the change in a pending patch, but I'm not sure it agrees with your "must define at least one cell" phrasing above... It sounds like your phrasing might allow the third example here.)

@Loirooriol
Copy link
Contributor Author

@dholbert The 3rd example must be invalid per

All strings must have the same number of columns, or else the declaration is invalid.

@dholbert
Copy link
Member

I like that, but I'm not sure it jives with Tab's "must define at least one cell" phrasing of the requirement.

@dholbert
Copy link
Member

Ah, I see -- I suppose that third example does define one cell, but then fails a separate requirement about number of columns having to match between each string, so it still fails. Sounds good to me!

@tabatkins
Copy link
Member

Correct.

@tabatkins
Copy link
Member

Tested by web-platform-tests/wpt@31bf745

@tabatkins tabatkins added Tested Memory aid - issue has WPT tests Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. and removed Needs Testcase (WPT) labels Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-grid-1 Tested Memory aid - issue has WPT tests
Projects
None yet
Development

No branches or pull requests

8 participants