-
Notifications
You must be signed in to change notification settings - Fork 686
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] Replaced elements canvas, img, etc should behave as expected when styled with relative sizes in grid, transferred size should not affect them unless specifically asked to #6278
Comments
You can also set Changing |
This could also benefit from
That seems precisely what you are trying to do. |
@tabatkins and I discussed this issue the last couple days, and here are our thoughts:
(Put
Possible solution: Make indefinite percentages resolve against zero and thereby clamp the automatic minimum size (similar logic as compressible elements), or at least do so for compressible elements (like the replaced element here)? Question: What is Firefox doing here that is getting the expected behavior? |
That's how all Chromium, WebKit, Firefox and old Edge used to behave. But Firefox changed in #2367, which wasn't completely clear, and in #3612 we settled on the current behavior, and I changed Chromium and WebKit. Changing back and forth doesn't sound that great. |
@Loirooriol Mmm, I think there's some confusion. We weren't proposing to change percentage height resolution in general, just when calculating the automatic minimum size. Because the automatic minimum size is not supposed to be stronger than a specified size. And also, as we explained, Firefox is generating the expected behavior on this testcase. Whether they're doing it the way we propose or some other way, we need to find out. |
Firefox doesn't implement the 2nd pass for this case correctly which is why it has its particular behaviour, in addition the engine currently doesn't correctly perform the transferred size suggestion more generally. (Lots of ways to trigger this, e.g. https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=9508 ) The best way out of this I believe is to add another exception to: "To provide a more reasonable default minimum size for grid items, the used value of its automatic minimum size in a given axis is the content-based minimum size if all of the following are true:" And add something like: I don't think changing the %-resolution size when determining the automatic minimum size is the correct solution here. |
@bfgeek That's roughly what we're saying, except we'd apply it to all compressible elements, and zero out the percentage in the preferred size cap rather than conditioning it on being a percentage, so that things like |
Here is a testcase which doesn't involve <div style="display: grid; grid-template: minmax(min-content, 1fr) / minmax(min-content, 1fr); width: 200px; height: 100px; border: thick solid;">
<img src="http://placehold.it/100x100" style="width:100%; height: 100%; min-width: 0; min-height: 0; opacity: .9;">
</div> My understanding of what's happening is, following https://drafts.csswg.org/css-grid/#algo-overview
The percentage resolves against 0 according to https://drafts.csswg.org/css-sizing-3/#replaced-percentage-min-contribution
So the image is sized in a cell of width 200px, and somehow via the aspect ratio this produces a min-content contribution of 200px. The percentage doesn't resolve against 0 like previously, presumably because
In other words, Chromium just produces the same as this non-grid testcase (interoperable with Firefox): <div id="fake-grid-container" style="width: 200px; height: 100px; border: thick solid">
<div id="fake-grid-cell" style="width: min-content; height: min-content; min-width: 100%; min-height: 100%;">
<img id="fake-grid-item" src="http://placehold.it/100x100" style="display: block; width:100%; height: 100%; min-width: 0; min-height: 0; opacity: .9;">
</div>
</div> But if I recall correctly, that "unless otherwise specified" was for css2 boxes, and we are considering grid layout. So my hypothesis is that if Chromium implemented #3973 in order to be able to size grid items under a min-content constraint in the block axis, this
(the spec may need edits to really enforce this). |
I think there needs to be a slightly larger discussion about how percentages resolve here, as this currently isn't how these expressions resolve in implementations (Safari, Firefox, and Chrome behave the same here). E.g. https://www.software.hixie.ch/utilities/js/live-dom-viewer/?saved=9512 I'd be slightly concerned changing implementations here as this would be a pretty significant change from a web-compat point of view.
I believe that would be correct from a pure spec point of view, but this ties into the point above that there is a pretty large difference between the implementations and spec. I still think the safest thing for this case would be if added another exception to https://drafts.csswg.org/css-grid-1/#min-size-auto |
It seems implementations just compress the entire intrinsic contribution 0 instead of only resolving the percentage to 0. But that seems orthogonal to this issue, I guess whatever implementations are doing for compressible elements outside grid can also be done in when sizing the grid tracks. BTW, Firefox behaves like Chromium when using <div style="display: inline-grid; grid-template: 1fr / 1fr; width: 200px; height: 100px; border: thick solid;">
<div style="aspect-ratio: 1 / 1; background: cyan; width:100%; height: 100%; opacity: .9;"></div>
</div>
<div style="display: inline-grid; grid-template: minmax(min-content, 1fr) / minmax(min-content, 1fr); width: 200px; height: 100px; border: thick solid;">
<div style="aspect-ratio: 1 / 1; background: cyan; width:100%; height: 100%; min-width: 0; min-height: 0; opacity: .9;"></div>
</div> When using |
The CSS Working Group just discussed
The full IRC log of that discussion<fantasai> Topic: replaced elements percentage-sized within rows<fantasai> s/rows/grid areas/ <fantasai> github: https://github.com//issues/6278 <fantasai> github: https://github.com//issues/6278 <fantasai> iank_: There's concept of compressible elements, if you put a percentage width in one axis, it will compress to min-size <fantasai> iank_: Chrome did a bunch of fixes in this area (replaced elements) recently <fantasai> iank_: that brought our behavior more inline with the spec here <fantasai> iank_: which applied an automatic minimum size in this case <fantasai> iank_: web author found this really surprising behavior, filed issue <fantasai> iank_: Question is do we want to ...? <Rossen_> q <fantasai> iank_: Firefox behaves as expected, because there is some behavior around transferred sizes that they still don't do correctly. <florian> s/.../follow the spec, and if not, how do we match the author's expectation/ <fantasai> https://github.com//issues/6278#issuecomment-863439988 <florian> fantasai: tab and I looked over this <florian> fantasai: our conclusion is summarized in the issue <florian> fantasai: the author's expectation is reasonable in this case <florian> fantasai: chrome and firefox used to match that, until chrome fixed to match the spec <florian> fantasai: [explains example] <fantasai> [ fantasai insert summary later] <Rossen_> 1fr has an auto min implied and that's what's causing the breakage. <Rossen_> fantasai: the auto min size is supposed to be xtronger constraint of specified height... and that's what's happening because the % isn't taking affect <fantasai> s/xtronger/weaker/ <Rossen_> fantasai: the sln we are interested in is to make % resolve against min definite height. resolving against 0 is to clamp the min size. do it for compatible elements and potentionally all els <Rossen_> fantasai: does that makes sense? <Rossen_> iank_: I did :) <TabAtkins> Summary: During Grid layout, we first lay out to figure out row heights, then lay out given that height to figure out column widths (then maybe do both of those again). A 1fr item size has an automatic minimum; during the first layout both dimensions are unknown so the auto-min doesn't trigger, but in the second layout there is a height so it *does* trigger, and this ends up giving us the bad behavior where the height is frozen as "large" <fantasai> s/does that makes sense/did everyone follow, or should I explain something again <fantasai> iank_: I'd be concerned about doing this for all elements <fantasai> iank_: if we do, should only do for compressible elements <TabAtkins> But our intention was always that auto-min be a relatively weak constraint - it shouldn't be taken as gospel in the middle of this algo and have this effect. <fantasai> Rossen_: what's downside to doing it for all elements? <fantasai> iank_: Currently, if you've got a non-replaced element with width:100% <fantasai> iank_: that would change behavior pretty drastically <fantasai> fantasai: what if we only clamp the transferred size <TabAtkins> (because of the ordering of row vs height resolution, this behavior doesn't have a bad effect when the space is tall; it only makes the element too-large when the space is wide, which is an annoying discontinuity) <fantasai> iank_: so only things with aspect ratio ... <fantasai> iank_: would need to think about that a bit more <fantasai> Rossen_: can add aspect-ratio to anything, now, right? <fantasai> fantasai: 2 inputs into auto min size: content size in that axis, and for items with aspect ratio (previously only replaced elements), size trasferred <fantasai> fantasai: suggestion would be to make transferred size weaker than percentage size <fantasai> iank_: there's a difference between spec and implementations atm with this stuff <fantasai> iank_: spec says to resolve lengths against zero, whereas implementations will actually ignore that length <fantasai> iank_: ignoring that difference, your suggestion and mine are effectively equivalent <fantasai> fantasai: no, yours was to do it to it for compressible elements <emilio> fantasai: no, because you are saying to do it only for compressible elements and I'm suggesting to do it for all elements with a-r <emilio> iank_: if I change my suggestion then it agrees right? <emilio> fantasai: [missed]. It'd be more consitent if we only clamp the transferred size. If we do clamp AMS if you have an aspect-ratio then it changes behavior <emilio> iank_: if a replaced element has an intrinsic size I don't think that we'll want to respect that in this case <emilio> ... if you have width 100% <emilio> ... that'd break the person's initial issue, the intrinsic size was actually quite large <emilio> fantasai: in that case I wonder if that should depend on whether they're compressible. Clamp transferred size always, clamp content-based size if it's compressible <emilio> iank_: why not do it for both then? <emilio> fantasai: mainly because it means that aspect-ratio affects sizing in an axis where it'd otherwise not have any effect <emilio> ... I don't have an objection but it's a bit weird <Rossen_> q? <emilio> ... if you apply an aspect-ratio to a div where we apply the content size, then it'd clamp to zero <fantasai> s/clamp/suddenly clamp/ <emilio> Rossen_: can we go back to the previous proposal, to make it apply to all elements? <emilio> ... we can try to constrain later if we have strong reasons to <emilio> fantasai: I think we may have problems if we try to apply to all elements <emilio> Rossen_: this is something we should be able to figure out quite quickly <emilio> iank_: I think we can do the change for compressible and a-r elements quite easily. I need to think about non-replaced elements with a-r inside an auto-sized thing, dunno how to implement that yet <TabAtkins> ian, let's do a video chat about this soon <emilio> Rossen_: so, let's start with compressible, would that work? <iank_> sure <emilio> fantasai: I think my preferred suggestion would be all elements, but if we can't we should do it for compressible elements, do it for transferred and content size, and for a-r elements just the transferred size <emilio> Rossen_: not opposed, but it's weird to make special elements more and more special, specially if the behavior is useful for all elements <emilio> ... so I'd prefer to pursue that if possible <tantek> +1 agreed with Rossen_ <tantek> and with that methodology in general <emilio> fantasai: I'm fine with all elements, but if not I prefer the suggestion above, I think it's the closest <emilio> Rossen_: can we resolve on all elements and if you come back with strong reasons why it doesn't work we can consider the other suggestion <emilio> iank_: when we say all elements we mean all elements with a-r? <emilio> fantasai: we meant all elements <fantasai> s/fantasai/rossen/ <emilio> iank_: That's a very big change, I don't think that'd be ok <emilio> fantasai: I'm also skeptical about that <emilio> Rossen_: so should we resolve on compressible elements + a-r? <emilio> fantasai: [explains her suggestion again] <emilio> Rossen_: sounds reasonable, other comments / objections? <Rossen_> s/a-r/aspect-ratio/ <fantasai> proposal: clamp automatic minimum size of compressible elements, and the transferred size suggestion of non-compressible aspect-ratio elements <tantek> sounds reasonable to limit to those elements <fantasai> RESOLVED: clamp automatic minimum size of compressible elements, and the transferred size suggestion of non-compressible aspect-ratio elements <fantasai> fantasai: Thanks everyone. That was a complicated issue, and I understood it better by talking it through with all of you. See official minutes |
The resolution seems reasonable, but the thing about compressible elements is that their min-content contribution is compressed. And the resolution only affects tracks with an |
…ntage specified and maximum sizes. #6278
Edited in. @Loirooriol, mind reviewing? |
Overall it looks good, but I don't get why the clamping happens for the transferred size suggestion, and then for all size suggestions. Wouldn't the latter alone suffice? Also, I had to reread it because the wording is a bit confusing, first checking "whichever are definite", but then saying that indefinite percentages are considered definite 0. The wording for clamping the transferred size suggestion seems easier to understand to me. Well, except for "If the item’s has a preferred size", the preferred size can't be |
The transferred size (and only the transferred size) always has its percentages resolved against zero for clamping. The other sizes only have percentages resolved against zero if the item is a compressible element.
Yeah, agreed it was worded in a slightly confusing way. We reworded to match the transferred size clamping text more closely: In all cases, the size suggestion is additionally clamped by the [=maximum size=] in the affected axis,
- if it's definite.
+ if it's [=definite=].
If the item is a [[css-sizing-3#min-content-zero|compressible replaced element]],
- the size suggestion is clamped
- by both the [=maximum size=] and the [=preferred size=], whichever are definite,
- with their indefinite percentages resolved for this purpose against zero
- (and therefore considered definite).
+ and has a [=definite=] [=preferred size=] or [=maximum size=]
+ in the relevant axis,
+ the size suggestion is capped by those sizes;
+ for this purpose, any indefinite percentages in these sizes
+ are resolved against zero (and considered [=definite=]).
Yes, it definitely wants to exclude intrinsic sizes. New text (also with very slight editorial rewording, to be perfectly parallel with the preceding clamping text): - If the item’s has a [=preferred size=] or [=maximum size=]
+ If the item’s has a [=definite=] [=preferred size=] or [=maximum size=]
in the relevant axis,
- the [=transferred size suggestion=] is capped by those sizes,
- resolving any indefinite percentages in these sizes
- against zero for this purpose.
+ the [=transferred size suggestion=] is capped by those sizes;
+ for this purpose, any indefinite percentages in these sizes
+ are resolved against zero (and considered [=definite=]). /cc @Loirooriol ? |
LGTM, but the quotes in the Changes section should probably be updated too. |
@Loirooriol Done. Thanks for catching that--and for your thorough reviews in general!! |
Almost all the pictures you see on the web are scaled. This has been the case ever since we first saw images on displays. It is a very, very rare exception that you want to display raw raster data on the web exactly matched to the screen resolution. Yes, it happens and we do it often, but that's a fraction of our actual usage and in those instances it's most often a deliberate choice instead of the expectation.
Firefox currently implements this as expected, Chrome also did until recently. Additionally, a somewhat similar issue has already been discussed before: https://stackoverflow.com/questions/60206276/chrome-80-css-grid-large-descendants-cause-grid-to-grow and there it was decided that yes, this behaviour change is not acceptable to what developers expect.
And expectations matter. Yes, Chrome's interpretation is, apparently (though not all are entirely sure if it actually is) according to the spec, but if so, the spec is not what developers expect and is also not what we are taught from the most used references (and I stress, Firefox behaves as expected).
If you use a search engine to find help and examples on grid, you'll end up on
A complete guide to Grid from css-tricks
or
Basic concepts of grid layout from MDN
I'm going to venture and say that these two guides account for the vast majority of all grid documentation people look for.
Thus, if we have a page like this
with style
Live example
We absolutely expect that
the page doesn't scroll (no margins and all sizes for html, body, section and img are relative).
the image automatically occupies the assigned grid location and is contained (preserve aspect ratio, maximum fit to the container) to it.
However, this is not the case on Chrome, and the actual dimensions of the image today cause the image to break these two strong expectations. I stress the third time, Firefox currently behaves as expected, and Chrome did, too, until recently.
We can try to add contain: strict; (or just size) to the img tag, but this breaks the second expectation, aspect ratio is not preserved.
Styling pages should first follow logical expectations, that work on Firefox currently and break on Chrome after a recent change, and only after provide specific changes to augment the behaviour if needed. No reasonable developer would, after seeing this markup, expect these two assumptions 1 and 2 to break.
As mentioned, the core of the issue is transferred size. The behaviour is not logical, nor what is expected and the change in the draft causes regressions and breakage. object-fit already exists and perfectly describes the wanted behaviour.
The solution offered by Chrome "grid-template-rows: minmax(0, 1fr) auto;" is not consistent with the expectations of developers. If such additional behaviour is wanted, the extra should enable transferred size logic, but it should not be the default.
This must change.
Issue tracker for Chrome where this is marked as working as intended, it's a feature and WontFix.
Other attempts I did while debugging this issue that variously demonstrate the issue with a canvas element
https://emil.fi/p/gridscreen/
https://emil.fi/p/dynamiccanvas/
https://emil.fi/p/gridresize/
https://emil.fi/p/canvasgridresize/
Cheers
The text was updated successfully, but these errors were encountered: