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-sizing] how should inline-axis intrinsic sizing work for 'fit-content' and 'fit-content()'? #3731

Closed
dbaron opened this issue Mar 13, 2019 · 16 comments

Comments

@dbaron
Copy link
Member

dbaron commented Mar 13, 2019

This issue is probably blocking implementation of the fit-content() function (which is in css-sizing-3) in Gecko, based on a bug comment I wrote a few months ago. I realize I never filed an issue here about it, though, so I'm doing so now.

The css-sizing-4 spec's definition of intrinsic sizes doesn't say how intrinsic sizing should treat the fit-content() function. It also defines a behavior for the fit-content keyword that I'm not convinced makes sense.

What I wrote in the above bugzilla comment about how I think things ought to behave was the following:

Intrinsic sizing behavior for this function (and for 'min-content', 'max-content', and 'fit-content' keywords) in the block dimension is not particularly relevant here, since they're all treated as the initial values. while intrinsic sizing in the block dimension isn't particularly well defined until after #2890 happens, we don't need to worry about that here.

I think the intrinsic sizing behavior for 'min-content' and 'max-content' in the inline dimension is pretty easy; they compute to a single length, and the intrinsic sizing behavior should be the same as for <length> type values (for whichever of 'inline-size', 'min-inline-size', or 'max-inline-size' is the case -- <length> values have different effects on the three of them).

Intrinsic sizing for 'fit-content(<length>)' is also easy, since it also computes to a single length.

However, intrinsic sizing for 'fit-content(<percentage>)' is harder, since what it resolves to in the end (during layout) depends on things outside the box. All the previous cases were easy because the size specified is a function only of the contents of the element and the properties on it, but now we need to consider what to do for percentages. Some of this may depend on how we treat inline-size: <percentage>, min-inline-size: <percentage>, and max-inline-size: <percentage>. However, there are also cases where the way we treat those was a compromise that we made for compatibility, and we might want to do something better here. (On the flip side, doing something different might also be confusing if developers are accustomed to the current behavior.) In the past, there have been three ways to approach handling <percentage> in intrinsic widths:
(1) treat them just like 'auto' (i.e., pretend they're not there) -- this way has basically won (for percentages) at this point
(2) reverse-compute them on their own, where a box with max-content size S and width: P (a percentage, expressed so 1.0 is 100%) has a max-content contribution to its parent of S/P. This prevents overlap in block-like layout, but isn't sufficient when there's more than one thing per line.
(3) reverse-compute them together, so that if a line/row/table-row has boxes with sizes S1...SN and percentages P1...PN and then other non-percentage (fixed) sized boxes F1...FN, the contribution of the entire row to its parent is the larger of sum(F1...FN)/(1 - max(sum(P1...PN), 1.0)) and any of Si/Pi. The first term ensures that the sizes for the non-percentage parts fit within the percentage remaining after all the percentage parts, and the remaining terms ensure that each percentage part is big enough. This is roughly what we do for tables.

So (1) is simple for percentages -- but it's not immediately clear to me how it works for 'fit-content(<percentage>)'. That's the part that I'd need to think about more. Doing (2) or (3) is probably even more complicated. But there's probably a simpler way to think about what's needed for fit-content() that I haven't quite thought of yet...

Intrinsic sizing for the 'fit-content' keyword is, I think, basically the same as 'fit-content(<percentage>)', except that it has to deal with padding/border/margin differently. But there's also a little less justification for it to behave like <percentage> does today.

So, in other words, I think it's easy to specify the inline-axis intrinsic size behavior for fit-content(<length>) although the spec doesn't do so today. It would just need to be added to the clause that now says:

If the computed inline-size of a block-level box is min-content, max-content, or a definite size ...

I think it's harder to specify the behavior for fit-content(<percentage>) (which the spec doesn't specify today) or for fit-content (which the spec currently says is the same as auto or stretch... but it's not clear to me that that's a good behavior).

@fantasai
Copy link
Collaborator

Sorry, I missed this. So the idea of fit-content was to represent the sizing formula of auto floats and tables so that it can be applied to other things, like regular blocks. And fit-content(100%) and fit-content are conceptually the same thing, assuming zero MBP; it would make sense if their behavior were essentially interchangeable. So the behavior here might need clarification in the spec, but unless there's some compelling reason not to, they should behave like auto does when it is interpreted as shrinkwrapping. Which is to say, contribute as for max-content when calculating max-content intrinsic sizes, and contribute as for min-content when contributing min-content sizes.

@fantasai
Copy link
Collaborator

fantasai commented May 16, 2019

Proposed edits:

@@ -444,6 +444,11 @@ Sizing Values: the <<length-percentage>>, ''width/auto'' | ''max-width/none'', '
                        i.e.
                        <code>min(''width/max-content'', max(''width/min-content'', <<length-percentage>>))</code>;
                        otherwise behaves as the property’s <a>initial value</a>.
+                       For the purpose of calculating <a>min-content contributions</a>,
+                       behaves as ''min-content'';
+                       for <a>max-content contributions</a>,
+                       as ''max-content''--
+                       clamped, if the argument is a <<length>>, by that length.
 
                        Negative values are invalid.
        </dl>

[css-sizing-4 is an experimental mess, hence the DO NOT IMPLEMENT warning; working against css-sizing-3]

@dbaron
Copy link
Member Author

dbaron commented May 16, 2019

I don't think that edit's handling of values that have no <percentage> part is correct -- for those, you just want to compute the single resulting length from the formula and use that for both the min-content contribution and the max-content contribution. (For both contributions, this value might be the min-content size, it might be the length, and it might be the max-content size.)

I'm still unsure about whether it's really a good solution for values that do have a <percentage> part, although it is simple.

@fantasai
Copy link
Collaborator

Um, good point. :) Updated text:

			For <a>intrinsic size contributions</a>,
			''fit-content(<<length>>)'' behaves as the resulting length;
			''fit-content(<<percentage>>)'' behaves
			as ''width/min-content'' for <a>min-content contributions</a>,
			and as ''width/max-content'' for <a>max-content contributions</a>.

I'm still unsure about whether it's really a good solution for values that do have a <percentage> part, although it is simple.

I'm open to suggestions, but in the absence of a clear benefit for another behavior, I think consistency with auto is a good principle to follow. So imho we'd need a) a solid proposal and b) use cases to justify any divergence. I don't have either atm...

@dbaron
Copy link
Member Author

dbaron commented May 16, 2019

OK, I guess, except that that needs slight rewording to account for calc() expressions that are combinations of <length> and <percentage> -- wording such as replacing "fit-content(<percentage>)" with "other cases (values with percentages)".

@fantasai
Copy link
Collaborator

@dbaron reviews are the best reviews. :) Tentatively checked in.

@atanassov atanassov changed the title how should inline-axis intrinsic sizing work for 'fit-content' and 'fit-content()'? [css-sizing] how should inline-axis intrinsic sizing work for 'fit-content' and 'fit-content()'? May 22, 2019
@Loirooriol
Copy link
Contributor

Loirooriol commented May 22, 2019

Why not rely on https://drafts.csswg.org/css-sizing-3/#percentage-sizing ?

Like if you have min-width: fit-content(calc(100px + 10%)), resolve the percentage against zero and treat it as fit-content(100px).

For max-width: fit-content(calc(100px + 10%)) and non-replaced width: fit-content(calc(100px + 10%)), treat the entire values as initial.

If I have an element with width: 1000px and an intrinsic size of 10px, I think it wouldn't be consistent to say that adding max-width: min(max-content, max(min-content, 0%)) results in an max-content contribution of 1000px according to §5.1.a, but that max-width: fit-content(0%) results in an intrinsic contribution of 10px according to fantasai's proposal.

IMO fit-content(arg) should be a perfect synonym of min(max-content, max(min-content, arg)), though maybe this could be achieved by treating min/max differently than calc in https://drafts.csswg.org/css-sizing-3/#percentage-sizing, in order to match fantasai's proposal

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed how should inline-axis intrinsic sizing work for 'fit-content' and 'fit-content()'?.

The full IRC log of that discussion <AmeliaBR> Topic: how should inline-axis intrinsic sizing work for 'fit-content' and 'fit-content()'?
<AmeliaBR> github: https://github.com//issues/3731
<AmeliaBR> Rossen_: The topic was introduced by David. He sent regrets. Who can talk?
<AmeliaBR> fantasai: Oriol posted new comments on GitHub. I can introduce, but want to follow offline.
<AmeliaBR> … Issue is when argument is a percentage, that depends on the container. Cyclic resolution.
<AmeliaBR> … One option is to ignore components with percentage. Other is to treat as zero, same as in other places.
<fantasai> s/zero/zero or auto/
<AmeliaBR> … I will probably pick the second & integrate in spec. Let me know in the issue if you have concerns.
<AmeliaBR> Rossen_: I'm inclined to agree with being consistent with how percentages behave elsewhere.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed how should inline-axis intrinsic sizing work for 'fit-content' and 'fit-content()'?.

The full IRC log of that discussion <dael> Topic: how should inline-axis intrinsic sizing work for 'fit-content' and 'fit-content()'?
<dael> github: https://github.com//issues/3731
<dael> fantasai: I haven't drawn up proposed edits. That needs to be done.
<dael> fantasai: Unless someone has comments on general direction of edits we can come back once there's spec text
<dael> Rossen_: Remove from agenda for now?
<dael> fantasai: That's what I'm thinking unless there's something to add.
<dael> Rossen_: It had been on the agenda for a few weeks. We can put it back to github only
<dael> fantasai: Seems like in last discussion we were going toward oriol proposal
<dael> fantasai: What I would do is edit that in. If anyone disagrees with that direction we can talk about that.
<dael> fantasai: Otherwise I'll try to edit that in and see where we get.
<dael> Rossen_: Reasonable

@atanassov atanassov removed the Agenda+ label May 29, 2019
tabatkins added a commit that referenced this issue Jul 20, 2020
…fer to normal % behavior, as suggested by Oriol in #3731.
@tabatkins
Copy link
Member

We simplified to just rely on the behavior of the plain argument, as Oriol suggested. That is, width: fit-content(50%) behaves the same as width: 50%; in all circumstances, just clamped by min/max-content.

Agenda+ to confirm that this resolves dbaron's and Oriol's concerns.

@dbaron
Copy link
Member Author

dbaron commented Jul 28, 2020

Where did the text that defines this actually end up? I'm having trouble finding it.

@astearns
Copy link
Member

The CSS Working Group just discussed Inline-axis intrinsic sizing for fit-content(%).
The full IRC log of that discussion
Topic: Inline-axis intrinsic sizing for fit-content(%)
github: https://github.com//issues/5119
https://github.com//issues/3731#issuecomment-661408877
astearns: so the edits are in the spec already, is it just confirming that it's good?
fantasai: yeah, oriol suggested to handle the fit-content argument like other percentages
oriol: fit-content() is just syntax sugar for nested min/max functions with min-content / max-content on the values
... so they should behave the same as the expanded form
6622d44
dbaron: should probably review offline, though should take me about 10 mins
astearns: let's do that

@astearns
Copy link
Member

@dbaron I think it's the commit above? 6622d44

@dbaron
Copy link
Member Author

dbaron commented Jul 29, 2020

That commit appears to remove the definition of how intrinsic sizes are handled. It's not clear to me what definition then takes over -- I couldn't find anything relevant in the spec.

@fantasai
Copy link
Collaborator

fantasai commented Aug 4, 2020

@dbaron

where the argument is resolved exactly as for values standing alone

Ends up referring to whatever the relevant layout module says about lengths and percentages, but also https://drafts.csswg.org/css-sizing-3/#intrinsic

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-sizing] how should inline-axis intrinsic sizing work for 'fit-content' and 'fit-content()'?, and agreed to the following:

  • RESOLVED: Close this issue as fixed
The full IRC log of that discussion <dael> Topic: [css-sizing] how should inline-axis intrinsic sizing work for 'fit-content' and 'fit-content()'?
<dael> github: https://github.com//issues/3731
<dael> fantasai: I believe the issue is solved. I'm looking for confirmation we're okay. Seemed there was some confusion but I answered the question. oriol looked and thought it was fine I think. I'd like to close the issue at some point. I don't believe further edits are necessary
<dael> astearns: dbaron isn't on. We could close and if there is anything additional he can re-open with a comment
<dael> astearns: Any objection to close this as fixed?
<dael> astearns: And keep commentor response pending tag on it
<dael> RESOLVED: Close this issue as fixed

@fantasai fantasai closed this as completed Nov 5, 2020
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 8, 2021
We simplified to just rely on the behavior of the plain argument. That is,
width: fit-content(50%) behaves the same as width: 50%; in all circumstances,
just clamped by min/max-content.

Note: for block axis, we treat fit-content() as initial value its minimal
and maximal value are identical and equal to the initial value in block axis.

From: w3c/csswg-drafts#3731 (comment)

Note: this patch doesn't include any update on flex and grid layout. We
may have to come back to check it.

Differential Revision: https://phabricator.services.mozilla.com/D113199

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1312588
gecko-commit: 41792e6152e6b5e8ad472e6cc21ce07352447bb8
gecko-reviewers: TYLin, emilio
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 8, 2021
…it-content(). r=TYLin,emilio

We simplified to just rely on the behavior of the plain argument. That is,
width: fit-content(50%) behaves the same as width: 50%; in all circumstances,
just clamped by min/max-content.

Note: for block axis, we treat fit-content() as initial value its minimal
and maximal value are identical and equal to the initial value in block axis.

From: w3c/csswg-drafts#3731 (comment)

Note: this patch doesn't include any update on flex and grid layout. We
may have to come back to check it.

Differential Revision: https://phabricator.services.mozilla.com/D113199
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jun 8, 2021
We simplified to just rely on the behavior of the plain argument. That is,
width: fit-content(50%) behaves the same as width: 50%; in all circumstances,
just clamped by min/max-content.

Note: for block axis, we treat fit-content() as initial value its minimal
and maximal value are identical and equal to the initial value in block axis.

From: w3c/csswg-drafts#3731 (comment)

Note: this patch doesn't include any update on flex and grid layout. We
may have to come back to check it.

Differential Revision: https://phabricator.services.mozilla.com/D113199

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1312588
gecko-commit: 41792e6152e6b5e8ad472e6cc21ce07352447bb8
gecko-reviewers: TYLin, emilio
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jun 9, 2021
…it-content(). r=TYLin,emilio

We simplified to just rely on the behavior of the plain argument. That is,
width: fit-content(50%) behaves the same as width: 50%; in all circumstances,
just clamped by min/max-content.

Note: for block axis, we treat fit-content() as initial value its minimal
and maximal value are identical and equal to the initial value in block axis.

From: w3c/csswg-drafts#3731 (comment)

Note: this patch doesn't include any update on flex and grid layout. We
may have to come back to check it.

Differential Revision: https://phabricator.services.mozilla.com/D113199
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants