Re: [CSS21] Errata - proposals for review - Margin collapsing, I

[Resending, because the formatting got mangled somehow on the first 
attempt!]

Thanks for the review, Arron!  I've added my comments and a revised 
proposal below.

On 23/02/2012 20:44, Arron Eicholz wrote:

> Proposal 1 (Anton's current proposals)
>
> =Bug 16049=
> Replace:
>     # These steps do not affect the real computed values of the above
>     # properties. The change of used 'height' has no effect on margin
>     # collapsing except as specifically required by rules for
>     # 'min-height' or 'max-height' in "Collapsing margins" (8.3.1).
>
> with:
>     | These steps do not affect the real computed values of the above
>     | properties. The change of used 'height' has no effect on margin
>     | collapsing; the real computed value of 'height' is used to
>     | determine which margins are adjoining in "Collapsing margins"
>     | (8.3.1) during these steps.
>
> =Bug 16036=
> Replace:
>     #   - bottom margin of a last in-flow child and bottom margin of its
>     #     parent if the parent has 'auto' computed height
>
> with:
>     |   - bottom margin of a last in-flow child and bottom margin of its
>     |     parent if the parent has 'auto' computed height and either the
>     |     parent has a zero computed min-height or the bottom margin of
>     |     the last in-flow child does not collapse with the top margin of
>     |     the parent
>
> =Bug 16037=
> Replace:
>     # The bottom margin of an in-flow block box with a 'height' of 'auto'
>     # and a 'min-height' of zero collapses with its last in-flow
>     # block-level child's bottom margin if the box has no bottom padding
>     # and no bottom border and the child's bottom margin does not collapse
>     # with a top margin that has clearance.
>
> with:
>     | The bottom margin of an in-flow block box with a 'height' of 'auto'
>     | collapses with its last in-flow block-level child's bottom margin if
>     | the box has no bottom padding and no bottom border and the child's
>     | bottom margin neither collapses with a top margin that has clearance
>     | nor (if the box's min-height is non-zero) with the box's top margin.
>
> Pros:
> - Matches the functionality of Webkit, Opera and Internet Explorer
> - No current CSS 2.1 test cases affected
>
> Cons:
> - Does not match Firefox
> - Is not consistent with how authors think min-height and height work together
> - Mixes concepts of margin collapsing and adjacency in the updated text
> - Complicated to read and understand


> -----------------------------------
> Proposal 2 (Arron's proposals):
>
> This proposal keeps the logic of "Proposal 1" the same but separates
> the concepts of collapsing and adjacency which we are trying to do in
> the spec. This also simplifies some of the working to make the topics
> easier to read without sacrificing necessary logic.
>
> =Bug 16049=
> Replace:
>     # These steps do not affect the real computed values of the above
>     # properties. The change of used 'height' has no effect on margin
>     # collapsing except as specifically required by rules for
>     # 'min-height' or 'max-height' in "Collapsing margins" (8.3.1).
>
> with:
>     | These steps do not affect the real computed values of the above
>     | properties. The change of used 'height' has no effect on margin
>     | collapsing.

I agree that removing the clause is better than leaving the current one 
in place, but for me the problem is that I don't find it clear what the 
second sentence actually means; that's why I added an explicit 
explanation in Proposal 1 above.  Perhaps I just have a mental block 
about it, since others seem not to have a problem with it!

I believe the sentence is trying to reinforce the following subtle 
point: in steps 2 and 3, we are not performing entire layout again with 
a different /computed/ value of 'height' (which would affect the margin 
collapsing relationships, and could potentially affect other aspects of 
layout as well since most layout rules throughout the spec revolve 
around computed value - although I don't think there actually are any 
other aspect that would be affected in CSS21).  Instead, we are just 
performing the rules of, specifically, 10.6 again with a substituted 
computed value of 'height', which might result in different used values 
of 'margin-top', 'margin-bottom', 'top' and 'bottom' but nothing else 
AFAICT.

Technically the sentence isn't needed; it's just a Note, after all.  But 
I do think that some sort of comment is useful.  Perhaps the problem I 
have with the current formulation is that it says "change of used 
'height'", but it's not the used height that's actually changing; the 
used height is the final outcome after three calculations.

How about:

   | These steps do not affect the real computed values of the above
   | properties. _Collapsed margins_<link to 8.3.1> remain unchanged
   | for each step, since the real computed value of 'height' is used
   | when determining which margins collapse.


> =Bug 16036=
> Replace:
>      #   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
>
> with:
>      |   top and bottom margins of a box that does not establish a new
>      |   block formatting context and zero or 'auto' computed 'height',
>      |   and no in-flow children
>
> Add a new bullet to:
> "Adjoining vertical margins collapse, except:"
>     | - The top and bottom margins, of a box with a 'min-height' value that
>     |    does not compute to zero, do not collapse. If the bottom margin, of such
>     |    a box's last inflow-child, collapses with the box's top margin then the
>     |    last inflow-child's bottom margin does not collapse with its parent
>     |    box's bottom margin.

I agree with your observation that my proposal uses the concept of 
margin collapsing in the specification of adjacency, and that in fact it 
would be better to keep them separated.

However, it seems rather perverse to define two margins as adjoining 
which intuitively aren't, only to then immediately prevent them from 
collapsing.

How do you feel about the following alternative:

Do not modify:
   #   - bottom margin of a last in-flow child and bottom margin of its
   #     parent if the parent has 'auto' computed height

but add a new bullet to:
   # "Adjoining vertical margins collapse, except:"

   | - If the top margin of a box with non-zero computed 'min-height'
   |   and 'auto' computed 'height' collapses with the bottom margin of
   |   its last in-flow child, then the child's bottom margin does not
   |   collapse with the parent's bottom margin.

> =Bug 16037=
> Replace:
>     # The bottom margin of an in-flow block box with a 'height' of 'auto'
>     # and a 'min-height' of zero collapses with its last in-flow
>     # block-level child's bottom margin if the box has no bottom padding
>     # and no bottom border and the child's bottom margin does not collapse
>     # with a top margin that has clearance.
>
> This proposal I think work better written as a list of sub-bullets.
>
> with:
>     | - The bottom margin of an in-flow block box with a 'height' of 'auto'
>     |    collapses with its last in-flow block-level child's bottom margin, if:
>     |        - the box has no bottom padding, and
>     |        - the box has no bottom border, and
>     |        - the child's bottom margin neither collapses with a top margin
>     |           that has clearance, nor (if the box's min-height is non-zero)
>     |           with the box's top margin.

I completely agree here.  The sentence was already hard to read, and 
with the extra clause it becomes downright convoluted.  The list 
approach works better.

> Pros:
> - Matches the functionality of Webkit, Opera and Internet Explorer
> - No current CSS 2.1 test cases affected
> - Adjacency and margin collapsing are still separate sections within
>   margin collapsing's normative text
> - Much easier to read and understand
>
> Cons:
> - Does not match Firefox
> - Is not consistent with how authors think min-height and height work together


> -----------------------------------
> Proposal 3
> This proposal is more in line with what I can gather Firefox does. I
> hope I have captured the correct proposal here. I think it's close but
> in my opinion if we are going for the proposal with the most interop
> then this proposal might not be worth discussing.
>
> =Bug 16049=
> Replace:
>     # These steps do not affect the real computed values of the above
>     # properties. The change of used 'height' has no effect on margin
>     # collapsing except as specifically required by rules for
>     # 'min-height' or 'max-height' in "Collapsing margins" (8.3.1).
>
> with:
>     | These steps do not affect the real computed values of the above
>     | properties.
>
> =Bug 16036=
> Replace:
>     #   - bottom margin of a last in-flow child and bottom margin of its
>     #     parent if the parent has 'auto' computed height
>
> with:
>     |   - bottom margin of a last in-flow child and bottom margin of its
>     |     parent if the parent has 'auto' computed height and the
>     |     parent has a computed min-height that is less than or equal
>     |     to the parent's used height

The trouble here is that you don't know the used height until you've 
completed the three steps in 10.7, yet at least the first step requires 
you to know how the margin collapsing works.

Is this proposal trying to make these two margins adjoining if and only 
if min-height doesn't "have an effect"?  Several people in the WG said 
that they didn't want to introduce a condition like that, although I 
recall dbaron mentioning somewhere that that is indeed what Gecko does. 
  (Bear in mind that we'd have to introduce a cycle breaker into the 
spec if we wanted that behaviour.)

> =Bug 16037=
> Replace:
>     # The bottom margin of an in-flow block box with a 'height' of 'auto'
>     # and a 'min-height' of zero collapses with its last in-flow
>     # block-level child's bottom margin if the box has no bottom padding
>     # and no bottom border and the child's bottom margin does not collapse
>     # with a top margin that has clearance.
>
> with:
>     | - The bottom margin of an in-flow block box with a 'height' of 'auto'
>     |   and a 'min-height' of zero collapses with its last in-flow block-level
>     |  child's bottom margin, if:
>     |        - the box has no bottom padding, and
>     |        - the box has no bottom border, and
>     |        - the child's bottom margin does not collapse with a top margin
>     |           that has clearance
>
> Pros:
> - Matches the functionality of Firefox
> - More consistent with how authors think min-height and height work together
> - Adjacency and margin collapsing are still separate sections within
>   margin collapsing's normative text
> - Much easier to read and understand
> - No current CSS 2.1 test cases affected
>
> Cons:
> - Does not match Webkit, Opera and Internet Explorer

So Proposal 3 tries to achieve a different behaviour.  Leaving that to 
one side for the moment, here's a Proposal 4 describing the same 
behaviour as Proposals 1 and 2, based on my comments above.

Proposal 4:

=Bug 16049=

Replace:
   # These steps do not affect the real computed values of the above
   # properties. The change of used 'height' has no effect on margin
   # collapsing except as specifically required by rules for
   # 'min-height' or 'max-height' in "Collapsing margins" (8.3.1).

with:

   | These steps do not affect the real computed values of the above
   | properties. _Collapsed margins_<link to 8.3.1> remain unchanged
   | for each step, since the real computed value of 'height' is used
   | when determining which margins collapse.

=Bug 16036=

Add a new bullet to:
   # "Adjoining vertical margins collapse, except:"

   | - If the top margin of a box with non-zero computed 'min-height'
   |   and 'auto' computed 'height' collapses with the bottom margin of
   |   its last in-flow child, then the child's bottom margin does not
   |   collapse with the parent's bottom margin.

=Bug 16037=

Replace:
   #  - The bottom margin of an in-flow block box with a 'height' of
   #    'auto' and a 'min-height' of zero collapses with its last in-flow
   #    block-level child's bottom margin if the box has no bottom
   #    padding and no bottom border and the child's bottom margin does
   #    not collapse with a top margin that has clearance.

with:
   | - The bottom margin of an in-flow block box with a 'height' of
   |   'auto' collapses with its last in-flow block-level child's bottom
   |   margin, if:
   |      - the box has no bottom padding, and
   |      - the box has no bottom border, and
   |      - the child's bottom margin neither collapses with a top margin
   |        that has clearance, nor (if the box's min-height is non-zero)
   |        with the box's top margin.

(The 'height' and 'min-height' being talked about here are /computed/ 
values; that should be the subject of another bug, I guess.)


>> =====================
>>
>> Bug 16072 - Margin collapsing: clarification needed as to where a
>> collapsed margin manifests itself
>
> I would like to handle this issue as a separately from the other
> issues above and I will send a separate email addressing this issue.

OK.

Cheers,
Anton Prowse
http://dev.moonhenge.net

Received on Monday, 27 February 2012 20:52:42 UTC