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-2] subgrid border/padding/margin #2592

Closed
MatsPalmgren opened this Issue Apr 17, 2018 · 10 comments

Comments

Projects
None yet
5 participants
@MatsPalmgren
Copy link

MatsPalmgren commented Apr 17, 2018

There's a problem with how subgrid BPMs (border/padding/margin) are supposed to work. As I understand the spec, the subgrid item itself doesn't participate in the TSA (track sizing algorithm) of its parent grid in a subgridded axis. Instead, the size contributions of any items in edge tracks are inflated with the BPM on that side. This doesn't work very well when there are no items, or only subgrid items, or items only in non-edge tracks. In those cases, the BPM isn't accounted for and the parent track size will be too small causing the subgrid to overflow it.

I suggest we fix this by making subgrids contribute its BPM as a span=1 contribution to the TSA in the relevant edge track(s). So, for example 3x3 subgrid with border:10px solid will contribute 10px to its start and end tracks. The 10px size is fed into the TSA the same way as a span=1 item would. Any items in those tracks should also be inflated by 10px, same as before. If the subgrid is 1x1 instead, then it contributes 20px. This should make the parent large enough to fit the subgrid. I've implemented this in my prototype subgrid implementation in Gecko and it seems to work well. (It was trivial to implement.)

ISSUE 1: I propose we add subgrid BPM contributions per above

Size contributions involving BPM's also needs to be floored at zero at some point and the spec needs to specify when that occurs. I tend to think the flooring should occur as late as possible, just before the size is fed into the TSA, so for example, an empty span=1 subgrid with margin-top:-20px; margin-bottom:30px contributes 10px. If the subgrid's span > 1 then it contributes 0px to its start track and 30px to its end track. When applying the relevant BPMs to the size of an item they are simply added as is and the final result is then floored at zero. E.g. an item with height:10px in the 1x1 case would contribute 20px. In the 3x3 case, a 10px item (w. span=1) in the first track contributes zero; in the last track it would contribute 40px; with span=3 it would contribute 10px. BPMs of nested subgrids are also accumulated as is without flooring and only the final result is floored.

ISSUE 2: specify that size contributions are floored at zero and that the flooring occurs after all lengths are summed up just before the final result is fed into the TSA

@fantasai

This comment has been minimized.

Copy link
Contributor

fantasai commented Jun 12, 2018

Wrt ISSUE 1: Currently the spec says

The subgrid itself lays out as an ordinary grid item in its parent grid, but acts as if it was completely empty for sizing purposes in the subgridded dimension.

So the MBP is accounted for, the accounting is just not constrained to any particular track. I agree, however, that this doesn't quite give the expected results if, e.g. the first track is empty but the second one contains an item. We have I think two options here:

  • Minimally account for the MBP by making sure that the combination of tracks from the last track with an item in it to the edge of the subgrid has enough space to accommodate the subgrid’s MBP.
  • Strictly account for the MBP in the track at the subgrid’s edge.

The difference between the two accounting methods is most easily seen in an empty subgrid, e.g. one spanning auto 20px auto tracks. Suppose the subgrid has 5px borders and 5px padding. In the first solution, the auto tracks will be zero width, since there's enough room for the borders and padding in the middle track. In the second solution, the auto tracks will be 10px each, because the borders and padding are forced to be accounted for in the outermost spanned tracks.

I'm not sure which solution is ideal from an authoring perspective. @rachelandrew @jensimmons @frivoal @bradkemper ?

@fantasai

This comment has been minimized.

Copy link
Contributor

fantasai commented Jun 12, 2018

(Wrt ISSUE 2, the TSA already floors tracks at zero, so we just need to make sure the span=1 with negative margins case you mention is handled correctly for subgrids if we go with option 2 above.)

@FremyCompany

This comment has been minimized.

Copy link
Contributor

FremyCompany commented Jun 12, 2018

This might be very naïve, but the way I implemented this in the subgrid Houdini demo is to count the mbp on the full subgrid (for which I compute a minimum width based on the items min-content for auto columns), then as margin on each subitem that starts/ends in the first/last row or column of the subgrid.

It's not very clear to me why it is desirable to ensure the mbp is accounted for in the first track, that track could be fixed-size and not able to accommodate that mbp, and you might want to accommodate it in the other tracks instead. But maybe I missed something.

@tabatkins

This comment has been minimized.

Copy link
Member

tabatkins commented Jun 12, 2018

Yeah, we can't make any assumptions about what size the tracks will be.

But Mats' suggestion (in addition to inflating margins, pretend there's an MBP-sized item in the first/last track) makes sense to me - it correctly handles the edge-case of the first/last track being empty.

@fantasai

This comment has been minimized.

Copy link
Contributor

fantasai commented Jun 20, 2018

@tabatkins Both solutions in #2592 (comment) handle the case of the first/last track being empty, they just do it in different ways.

@css-meeting-bot

This comment has been minimized.

Copy link
Member

css-meeting-bot commented Jul 5, 2018

The Working Group just discussed CSS Grid, and agreed to the following:

  • RESOLVED: The MBP of the subgrid plus the items in the first column that contains items are attached together to create a fake item which spans those columns in order to reserve space for the subgrid‘s MBP.
  • RESOLVED: Keep the rule in the spec which treats subgrid as an empty item which spans all of its spanned tracks, in addition
The full IRC log of that discussion <fantasai> Topic: CSS Grid
<fantasai> github: https://github.com//issues/2592
<fantasai> fantasai: This is about handling margins/borders/padding of the subgrid itself
<myles> ScribeNick: myles
<myles> fantasai: The previous rule we had didn’t work if your sub grid had some empty columns and things. We wanted to fix it. 2 solutions, i’ts about how strictly you tie the BPM to the outermost column when the columns are missing on the edges.
<myles> TabAtkins: Our current approach: the sub grid and its items are laid out in the parent grid independently but we take the sub grids MBP and apply the sum of their widths as magical extra margin on the appropriate edges of sub grid items that are in the first or last tracks
<myles> TabAtkins: So that they position themselves naturally as if they were inside the sub grid
<myles> fantasai: That also takes into account the grid itself. The problem is when there is no item on the side, we can attach the extra BPM to the item, then it takes the space.
<myles> fantasai: Then everything collapses away
<myles> fantasai: There are two approaches we have: 1) Strictly account for the margins and borders and padding in the track of the sub grid edge so even if the track is empty then we have a fake item that has the MBP fo the subgid attached to it.
<myles> fantasai: IT reserves space for the MBP and requires the MBP to be in this specific colum
<myles> fantasai: 2) Make sure the combination of tracks from the last track with an item in it has enough space for th MBP fo the subgrid
<myles> fantasai: Let’s say the subgrid has a really thick border. You could do *draws picture*. But in the other approach we would have to bump out the column to make it wider to fit the MBP. And then this track would be fully empty and we’ve got our item here
<myles> fantasai: The question: Do we want to use the tracks other than the last track to accommodate the MPB or do we want to force the MBP to be on the outer most track
<myles> Rossen: We discussed internally. Our idea: the one where MBP of the subgrid is accounted for in addition to whatever the items are in the first and last columns of the span of the subgrid. In other words, the subgrid acts as an item itself that spans all of the other items and the only thing that contributes is its MBP
<myles> fantasai: That was the original model, and it doesn’t work because if you treat it as an empty thing which spans all of them, then the MBP, like let’s say we have 2 items, the MBP could be handled, to be treated as a spanning item, so we could have MPB here and here, and we don’t’ have enough space
<myles> Rossen: When I said spanning, that was poorly chosen. I meant like a grid item spanning.
<myles> fantasai: We are treating MPB as an item. The spec said that MBP was an item wherever it ended up, but that doesn’t work if there are empty columns. Two alternatives: 1) Treat the MBP as being a grid item in the very last column only, or treat it as a grid item in all of the empty columns up until the last item
<frremy> q+
<myles> florian: In most use cases, you don’t have thick MBP, so most of the time the two will be the same
<myles> florian: But some time it might be and then solution 2i s nicer but more complicated
<myles> Fantasai: it’s not more difficult
<astearns> ack frremy
<Rossen> ack frremy
<myles> frremy: Optino 1 is not a good option because if the first one has as size of 5 pax and the border is 1 pax, you don’t want the track to change the size fo the grid overall.
<myles> frremy: I implemented just like Rossen said
<myles> Fantasai: If it’s a fixed size, you overflow, whoever, that’s your fault
<myles> frremy: But in the example, you still want the remaining 10 pix you can’t fit in the first one to fit
<myles> frremy: So i did exactly what Rossen mentioned, the grid was doing a layout itself with the layout fo the inner item.
<myles> frremy: In think you never end up in the case where the column is too small
<myles> Fantasai: the question we need to resolve is whether MBP can intrude into a column that is not the last column it spans
<myles> Rossen: agree
<myles> Rossen: What do people think?
<myles> TabAtkins: *head nod*
<myles> Rossen: Proposal: The sub grid will afford for its MBP by contributing its MBP to either the last column if it happens to be auto, or any other auto column of the main grid.
<myles> Fantasai: That’s way more complicated. Because then you can say, let’s say all the columns are auto, then this subgrid could fit within, if we allow it to intrude into this column which is empty int he subgrid, then the MBP could be accommodated without changing the size fo the column. But if we did what you said, then teh column would have to be pushed out.
<myles> Rossen: right
<myles> Rossen: This is slightly more complex.
<myles> Fantasai: The proposal in the issue is the MBP of the subgrid has to be accommodated int eh subgrid after the item it appears in
<myles> Rossen: This is the first option, right?
<myles> Fantasai: yes
<frremy> frremy: this means you account for the mbp independently on the four sides
<frremy> fantasai: yes
<myles> Rossen: Minimally account for the MBP by making sure that the combination of tracks from teh last track with an item in it to the edge fo teh subgrid has enough space to accomodate the subgrid‘ s MBP
<myles> Rossen: That is a worthy resolution, but this is the proposed resolution
<myles> florian: Clarification, what if there are multiple empty tracks, and all of them are auto, and the sum is too small, which do you grow?
<myles> Fantasai: You attach it to the last time.
<myles> S/time/item/
<myles> frremy: In the case where all the columns except the last one are fixed size .... so you do each side and the total
<myles> Fantasai: So you have a subgrid in the middle, and the subgrid is who knows well and this item is in here and we have a lot of columns. When we try to do all the sizing, the subgrid si going to contribute its items for sizing. This one does not get MBP attached because it’s’ not on the edge. But then we need to account for sub grid’s MBP, so we take this item, and we attach to this edge the MBP of
<myles> this subgrid item and treat this as an item which spans from this column to the edge of the subgrid
<myles> frremy: So we will still do a big one with all of the subrgrid
<myles> frremy: because if all the columns before are fixed size, you still may want to increase the middl ecolumn
<myles> Fantasai: But this guy can’t move, This item can’t leak into this column. So this column has to accomodate the item and the subgrid’ s MBP.
<myles> Fantasai: *draws on whiteboard*
<myles> frremy: *draws on whiteboard*
<Rossen> Example on the board: https://lists.w3.org/Archives/Public/www-archive/2018Jul/0004.html
<myles> Fantasai: The left side MBP can’t be accommodated here, and it will end up overflowing.
<myles> frremy: The way I wrote it, would be this would be there.
<myles> Fantasai: The content edge of the subgrid cannot be further to the right of the margin edge of the item.
<myles> frremy: But i fyou have 0 items, you have different behavior than if you have 1 item
<myles> Fantasai: but that’s the point. The item has to say in teh column in which its assigned. It can’t move
<myles> frremy: I would prefer it overflows the subgrid
<myles> frremy: The subgrid fits, but the item overflows.
<myles> frremy: I would do left side right side up side down side (hokey pokey)
<myles> Fantasai: I think that’s weird
<myles> frremy: but it’s better because it’s consistent
<myles> frremy: Most cases will do the same thing, its’ fine. your track will not change size when you add an item.
<myles> Fantasai: I see your argument, but I still think it’s super weird
<myles> Rossen: but the case you’re describing is weird
<myles> Fantasai: frremy: *repeats the arguments*
<myles> Fantasai: okay fine
<myles> Fantasai: we’ll figure it out
<myles> Rossen: Let’s capture this in a resolution
<myles> Fantasai: The case we’re talking about is when all the tracks of the subgrid from the edge of the subgrid all the way through and including the first track with an item are fixed size, and a track on the other side of the item is auto sized, then if the MBP of the subgrid is too big to fit within the fixed set of tracks, then it will cause an increase int eh size of the auto tracks anywhere on teh
<myles> other side, and will thus paint underneath the item and into that auto track rather than being attached to the outside fo the item and overflowing to the left
<myles> florian: so if that doesn’t fit there, but it fits there, you go underneath, but you don’t increase the size. But if you have to go beyond, then you increase the size to fit it there
<myles> Fantasai: that seems okay.
<myles> Rossen: This is more or less what I was suggesting
<myles> frremy: If you have a border you want the border not to be included int eh column of the item. The border should be outside.
<myles> Fantasai: That’s great, let’s’ resolve
<myles> Fantasai: We need 2 resolutions.
<myles> Fantasai: Proposed resolution: The MBP of the subgrid plus the items in the first column that contains items are attached together to create a fake item which spans those columns in order to reserve space for the subgrid‘s MBP.
<myles> Rossen: Did anyone follow tha tproposed resolution? Any objections?
<florian> +1
<myles> 10:38 myles: Fantasai: Proposed resolution: The MBP of the subgrid plus the items in the first column that contains items are attached together to create a fake item which spans those columns in order to reserve space for the subgrid‘s MBP.
<myles> RESOLVED: The MBP of the subgrid plus the items in the first column that contains items are attached together to create a fake item which spans those columns in order to reserve space for the subgrid‘s MBP.
<myles> fantasai: In addition, to accomodate the situations where the columns of the above item are all fixed, we do a calculation which allows that MBP to bleed through the first column with all the items in it and potentially expand an auto column on the other side
<myles> frremy: Proposal: Keep what’s in teh spec and add teh previosu resolution. Don’t replace what’s int he spec.
<myles> Fantasai: Cool
<Rossen> s/teh/the/
<myles> Fantasai: Resolution: Keep the rule in the spec which treats subgrid as an empty item which spans all of its spanned tracks
<Rossen> additional subgrid example: https://lists.w3.org/Archives/Public/www-archive/2018Jul/0005.html
<myles> Fantasai: Resolution: Keep the rule in the spec which treats subgrid as an empty item which spans all of its spanned tracks, in addition
<myles> Rossen: Any objections?
<myles> <silence>
<myles> RESOLVED: Keep the rule in the spec which treats subgrid as an empty item which spans all of its spanned tracks, in addition
<emilio> ScribeNick: emilio
@fantasai

This comment has been minimized.

Copy link
Contributor

fantasai commented Jul 24, 2018

Alright, committed changes based on the WG discussion! Had to think through a bit what happens when there's items with different spans in that last occupied column and varying configurations of fixed/auto tracks, but I think I got it right. @MatsPalmgren @tabatkins @FremyCompany I can haz review?

Spec: https://drafts.csswg.org/css-grid-2/#subgrid-edge-placeholders
Changeset: c2e0acf

@FremyCompany

This comment has been minimized.

Copy link
Contributor

FremyCompany commented Jul 30, 2018

Thoughts: c2e0acf#r29892956

@fantasai

This comment has been minimized.

Copy link
Contributor

fantasai commented Jul 31, 2018

@FremyCompany OK, good catch. Tried making corresponding edits. I can haz review? :)

@FremyCompany

This comment has been minimized.

Copy link
Contributor

FremyCompany commented Jul 31, 2018

Sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment