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-4] Should we mention aspect-ratio in margin collapsing? #5328

Closed
BorisChiou opened this issue Jul 15, 2020 · 15 comments
Closed

[css-sizing-4] Should we mention aspect-ratio in margin collapsing? #5328

BorisChiou opened this issue Jul 15, 2020 · 15 comments

Comments

@BorisChiou
Copy link
Contributor

BorisChiou commented Jul 15, 2020

Based on the spec definition of margin-collasping, there are at least two items which mention auto computed height:

Two margins are adjoining if and only if:
...
both belong to vertically-adjacent box edges, i.e. form one of the following pairs:

  • top margin of a box and top margin of its first in-flow child
  • bottom margin of box and top margin of its next in-flow following sibling
  • bottom margin of a last in-flow child and bottom margin of its parent if the parent has auto computed height
  • top and bottom margins of a box that does not establish a new block formatting context and that has zero computed min-height, zero or auto computed height, and no in-flow children

If height is the ratio-dependent axis, should we also take aspect-ratio into account for these cases?

For example:

#parent {
  margin: 0px;
  width: 100px;
}
#child {
  margin-top: 50px;
  margin-bottom: 200px;
  width: 100px;
  aspect-ratio: 1/1;
}
<div id='parent'>
  <div id='child'></div>
</div>

Based on the current spec words, the computed height of parent is auto, so we merge the bottom margins of parent and child, and so the final bottom margin of parent is 200px (i.e. std::max(0px, 200px)).
And child is not empty (because of aspect-ratio), so we don't merge the top and bottom margins of child (and parent). Therefore, the final top margin is 50px (i.e. std::max(0px, 50px)). Is this correct?

Another case

#parent {
  margin: 0px;
  width: 100px;
  aspect-ratio: 1/1;
}
#child {
  margin-top: 50px;
  margin-bottom: 200px;
  width: 100px;
}
<div id='parent'>
  <div id='child'></div>
</div>

The child is content empty, so we merge the margins of the child as 200px (i.e. std::max(50px, 200px)).
The parent has aspect-ratio, so it has the block context (and its used height is 100px), right?
So the final top margin of parent is 200px (which is carried out from child), and the final bottom margin of parent is 200px (which is also carried out from child, because the computed height of parent is auto)? I may be wrong.
I'm confused about this case actually. It seems the final bottom margin is 0px in Chrome now (note: I guess it treats the aspect-ratio:1/1 as a non-auto height?).

cc @fantasai @cbiesinger @bfgeek

@BorisChiou
Copy link
Contributor Author

and cc @emilio.

@BorisChiou
Copy link
Contributor Author

Not sure what are the expected behaviors. In my opinion, I think we should treat "non-auto width & aspect-ratio" as a "non-auto height" for these cases.

@bfgeek
Copy link

bfgeek commented Jul 19, 2020

@BorisChiou Right - this should be treated as a non-empty block, and therefore margins don't "collapse through". However all the other standard margin collapsing rules apply.

For the 1st case it should be no different if the "child" had an explicit height of "100px" e.g. the parent gets a "top" margin of "50px", and a "bottom" margin of "200px".

For the 2nd case it should be no different if the "parent" had an explicit height of "100px" e.g. the parent gets a top margin of "200px" (due to collapsing through), and a bottom margin of "0px".

Do you disagree at all with above description?

Ian

@BorisChiou
Copy link
Contributor Author

I agree with your answer. Treat aspect-ratio as a non-empty block, so it should be no different if the block had an explicit "height". Thanks for this clarification.

@bfgeek
Copy link

bfgeek commented Jul 20, 2020

There is one case where it should be considered empty is if you have something like:

#parent {
  margin: 0px;
  width: 100px;
}
#child {
  margin-top: 50px;
  margin-bottom: 200px;
  width: 0px; /* FUN */
  aspect-ratio: 1/1;
}
<div id='parent'>
  <div id='child'></div>
</div>

I.e. the "top" margin will be 200px. This is the same if #child had an explicit height of 0px.

@BorisChiou
Copy link
Contributor Author

OK. This makes sense for margin through on #child because of "zero or auto computed height". The basic rule is just to replace the auto computed height with the calculated height (from aspect-ratio and width). Then it's pretty easy to map it into the spec rules.

@emilio
Copy link
Collaborator

emilio commented Jul 27, 2020

@bfgeek do we actually need to make inline-size: 0 work?

It'd be nice not to add more special cases to margin collapsing, I'd prefer to consider any finite ratio as not empty for margin collapsing purposes, but...

@bfgeek
Copy link

bfgeek commented Jul 30, 2020

It'd be nice not to add more special cases to margin collapsing, I'd prefer to consider any finite ratio as not empty for margin collapsing purposes, but...

@emilio I'd sort of see not making the inline-size: 0 work a special case. :)

FWIW we basically did no work to get aspect-ratio + margin-collapsing to work, instead of relying on all of the existing rules.

@tabatkins
Copy link
Member

@fantasai and I are inclined to go with "if aspect-ratio gives it a non-zero height, that non-zero height inhibits collapse-thru", so in the inline-size: 0 case, it would continue to collapse thru, exactly as if you'd put block-size: 0 on it.

@fantasai
Copy link
Collaborator

fantasai commented Aug 4, 2020

CSS2 specifies in https://www.w3.org/TR/CSS2/box.html#collapsing-margins the following condition for top & bottom adjoining margins:

top and bottom margins of a box that does not establish a new block formatting context and that has zero computed 'min-height', zero or 'auto' computed 'height', and no in-flow children

In the case of applying aspect ratio, the computed height is clearly not zero, or else we would be ignoring the aspect ratio. Only the used height is.

That said, CSS2 has no way to distinguish the above condition vs the same condition with s/computed/used/.

So the question now becomes, should margin collapsing be checking for a zero used height instead?

CC @dbaron

@fantasai fantasai added the CSS2 label Aug 4, 2020
@emilio
Copy link
Collaborator

emilio commented Aug 5, 2020

I think ideally margin-collapsing would not depend on used values, fwiw. It seems useful to know whether a margin collapses without having laid out the box yet.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed . [css-sizing-4] Should we mention aspect-ratio in margin collapsing?, and agreed to the following:

  • RESOLVED: Non-0 height as a result of aspect-ratio disables margin collapsing between top and bottom
The full IRC log of that discussion <dael> Topic:. [css-sizing-4] Should we mention aspect-ratio in margin collapsing?
<dael> github: https://github.com//issues/5328
<dael> fantasai: Question was let's say there's an aspect-ratio and the width is 0 so height is therefore 0
<dael> fantasai: Should that trigger margin-collapsing between top and bottom margin of box
<dael> fantasai: In CSS 2 top and bottom margins of box with 0 computed height and no inflow children.
<dael> fantasai: a-r does not cause computed height to be 0, it's auto.
<dael> fantasai: Question that came up was if the used value is 0 should it be the same was computed value is 0
<dael> fantasai: In CSS 2 there's no way to get 0 used height that doesn't fall into this category. If we change computed to used in CSS 2 nothing changes. BUt not lot sof ways for used height of 0 where it's not computed 0. margin-collapsing could be changed to based on used. BUt do we want to?
<dael> fantasai: Based on emilio comment and others in issue it seems like Chrome is using used value. Seems their a-r impl collapsed but MOzilla wants to use computed
<dael> iank_: We use used heights and it falls out straight forward. It sort of makes sense to do in light of everything else
<dael> florian: If a-r is the only thing causing top and bottom margin to not be adjacent is that question solved?
<dael> fantasai: Good point. a-r causes height to be 1px. That should not cause top and bottom to collapse.
<dael> fantasai: For continuity it would be weird if 1px does not but 0px does
<dael> iank_: Same as height 0px
<dael> AmeliaBR: I think the most important case is it's clearly defined that and auto height that's definite b/c a-r is treated as a definite height for margin-collapsing. I don't know if we need to special case a-r width:0 and therefore height:0.
<dael> AmeliaBR: For basic case of empty element with a-r there's no way we can collapse margin through something with definite height
<dael> florian: I don't think people have strong idea of if they should collapse. But when margins are not visually next to each other people are clear it should not collapse. Make sure that's true and then we can be sensible about habdling 0 b/c people don't have expectations
<dael> fantasai: Difference expectations from different impl. My take is it was a mistake for height0px and height:1px to have different behaviors. Adding more cases that do that is not good. So I would lean toward saying it should not collapse if a-r takes effect
<dael> iank_: Don't agree. Rule is simple at the moment. If used height is 0 margins collapse through
<dael> fantasai: Spec says computed
<dael> iank_: Previously that was the same
<dael> fantasai: Back to css 2.0
<dael> iank_: We had to do 0 work to get this to work correctly. It falls out from what happens if you set height:0 on an element
<dael> fantasai: I don't think we have anyone from Mozilla. I think their input is important
<fantasai> s/anyone from Mozilla/emilio or dbaron from Mozilla/
<dael> heycam: dholbert and I are here. I have not looke dat margin collapsing much.
<dael> dholbert: I have not recently
<dael> Rossen_: If we have concrete prop everyone agrees on we can resolve and when dbaron and emilio read we can revisit. Do we have consensus?
<dael> florian: I don't think we do
<dael> Rossen_: Then it's fine to postpone for another week until emilio is here
<dael> florian: We have consensus on if it's non-0 the margins don't collapse. I don't think anyone disagrees with that.
<dael> Rossen_: That's good. I'd like to resolve on that one
<dael> fantasai: Prop: Non-0 height as a result of aspect-ratio disables margin collapsing between top and bottom
<dael> Rossen_: Behaves similar to fixed height
<dael> florian: I think just the result. If it's non-0 due to aspect ratio it does not collpase. Why we'll get to later
<dael> Rossen_: Additional thoughts or objections?
<dael> RESOLVED: Non-0 height as a result of aspect-ratio disables margin collapsing between top and bottom
<dael> Rossen_: Any additional progress or back to GH?
<dael> fantasai: Back to GH. I think having people from Mozilla who are familiar with this code is important. It's a complex part of layout so people have opinions
<dael> Rossen_: Agree. Just looking for other low hanging fruit

@bfgeek
Copy link

bfgeek commented Aug 5, 2020

I think ideally margin-collapsing would not depend on used values, fwiw. It seems useful to know whether a margin collapses without having laid out the box yet.

But you need to layout, or at least walk the sub-tree to determine if it has any content?

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 17, 2020
Basically, we treat aspect-ratio (together with inline size) as a
non-auto block size. This means the block is not empty when using
aspect-ratio.

Also, add 2 tentative wpts for this, based on the current spec issue
examples.

w3c/csswg-drafts#5328

Differential Revision: https://phabricator.services.mozilla.com/D84452
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 21, 2020
Basically, we treat aspect-ratio (together with inline size) as a
non-auto block size. This means the block is not empty when using
aspect-ratio.

Also, add 2 tentative wpts for this, based on the current spec issue
examples.

w3c/csswg-drafts#5328

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

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1639963
gecko-commit: a5e9b2bede3a76b9587985a37eed227dcbabd620
gecko-integration-branch: autoland
gecko-reviewers: emilio
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 21, 2020
Basically, we treat aspect-ratio (together with inline size) as a
non-auto block size. This means the block is not empty when using
aspect-ratio.

Also, add 2 tentative wpts for this, based on the current spec issue
examples.

w3c/csswg-drafts#5328

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

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1639963
gecko-commit: a5e9b2bede3a76b9587985a37eed227dcbabd620
gecko-integration-branch: autoland
gecko-reviewers: emilio
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this issue Sep 23, 2020
Basically, we treat aspect-ratio (together with inline size) as a
non-auto block size. This means the block is not empty when using
aspect-ratio.

Also, add 2 tentative wpts for this, based on the current spec issue
examples.

w3c/csswg-drafts#5328

Differential Revision: https://phabricator.services.mozilla.com/D84452
@tabatkins
Copy link
Member

So for now we've committed the following:

For the purpose of margin collapsing ([[css2#collapsing-margins]]),
if the [=block axis=] is the [=ratio-dependent axis=],
it is not considered to have a [=computed value|computed=] 'block-size' of ''height/auto''.

This communicates that a non-zero height caused by aspect-ratio definitely inhibits margin-collapse (as per the resolution above), but it also implies that aspect-ratio inhibits margin-collapse in all cases, even if the resulting height ends up being zero. This matches Firefox, but doesn't match Chrome currently. (Don't know about WebKit.) It seems like adjusting Chrome to pay attention to aspect-ratio is a smaller task than adjusting Firefox to care about used values when doing margin-collapsing, so hopefully this is ok? Overall this is a corner-case anyway; the only time an ar-having box would get a zero height is if either the a-r is 0/X (weird) or is zero-width (rare).

There's a follow-up CSS2 issue about non-interop on collapsing margins at #5571

Let us know if this seems adequate to close the issue, or if further edits are wanted.

@cbiesinger
Copy link

@bfgeek knows our margin collapsing code better but that seems easy enough to implement, though a bit weird. still, agree that this is a corner case and it doesn't matter too much what we decide.

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