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] Percentage sizing section is kind of vague #1132

Closed
cbiesinger opened this Issue Mar 27, 2017 · 13 comments

Comments

Projects
None yet
6 participants
@cbiesinger
Copy link

cbiesinger commented Mar 27, 2017

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:

  • 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 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
  • 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.
@tabatkins

This comment has been minimized.

Copy link
Member

tabatkins commented Aug 8, 2017

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

This comment has been minimized.

Copy link
Author

cbiesinger commented Nov 5, 2017

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

This comment has been minimized.

Copy link
Contributor

fantasai commented Jan 19, 2018

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

This comment has been minimized.

Copy link
Contributor

fantasai commented Feb 6, 2018

(Wrt sidenote, IIRC Gecko agreed to change their behavior. See #347.)

fantasai added a commit to fantasai/web-platform-tests that referenced this issue Feb 7, 2018

[css-sizing-3] Add tests for intrinsic size contribution (width/inlin…
…e-size) of non-replaced blocks with percentage widths. See w3c/csswg-drafts#1132
@fantasai

This comment has been minimized.

Copy link
Contributor

fantasai commented Feb 7, 2018

OK, submitted WPT tests in web-platform-tests/wpt#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

@fantasai fantasai added the Agenda+ label Feb 7, 2018

fantasai added a commit to fantasai/web-platform-tests that referenced this issue Feb 7, 2018

[css-sizing-3] Add tests for intrinsic size contribution (width/inlin…
…e-size) of non-replaced blocks with percentage widths. See w3c/csswg-drafts#1132

fantasai added a commit to fantasai/web-platform-tests that referenced this issue Feb 7, 2018

[css-sizing-3] Add tests for intrinsic size contribution (width/inlin…
…e-size) of non-replaced blocks with percentage widths. See w3c/csswg-drafts#1132

fantasai added a commit to fantasai/web-platform-tests that referenced this issue Feb 7, 2018

[css-sizing-3] Add tests for intrinsic size contribution (width/inlin…
…e-size) of non-replaced blocks with percentage widths. See w3c/csswg-drafts#1132

@fantasai fantasai removed the Needs Edits label Feb 10, 2018

@css-meeting-bot

This comment has been minimized.

Copy link
Member

css-meeting-bot commented Feb 21, 2018

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//issues/1132
<dael> fantasai: There's...the last comment is a summary.
<astearns> https://github.com//issues/1132#issuecomment-363623845
<fantasai> https://github.com//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

This comment has been minimized.

Copy link
Contributor

FremyCompany commented Mar 5, 2018

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.

web-platform-tests/wpt#9418 (comment) 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

This comment has been minimized.

Copy link
Contributor

FremyCompany commented Mar 5, 2018

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

@FremyCompany FremyCompany assigned fantasai and unassigned FremyCompany Mar 5, 2018

@fantasai

This comment has been minimized.

Copy link
Contributor

fantasai commented Mar 6, 2018

@FremyCompany

This comment has been minimized.

Copy link
Contributor

FremyCompany commented Mar 6, 2018

@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

This comment has been minimized.

Copy link
Member

dbaron commented Mar 7, 2018

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

This comment has been minimized.

Copy link
Member

dbaron commented Mar 8, 2018

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

This comment has been minimized.

Copy link
Member

css-meeting-bot commented Mar 8, 2018

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

  • RESOLVED: Accept the edit in https://github.com/w3c/csswg-drafts/issues/1132#issuecomment-363623845 with a clarification and a hyperlinked term.
The full IRC log of that discussion <dael> Topic: [css-sizing] Percentage sizing section is kind of vague
<dael> github: https://github.com//issues/1132#issuecomment-363623845
<dael> fantasai: Sizing issue was that we...we're asking for review of the comment: https://github.com//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//issues/2297
<dael> RESOLVED: Accept the edit in https://github.com//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

@css-meeting-bot css-meeting-bot removed the Agenda+ label Mar 8, 2018

@tabatkins tabatkins closed this in efc4bb9 Mar 26, 2018

fantasai added a commit to web-platform-tests/wpt that referenced this issue Mar 26, 2018

[css-sizing-3] Add tests for intrinsic size contribution (width/inlin…
…e-size) of non-replaced blocks with percentage widths. See w3c/csswg-drafts#1132

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 15, 2018

Bug 1436404 [wpt PR 9418] - [css-sizing] Intrinsic percentage non-rep…
…laced widths, a=testonly

Automatic update from web-platform-tests[css-sizing-3] Add tests for intrinsic size contribution (width/inline-size) of non-replaced blocks with percentage widths. See w3c/csswg-drafts#1132

wpt-commits: 5957997833c0c2dacbcf29b2e2eebd0a7c096c91
wpt-pr: 9418
wpt-commits: 5957997833c0c2dacbcf29b2e2eebd0a7c096c91
wpt-pr: 9418

mykmelez pushed a commit to mozilla/gecko that referenced this issue Apr 16, 2018

Bug 1436404 [wpt PR 9418] - [css-sizing] Intrinsic percentage non-rep…
…laced widths, a=testonly

Automatic update from web-platform-tests[css-sizing-3] Add tests for intrinsic size contribution (width/inline-size) of non-replaced blocks with percentage widths. See w3c/csswg-drafts#1132

wpt-commits: 5957997833c0c2dacbcf29b2e2eebd0a7c096c91
wpt-pr: 9418
wpt-commits: 5957997833c0c2dacbcf29b2e2eebd0a7c096c91
wpt-pr: 9418

fergald added a commit to fergald/csswg-drafts that referenced this issue May 7, 2018

[css-sizing] Specify that sizing properties containing %s are treated…
… as property's initial value for intrinsic size contribution. Fixes w3c#1132.
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.