Section 8.3.1 (margin collapsing) review report (= 39 tests out of 129 tests)

Hello all,

This is mostly for Arron.

Unless specified for "author: Ian Hickson", all individual comments
refer to Microsoft's testcases.


Summary (mostly for Fantasai and Arron)
***************************************

- many testcases declare div {width: 5em} when it's not necessary
- many testcases declare div {font: 20px/1 ahem;} when ahem font is not
necessary
-
http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-007.htm
must be corrected
-
http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-012.htm
must state that expected result is optional, with a may flag
- I had a difficult time with
http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-027.htm
- I couldn't figure out
http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-019.htm
http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-030.htm
- More tests could be done from
http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-028.htm


Section 8.3.1
=============

Author: Ian Hickson

http://test.csswg.org/suites/css2.1/20100727/html4/abspos-021.htm
http://test.csswg.org/suites/css2.1/20100727/html4/abspos-022.htm

Rejected

Because of the lenient latitude given in

Section 10.3.7 Absolutely positioned, non-replaced elements
"
(...)  The static position for 'left' is the distance from the left edge
of the containing block to the left margin edge of a hypothetical box that
would have been the first box of the element if its 'position' property
had been 'static' and 'float' had been 'none'. (...)
But rather than actually calculating the dimensions of that hypothetical
box, user agents are free to make a guess at its probable position.
"
http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-width

Section 10.6.4 Absolutely positioned, non-replaced elements
"
(...) the static position for 'top' is the distance from the top edge of
the containing block to the top margin edge of a hypothetical box that
would have been the first box of the element if its specified 'position'
value had been 'static' and its specified 'float' had been 'none' and
'clear' had been 'none'.
(...) But rather than actually calculating the dimensions of that
hypothetical box, user agents are free to make a guess at its probable
position.
"
http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-height


[Note:
http://test.csswg.org/suites/css2.1/20100727/html4/abspos-023.htm
should be rejected for the same reasons.
And the expected results of
http://test.csswg.org/suites/css2.1/20100727/html4/absolute-non-replaced-height-001.htm
should be tweaked to indicate that the blue square may or may not touch
the upper-left corner's black borders.
]

----------------------

Author: Ian Hickson

http://test.csswg.org/suites/css2.1/20100727/html4/blocks-017.htm

Rejected.

For a more compact code, height: 1em; could be removed:

There is a greater gap (at the top) between the blue border at the top
and the first black stripe than between the last black stripe and the
bottom blue border. The gap differential is caused because the thin
solid silver has not been taken into account when establishing the
height of the blue-bordered wrapper div: it should have been 188px, not
180px.

  <style type="text/css">
   div {
     border: solid blue;
     height: 180px;
   }

   div > * {
     border: thin solid silver;
     margin: 1em;
     height: 1em;
     border-spacing: 0;
     display: block;
     font: 20px/20px Ahem;
   }
   table, td { padding: 0; border-spacing: 0; }
  </style>

should be changed to

<style type="text/css">
   div {
     border: solid blue;
     height: 188px;

    /*
    20px (margin-top) + 1px + 20px (content) + 1px + max(20px, 20px) 1st
table
    + 1px + 20px (content) + 1px + max(20px, 20px)  2nd table
    + 1px + 20px (content) + 1px + max(20px, 20px)  1st p
    + 1px + 20px (content) + 1px + 20px 2nd p
    --------------------------------------------------------
    188px
    */
     }

   div > *
   {
     border: silver solid 1px;
     margin: 1em;
     display: block;
     font: 20px/20px Ahem;
   }

   td {padding: 0;}
  </style>


Rejected

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-001.htm

For completeness, I think the assert should state that vertical margins
have no effect on non-replaced inline elements; otherwise
    #test span
            {
                color: blue;
                margin: 5em;
            }
should be changed to
    #test span
            {
                color: blue;
                margin: 0em 5em;
            }

Nevertheless, the testcase "as is" is technically correct.

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-002.htm

        div
            {
                font: 20px/1em ahem;
                width: 5em;
            }

1- Ahem font is not required in that testcase: there is no "X" anywhere
in the testcase. So, the default user agent stylesheet font-family was
good enough. If you replace ahem with serif in the font declaration, the
testcase is not affected in any way.

2- Since margin collapsing only happens with vertical margins, then
there was no risk with declaring the <div>s as 'width: auto' or width
any length actually. Either way, it was ok. The thing with 'width: auto'
is that it does not need to be declared.

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-003.htm

The wrapping div is unneeded.

Reviewed and approved.

http://www.gtalbot.org/BrowserBugsSection/css21testsuite/margin-collapse-003-gt.html

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-004.htm

The wrapping div is unneeded. Nevertheless,

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-005.htm

Ahem font is not required for this test.
div's width: 5em; declaration is not necessary for this test

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-006.htm

