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-1] Empty grid tracks should contribute to scrollable overflow #3638

Closed
charbelnicolas opened this issue Feb 8, 2019 · 22 comments

Comments

Projects
None yet
9 participants
@charbelnicolas
Copy link

commented Feb 8, 2019

This is related to:

https://bugzilla.mozilla.org/show_bug.cgi?id=1525310

And

https://bugs.chromium.org/p/chromium/issues/detail?id=928885

I've made a CODE PEN outlining this issue with extra info:

https://codepen.io/charbelnicolas/pen/omGBjN

Please consider reading it, and let me know if what I'm saying makes sense.

Thank you

In case you don't want to read the Code Pen I'll paste the text here:

As per the current CSS Grid spec, it seems Null Cell Tokens placed in ROWS after the LAST grid item are not supposed to contribute anything to the grid vertical dimensions.

Fortunately, Google Chrome, as per version 72.0.3626.96 does take into account Null Cell Tokens placed after the last grid item to contribute to its parent vertical dimensions, which to me, seems the most logical too.

It is easier to layout your whole website with one grid, while being able to center all your items both vertically and horizontally with the use of Fr units on Null Cell Tokens.

This CODE PEN will run differently on Firefox because of this issue.

chromevsfirefox

If you check the CSS code, you can see the minmax(2rem, 1fr) set on the first row and on the last one will always center the grid items vertically on the whole viewport without the need of anything else. I think it is counterintuitive to allow the use of Null Cell Tokens on the top row and in between items but not at the last row in the grid.

You could argue that while not the best solution, it would be easy just to name the grid-area where the Null Cell Token is and add a div to the html, but why do it when Google Chrome's solution is already so elegant? Why clutter the CSS and HTML files with more lines of code?

Or better yet, please tell me why it shouldn't work like it already does in Google Chrome. Why not make things easier for everyone and more intuitive?

Please check this CODE PEN on Chrome and on Firefox to see the difference. A scrollbar must be present to see the issue. The box containing this text should NOT touch the bottom of the browser window.
CSS Grid Spec:

https://www.w3.org/TR/css-grid-1/

@charbelnicolas charbelnicolas changed the title {css-grid-1] Null Cell Tokens at the end row of a grid template are ignored per the current CSS Grid spec [css-grid-1] Null Cell Tokens at the end row of a grid template are ignored per the current CSS Grid spec Feb 8, 2019

@fantasai

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2019

OK, so it looks like the problem here is that Chrome includes the entire grid in the scrollable overflow area (in the vertical dimension; not horizontal), whereas Firefox only considers the areas occupied by content. Simplified Testcase

I think we should fix that. I think authors would expect the entire grid to be included in the scrollable overflow area, content or no, and also in both dimensions. (Grid layout should be symmetrical.)

CC @MatsPalmgren @javifernandez @mrego @jensimmons @rachelandrew @tabatkins

A couple of extra notes for you, @charbelnicolas:

  • I would recommend learning about the align-content and justify-content properties. When set to center on grid container, they will center the grid. See https://www.w3.org/TR/css-align-3/#align-justify-content You might also want to use padding and gap to create the 2rem spaces. I think this will be easier than having to create extra rows just to add such space. (I suspect there are good tutorials for grid and gaps and alignment properties online somewhere... I'm pretty sure e.g. Rachel Andrews has articles and videos on it. Or you can read the specs or MDN and just play with the property values to see what they do. https://developer.mozilla.org/en-US/docs/Web/CSS/gap https://developer.mozilla.org/en-US/docs/Web/CSS/align-content)

  • The grid-template-area names, whether they are null or not null, don't affect how those cells are sized. They're just names. The only power they have in sizing the grid is to create an extra row or column if there wasn't one in specified by grid-template-rows/columns.

  • Thanks for copying your message over. Codepen is a useful tool, but as a standards organization we need to make sure all relevant discussion is archived for future reference, so it's important that it be copied here.

