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] Resolution of percentage row tracks and gutters with indefinite height #5566

Closed
mrego opened this issue Oct 1, 2020 · 16 comments
Closed

Comments

@mrego
Copy link
Member

mrego commented Oct 1, 2020

I'd like to propose we change how percentage row tracks and gutters are resolved when the grid container has an indefinite height.

Right now, according to the spec, they should be resolved against the intrinsic height of the grid container. We decided to do this in order to keep symmetry in both axis of a grid, however this has some problems:

  • Causes that tracks overflow the grid container in many situations.
  • Requires a 2nd pass of the grid track sizing algorithm in more situations, which is worse for performance.
  • Makes implementations more complex.
  • Differs from the behavior of flex gutters, which might cause misunderstandings to web authors.
  • Doesn't have interoperability between Chromium/Webkit vs Firefox for grid tracks.

My proposal would be to resolve them as auto for tracks (like it happens on regular blocks) and to 0 for gutters. Coming back to the previous behavior we had a few years ago.

Examples

I'll show 2 examples and how they'd look like now vs with the proposed behavior.

Grid tracks

For grid tracks the behavior on the spec is implemented in Chromium and WebKit but Firefox is implementing the proposed behavior.

<div style="display: inline-grid; border: solid thick;
            grid-template-columns: 200px; grid-template-rows: 50%;">
  <div style="background: magenta;">foo</div>
</div>

Output of the previous example, comparing current behavior when the height of the track would be 50% of its intrinsic size causing overflow of the contents, with the proposed behavior when the track would be treated as auto and there would be no overflow

Grid gutters

For grid gutters the behavior on the spec is implemented in Chromium, Firefox and WebKit. Nobody is implementing the proposed behavior right now.

<div style="display: inline-grid; grid-gap: 10%; border: solid thick;
            grid-template-columns: 200px; grid-template-rows: 100px 100px;">
  <div style="background: magenta;">foo</div>
  <div style="background: cyan;">bar</div>
</div>

Output of the previous example, comparing the current behavior where the gap has a 20px height and the tracks overflow the grid container, with the proposed behavior when the gap is 0px and there is no overflow

History

For historical background, it's worth noting that this topic has been discussed previously a few times already, first a few years ago in #509 and #1921, and again recently in #5081.

I'll try to summarize a little bit the history behind this topic and what has happened:

In #5081 I brought up the topic to change grid layout too to make it consistent and a few people showed interest on that proposal, that's why I'm creating this issue now.

Backwards compatibility

About web compatibility, despite of that lack of interop in this area I could only find 2 bugs on Chromium bugtracker about these topics:

Also the bug for Firefox grid row tracks has 0 votes and no comments from authors asking about it: https://bugzilla.mozilla.org/show_bug.cgi?id=1481876

We have 2 use counters in Chromium to check about this, we've been analyzing the pages affected and these is the information we got:

  • For grid tracks the use counter is 0.03%:
    • 1/200 of the pages breaks (the breakage is some text overlapping other text).
    • 6/200 of the pages have some small changes (but without causing any breakage).
    • And surprisingly 3/200 of the pages get "fixed" if we do the change, as some elements of a carousel widget were hidden in Chromium/WebKit and only visible in Firefox.
    • The rest don't change.
  • For grid gutters the use counter is 0.003%:
    • 1/40 of the pages break in that case (again some overlapping).
    • 9/40 of the pages have some small change (without causing any big issue).
    • The rest don't change. Many of them are related to a widget from Wordpress Genesis Theme that uses grid-gap: 1% but the widget usually has only 2 rows and the 2nd one is empty, so the difference is imperceptible.

You can check all the cases in the following spreadsheet including screenshots when things change: https://docs.google.com/spreadsheets/d/1khsAsLpF_Y9wZ8BmKQKBN7gUJQQAjI1PzFcWhjFuUP4/edit?usp=sharing

CC/ @atanassov, @MatsPalmgren

@chrishtr
Copy link
Contributor

chrishtr commented Oct 1, 2020

/sub

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Resolution of percentage row tracks and gutters with indefinite height.

The full IRC log of that discussion <heycam> Topic: Resolution of percentage row tracks and gutters with indefinite height
<heycam> github: https://github.com//issues/5566
<heycam> rego: this is an old discussion
<heycam> ... this is about how we resolve percetange row tracks/gutters
<heycam> ... with a minimum height
<heycam> ... we resolve to intrinsic height that we compute
<heycam> ... that causes overflow in many cases
<heycam> ... and makes it more complicated, and no interop
<heycam> ... in Firefox it is doing the old behavior. %age tracks resolve to auto
<heycam> ... I was proposing here to change again, and get interop in this case
<heycam> ... and then also change how gutters work, which Firefox does do
<heycam> ... we have been discussing for a long time, checking compat issues, we already had a use counter for %age tracks, also for gutters
<Rossen_> q?
<heycam> ... checking the results, grid tracks where 0.03% of page views
<heycam> ... that's going down, to not count 1 row with 100%
<heycam> ... we analyzed the pages from that, only 1 broke
<heycam> ... with gutters, the usage is very low. 0.003% of page views
<heycam> ... 40 pages, only 1 broke
<heycam> ... so this change will align us with flexbox, and get interop in the tracks part that we don't have right now
<heycam> Rossen_: we did discuss this in the past, in the context of flexbox, grid and gutters. every time the discussion goes around and comes back to one of the main points/principles for grid
<heycam> ... which is to hold the behavior of rows and columns, and row gutters / col gutters, to be symmetric
<heycam> ... strong desire for this
<heycam> ... strong pushback against things going against this principle
<heycam> ... don't want to relitigate that
<heycam> ... so why should we change it for this one?
<heycam> rego: it will break that principle. the reasons we should change are in the top of the issue
<heycam> ... it usually causes overflow when people use it
<heycam> ... it requires worse perf in track sizing
<heycam> ... and we don't have interop
<heycam> ... but I agree it will break that principle
<Rossen_> q?
<heycam> Rossen_: would like to hear from more people. personal preference to hold on to that principle
<heycam> ... it's an easy slippery slope to get on
<heycam> ... yes there are expectations when they compare other 1D layout systems like block and flexbox.
<heycam> ... with this particular one, I think we should hold a high bar in keeping that principle going forward
<heycam> jensimmons: I haven't heard much about this kind of stuff at all
<heycam> ... feel like authors haven't quite grasped grid fully yet
<heycam> ... lack of compat is a problem, would like interop asap, even if the behaviour feels a bit buggy
<heycam> ... and stop using percents for gutters anyway
<heycam> ... so in some ways I don't really care about this, since they shouldn't be using percentages
<heycam> Rossen_: I think the data agrees with your observations, few cases in the wild
<heycam> ... doesn't feel like it's strongly motivated
<heycam> miriam: that's my experience as well
<heycam> fantasai: I think it'd be good to hear from mats and rachel
<heycam> ... if nobody else had anything else to add, could defer to hear from them
<heycam> Rossen_: in the meantime we can publish grid-1 and grid-2 as CRDs

@astearns
Copy link
Member

@MatsPalmgren @rachelandrew your input was requested on this, so we will bring this up again on Friday’s meeting

@MatsPalmgren
Copy link

The proposal sounds good to me and I agree with the reasoning.

@rachelandrew
Copy link
Contributor

So one reason people are using % for gutters/tracks is adding grid components to an older layout where everything is sized using %. So any flex, or float-based grid essentially. So I'm not sure why authors "shouldn't be using percentages". We've allowed percentages and it's completely valid to be using them. Some of the stuff that people show to me that is using % would definitely be better off using fr, however when I ask the question, the "this is a component for an older site" has come up a few times.

If we take that use case, what people are generally concerned about is the gap between columns (in horizontal-tb) as that's the thing they are controlling with % margins elsewhere. So what happens in the other dimension needs to make sense in that regard.

I've always been a fan of symmetry in grid, however I'm not sure whether that is actually what authors expect either. As probably the behaviour in block layout is quite ingrained in people so they might expect 10% means 10% margin all round (https://codepen.io/rachelandrew/pen/PozGqRy).

So all that said, I think the change would be ok. I'd rather have something clear to explain and to have interop. I also think it would be good for flex and grid to behave in the same way as then we only have two things to explain - it's like this in blok layout, and like this for flex and grid.

I'll be on the Europe friendly call times later in the week anyway.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Grid and % row tracks and gutter.

The full IRC log of that discussion <TabAtkins> Topic: Grid and % row tracks and gutter
<TabAtkins> github: https://github.com//issues/5566
<chris> s/the announcement/the SOTD/
<TabAtkins> rego: We discussed on Monday, asked for feedback
<TabAtkins> rego: Rachel had some comments
<TabAtkins> rachelandrew: The reason I'm seeing %s in use is because people are putting a grid component in an older layout that uses %s, becuase that's how we did things with floats or even flexboxes
<TabAtkins> rachelandrew: And people generally only cared about columns, then
<TabAtkins> rachelandrew: I like staying with the symmetry, but I don't know whether authors really expect symmetry. Probably more important that Flex and Grid behave similarly, so I'd be generally okay with the change.
<Rossen_> q?
<TabAtkins> astearns: Can yo usummarize for a resolution?
<Rossen_> q+
<astearns> ack Rossen_
<TabAtkins> rego: resolve % of row tracks and gutters of grid with indefinite height to auto for tracks and 0 for gutters
<TabAtkins> Rossen_: Gonna be the shining voice on the hill
<TabAtkins> Rossen_: Symmetry was an important part of grid and made it what it was
<TabAtkins> Rossen_: We're trying to break that principle here. I object to this decision.
<TabAtkins> Rossen_: But before I do that I want to go back and understand what the current interop is
<TabAtkins> Rossen_: Because we don't have a 2d layout system that we can have precedent with
<TabAtkins> Rossen_: We can only have interop between the Grid impls themselves
<TabAtkins> Rossen_: So are we talking about interop with 1d layout systems like Block and Flex, or between the UA impls?
<TabAtkins> astearns: Is there a consensus across impls yet?
<TabAtkins> rego: There are no track interop, there's interop on gutters. Firefox would have to change track, but Mats says he wants to change.
<TabAtkins> rego: With regards to Rossen's other issues, I don't know about that.
<TabAtkins> astearns: Rossen, do you have a plan to bring people around to your objection?
<TabAtkins> Rossen_: Not as part of this call.
<TabAtkins> Rossen_: Issue is, what's the pressing issue? Why do we need to resolve now? Take it another way - we've discussed this in the apst many times, another fix is to get rid of % in gutters.
<TabAtkins> fantasai: Far too late for that
<TabAtkins> Rossen_: But not too late to break grid, apparently?
<TabAtkins> fantasai: %s in gutters works just fine between columns, and people are using
<TabAtkins> fantasai: This is just about between rows
<TabAtkins> (and in an indef height)
<TabAtkins> fantasai: So what we need is interop between browsers. And right now we have interop on the behavior you want for gap between rows.
<TabAtkins> fantasai: But we don't have interop for sizing tracks
<TabAtkins> fantasai: Chrome/webkit have one behavior, the one you wanted iiuc, Firefox has another behavior
<TabAtkins> fantasai: This is causing problems bc we have impls behaving differently, so rego wants to fix that.
<TabAtkins> fantasai: So either Firefox has to change their behavior for tracks, or Chrome/WebKit has to; one of those has to happen, then we have interop
<TabAtkins> fantasai: And if Chrome/WebKit changes behavior, we should make gaps behave the same way as well, which is further change
<TabAtkins> fantasai: We could theoretically go either way, but we need at least one group of people to switch their behavior.
<TabAtkins> florian: One step I didn't follow - if Chrome/WebKit align with Firefox, that would mean % on gaps and tracks dont' work the same?
<TabAtkins> fantasai: Yeah, which is why the issue says if we do that we shoudl also switch the gap behavior, even tho we currently have gap interop.
<TabAtkins> fantasai: I think that % gaps between rows are rarely used so their behavior isn't a big issue, it's mainly just a cleanup step to keep them consistent.
<TabAtkins> fantasai: It's really about which behavior we end up with for row sizing
<TabAtkins> Rossen_: So if current impls impl % row-gaps behavior the same, it's essentially the same behavior that they need for row tracks?
<TabAtkins> Rossen_: I'm curious about the new Chrome Grid rewrite, which behavior is currently there
<TabAtkins> Rossen_: It seems like it's the symmetric one, right?
<TabAtkins> rego: I dunno if the new grid impl has this behavior done yet
<TabAtkins> [iank_: yo]
<TabAtkins> Rossen_: What I see currently is that Chrome/Firefox/WebKit have same beahvior
<TabAtkins> rego: For gutters, yes. For tracks Firefox has the different behavior
<iank_> ops sorry - no currently implemented yet afiak.
<iank_> not*
<TabAtkins> rego: Firefox resolves % rows to auto, and that's not following the spec, it's alone in that behavior
<TabAtkins> rego: So if we keep the spec as is, only Firefox has to change.
<TabAtkins> rego: This proposal would take Firefox's behavior, so the other browsers would change.
<TabAtkins> rego: When I did back-compat analysis, there were three pages that did better in Firefox and only 1 that did worse
<TabAtkins> rego: But Chrome/WebKit aren't getting bugs reported on this, despite the lack of interop
<TabAtkins> astearns: So sounds like we won't have agreement here
<TabAtkins> rego: Should ask the layoutng people about things
<TabAtkins> rego: And Mats
<TabAtkins> astearns: yeah he said yes one way, should see if he's okay with the other way
<TabAtkins> astearns: So we'll table this for now

@mrego
Copy link
Member Author

mrego commented Oct 23, 2020

So @atanassov clearly objects to this change as this would break the symmetry principle in Grid Layout.
I still believe my proposal is worth it, as this will just break symmetry in one case, but align gird with flexbox in how percent gutters are resolved (which seems useful if people jump to grid to flexbox elements depending on media queries or things like that).

We agreed that the next steps would be:

Please share your thoughts so we can take a final (hopefully) decision on this topic.

@KurtCattiSchmidt
Copy link

KurtCattiSchmidt commented Oct 29, 2020

We discussed this briefly over email, but I'm bringing this discussion back to GitHub as requested. Here's a summary as to where this issue currently stands amongst a few important goals:

  1. Interop - right now we don't have interop. Since Firefox already implemented this behavior, this proposal would bring interop if Blink/WebKit implement it, and another option would be for Firefox to revert to matching the existing spec. The good news is that per Rego's investigations, very few live sites will change with either proposal.

  2. Spec philosophy/symmetry - grid was designed with different goals than flexbox. While grid and flexbox share many similarities, this proposal lies in one of the main differentiations between the two.

I think the clearest explanation as to the differences between grid and flexbox is this paragraph from the the grid spec introduction - https://www.w3.org/TR/css-grid-1/#intro

"Although many layouts can be expressed with either Grid or Flexbox, they each have their specialties. Grid enforces 2-dimensional alignment, uses a top-down approach to layout, allows explicit overlapping of items, and has more powerful spanning capabilities. Flexbox focuses on space distribution within an axis, uses a simpler bottom-up approach to layout, can use a content-size–based line-wrapping system to control its secondary axis, and relies on the underlying markup hierarchy to build more complex layouts. It is expected that both will be valuable and complementary tools for CSS authors."

The "Grid enforces 2-dimensional alignment" part implies that grid has symmetry between row/column behavior, while "Flexbox focuses on space distribution within an axis" implies that inline/block behavior is expected to differ in flexbox.

  1. Least surprise for web developers - we want to follow the behavior that web developers expect. Rego's investigations suggest that the proposal would be less surprising than the current behavior for developers porting old layouts to grid (and would mirror flexbox's behavior). However, the above quote from the grid spec implies that it could be surprising to developers who are familiar with grid, or for someone who explicitly chooses grid over flexbox for symmetry. So where we currently stand on this goal is less clear to me. I think the working group's guidance would really help in making a decision here.

  2. Implementation - This proposal has some benefit in performance. However, as noted in Rego's interop/regression investigations, % grid columns and gaps on auto-sized grids aren't commonly used by web authors, so this benefit would not be universal. I can confirm that the proposed behavior would reduce some complexity in implementation, but I don't think it's significant enough to be a deciding factor.

@mrego
Copy link
Member Author

mrego commented Oct 30, 2020

Thanks @KurtCattiSchmidt for your comments, just one small clarification.

  1. Interop - right now we don't have interop. Since Firefox already implemented this behavior, this proposal would bring interop if Blink/WebKit implement it, and another option would be for Firefox to revert to matching the existing spec. The good news is that per Rego's investigations, very few live sites will change with either proposal.

It's not that Firefox has to revert anything, the current Firefox behavior was the one all browsers have back in 2018. Chromium and WebKit implemented the new behavior (the one currently in the spec) but Firefox and EdgeHTML didn't.
So to have interop it'd be needed that Firefox implements the current spec text (see https://bugzilla.mozilla.org/show_bug.cgi?id=1481876).

@MatsPalmgren
Copy link

MatsPalmgren commented Nov 16, 2020

I took a stab at fixing that bug and I found what looks like a bug in Chrome, consider:

<!DOCTYPE HTML>
<style>
.grid {
  display: grid;
  grid-template-rows: minmax(0,0.1fr) minmax(0,0.2fr) minmax(0,0.1fr) minmax(0,0.2fr) minmax(0,0.1fr);
  grid-auto-columns: 40px;
  border: 1px solid;
}

.c1 { grid-column:1; grid-row:1 / span 2; background:pink; }
.c2 { grid-column:2; grid-row:2 / span 3; background:lime; }
.c3 { grid-column:3; grid-row:3 / span 1; background:grey; }

span { height: 100px; }
</style>

<div class="grid">
  <span class="c1"></span>
  <span class="c2"></span>
  <span class="c3"></span>
</div>

Chrome has: grid-template-rows 10px 20px 10px 20px 10px (and the resulting content-box height is 70px).

I get grid-template-rows: 7px 14px 7px 14px 7px (also with height 70px), which seems correct to me because when the sum of fr sizes <= 1 then they are just a factor of the content-box size, so 0.1 x 70px = 7px etc.

Maybe this is just an over-aggressive optimization in Chrome?

Also, the spec could be a bit clearer about how this block-axis intrinsic sizing pass should be implemented. I'm running it unconditionally "under no constraint" (as opposed to "under a min/max-content constraint" in any situations). This seems to be what Chrome does too(?)

This would kind of contradict #3684 though, which suggests that e.g. block-size: min-content should make this intrinsic sizing step run under a min-content constraint(?) Should block-size: min-content/max-content/fit-content/stretch affect this intrinsic sizing step at all?

@Loirooriol
Copy link
Contributor

@MatsPalmgren Filed the fr issue in https://crbug.com/1149627.
My understanding is that the block axis should basically behave like the inline one (should be symmetric).

@KurtCattiSchmidt
Copy link

@MatsPalmgren, how is your change coming along in Firefox?

In Chromium, we have since implemented the currently-spec'd behavior for grid-gutters that requires an extra pass (https://chromium-review.googlesource.com/c/chromium/src/+/2503402). This would be trivial to remove the extra pass and replace with 0 as specified in this issue if it is approved.

@astearns astearns modified the milestones: EUR VF2F-2021-04-06, EUR VF2F-2021-07-27, APAC VF2F-2021-07-29 Jul 24, 2021
@bfgeek
Copy link

bfgeek commented Jul 24, 2021

Chromium M93 I believe is now rendering the testcase in:
#5566 (comment)

Correctly. I'd be now against the proposed change. There are some nice performance benefits which come from the current spec'd behaviour.

Ian

@mrego
Copy link
Member Author

mrego commented Jul 26, 2021

BTW, I'm fine with forgetting about my proposal here, as it seems people prefer the current spec behavior. It'd be nice if Firefox could update the behavior there.

There are some nice performance benefits which come from the current spec'd behaviour.

@bfgeek I'm curious about that, if you can comment some deatils I'd be happy to hear about it. Thanks.

@bfgeek
Copy link

bfgeek commented Aug 2, 2021

@mrego Most layout modes care about if their initial block-size is definite or not. E.g. if the initial block-size is definite, then %-children can resolve etc.

Grid is special in that it'll re-resolve these things with the used block-size. Due to this you don't care if this initial block-size goes from indefinite->definite. The indefinite->definite transition is very common these days. E.g. a grid as a grid-item will go from indefinite->definite, grid as a row flex-item will typically go through this transition as well.

It's counter intuitive, but ends up being faster for a lot of cases.
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/ng/grid/ng_grid_layout_algorithm.cc;l=342-346?q=ng_grid_layout_algorithm.cc&ss=chromium

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed percentage resolution of row tracks and gutters with indefinite height, and agreed to the following:

  • RESOLVED: Close no change
The full IRC log of that discussion <fantasai> Topic: percentage resolution of row tracks and gutters with indefinite height
<fantasai> github: https://github.com//issues/5566
<fantasai> [ discussion of whether we have time for this issue ]
<fantasai> Rossen_: [ reads mrego's comment ]
<fantasai> https://github.com//issues/5566#issuecomment-886483173
<fantasai> Rossen_: would be nice to close the issue and move on, if we all already agree
<fantasai> Rossen_: Anyone object to closing the issue with no change?
<fantasai> RESOLVED: Close no change

blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Sep 18, 2021
Deleted two TODOs: The first one is related to auto-sized grids with
percent or calc row gap. We already support that case and the behavior
is tested in the web tests grid-gutters-[14,16].html. The second one is
related to the outcome of the WG discussion in
w3c/csswg-drafts#5566. As it was decided the
spec won't change and we are compliant, that TODO is no longer needed.

Bug: 1045599
Change-Id: Ifa983e36306a19fe8f91c32ae002174ea6a428f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3169332
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Commit-Queue: Ana Sollano Kim <ansollan@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#922709}
@fantasai fantasai added Closed Rejected as Wontfix by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. labels Oct 27, 2021
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Deleted two TODOs: The first one is related to auto-sized grids with
percent or calc row gap. We already support that case and the behavior
is tested in the web tests grid-gutters-[14,16].html. The second one is
related to the outcome of the WG discussion in
w3c/csswg-drafts#5566. As it was decided the
spec won't change and we are compliant, that TODO is no longer needed.

Bug: 1045599
Change-Id: Ifa983e36306a19fe8f91c32ae002174ea6a428f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3169332
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Commit-Queue: Ana Sollano Kim <ansollan@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#922709}
NOKEYCHECK=True
GitOrigin-RevId: d252303a37bad9460448515d031737da4bd313d1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Rejected as Wontfix by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. css-grid-1
Projects
CSS Grid Level 1 Revision 4
Need Wider Discussion
Development

No branches or pull requests

10 participants