w3c / csswg-drafts

CSS Working Group Editor Drafts
https://drafts.csswg.org/
Other
4.5k stars 668 forks source link

[css-sizing] Percentage sizing section is kind of vague #1132

Closed cbiesinger closed 6 years ago

cbiesinger commented 7 years ago

https://drafts.csswg.org/css-sizing-3/#percentage-sizing

That section is a bit vague. It currently says:

Percentages specify sizing of a box with respect to the box’s containing block.

Although this may require an additional layout pass to re-resolve percentages in some cases, the auto, min-content, max-content, and fit-content values of min-width and min-height do not prevent the resolution of percentage sizes of the box’s contents. However, in order to prevent cyclic sizing in the general case, percentages do not otherwise resolve against indefinite sizes, and instead are treated as auto.

I actually think the text is wrong, in that sizes can only be resolved against definite sizes but the text does not specify that limitation. But beyond that:

tabatkins commented 7 years ago

We rewrote the entire section, because even we found it pretty inscrutable! It's definitely still under review and open to edits, so please let us know what you find bad or wrong.

The text implies that max-height can affect percentage resolution of the height (if it is something like min-content), but does not specifically say so

It should now be much clearer that it does do so.

It seems to imply also that min-content will never prevent percentage resolution of children, but I am not sure that my reading is correct and would appreciate clarification

Did you mean 'min-width/height' here? If so, then yes, it never prevents resolution of child percentages, if they could resolve otherwise (ignoring the min-size property). This should also be much clearer now.

(Flexbox and Grid explicitly say that "min-*: auto" doesn't prevent percentage resolution; it doesn't seem any more difficult to apply that to all the intrinsic keywords, we think. Please let us know if we're wrong!)

It would be nice to clarify that while max-content can prevent percentage resolution of children/descendants, it never affects the value the percentage gets resolved against (that value is always the specified height). OR, if I misunderstood, it would be good to explicitly say so.

Again, assuming you mean 'max-width/height'. 'max-height' can indeed prevent percentage resolution. As currently written, 'max-width' does not, and it does affect what size the percentage resolves against. Should either of these be different?

cbiesinger commented 7 years ago

Thanks, this is much better. It addresses my previous comments. But I think the rewrite does not address the case where the percentage in question is specified for a min- or max-size property. For example: "When calculating the containing block’s size, the percentage behaves as auto." auto is not valid for max and is probably not the intention for min, here.

As a sidenote: "Similarly, percentage margins and padding behave as zero in such cyclic cases when calculating the containing block’s size" I believe this is incompatible with Gecko's current implementation (from memory -- I did not retest)

fantasai commented 6 years ago

OK, so, afaict: width/min-width/max-width values containing a percentage are treated as the property's initial value for the purpose of calculating the min/max-size contributions. Afterwards, they resolve. E.g. in width (or min/max-width): calc(300px + 40%) we ignore the 300px as well as the percentage while calculating its size contribution.

This needs edits and testcases.

fantasai commented 6 years ago

(Wrt sidenote, IIRC Gecko agreed to change their behavior. See https://github.com/w3c/csswg-drafts/issues/347.)

fantasai commented 6 years ago

OK, submitted WPT tests in https://github.com/w3c/web-platform-tests/pull/9418 for this issue. We still need a WG resolution to add prose for this, and also, I'd really like someone to look this all over because I'm not 100% confident I'm triggering the right cases / understanding the results correctly. :(

Proposed edits would be, append to the first paragraph of https://drafts.csswg.org/css-sizing-3/#intrinsic-contribution

If the box is non-replaced, then the value of any sizing property specified on it as an expression containing a percentage is treated for the purpose of calculating the box’s intrinsic size contribution only as that property’s initial value. For example, given a box with ''width: calc(20px + 50%)'', its max-content contribution is calculated as if its 'width' were ''width/auto''. The percentage is honored as usual, however, during the actual sizing of the box itself.

Suggestions on wording improvement welcome...

Note: Testcases can be run live here: https://lists.w3.org/Archives/Public/www-archive/2018Feb/0000.html

css-meeting-bot commented 6 years ago

The Working Group just discussed Percentage sizing section is kind of vague.

The full IRC log of that discussion <dael> Topic: Percentage sizing section is kind of vague
<dael> github: https://github.com/w3c/csswg-drafts/issues/1132
<dael> fantasai: There's...the last comment is a summary.
<astearns> https://github.com/w3c/csswg-drafts/issues/1132#issuecomment-363623845
<fantasai> https://github.com/w3c/csswg-drafts/issues/1132#issuecomment-363623845
<dael> fantasai: Proposed edits for the behavior is [reads] and seems to be interoperably impl.
<dael> fantasai: If you have a mix width of calc 20px+50% we consider it as [missed] and once containing block resolves we resolve the %s. THere's a WPT PR and we're asking for WG review
<dael> astearns: Has anyone reviewed the test PR?
<dael> fantasai: There's no spec text. Somebody needs to look at tests and see if that's what people want to add to the spec. We need a review of the tests.
<dael> dbaron: Proposal is matching the behavior of % widths?
<dael> fantasai: There may be cases I didn't test correctly.
<dael> dbaron: I think it does. But I think if it doesn't it ought to match.
<dael> astearns: dbaron could you review?
<dael> dbaron: Probably but maybe next week.
<dael> astearns: Is that enough for now fantasai ?
<dael> fantasai: If anybody else wants to review I'd prefer they do it in paralel with dbaron. This is the last open issue as far as I'm aware at which point we'd like to repub and issue LC.
<dael> fantasai: It's been on the agenda for 2 weeks. If someone needs to review and they need time let's assign it to that person.
<dael> astearns: Any other engines have a volunteer? fremy can I ask you to look?
<dael> fremy: I guess yes. It may not happen this week, but in theory yes.
<dael> astearns: Other volunteers?
<dael> fantasai: You can just defer to dbaron . But if you want more time I need to know how much.
<dael> astearns: Since this is a WD we can publish what w have and leave this open.
<dael> fantasai: Yes, but I'd like to get to a point where we can ask for final review. But I am happy to publish a WD.
<dael> astearns: Opinions on an interrum draft or wait for this to resolve?
<fantasai> s/interrum/interim/
<dael> astearns: It is a WD so fantasai I can leave it up to you. I'm happy to call for resolution at any point.
<dael> fantasai: Now is fine. It's out of date.
FremyCompany commented 6 years ago

I think the text is reasonable, but I think assuming that browsers implemented the same thing for all properties is being a bit too hopeful here.

https://github.com/w3c/web-platform-tests/pull/9418#issuecomment-370524179 points out that there is no interop for margin at least, and that no browser follows the current spec. I am not sure it's worth changing the spec text just for that case, but we might want to add a test for that and file bugs, and maybe ask people to review that particular test in particular.

FremyCompany commented 6 years ago

We might also start wondering about other similar uses like flex-basis or grid-gap, etc... It would be nice to have a test case for all of them to see if we have interop on them

fantasai commented 6 years ago

@FremyCompany Margin is not a sizing property. https://drafts.csswg.org/css-sizing-3/#sizing-property

FremyCompany commented 6 years ago

@fantasai The text definitely looks fine then.

However, we still need some text to explain the behavior of padding/margin/flex-basis and other properties, right? If yes, I would be fine reusing the same logic if we have rough interop (or, like in the margin case, no interop at all anyway).

dbaron commented 6 years ago

One thing that's ambiguous in the text is whether "containing a percentage" means syntactically containing a percentage or mathematically containing a percentage -- that is, whether expressions like calc(50px + 0%) contain a percentage. In Gecko I think they do, although I'm not 100% sure.

I'd also note that the proposed text doesn't match Gecko behavior for margins and padding, where we currently account for percentages in a way that's not quite correct but pretty close, and the obvious change to stop doing that would causes us to treat only the percentage component of a calc() as 0. So please do test that this matches the behavior of other implementations for margin and padding (i.e., that they don't use the non-percent components). If that's not the case, then we likely need different behavior for width/height (and min-* and max-*) versus margin/padding.

dbaron commented 6 years ago

OK, based on @fantasai's comments in the telecon, sizing property needs to be hyperlinked to the definition; I wasn't aware it was a term.

css-meeting-bot commented 6 years ago

The Working Group just discussed [css-sizing] Percentage sizing section is kind of vague, and agreed to the following resolutions:

The full IRC log of that discussion <dael> Topic: [css-sizing] Percentage sizing section is kind of vague
<dael> github: https://github.com/w3c/csswg-drafts/issues/1132#issuecomment-363623845
<dael> fantasai: Sizing issue was that we...we're asking for review of the comment: https://github.com/w3c/csswg-drafts/issues/1132#issuecomment-363623845
<dael> astearns: There is a test. There are proposed edits in the comment.
<dael> astearns: Basically you either want a resolution to make the edit or reasons why not or things to improve?
<dael> fantasai: Yeah. Looking for review.
<dael> fantasai: For margins and padding case I think it's asep issue that we need to discuss.
<dael> fantasai: This text is just about sizing properties. margins is not a sizing property.
<dael> fantasai: For margins and padding we could honor whatever is in the calc that's n ot the % and treat % as 0. I suspect that would not cause a problem and makes a bit of sense to do if we can.
<dael> fantasai: For sizing properties it's more complicated because you want ot be able tomeasure the content. but for margins and padding there isn't a thing to measure. So we could resolve % against 0 to calc the contining block rather then ignore the margin entirely.
<dael> franremy: This was brought up 2 weeks ago. me and dbaron reviewed and I think we were both fine with the proposal. dbaron pointed out you want to link to the sizing properties. I think this is fine. We found more htings tow ork on but it's fine to open a new issue.
<dael> dbaron: The one sentence...the one comment is I think containing a % could be two different things. It could be syntatic or mathematic.
<dael> franremy: I think there is a test that covered this. We can clarify the text, but there is a test I think.
<dael> fantasai: Yeah, we need clarification.
<dael> dbaron: I'm fine given the clarification and hyperlink.
<dael> astearns: Other comments?
<TabAtkins> tabatkins: 0% definitely should count as "containing a percentage".
<dael> astearns: Does anyone object to the change with the clarification and hyperlink?
<fantasai> Discussion of margins is kinda mixed in here : https://github.com/w3c/csswg-drafts/issues/2297
<dael> RESOLVED: Accept the edit in https://github.com/w3c/csswg-drafts/issues/1132#issuecomment-363623845 with a clarification and a hyperlinked term.
<dael> fantasai: I think margin stuff is in issue #2297
<dael> astearns: So franremy please look there and see if you can continue in that or open a sep issue