-
Notifications
You must be signed in to change notification settings - Fork 660
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] Stretching image grid items in both dimensions #523
Comments
We were discussing that yesterday at TPAC (@fantasai, @jensimmons and me). |
Well, actually manually setting the width won't cause the aspect ratio to be observed: it just sets the width in that dimension, and since the initial value of align-self is Squishing images seems a little odd, but as a starting point it's useful: the |
(Plus it's consistent with how blocks behave so not surprising at all.) |
@mrego I don't think that solution works, because setting width:100% My proposal doesn't have that problem. |
@fantasai, I don't think object-fit:contain works either since |
Cover example, image proportionally sized and clipped to fit:
Contain example, margin/border/padding fitted to grid slot and image proportionally sized into it:
Contain example, margin/border/padding fitted to proportionally scaled-down image:
Contain example with future syntax, margin/border/padding fitted to proportionally scaled-down image:
|
As you say, this leads to the image being clipped and is thus
This leads to the image overflowing when the window is very wide
Suggesting magic future tech to solve this problem isn't serious.
I've made a demo of I see the following problems:
My proposal doesn't have any of the above problems and it provides |
To be clear, what I'm proposing is that spec says something like this
|
(It's possible you might want that rule to be conditional on the element having |
It seems that the proposed text is a bit underdefined in terms of how min and max sizes interact with the "ratio-preserving way". Also I'm not sure that the current definition of min sizes in grid actually allow you to preserve the aspect ratio if the image is too big in both axes and does not have a width or height explicitly specified on the image. |
There are some pros and cons with that, but I don't have a preference. |
Presumably using the rules in CSS2 section 10.4 that apply to this case. |
Just discussed in today's teleconference (I'll try to link minutes later). The conclusion is that we can change the mapping of |
OK, so |
I really like the proposed solution to make Could you please clarify which behavior you want when the *-self value I think it should fill the grid area in the axis that has |
… 'stretch' alignment for grid items with an intrinsic ratio. r=dholbert w3c/csswg-drafts#523
… 'stretch' alignment for grid items with an intrinsic ratio. r=dholbert w3c/csswg-drafts#523
@fantasai / @tabatkins, I think this ^ is a question for you. Mats' suggestion here makes sense to me, FWIW (and I can't come up with anything that's more sensible). |
… 'stretch' alignment for grid items with an intrinsic ratio. r=dholbert w3c/csswg-drafts#523
#523. (First cut, needs review, didn't address object-fit question.)
Checked in an attempt at fixing this per above discussion. :) Please let me know if anything is askew. I took Mats's suggestion for handling Wrt basing on |
Agenda+ for certification of edits and the details wrt |
Oh, just wanted to check that we're all understanding that |
No, I would summarize what we have implemented as:
IOW, the intrinsic ratio is preserved if at least one of the values is It gets more complicated when one or both axis also has min-/max-size constraints. I think we're following the CSS2 rules[1] there -- we're re-using the code that already implements that. The stretching occurs before applying those rules. Also, when the track max-sizing functions are definite, clamping the item to fit the grid area (per §6.6 in CSS Grid https://drafts.csswg.org/css-grid/#min-size-auto) also affects the above rules (the "may overflow..." above should not be allowed when clamping in that axis IMO, but let's sort out the clamping effect on this in #767) Here's a couple of testcases to illustrate our stretching behavior (I intentionally Testcase for shrinking an image to fit a smaller grid area: (You need a Nightly build for these tests to display correctly: https://nightly.mozilla.org/ ) |
OK, so, what I want to point out is that from an authoring point of view, the intrinsic aspect ratio should be preserved unless there's specific instructions not to. Maintaining the intrinsic aspect ratio trumps preserving the intrinsic size in any single dimension. Always. Changing a dimension effectively modifies the user-perceived "intrinsic size" in the other. Internally, we can define terms however we want, but I'm pretty sure that as far as the author is concerned, anywhere that we are failing to do this is an error. An alignment value of To address your feedback that the default behavior should be to preserve the aspect ratio, we decided to add an independent behavior for p.s. Fwiw, I'm a little concerned that the new |
…operly capture the resolutions in #523.
Initial draft of |
…es to be readable, and properly capture the resolutions in w3c/csswg-drafts#523
I'm going to implement this on Chromium, just to be sure that I'm not missing anything, I understand this only applies to Grid Layout. So |
Now that issue w3c/csswg-drafts#523 has been resolved, I'm adding a new test to verify the default alignment ("normal") for replaced and non-replaced elements.
Now that issue w3c/csswg-drafts#523 has been resolved, I'm adding a new test to verify the default alignment ("normal") for replaced and non-replaced elements.
"normal" alignment used to be "stretch" for all the elements, however the CSS Grid Layout spec has been updated to change this behavior for replaced elements: w3c/csswg-drafts#523 A few tests need to be updated due to this change. We also take advantage to make fast/css-grid-layout/grid-align-stretching-replaced-items.html a testharness.js test so we can get rid of the expected file. BUG=666961 TEST=external/csswg-test/css-grid-1/grid-items/grid-items-sizing-alignment-001.html Review-Url: https://codereview.chromium.org/2722613003 Cr-Commit-Position: refs/heads/master@{#454133}
Now that issue w3c/csswg-drafts#523 has been resolved, I'm adding a new test to verify the default alignment ("normal") for replaced and non-replaced elements. Build from revision f8dfabbc16a0e061f6ba61d1ad62e8d20e388c43
Just a ping to allow people to review issue 1112 and give an opinion there? |
Wrt conclusion stated in #523 (comment) ... here are the minutes. The key excerpt is that three solutions were considered for handling replaced elements:
The problem with option 3 is, as described in #523 (comment) , that what should be an alignment control devolves into a sizing control, which is objectionable. This left two options: 1 & 2. Both allow for all of the relevant behavior to be specified, they just differ on the default. Several arguments tilted in favor of option 1:
The WG therefore resolved on option 1, and actioned me and Tab to draft up a spec for @MatsPalmgren Sorry for taking so long to post a proper summary. Please let us know what you think, and if this is acceptable to you. For my part, I am OK with either 1 or 2, but I would have to object to 3. |
I still think image grid items should grow (or shrink) to fill their grid area while preserving their intrinsic ratio by default. I think
Opting out from that can be done by specifying If option 2 makes image grid items grow/shrink to fill their grid area in a "contain sizing" way by default, that's certainly acceptable to me. I'd be interested to review a more fleshed out proposal though... in particular how it interacts with |
The CSS Working Group just discussed The full IRC log of that discussion<dael> Topic: Stretching image grid items in both dimensions<dael> Github Topic: https://github.com//issues/523#issuecomment-295075752 <dael> Rossen: I don't see fantasai yet. <zcorpan> Github Topic: https://github.com//issues/523 <dael> Rossen: Anyone from Mozilla want to handle this? Or do we move on? Current tag is customer not satisfied. <dael> TabAtkins: We should wait on fantasai because she might have more to the topic to explore. |
The CSS Working Group just discussed
The full IRC log of that discussion<dael> Topic: Stretching image grid items in both dimensions<TabAtkins> Yeah, I think that's probably the best error, zcorpan <dael> Github topic: https://github.com//issues/523 <dael> fantasai: In seattle we had three options. One we wouldn't take b/c violates design guidelines. Other two we would have contain & intrinsic size behavior. You could opt into either. We picked intrinsic for the default. There's details in https://github.com//issues/523#issuecomment-295075752 <dael> fantasai: We closed that, but Mats disagrees with the resolution. All other issues on Grid are closed as of when we stopped accepting issues. <dael> fantasai: This issue has a commentor disagreement so comes back to WG. Do we stick or do we change to contain sizing by default and opt-in for intrinsic via keyword-max-content or something like that. <dael> Rossen: Opinions? <dael> jensimmons: My opinion is I agree #3 should not be considered. I'd be okay with leave as-is, but I agree with Mats that as an author having the default be contain would be more handy and make more sense. We need both as an option and the tool to switch definitely. The default I'd lean contain. <dael> TabAtkins: Reason why we didn't go for contain is it puts sizing control in this prop... <dael> fantasai: TabAtkins we changed. <dael> jensimmons: That was 3 which we agreed not to do <dael> fantasai: Option was default is contian on all keywords, not jsut normal. <tantek> do we have comments from anyone else who is actually implementing this besides Mats? <tantek> reviews the issue <dael> jensimmons: Ideally we would have tool to switch between, but the wayt he world works if we set default to contain we would get the switching tools quicker. Not certainly, but might. <rachelandrew> I'm still keen on the resolution <dael> Rossen: But forcing a less then optimal default as a forcing tool i sn't doing much good. <tantek> long issue is long <dael> jensimmons: Agreed. I still like contain as a default. That's a secondary non-heavy <dael> TabAtkins: I'm concerned that when we have contain sizing....having different defaults based on layout of parent it makes the design confusing. It's better if we have consistancy. <dael> jensimmons: [missed] <Rossen> +1 to what Tab said <dael> fantasai: Flexbox resizes to fit hte container. It does only do that with a single line. If you have wrapping it takes intrinisic <dael> TabAtkins: It obeys constraint of flexbox and a bit of aspect ratio magic. The sizing is normal flexbox, not just images. <dael> jensimmons: You can make the same arguments here. <dael> TabAtkins: I disagree. Part of flex is doing this one thing. We're not adding a different way of sizing. <dael> jensimmons: I think from author prospective there would be something similar to having default of flexbox where they are both text and image contained. If there's an image just left with intrinisic size it starts to overflow and that's not typically what authors want. Authors expect content to stay in grid cells. <tantek> "won't spill outside" e.g. the [CSS is Awe]some problem <dael> fantasai: Things that are smaller then grid size, contain will cause them to size up. <dael> rachelandrew: I don't like the upscalling. I like the original resolution b/c i t's the most consistant and easier to explain. the upscalling will cause problems. <tantek> didn't follow Rachel's comment <gregwhitworth> +1 to TabAtkins rachelandrew <dael> fantasai: Another thing to consider is grid is being deployed and we don't have a definition of contains that WG approved. Changing will c ause impl stress. <dael> TabAtkins: Adding that, even if we had a contain definition, it's still transition stress because it's shipped without this. They're getting things to look well and then this change will break pages. <dael> fantasai: I would say if it was significantly better thing to do it might be worth doing, but since we're debating and there's good for both sides, given the status we're at it makes sense to go with intrinsic a nd add the contain keyword. That would be my position. <dael> TabAtkins: I agree. <rachelandrew> +1 to fantasai <dael> fantasai: I'm sympathetic, but it's not so clearly better that it's definitely the right answer. <dael> TabAtkins: I would also threat it differently if it was obvious fix instead of possibly better. <dael> Rossen: I'm seeing people favor option 1. Let's try for resolution. <dael> Rossen: Unless jensimmons you feel there's something else you want to add. <dael> jensimmons: I don't want to block it, but I think we need to define contain. <dael> fantasai: We have a definition, we need comments. THere is a definition in sizing 4. Mostly waiting for dbaron and Mats there. <dael> jensimmons: We should keep going forward on getting that done b/c it's desperately needed. <dael> dbaron: Does option 1 match what impl ship today? <dael> Rossen: I believe so. <dael> tantek: It does. <dael> s/tantek/ TabAtkins <dael> jensimmons: I'm curious about that. I'm not sure what Mats impl and he did Gecko. <dael> TabAtkins: I'd hope he's not impl something different. Even if he is, the other browsers aren't. <dael> dbaron: I would disagree with TabAtkins' hope. But I don't know what he did. <tantek> this issue feels like it has many aspects, are there at least some aspects we can converge on? <dael> Rossen: Let's go back to this. Sincec Mats' isn't here I'd liket o call for consensus on #1. We'll continue working on contain. <tantek> I'm having trouble keeping track of n resolutions over the past on this and n options <dael> Rossen: Obj to sticking with Seattle resolution: making the default instrinsic? <fantasai> testcase - <jensimmons> we don’t have interop on https://jsfiddle.net/1fd948nz/1/ at the moment, fyi <dael> RESOLVED: ticking with Seattle resolution: making the default instrinsic |
Test case for the above discussion added by fantasai: |
Was just tech-editing someone's introduction to Grid Layout and noticed her description of the default alignment behavior for grid: it's stretching unless there's a specified size, in which case it's start. Having replaced elements behave as start by default matches this -- technically they don't have a “specified” size, but the intrinsic size stands in for that specified size, yielding consistent behavior for items that have a concrete size. I think this behavior makes sense and is understandable for authors. |
… 'stretch' alignment for grid items with an intrinsic ratio. r=dholbert w3c/csswg-drafts#523 UltraBlame original commit: 4e9d3d21f20db22985bf61346d03b40097bd47e3
… 'stretch' alignment for grid items with an intrinsic ratio. r=dholbert w3c/csswg-drafts#523 UltraBlame original commit: 4e9d3d21f20db22985bf61346d03b40097bd47e3
… 'stretch' alignment for grid items with an intrinsic ratio. r=dholbert w3c/csswg-drafts#523 UltraBlame original commit: 4e9d3d21f20db22985bf61346d03b40097bd47e3
I'm wondering about how we should stretch grid items that has
an intrinsic ratio, such as images. The Grid spec just refers to
css-align which says:
https://drafts.csswg.org/css-align-3/#justify-self-property
"the stretch keyword sets the box’s used size to the length necessary
to make its outer size as close to filling the alignment container
as possible while still respecting the constraints imposed by
min-height/min-width/max-height/max-width."
which I think means that having 'stretch' in both dimensions
will resize the image to fill the grid area without respecting
the image aspect ratio. I think this is a rather unfortunate
default behavior for Grid.
Some background; this topic was discussed for flex items here:
https://lists.w3.org/Archives/Public/www-style/2012Oct/0781.html
where everyone seems to agree that respecting the ratio is desirable,
but for various flex layout specific reasons, this could not be
achieved and it was decided to ignore the ratio (IIUC).
As far as I can tell, those reasons do not apply to Grid, so I see
no reason why we can't respect the ratio when stretching grid items
in both dimensions.
I think web authors generally prefer to preserve aspect ratios,
so that's what I think we should do for grid items.
The text was updated successfully, but these errors were encountered: