Re: multicol-margin-001 test: review comments

Hello Gérard, 

 > [nightly-unstable]
 > http://test.csswg.org/suites/css3-multicol/nightly-unstable/html4/multicol-margin-001.htm
 > 
 > [src]
 > http://test.csswg.org/source/contributors/opera/submitted/multicol/multicol-margin-001.xht
 > 
 > As far as I can understand, this test is supposed to be verifying that
 > the top margin of the first block child element of a multicol element
 > does not collapse with the top margin of the multicol element.

Correct.

 > line 11: body {
 > 	margin: 0;
 > }
 > line 14: p {
 > 	border-bottom: 2em solid white;
 > 	margin-bottom: 1em;
 > }
 > 
 > These rules are not part of the test itself. The <p> has no content
 > anyway in this non-self-describing test.
 > We usually prefer to use/rely on browser defaults for body margins and p
 > margins. This way, tests can be shorter and more straightforward and
 > they also will work on browsers with different body margins and p
 > margins.
 > There is no need in such tests for resetting those (body and p) margins.

Agree, they should be removed. 

 > line 19: 	font-family: ahem;
 > line 20: 	font-size: 1em;
 > 
 > Whenever the Ahem font is used, we need to set a font-size whose
 > computed value will be dividable by 5px without a remainer. We do this
 > to avoid rounding issues and to ensure accurate vertical positioning of
 > content on the baseline, mostly for the Linux platform.
 > 
 > "
 > If the test uses the Ahem font, make sure its computed font-size is a
 > multiple of 5px, otherwise baseline alignment may be rendered
 > inconsistently (due to rounding errors introduced by certain platforms'
 > font APIs). We suggest to use a minimum computed font-size of 20px.
 > "
 > http://wiki.csswg.org/test/format#acceptable-test-formats
 > 
 > And so its current associated reftest would need to be updated to
 > reflect this too.

So, you suggest setting?:

  div { font-size: 20px } 

I still seee vertical lime stripes on my Linux screen. But that may be something else?

 > All the multi-column tests submitted by Opera will need to be adjusted
 > on this font-size of Ahem font issue.

Noted. Thanks for pointing this out. I'll try go through them.

 > line 26: position: relative;
 > line 37: div::after {
 > 	 content: "";
 > 	 background: white;
 > 	 height: 1em;
 > 	 width: 2em;
 > 	 position: absolute;
 > 	 right: 0;
 > 	 bottom: 0;
 > 	 display: block;
 > line 46: }
 > 
 > I do not see the reason for the generated 2x1 white rectangle in the
 > test. If margin-top of first child collapses with the margin-top of the
 > multi-column element, then we will see a bright green
 > 32px-wide-by-16px-tall rectangle at the bottom right corner. If
 > margin-top of first child does *not* collapse with the margin-top of the
 > multi-column element, then we should see such bright green
 > 32px-wide-by-16px-tall rectangle at the top left corner. And so, the
 > comparison with the reftest should work by itself; the lime stripes
 > position and dimension would need to be identical.
 > 
 > So, I think that the div::after rule is not needed, not required in the
 > test and that line 26 is also not needed, not necessary.

Your analysis seems right to me; I don't know why the white rectangle was there.

 > As is, the test is still a correct one but it's imprecise, not
 > straightforward, streamlined.

Thanks for your detailed analysis.

I'll update the test in the repository once I hear back about the font size.

BTW, I'm also working my way through the tests -- here are my sketchy
notes on conformance:

  http://people.opera.com/howcome/2013/tests/multicol-table.html

There are two good implemenetations that should help us through to PR.

I'll need help in having the tests reviewed, though -- I can't review
tests submitted by Opera, I believe.

Cheers,

-h&kon
              Håkon Wium Lie                          CTO °þe®ª
howcome@opera.com                  http://people.opera.com/howcome

Received on Wednesday, 3 July 2013 00:02:40 UTC