Ahem font is not required for this test.

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-007.htm

1- Ahem font is not required for this test.

2- #div1's computed height is 0; so, setting a background-image on it
won't do anything actually. In fact, if I remove any/all inner divs
(#div2, #div3 and #div4) or any/all of their respective margin-bottom, I
still do not see (but should see) the background-image:
url("support/margin-collapse-2em-space.png"); anyway.

3- Containers are normally "blind" wrt to floated children; containers
will not increase their height because of their floated children. Only
inline non-floated children will grow the container's height to
accomodate children.

Correction to do:

        #div1
            {
                background: url('support/margin-collapse-2em-space.png')
                height: 4em;
            }


Rejected

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-008.htm

Ahem font is not required for this test.

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-009.htm

Ahem font is not required for this test.

            div
            {
                font: 20px/1em ahem;
                height: 5em;
                width: 5em;
            }


height: 5em; and width: 5em; are not necessary, not required for the
testcase to test margin collapsing wrt overflow: hidden.

The wrapping div is not needed for the testcase.

E.g.:
http://www.gtalbot.org/BrowserBugsSection/css21testsuite/margin-collapse-009-gt.html

Nevertheless, the testcase "as is" is technically correct.

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-010.htm

Same comment as in margin-collapse-009.htm

E.g.:
http://www.gtalbot.org/BrowserBugsSection/css21testsuite/margin-collapse-010-gt.html

Nevertheless, the testcase "as is" is technically correct.

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-011.htm

Same comment as in margin-collapse-009.htm

E.g.:
http://www.gtalbot.org/BrowserBugsSection/css21testsuite/margin-collapse-011-gt.html

Nevertheless, the testcase "as is" is technically correct.

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-012.htm

1- width: 5em; is not necessary, not required for the testcase

2- ahem is not needed

3- top: auto; of an absolutely positioned element gives rise to lenient
latitude from the spec:

"the static position for 'top' is the distance from the top edge of the
containing block to the top margin edge of a hypothetical box that would
have been the first box of the element if its specified 'position' value
had been 'static' and its specified 'float' had been 'none' and 'clear'
had been 'none'. (...) But rather than actually calculating the
dimensions of that hypothetical box, user agents are free to make a
guess at its probable position."
10.6.4 Absolutely positioned, non-replaced elements
http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-height

Rejected.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-013.htm

ahem is not needed

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-014.htm

1- ahem is not needed

2- vertical-align: bottom along with div {font: 20px/1em serif;} is better
[Need to better explain this]
E.g.:
http://www.gtalbot.org/BrowserBugsSection/css21testsuite/margin-collapse-014-gt.html

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-015.htm

1- ahem is not needed

2- #div3 inline box should be "sitting" on the bottommost point of the
#div2 inline box, not on its baseline; vertical-align: bottom along with
div {font: 20px/1em serif;} is better.
[Need to verify and better explain this]

E.g.:
http://www.gtalbot.org/BrowserBugsSection/css21testsuite/margin-collapse-015-gt.html

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-016.htm

ahem is not needed

I suggest to tweak the assert text as following:

<meta name="assert" content="When top and bottom margins of a box (empty
element) are adjoining, margins collapse through that element.">

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-017.htm

The assert says

<meta name="assert" content="When the margin top of a box collapses with
the top margin of its parent the top edge is the same as the parent's.">

but the expected results do not allow to actually verify this in the
test. I do not know right now how to code/overcome this.

All that could be said is that if a box and its parent margin-top
collapse, then their respective content should be "sitting" at the
parent's border-bottom (assuming the same height and same padding-bottom
for the parent and the box).

I suggest the following:

<meta name="assert" content="When the top margin of a child box
collapses with the top margin of its parent, then the margin-top edge of
such child box is the same as the parent's and their respective content
should be 'sitting' at the parent's border-bottom (in the testcase, the
black line) assuming the same height value and the same padding-bottom
value for the parent and its child box.">

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-018.htm

Floating box's height minus margin-top of clearing box = clearance

I suggest to add this comment in the code:

/* #div3's height (3em) minus #div4's margin-top (1em) = clearance (2em) */

and I suggest to change .class1 into .ordinary-sibling

Eg.:
http://www.gtalbot.org/BrowserBugsSection/css21testsuite/margin-collapse-018-gt.html

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-019.htm

and

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-030.htm

have not been reviewed.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-020.htm

1- Ahem font is not needed.

2- DTD should be strict

3- #root should be replaced by html
<html id="root"> should be replaced with <html>

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-021.htm

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-022.htm

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-023.htm

<meta name="assert" content="If a block-level sibling has clearance it
will not adjoin its top margin with the bottom margin of a previous
sibling.">

meaning...
#div4's margin-top is not adjoining with #div2's margin-bottom;
therefore no margin collapse between those 2.

I suggest this modification to the assert:

<meta name="assert" content="If an in-flow block-level sibling (#div4)
has clearance applied to it, then it will not adjoin its top margin with
the bottom margin of its previous in-flow sibling (#div2).">

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-024.htm

I suggest this modification to the assert:

<meta name="assert" content="When an in-flow block-level element (#div3)
is adjoining its in-flow block-level child's top margin and the child
has no top border, no top padding or no clearance applied to it, then
such top margins collapse.">

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-025.htm

Due to C.5.26 change
http://www.w3.org/TR/CSS21/changes.html#q313
I suggest this modification to the assert:

<meta name="assert" content="The bottom margin of an in-flow block-level
element(#div2) with a 'height' of 'auto' will adjoin to its last in-flow
block-level child's (#div3) bottom margin if such auto-height in-flow
block-level element has no bottom padding and no bottom border. The
bottom margins will then collapse.">

http://www.gtalbot.org/BrowserBugsSection/css21testsuite/margin-collapse-025-gt.html

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-026.htm

I suggest this modification to the assert:

<meta name="assert" content="An element's margins are adjoining and can
collapse if the 'min-height' is zero, if it has no top or bottom borders
and no top or bottom padding, if it has a 'height' of zero or 'auto', if
it has no line boxes and if all of its in-flow children's margins are
adjoining.">

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-027.htm

"
When an element's own margins collapse, and that element has had
clearance applied to it, its top margin collapses with the adjoining
margins of subsequent siblings but that resulting margin does not
collapse with the bottom margin of the parent block.
"

"its own margins collapse and it collapsed its margins with a previous
sibling"
subsequent != previous

#div5's margins (minus the clearance) should collapse with #div6's margin
            {
                clear: both;
                margin-top: 1.5em;
                margin-bottom: 1em;
            }

#div2's margin-bottom (2em) should not collapse with #div6's margin top.
Result of a margin collapse between #div5's margin-top and
#div6's margin-top: max(max((1.5em - 0.5em), 1.0em), 0em)
[I could be wrong on that.]

---------------------------
|  #div3 green background |
|   content height: 1em   |
|-------------------------|
|  #div4 float            |
|   content height: 1em   |
|-------------------------|
|=========================|  <-- #div5 with a height of 0
|                         |
|  #div2's margin-bottom  |
|   height: 2em           |
|                         |
|-------------------------|
|  #div6 margin-top       |
|   height: 1em           |
|-------------------------|
|  #div6 green background |
|   content height: 1em   |
|-------------------------|

The assert is wrong.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-028.htm

#div4
            {
                border: inherit;
                margin: 50%;
                padding: 50%;
            }

The computed values of border, margin and padding are all 0. So, the
test does not test situations where an inherited border is not 0,
inherited margin is not 0, inherited padding is not 0.
The inherited values are the same as default values (0); so the testcase
has limited relevance. More tests could be developed to cover those
situations.

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-029.htm

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-019.htm

and

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-030.htm

have not been examined but I couldn't figure these out.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-102.htm
http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-103.htm
http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-104.htm

A test assertion for that test should state that
"If the top and bottom margins of a box are adjoining, then it is
possible for margins to collapse through it." and that abs. pos.
container/fixed pos. elements/floated elements with 'width: auto' will
resort to shrink-to-fit width calculation.

The test by itself has actually little to do with absolutely positioned
elements (102), fixed positioned elements (103), floated elements (104).

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-106.htm

"The pattern below should be a stack of horizontal bars with no red."
should be
"The pattern below should be a stack of horizontal bars (from top to
bottom: orange, yellow, orange, bright green, orange) with no red."

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-110.htm

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-111.htm

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-112.htm

"The pattern below should be a stack of horizontal bars with no red."
should be
"The pattern below should be a stack of horizontal bars (from top to
bottom: orange, yellow, orange, bright green, orange) with no red."

I see no significative or meaningful difference between
margin-collapse-106.htm and margin-collapse-112.htm

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/margin-collapse-122.htm

Reviewed and approved.

----------------------

http://test.csswg.org/suites/css2.1/20100727/html4/table-margin-004.htm

Corrections at
http://lists.w3.org/Archives/Public/public-css-testsuite/2010Aug/0015.html
and
http://test.csswg.org/suites/css2.1/20100727/html4/table-margin-004.htm


regards, Gérard
-- 
Contributions to the CSS 2.1 test suite:
http://www.gtalbot.org/BrowserBugsSection/css21testsuite/

CSS 2.1 test suite (beta 2; July 27th 2010):
http://test.csswg.org/suites/css2.1/20100727/html4/toc.html

CSS 2.1 test suite contributors:
http://test.csswg.org/source/contributors/

Received on Wednesday, 11 August 2010 23:23:59 UTC