@fantasai fantasai changed the title [css-grid-1] Null Cell Tokens at the end row of a grid template are ignored per the current CSS Grid spec [css-grid-1] Empty grid rows should contribute to scrollable overflow Feb 9, 2019

@fantasai fantasai changed the title [css-grid-1] Empty grid rows should contribute to scrollable overflow [css-grid-1] Empty grid tracks should contribute to scrollable overflow Feb 9, 2019

@MatsPalmgren

This comment has been minimized.

Copy link

commented Feb 9, 2019

I think we should fix that. I think authors would expect the entire grid to be included in the scrollable overflow area, content or no, and also in both dimensions.

I'm strongly opposed to such a change, fwiw. The grid is NOT a box and should not affect the overflow.
FYI, Chrome is agreeing that their layout is a bug and that Firefox is correct.

@charbelnicolas

This comment has been minimized.

Copy link
Author

commented Feb 9, 2019

Thanks for the suggestions @fantasai, even though I'm a newbie, I am well aware of various ways to center content, I'm just really annoyed that this method doesn't work on all major browsers because of the current CSS spec apparently.

@MatsPalmgren Other than the grid not being a box, would this functionality hamper/impede you from doing CSS coding in your own way? Or would you say it allows more options for the developers/users? Not everyone thinks alike you know, and seeing how beautifully it works on Chrome, I'd say it would be a nice addition to the spec.

Options are always good, even more so when it's something that doesn't really stop you from doing things the way you are already used to. The spec can always evolve.

Anyways, that's my 2 cents. I hope this gets in.

@javifernandez

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

I agree with @MatsPalmgren, from the implementation point of view is strange than an abstraction like the grid can contribute by itself to the grid container's overflow area. As a matter of fact, we have just fixed the bug while we don't agree on a good reason to define such change in the layout behavior.

@tabatkins

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Yeah, I'm with Mats too. The grid lines are a layout abstraction, not content; we don't rely on the grid itself for anything in layout besides helping us figure out which element to look at (for things like baselines, etc).

@charbelnicolas

This comment has been minimized.

Copy link
Author

commented Feb 11, 2019

I don't know for how long this "bug" has been on chrome, maybe a year or more, who knows, but if it's been there for this long and it really didn't bother anyone, why not go the other way and add it to the spec.

Saying that you won't add it because the spec says a grid is only for layout abstraction and so and so.... Is self-defeating.

Make the grid more powerful, don't limit it just because "the grid is not a box".

And like I said, being able to use null cell tokens at the top but not at the bottom is counter-intuitive.

What's the worst thing that can happen if this gets added?

@charbelnicolas

This comment has been minimized.

Copy link
Author

commented Feb 11, 2019

Please let whitespace be a first class citizen in css grid layouts.

@astearns

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

I have sympathy for this use case - if you're using the grid for spacing content it's annoying that it's not symmetrical (top and left offsets add to the scrollable area, but right and bottom offsets do not). Adding empty divs to 'contentize' the abstract grid lines is really clunky.

When we get to the point of being able to style grid areas/gaps in CSS that may or may not have content in them, perhaps the areas with boxes from content OR boxes from styling should be considered in the scrollable area? This at least would be a CSS-only fix.

@tabatkins

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Saying that you won't add it because the spec says a grid is only for layout abstraction and so and so.... Is self-defeating.

Heh, I'm not arguing circularly; I wrote the original text, so I'm stating my original intentions for the spec. ^_^

And like I said, being able to use null cell tokens at the top but not at the bottom is counter-intuitive.

CSS is asymmetric with respect to left vs right and top vs bottom in many ways, all linked to the fact that the origin of the coordinate system is in the top-left corner (for English/etc). Auto-placement only runs downward and rightward, for instance; scrollable area only extends downward and rightward.

"Being able to use null cell tokens at the top" is just because the scrollable area always extends up to the zero point, but only extends downward to the last scrollable content. I understand that's not a particularly satisfying answer, tho. ^_^ But the use-case you've presented is already doable in other, better ways: use padding on the grid container, and then center the grid with align-content; this will center your grid without affecting your use of fr units elsewhere in the grid.

If there are other use-cases where empty right/bottom tracks should be included in the scrollable area, that aren't already possible with existing properties (and, hopefully, in a non-hacky way), then it's a good reason to consider changing.

When we get to the point of being able to style grid areas/gaps in CSS that may or may not have content in them, perhaps the areas with boxes from content OR boxes from styling should be considered in the scrollable area? This at least would be a CSS-only fix.

Boxes from pseudo-elements are boxes, so they'd definitely be counted there, yes. That's definitely not in question.

@fantasai

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2019

CSS is asymmetric with respect to left vs right and top vs bottom in many ways

Yes, and wrt scrollable overflow, this is really annoying to authors. You know that. We've been trying to fix it with the align properties in https://www.w3.org/TR/css-align-3/#overflow-scroll-position

I agree with @MatsPalmgren, from the implementation point of view is strange than an abstraction like the grid can contribute by itself to the grid container's overflow area.

Padding, gaps, and boxes are also abstractions. I don't think authors see grid lines as particularly more abstract than these things, even if implementations do.

@FremyCompany

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

I think empty tracks should be taken into account because those tracks are used when sizing the other tracks (percentages, fr units, etc...) so it would seem logical that they contribute space in the end.

@css-meeting-bot

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

The CSS Working Group just discussed Empty grid tracks should contribute to scrollable overflow.

The full IRC log of that discussion <dael> Topic: Empty grid tracks should contribute to scrollable overflow
<dael> github: https://github.com//issues/3638
<dael> astearns: Short intro to this topic
<dael> fantasai: Filed by someone using empty grid tracks. It was showing...it's in a scrollable grid container and spacing left by empty ttracks not showing in FF b/c different interpretations. Should scrollable overflow area of grid container be only big enought ot cotnaint elements or big enough for entire explicit grid. It's an abstraction that's not a box
<dael> TabAtkins: Grid exists like flex lines. It's not a thing in thebox tree
<dael> fantasai: Argument against is impl that's the only thing that's real. Authors expect inluding all grid tracks
<dael> Rossen: A grid element that itself is overflow scroll?
<dael> fantasai: A grid container that's overflow:scroll with a bunch of tracks and some items in it.
<dael> TabAtkins: Items in the grid fit within the visible area, but tracks don't. Scrollbar or no?
<dael> jensimmons: Super itneresting. If authors have extra tracks I think they would expect to scroll
<dael> rachelandrew: Authors would expect a scrollbar. They expect the grid to be real
<dael> plinss[m]: Would you expect it to size to explicit grid lines if it's not overflow:scroll?
<dael> fantasai: It does
<dael> plinss[m]: Bheavior should be consistent
<dael> Rossen: Are you saying the intrinsic size of the grid is the extent for scrolling?
<dael> plinss[m]: Yes
<dael> astearns: We're at time. That's an intro for this issue. It's an interesting one and we should discuss in the future.
@astearns

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

@MatsPalmgren @javifernandez We just had a very short discussion on this topic in the CSSWG meeting, and will likely get to it again next week. One thing that came up is that the explicit grid tracks are included in sizing the grid container, so it would be reasonable (and consistent) to use that size in determining the scrollable area. We also heard emphatic support for doing this from the author perspective.

@javifernandez

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

If I'm not missing anything, the spec defines quite clear in the 5.2. Sizing Grid Containers section how a grid container should be sized. The only reference to the tracks being used in the grid container sizing logic is the following one:

The max-content size (min-content size) of a grid container is the sum of the grid container’s track sizes (including gutters) in the appropriate axis, when the grid is sized under a max-content constraint (min-content constraint).

The statement above determines how to compute the grid container's intrinsic size (lets assume for simplicity that we have such concept of intrinsic height), which as far as I know, it's the size we can compute without performing a layout.

I couldn't find any other reference about tracks being used to size the grid container. The example of paddings doesn't fit in this case because they contribute to the box's size but not to the box's content, like this new behavior seems to imply with the tracks.

Lets consider the following example:

<div style="display: grid; grid: 100px 100px / 200px 200px; overflow: auto; width: 300px, height: 100px; background: grey"></div>

Do we really want to show both vertical and horizontal scrollbars even there is no item inside the grid ? Even the horizontal bar ?

What if we add an item in the cell (1,1) ?

<div style="display: grid; grid: 100px 100px / 200px 200px; overflow: auto; width: 300px; height: 100px; background: grey">
   <div style="width: 50px; height: 50px; background: blue;"</div>
</div>

The scrolls would indicate that there are items in the grid areas out of the scrolling area, but that's not the case.

If we add an item i the cell (1,3) we really want to show the vertical scroll, since the item is bellow the scrolling area's height.

<div style="display: grid; grid: 100px 100px / 200px 200px; overflow: auto; width: 300px; height: 100px; background: grey">
   <div style="width: 50px; height: 50px; background: blue; grid-row: 3"</div>
</div>

However, we already would have both scroll bars visible because of the tracks. A similar example could be done in for the horizontal bar.

@tabatkins

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Yeah, the argument (as @astearns summarized) is that if the intrinsic size of the grid container is 200x400 as in your example, why wouldn't it show scrollbars if you explicitly sized it to smaller than that? This might be the only case in CSS where the scroll size is smaller than the intrinsic size.

@charbelnicolas

This comment has been minimized.

Copy link
Author

commented Feb 16, 2019

I will just leave this here:

https://youtu.be/YfIjFeBLhyA

;)

@Loirooriol

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2019

This seems somewhat analogous to #129. You can use padding in start sides in order to add some space between the inner border edges and the start of the scrollable content, but you cannot do this for the end sides. Or at least that's what happens on Firefox, Chromium adds the space in the block-end side, and sometimes in the inline-end one.

I don't have a strong opinion about what should happen, but it would probably be better to be consistent and treat end padding and end empty grid tracks the same way in this regard.

@MatsPalmgren

This comment has been minimized.

Copy link

commented Feb 17, 2019

@fantasai

Padding, gaps, and boxes are also abstractions.

All CSS concepts are abstractions, but that doesn't imply they are created equal. Margin-, padding-, border- and content-boxes are included in the CSS box model and the overflow falls out from that. Tracks, lines, gaps and other concepts are not included. So, if you want to include the extent of the grid in the scrollable overflow you are changing the CSS box model. That's fine of course, but it's not something that should be taken lightly. It could quite easily open up a can of worms both in specs and implementations. Before we do changes of this fundamental nature we should have a very strong motivating use case.

So, the use case at hand is that we want to distance the block-end border edge of the last grid item to the end of the container by at least 2rem. Fortunately, it turns out that CSS already has a feature for exactly this use case. (Concretely for the given testcase: remove the last track size and add #text { margin-block-end: 2rem; } - problem solved.). I can think of numerous other ways that would also solve this use case using existing CSS features only. Conclusion: the motivating use case is weak.

(FYI, I filed #3653 to sort out the exact definition of how child margins should contribute to the scrollable overflow. I intend to change Gecko so that is compatible with Chrome in Grid and Flexbox layout here, assuming the CSSWG agrees on that.)

@MatsPalmgren

This comment has been minimized.

Copy link

commented Feb 17, 2019

@tabatkins

the intrinsic size of the grid container is 200x400 ...

The grid container has height: 100vh; width: 100vw so it is not intrinsically sized at all. You're saying that its hypothetical intrinsic size should contribute to overflow, to be precise.

@css-meeting-bot

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

The CSS Working Group just discussed Empty grid tracks should contribute to scrollable overflow, and agreed to the following:

  • RESOLVED: Include space in empty explicit grid tracks in the scrollable overflow area
The full IRC log of that discussion <dael> Topic: Empty grid tracks should contribute to scrollable overflow
<dael> github: https://github.com//issues/3638
<dael> astearns: Had some discussion, sounded to me like we were breaking toward including explicit tracks in scrollable area of grid container
<dael> fantasai: I agree with that
<dael> astearns: Mats had a couple responses at end of thread that sounded like he's not objecting but not happy
<dael> fantasai: Wondering if that's b/c impl track info about tracks temp but don't store. Then managing overflow requires tracking additional information somehow. But I think authors really do expect those tracks to be in scrollable overflow so we should do that
<dael> astearns: Other opinions?
<AmeliaBR> RRSAgent, pointer?
<RRSAgent> See https://www.w3.org/2019/02/20-css-irc#T17-17-56
<dael> lajava: I also have doubts about impl this in Chrome. I commented some of my issues with this approach. Because the example about grid tracks contributing to intrinisic size I don't think is the ame as contributing to overflow area as Mats pointed out in last comment
<dael> florian: You mean technically they're different so no technical reason to handle the same?
<dael> lajava: Conceptual PoV. I don't think it's technically difficult but the argument for doing it is tracks contribute to intrinsic size. BUt in this case grid container has a specific size so shoudln't consider tracks as content by themselves. Why the overflow area is bigger then specific size if there is no content?
<dael> florian: I think logic is the author explicitly asked for that size. Even if not putting anything in it putting a size indicates theye xpect tht large. The intrinsic size is one way to see that the size they ask is what they get. If they don't observe it as that size it breaks that assumption. yeah there's no content but grid is often to use whitespace as an active part of the design
<dael> astearns: If you have a grid container that has explicit grid tracks, is auto sized, and not scrolling you'll see trailing white space before next bit of content. If you then constrain height and make it scrollable that whitespace disappears. That seems surprising to me
<dael> lajava: I'm not sure. I think if there are items in the second row even that first is empty I think we use the bottom position of first item to define scrollable area. In a why the whitespaces in the first row are not lost. Youmean the trailing ones? Yeah, I see
<dael> lajava: As Mats said there are other ways to achieve that
<dael> fantasai: But there's the question of what do authors expect. They expect the rows to be considered as space that'st here. That they're not scrollable is confusing to them and not what expected. If creating a grid and want random empty spots, if they happen to be in one row that row shoudln't collapse away. If theys aid there is a 100px column it should be there. It might be there from impl but end result of the page is you can't tell it's there.
<dael> florian: Thought of something else. Invisible things causing scrollbars to appear tends to be a thing authors hate. Given this isn't part of box model this is harder to find. I think with FF grid dev tools you could figure out what's going on but not other browsers
<dael> fantasai: I think if you give explicit track sizes you should be able to figure out it's causing the scroll. We also have padding and it should be considered in scrollable and we get a lot of bugs on why it's not. Initial behavior for overflow is you trigger only as necessary to show content. There are a lot of general expecttations that there will be scrollbars
<dael> fantasai: Authors are unhappy when don't include padding in scrollable overflow. We're limited by web compat there, but we're not here. I don't think authors will be they put 10 tracks of 100px and why is my scrollable overflow not 500px because only half are full.
<dael> florian: I'm back to agreeing
<dael> astearns: Your point is fair that this is a new thing that causes space to be added. This isn't in pre-grid dev tools, but that's a dev tool issue
<dael> plinss: General principle: If you change the overflow behavior it should not effect it's size
<dael> astearns: And size before changed overflow is what we're trying to arrive at where emptytracks contribute to scrollable
<dael> iank_: What is use case authors want to have additional tracks?
<dael> astearns: Preserving layout intent. It had the whitespace before scrollable so should contibue once it is
<dael> florian: And what fantasai said about breathing space between content and edge
<dael> iank_: So what padding was previous
<dael> AmeliaBR: Or saving space for content that will be added
<dael> astearns: Good point. Not changing the size as you add things to explicit grid is pretty useful
<dael> iank_: Really good point AmeliaBR
<rego> the padding is also lost as oriol explained https://github.com//issues/3638#issuecomment-464400716
<dael> astearns: I thinkw e are converging on including explicit grid track sizing to the scrollable area of a grid container
<rego> it'll be nice to be consistent on that case in the future too
<dael> astearns: Anyone on the call that has an argument against or would object to resolving?
<dael> lajava: I think rego has a point with padding not being consistant. I think oriol mentioned that in issue
<dael> oriol: Prob with padding is there is no interop. In FF it does not add space but in Chromium the space is always added in block axis and sometimes in inline. So there is no interop. I think this is a similar case and would prefer consistent model.
<dael> fantasai: We'd like consistent but we're constrained on compat. There are issues on overflow spec and hte goal is to include the padding.
<dael> astearns: If oriol says there is no interop there may be more possibility to get behavior we want
<fantasai> See issues in https://drafts.csswg.org/css-overflow-3/#scrollable
<dael> fantasai: The amount to which chrome includes padding we're trying to do that. yOu can see those issues on overflow spec. but in inline axis it's pretty random. Sep discussion
<dael> daniel: I'm testing chrome 24 and it seems to behave same as FF
<dael> lajava: Yes because we just fixed the bug
<iank_> s/chrome 24/chrome 74/ (I assume)
<dael> lajava: b/c there is nothing in the spec that suggests we should use grid tracks to contribute to overflow area. If we want this to be new we should propose new text
<dael> fantasai: Yes, this is not a clarification
<dael> astearns: That is a slight concern. There is a bug Chrome canary responded to. I'm not sure if that's interop or if person that wrote the chrome bug wanted space to collapse.
<dael> fantasai: Don't know, but guessing spec compliance.
<dael> astearns: My guess as well
<dael> florian: If I'm looking at right thing, chrome bug is from Mats
<dael> lajava: Yeah, original bug was to FF but Mats argued they followed spec and he filed bug against Chrome. WE decided to follow spec
<dael> astearns: Thanks for the quick response on that
<dael> lajava: Means we need to agree with Mats this is behavior we will impl
<dael> astearns: Certainly. If we resolve we need to get Mats to agree to change behavior to spec. Unfortunate he's not on call, but I don't see in his issue comments reasons to not resolve the way we intend
<dael> florian: Clarification point or extra argument. Given that Chrome in some cases includes padding if we have empty tracks and padding it seems it will be confusing to including padding and not empty tracks
<dael> astearns: Interop on incl padding?
<fantasai> https://drafts.csswg.org/css-align-3/#overflow-scroll-position
<dael> florian: No but we're trying to including it at least as much as we can. And when alignment is not the default we always do. So when we have some cases where include the padding not including empty track seems weird. Or am I confused?
<dael> fantasai: I think we should try and include padding in grid if we can. If we can include empty tracks we should because it's used for many more cases then padding
<dael> florian: Yes, but given padding is outside not including the inside is confusing
<dael> fantasai: Tackle in sep issue but I think you're right we should adjust grid and flexbox to do thing authors want to do
<dael> astearns: Objections to including space in empty explicit grid tracks in the scrollable overflow area?
<florian> +1
<dael> RESOLVED: Include space in empty explicit grid tracks in the scrollable overflow area
<dael> astearns: Hopefully impl can follow. We'll get feedback as we go.
<dael> astearns: Do we have issues for including padding in scrollable areas?
<dael> florian: We do
<dael> fantasai: Might want a separate one on grid
<dael> astearns: Is that overflow spec or grid?
<dael> fantasai: mmm...I think grid. I'll file
<AmeliaBR> Lots of open issues: https://github.com/w3c/csswg-drafts/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+scrollable+padding
@charbelnicolas

This comment has been minimized.

Copy link
Author

commented Feb 20, 2019

Thank you guys, I really appreciate it. I think this will make web design more straightforward.

One grid to rule them all.

@fantasai

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2019

Alright, fixed in the spec. Thanks for the report, @charbelnicolas ! That triggered some follow-up discussions/fixes for both Flexbox and Grid in #3665 and #3653 as well, fwiw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.