w3c / csswg-drafts

CSS Working Group Editor Drafts
https://drafts.csswg.org/
Other
4.42k stars 652 forks source link

[css-grid-2] How to properly accommodate margin/border/padding of a subgrid with no item on the edges #7465

Open ethanjv opened 2 years ago

ethanjv commented 2 years ago

Consider the following example:

<style>
.grid {
  display: inline-grid;
  grid-template-columns: repeat(3, auto);
}
.subgrid {
  display: grid;
  background: grey;
  grid-column: 1 / -1;
  padding: 0px 10px 0px 90px;
  grid-template-columns: subgrid;
}
.subgrid > div {
  width: 100px;
  height: 20px;
  grid-column: 2;
}
</style>
<div class="grid">
  <div class="subgrid">
    <div></div>
  </div>
</div>

From this section of the spec, my understanding is that just a single hypothetical item should be added to accommodate the subgrid's padding, it should span all columns on the grid and should have a size of 100px from the subgridded item width + the padding of the subgrid for both edges, 100px too, a total of 200px.

My question is: after the track sizing algorithm distributes a 100px size to the column in the middle (starting in line number 2), the algorithm should equally distribute 200px from the hypothetical to the 3 columns, causing the first column to have 50px, the second 100px and the last one 50px. When we perform layout, is it the expected behavior that the padding does not match the column sizes? The left padding on the subgrid should push the subgridded item 90px, but in practice the first column will be 50px, possibly producing some undesirable side effects when the subgrid's padding goes beyond the first column.

My suggestion is to remove the later bit in the following sentence: "This [hypothetical] item’s sizes are taken from the sizes of the largest such item of each span size, and are additionally inflated by the subgrid’s own margin/border/padding at that edge (or both edges, if it happens to be the most extreme item on both sides and is also the smallest span size)", since that merges both extra margins.

Currently is difficult to provide a live example to observe what happens, it seems like Gecko and Webkit are not introducing hypotheticals to replicate the issue I'm mentioning; the example above runs as we would expect. However, if we replace the template columns from the outer-grid and use grid-template-columns: 0px auto 0px instead, the padding of the subgrid is not accommodated. Looking at their implementations, it seems they're accommodating the subgrid's margin/border/padding at the first track from the respective edge if such track is intrinsically sized, and I have to agree that it is less disruptive to implement.

While we are on topic: it is easy to miss the mbp in current implementations, we have no test coverage for both examples above. I would argue that is probably better to keep things simple and we should land at some middle ground where the spec defines an easier way (implementation-wise) to accommodate the mbp in those edge cases, but I don't have a definitive answer on how to achieve that.

dlibby- commented 2 years ago

@mattwoodrow @dholbert - any insight into how WebKit/Gecko are handling hypothetical items? If these are not handled per spec (as perhaps the alternate test case Ethan mentioned indicates), any thoughts on how we can define how subgrid MBP should be accommodated?

mattwoodrow commented 2 years ago

WebKit is definitely doing the same as Gecko here, including the sub grid's margin/border/padding into the size of the edge tracks spanned by the subgrid.

I think it'd be worth trying to come up with examples (and WPT tests!) for cases that require the currently specified behaviour in order to produce a good/expected result.

I'm also struggling to figure out how to interpret/implement the current spec when it comes to multiply nested subgrids.

tabatkins commented 1 year ago

Okay, @fantasai and I resolved the issue in the first comment in https://github.com/w3c/csswg-drafts/commit/4bc828a3e535f12bbe02dae538741bafff763174 : we should indeed be generating separate hypothetical items for each edge, and not ever combining any, so that assymmetric MBP is handled correctly.

But looking into the actual browser behavior, as summarized by @mattwoodrow, we probably should simplify and match browsers. In short, the current browser behavior is just to insert a single span-1 hypothetical item into the first/last track, with size equal to the MBP on that edge.

This is definitely simpler, and probably fine in practice. In particular, because the subgrid's MBP is only attached (as "magic margin") to items in the outermost tracks when actually laying out the items, the current spec's behavior will give bad results whenever we try to include MBP in sizing tracks other than the outermost. For example:

.grid {
  grid-template-columns: 10px auto;
  }
.subgrid {
    grid-column: 1 / -1;
    border-left: 50px dotted;
}
.grid-item {
    grid-column: 2;
    width: 50px;
}

In this example, the current spec will generate a hypothetical item that spans both columns, and is 100px wide. Since the first track is fixed, this will cause the second track to grow to accommodate the extra space, resulting in columns of 10px 90px. But then the actual grid item living in the second column just naively lays out into that 90px-wide space, without moving aside to avoid overlapping the border, so why did we change the column's size in the first place? Unless we spec "magic margin" that spills over into items in interior tracks in some cases, we wouldn't get a good result here anyway.

On the other hand, the current browser behavior works just as good in cases where the first/last track can grow, and when the first/last track is fixed (or capped at a too-small size), at least the items lay out reasonably even if the subgrid box itself is not drawn around them (rather than potentially having their tracks grow for mysterious reasons).

CC @FremyCompany in case there's anything here we missed. Agenda+ to discuss and potentially resolve on this behavior.

fantasai commented 1 year ago

Once we've decided on the behavior here, we might want to restore mats's testcases with appropriately updated references.

FremyCompany commented 1 year ago

I was under the impression that we wrote the second part mainly in the case where the subgrid only spans one track, and that you have to sum both of the margins and paddings in that case.

I can see how ensuring the margin and padding stay in the first track if possible makes sense, I would not object to that. I think that this requires more change than just removing the subsentence however, but I could be wrong.

But we still need to cover the case where both sides of the subgrid are in the same track.

PS: For the better or worse, my handle is @FremyCompany not @Fremy ;-)

css-meeting-bot commented 1 year ago

The CSS Working Group just discussed [css-grid-2] How to properly accommodate margin/border/padding of a subgrid with no item on the edges, and agreed to the following:

The full IRC log of that discussion <dael> Topic: [css-grid-2] How to properly accommodate margin/border/padding of a subgrid with no item on the edges
<dael> github: https://github.com/w3c/csswg-drafts/issues/7465#issuecomment-1265913276
<dael> astearns: Can you take fantasai?
<dael> fantasai: Yep. The subgrids layout we have a bunch of rules for laying out stuff within a subgrid as part of parent
<dael> fantasai: We figure out which col and rows items in subgrid assigned to and layout as if in parent
<dael> fantasai: Difference is how we handle margins and padding. Figure out total amount of margin padding border for items at edge of subgrid and add as magic margin to inflate auto-sized
<dael> fantasai: When you layout in grid takes into account extra margin and padding I should stay away from
<dael> fantasai: in order to do that accurately we have a bunch of rules to insert hypothetical items at edge of subgrid to make sure even if it's empty it's sized
<dael> fantasai: Spec rules and browser impl are different. We had a complex rule to inster item with span of one then with span of two and however many we need to cover the spans and make sure there's enough space
<dael> fantasai: Problem is that it creates a bunch of space and whne you put items down ones in second column don't have magic margin. Doesn't work out to match up in that way
<dael> fantasai: Browsers don't do complex spanning. Only put items in first and last track. If you have fixed width 20px first track and second is auto and subgrid has 40px of padding the padding crosses into second track and items don't know about it.
<dael> fantasai: If we wanted to figure that out it's quite complicated to figure out the margin you want to add. None of the browsers are doing it and not sure it's necessary to do the complex thing
<dael> fantasai: Question for WG is do we simplify spec down to only look at first and last or investigate what we can do to try and make it work for inner tracks?
<dael> fantasai: Our suggestion is simplify down and see what happens, but open for questions and comments
<dael> astearns: Test cases we removed because missing spec text. Would those become valid?
<dael> fantasai: Don't know. I think whatever we do we'll want the test cases but maybe different references
<mattwoodrow_> q+
<dael> iank_: I support removing hypotetical item concept. I believe MS folks studied this and super hard to get correct
<astearns> ack mattwoodrow_
<dael> mattwoodrow_: Also support simplifying it. I spent a bit of time trying to figure out how to impl and very complex when you nest grids
<dael> fantasai: Will still need some margin padding added, it's just not leaked to multiple tracks
<dael> astearns: Is there interop in browsers? Or are we coming up with something that might need bug fixes?
<dael> iank_: From what I've seen broadly speaking subgrid is lightly tested in some areas so wouldn't shock if there are subtle differences between browsers
<dael> fantasai: Either way should make sure have enough tests. Might be some bug fixes but closer to interop on simplified version
<dael> astearns: Prop: Change the spec to simplify this sizing and figure out what tests we need
<dael> astearns: Obj?
<dael> RESOLVED: Change the spec to simplify this sizing and figure out what tests we need