477462 - create an extensive reftest suite for margin-collapsing

Status: NEW

(Core :: Layout: Block and Inline, defect)

Description

[Daniel.S]

Attachment: create basic infrastructure

In bug 451791 comment 5 roc mentioned that we need a robust test suite for margin properties and especially the margin-collapsing cases in order to start reworking that code.

I'd like to tackle this at least partially. My simple starting point is to create a margin directory in layout/reftests/ where all this work should land.

If there are any suggestions or hints please don't hesitate to mention them before I actually start working on this.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 361136 [details] [diff] [review]
create basic infrastructure

I think I would actually prefer calling the directory "margin-collapsing"; that's the big thing that needs a lot of testing.

Comment 2

[Jeff Walden [:Waldo]]

(FYI, bugs for specific tests go in the most closely related component.  Bugs in the test harnesses themselves go in Testing:*.)

Comment 3

[Daniel.S]

Attachment: create basic infrastructure, for checkin [Checkin: Comment 5]

(In reply to comment #1)
> (From update of attachment 361136 [details] [diff] [review])
> I think I would actually prefer calling the directory "margin-collapsing";
> that's the big thing that needs a lot of testing.

No problem.

Comment 4

[Daniel.S]

Attachment: Wave 1 (for review)

Here's the first wave of tests. The tests are based on the 2007-07-19 CR, incorporating the Errata as it stands today (2009-02-15).

To ease the review process I've put all tests online at http://www.freewebs.com/the-dees/wave1/summary.html

I've got some questions as well:

1. In the references I still use single margins to get for example a space between two elements. Is this ok, or should they be replaced by border/padding/transparent boxes?

2. About overflow tests 3 and 4: They scroll, and I'm not sure wether they should. In Fx 3 and earlier as well as in all other browsers they don't scroll. The IE team discarded a bug report for this in reference to CSS 2.1 §10.6.3 (http://www.w3.org/TR/CSS21/visudet.html#normal-block).
That part is a bot over my head, so should we scroll or do we actually fail the tests?

3. The 6 tests for clearance fail. This seems to be correct, but I'd like to hear your opinion on this.

4. What this patch doesn't include is what should happen with a elements bottom margin, when it's parent got min-height and max-height. According to the Errata the spec changed, but I couldn't find out to what it changed.
Currently, Safari and Opera always collapse the margin (even when not adjoining) while IE8 never collapses them.
Can I read anywhere, what the corrected spec currently says?

5. This wave consists mostly of display: block; tests that I wrote while reading the Collapsing Margin chapter.
It's clear that I've got to write some tests for tables, but I wonder if i should repeat the current test set for display: list-item as well?

6. David, in Bug 87277 you said that this bug were invalid because the spec changed. I don't know how you come to this conclusion.

Tables (well, the anonymous boy around it) establish a new block formatting context. But the other in-flow bfc's (elements with overflow != visible) also collapse their outer vertical margins. Tables don't seem to have special rules here.

The only thing that should've changed by the the spec it that caption elements no longer collapse their margins with the table as they're in-flow children of the table's block formatting context. Am I wrong?

So far so good, happy reviewing.

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 362218 [details] [diff] [review]
create basic infrastructure, for checkin
[Checkin: Comment 5]


http://hg.mozilla.org/mozilla-central/rev/9537d42f8308

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks heaps for doing this!!

(In reply to comment #4)
> 1. In the references I still use single margins to get for example a space
> between two elements. Is this ok, or should they be replaced by
> border/padding/transparent boxes?

That sounds fine.

> 2. About overflow tests 3 and 4: They scroll, and I'm not sure wether they
> should. In Fx 3 and earlier as well as in all other browsers they don't
> scroll. The IE team discarded a bug report for this in reference to CSS 2.1
> §10.6.3 (http://www.w3.org/TR/CSS21/visudet.html#normal-block).
> That part is a bot over my head, so should we scroll or do we actually fail
> the tests?

I think scrolling is correct. See bug 458296, which we fixed recently. I'm not sure what 10.6.3 has to do with it; that's for elements with overflow:visible.

> 3. The 6 tests for clearance fail. This seems to be correct, but I'd like to
> hear your opinion on this.

You mean your block-clearance-* tests? They don't contain floats, so I'm not sure what you expect 'clear' to be doing. 'clear' always does nothing when there are no floats.

> 4. What this patch doesn't include is what should happen with a elements
> bottom margin, when it's parent got min-height and max-height. According to
> the Errat the spec changed, but I couldn't find out to what it changed.
> Currently, Safari and Opera always collapse the margin (even when not
> adjoining) while IE8 never collapses them.
> Can I read anywhere, what the corrected spec currently says?

I'm not sure, but maybe this thread starting here would help?
http://lists.w3.org/Archives/Public/www-style/2008Sep/0081.html

> 5. This wave consists mostly of display: block; tests that I wrote while
> reading the Collapsing Margin chapter.
> It's clear that I've got to write some tests for tables, but I wonder if i
> should repeat the current test set for display: list-item as well?

No, that shouldn't be necessary.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you update the patch for roc's comments and then request review again on the new patch?

Comment 8

[Daniel.S]

Attachment: Wave 1, rev 1 (for review)

(In reply to comment #6)
> I think scrolling is correct. See bug 458296, which we fixed recently. I'm not
> sure what 10.6.3 has to do with it; that's for elements with overflow:visible.

Bug 458296 mentions bug 47710 which itself mentions 10.6.3 though the CSS 2 version. Looking at these bugs, I agree that scrolling is correct.

> You mean your block-clearance-* tests? They don't contain floats, so I'm not
> sure what you expect 'clear' to be doing. 'clear' always does nothing when
> there are no floats.

Ok, I read "An element that has had clearance applied to it never collapses its top margin with its parent block's bottom margin.", but I couldn't imagine how that would work with floats. Guess I misunderstood the spec then. I've removed those tests from the patch.

> I'm not sure, but maybe this thread starting here would help?
> http://lists.w3.org/Archives/Public/www-style/2008Sep/0081.html

Looks good, thank you. I'll write tests for that later.

(In reply to comment #7)
> Could you update the patch for roc's comments and then request review again on
> the new patch?

Yeah, here it is. I also updated the online reference.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #8)
> Ok, I read "An element that has had clearance applied to it never collapses its
> top margin with its parent block's bottom margin.", but I couldn't imagine how
> that would work with floats. Guess I misunderstood the spec then. I've removed
> those tests from the patch.

An element has clearance applied to it only when it has the 'clear' property *and* it actually has to move to clear floats. This part of the spec is extremely tricky and needs to be read very carefully :-).

Comment 10

[Daniel.S]

(In reply to comment #9)
> An element has clearance applied to it only when it has the 'clear' property
> *and* it actually has to move to clear floats. This part of the spec is
> extremely tricky and needs to be read very carefully :-).

It really is. Maybe you could help me with another clear-related question?

There is bug 376365. It refers to a CSS WG decision that is not public. It seems to suggest that clearance can't become negative. On the other hand, the slightly newer draft of the spec notes that clearance can indeed become negative. This is also what everyone seems to implement.

Could you please tell me wether that bug is valid or not? I'd like to add a test for this situation because it's also a special case where marin-collapsing has no effect. Thanks.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The decision that bug refers to is in the spec:
http://www.w3.org/TR/CSS21/visuren.html#flow-control

> If this hypothetical position of the element's top border edge is not past the
> relevant floats, then its clearance must be set to the greater of:
>
>   1. The amount necessary to place the border edge of the block even with the
> bottom outer edge of the lowest float that is to be cleared.
>   2. The amount necessary to make the sum of the following equal to the
> distance to which these margins collapsed when the hypothetical position was
> calculated:
>          * the margins collapsing above the clearance
>          * the clearance itself
>          * if the block's own margins collapse together: the block's top margin
>          * if the block's own margins do not collapse together: the margins
> collapsing below the clearance 
>
> Note: The clearance can be negative.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 363299 [details] [diff] [review]
Wave 1, rev 1 (for review)

I'm a little surprised you wrote this many tests without encountering
any bugs.  Or did you exclude the tests that showed bugs from this
patch?

You might want to make a few of the references even more explicit, e.g.,
change block-sibling-1-ref.html (or add a second reference) to have
three divs, all 20px high, where the first and third have a background
and the middle one is transparent.  Likewise for block-float-1a.  (Maybe
some others, where you have tree structure in the reference that isn't
necessary.)  Likewise, it might be good to have some of the
root/html/body tests have additional references that have the html/body
margins all 0 and the stuff that shows up actually be divs inside them.

In the block-negative-* series, it would probably be good to also test:
 * two negative margins collapsing with each other (to the more negative
   value)
 * a negative margin collapsing with a positive one, in cases where the
   result is positive and in cases where the result is negative

I don't think there's anything in the spec to back up that we should
pass block-abs-pos-1.  It's still ok to test it, but please note that in
a comment in the test.  (The interaction of "static position" with
margin collapsing is not well defined.)

To add to inline-block-child, it would probably be good to have a test
of an inline-block with a block child.

Maybe block-used-value-1 should just be called block-percent-1 ?  How is
it testing used values?




The suggestions for additional tests or references don't need to hold up
this patch landing.

Of course, there are still tons more tests that could be written other
than the ones I suggested.

It might also be good to get into the habit of adding some != tests to
make sure that things that you expect to be unequal to be unequal, just
to verify that the tests are still being useful.


Thanks for writing all these, and I hope more are coming.  Sorry for
taking so long to get to the review.

Comment 13

[Daniel.S]

Attachment: Wave 1, rev 2 (for checkin) [Checkin: Comment 17]

(In reply to comment #12)
> The suggestions for additional tests or references don't need to hold up
> this patch landing.

Alright, I addressed the specific comments and memorized your suggestions.

> I'm a little surprised you wrote this many tests without encountering
> any bugs.  Or did you exclude the tests that showed bugs from this
> patch?

The latter should be closer to the truth. I didn't encounter bugs while writing this set of bugs. However, I also excluded many tests I knew they'd fail, e.g. for tables and captions. I've written most of these in the meantime.

By the way, should testcases of already filed bugs also go here (reftests/margin-collapsing) if I discover them while writing tests?

> I don't think there's anything in the spec to back up that we should
> pass block-abs-pos-1.  It's still ok to test it, but please note that in
> a comment in the test.  (The interaction of "static position" with
> margin collapsing is not well defined.)

Done, partially repeating this passage.

> Maybe block-used-value-1 should just be called block-percent-1 ?  How is
> it testing used values?

Not really. I renamed the test as suggested. I'll try to find a situation where the resulting margin differes between given and used values.

> Thanks for writing all these, and I hope more are coming.  Sorry for
> taking so long to get to the review.

More tests are on the way, the next patch should be ready soon.
No problem. There were no regressions in the meantime ;-)

Comment 14

[Daniel.S]

Attachment: Wave 1, rev 3 (for checkin)

Added checkin-message and corrected a minor mistake.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Too late, I just committed rev 2!

Comment 16

[Daniel.S]

Attachment: Bugfixx for rev 2 [Checkin: Comment 17]

(In reply to comment #15)
> Too late, I just committed rev 2!

Here's the fix. I forgot to edit the reftest.list..
This single test will fail if the fix was not quick enough. Sorry.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/735447ba20a6
http://hg.mozilla.org/mozilla-central/rev/dfff608ebad0

Thanks!

I assume I'm supposed to leave this open.

Comment 18

[Daniel.S]

(In reply to comment #17)
> I assume I'm supposed to leave this open.

Yes, please. I think there'll be at least two more patches before this can be considered 'fixed'.

Comment 19

[Daniel.S]

Attachment: Wave 2 (for review)

In this patch:

 * Slightly better IDs and class names.
 * Tests for horizontal margins.
 * New tests for negative margins.
 * One block-non-sibling test with display: none;
 * Tests for table and table-caption elements.
 * Tests for the fieldset element.
 * Test for inline-block with block child.
 * Added simple tests for the top border edge when bottom margins collapse.
 * Added correct tests for clearance.
 * Added more references for root elements and body.

I've added fieldset because HTML 5 expects it to establish a new block formatting context; this is in line with IE, Safari and Opera.

The caption-only tests fail due to bug 144517 (only the first of consecutive table-caption elements is rendered).

Again, I've put the tests online: http://www.freewebs.com/the-dees/wave2/summary.html

Most of these tests were written before the review of Wave 1, but except for more explicit testcases I should've addressed every comment.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 374600 [details] [diff] [review]
Wave 2 (for review)

The manifest needs to say "fails" rather than "fail".

It would be great if you could add tests for the other values of 
'caption-side' (we support 6 of them).  

I'm a little skeptical about the caption-sibling tests.  Does the spec 
even define what happens when a table has multiple captions?  Could you
at least note in the comments that the tests (and bug 144517 itself) are
only questionably valid?

r+sr=dbaron.  Sorry for the delay.

Comment 21

[Daniel.S]

Attachment: Wave 2, rev 1 (for checkin) [Checkin: Comment 22]

(In reply to comment #20)
> The manifest needs to say "fails" rather than "fail".

Fixed.

> It would be great if you could add tests for the other values of 
> 'caption-side' (we support 6 of them).

Ah, I was not aware that there were 6 of them. I'll make sure to add tests for these later.

> I'm a little skeptical about the caption-sibling tests.  Does the spec 
> even define what happens when a table has multiple captions?

Yes, it does. At least the latest CR says:
> (...) the table box generates an anonymous box that contains (...) any
> caption boxes (in document order). The caption boxes are block-level boxes
> (...) and are rendered as normal blocks inside the anonymous box.
> (...) Whether the caption boxes are placed before or after the table box is
> decided by the 'caption-side' property...

> Could you at least note in the comments that the tests (and bug 144517 itself)
> are only questionably valid?

Comments added. However, from my point of view CSS 2.1 clearly defines what should happen when multiple captions are present.

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 378653 [details] [diff] [review]
Wave 2, rev 1 (for checkin)
[Checkin: Comment 22]


http://hg.mozilla.org/mozilla-central/rev/06e585dab9f7

Comment 23

[Daniel.S]

Attachment: Wave 3 (for review) [Checkin: comment 25]

Sorry for the delay, I'm rather busy at the moment.

In this patch:

 * Additional tests involving clearance.
   5: clear with top margin doesn't influence the parent element
   6: margin-collapsing of top and bottom margins of a clearing element.
   7: A clearing element's top margin collapses with margins of subsequent
      siblings but that resulting margin does not collapse with the bottom
      margin of the parent block.
 * Test for height and bottom-margin-collapsing with last-child.
 * Tests for min-height = 0
 * Additional references for existing tests.

Again, I've put the tests online:
http://www.freewebs.com/the-dees/wave3/summary.html

While writing these tests I discovered bug 493380.

I hope relying on the font-size in tests block-zero-min-height-2 a to c is ok?

I expect to write at least one more wave of tests.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 388049 [details] [diff] [review]
Wave 3 (for review) [Checkin: comment 25]

I've been meaning to review this, but it's a lot of work, and I'm not sure how much value reviewing this stuff in detail really adds.  And, in any case, there's certainly no reason not to check this in now, so I'm rubber-stamping it.

Sorry for waiting a month until deciding that.

In any case, it would probably be good to get this in on both mozilla-central and mozilla-1.9.2.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Part 3 landed:
http://hg.mozilla.org/mozilla-central/rev/73bb4a556e4f
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f3b972dee24c

Comment 26

[Daniel.S]

Attachment: remove some invalid comments about failures

This removes the comments about bug 493380 (which is invalid).

(In reply to comment #24)
> I've been meaning to review this, but it's a lot of work, and I'm not sure how
> much value reviewing this stuff in detail really adds.  And, in any case,
> there's certainly no reason not to check this in now, so I'm rubber-stamping
> it.

We've discussed bug 493380 anyway and I'm sure the other tests are fine.
So I don't think it was a problem at all.


Sorry for having this on hiatus. If I'm correct, there's going to be new prose for 8.3.1 and I prefer to wait for that.

On the other hand, I guess dynamic tests will also be needed.

Are "tricks" like reflow=document.body.offsetHeight between elements and adding/manipulating nodes using the onLoad event reliable?

Any tips for work in that area?

Comment 27

[Daniel.S]

(In reply to comment #26)
> Created attachment 499857 [details] [diff] [review]
> remove some invalid comments about failures

Can this land? How would you like to proceed with patches? Review then land or just land (with simple review)?

David/roc,

> Sorry for having this on hiatus. If I'm correct, there's going to be new prose
> for 8.3.1 and I prefer to wait for that.

Is the current Editors Draft at http://www.w3.org/Style/css2-updates/draft-PR-CSS21-201103XX/box.html#collapsing-margins 'ready' to be used as reference or will there be more foreseeable edits?

> Are "tricks" like reflow=document.body.offsetHeight between elements and
> adding/manipulating nodes using the onLoad event reliable?

The question is: If I add an element dynamically, are the two methods interchangeable or do they activate different behaviour that may affect the final rendering?

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the editor's draft is stable.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #27)
> Can this land? How would you like to proceed with patches? Review then land or
> just land (with simple review)?

Just request review on the patch, I think.

> > Are "tricks" like reflow=document.body.offsetHeight between elements and
> > adding/manipulating nodes using the onLoad event reliable?
> 
> The question is: If I add an element dynamically, are the two methods
> interchangeable or do they activate different behaviour that may affect the
> final rendering?

Inserting offsetHeight calls should not affect the rendering. If you want to test dynamic changes more reliably, make the test reftest-wait and listen for MozReftestInvalidate to do your dynamic change. See existing reftests that use MozReftestInvalidate. Thanks!

Comment 30

[Daniel.S]

Attachment: Wave 4 (checked in)

I'm getting started again, so here are some simple tests.

 * Additional horizontal margin tests (static and dynamic).
 * != and dynamic tests for margins of block siblings
 * != and dynamic tests for margins of table siblings
 * != and dynamic tests for margins of caption siblings

There are some completely new tests, but most build on existing tests and references.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 528083 [details] [diff] [review]
Wave 4 (checked in)

r=dbaron.  Sorry for the delay in getting to these.

Do you need somebody to land them for you?

It's likely worth pushing to try before landing on mozilla-central, or just landing on cedar.

Comment 32

[Daniel.S]

(In reply to comment #31)
> r=dbaron.  Sorry for the delay in getting to these.

No problem, you gave me time to get familiar with the new spec text.

> Do you need somebody to land them for you?

Yes, please. I'd really appreciate if somebody could take care of pushing the tests to try and any branches you'd like to land them on. Thanks.

However, there's no need to rush. The next batch of tests is not finished yet.

Comment 33

[Boris Zbarsky [:bzbarsky]]

http://tbpl.mozilla.org/?tree=Try&rev=1bb4bd121383 pushed; let's see how it looks.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Looks green.

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 528083 [details] [diff] [review]
Wave 4 (checked in)

http://hg.mozilla.org/mozilla-central/rev/65bae86de3be

Comment 36

[Daniel.S]

Attachment: Wave 5 (for review)

I just realized that I wrote a lot of tests since I've submitted wave 4.
I hope this won't cause too much trouble.

Exiting new tests:

 * margins of html and body in HTML
 * margins of html, body and an generic 'root' element in XHTML
 * margins of XML root elements
 * dynamic and != tests of negative margins (siblings)
 * dynamic and != tests of non-sibling element margins
   * new tests for parent and first-child top margins
   * new tests for parent and last-child bottom margins
     * where parent's height = auto

I have a few questions regarding the test block-xml-root-2.xml
The test fails because the bottom margin of the root element is not redered.
Is this related to bug 47710 or is it another (maybe unfiled) bug?

The test is currently inactive because I couldn't figure out whether the test can be handled by the harness.
Is the reftest harness taking into account the whole canvas or only a defined viewport?
If the latter, does the harness take into account the scrollbar? If not, I don't know how to test that situation.


The existing non-sibling tests have been updated slightly to have no margins that can add up to a given length in the test.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The harness tests a 800x1000 viewport.  It should definitely render the scrollbar on child frames; I'm not sure about the toplevel document -- look at the images to check.  You can always use a frame if you need one.

I suspect bug 47710 is the right one.  (What do you mean by "inactive"?  Can you just mark the test as currently failing?)

Comment 38

[Daniel.S]

Attachment: Wave 5, rev 1 (for review)

(In reply to comment #37)
> (What do you mean by "inactive"?
> Can you just mark the test as currently failing?)

It was commented out, because I didn't know if the harness would recognize the failure.

> The harness tests a 800x1000 viewport.  It should definitely render the
> scrollbar on child frames; I'm not sure about the toplevel document -- look
> at the images to check.  You can always use a frame if you need one.

I've added a frame for now. I'll look into the images when I have more time to set up a proper test build. Thanks for the hint.

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, let me know once you've had a chance to look at the images and figure that out

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 534819 [details] [diff] [review]
Wave 5, rev 1 (for review)

Could you request review again once you've done that?  Or, if you won't be able to do it, could you ask me to do it?

Comment 41

[Daniel.S]

(In reply to David Baron [:dbaron] from comment #40)
> Could you request review again once you've done that?  Or, if you won't be
> able to do it, could you ask me to do it?

Sorry, I won't be able to test this in the forseeable future. Could you please take care of this task instead?

Does that mean that you haven't had the chance to look at this patch yet? If so, would you mind if I put another set of tests up for review?
I think I can improve the current set of tests a little.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 534819 [details] [diff] [review]
Wave 5, rev 1 (for review)

If you have updates for the patch right now, feel free to update it.

Comment 43

[Daniel.S]

Comment on attachment 534819 [details] [diff] [review]
Wave 5, rev 1 (for review)

(In reply to David Baron [:dbaron] from comment #42)
> If you have updates for the patch right now, feel free to update it.
I don't have an update for Wave 5 at the moment. But I will write one because it is not reviewed yet.

Instead, I can offer a lot of other tests that I've written over the past months. I can prepare another Wave at any time to keep you busy :)

Actually I think I'm almost done locally, missing only clearance, left/right captions and tests for min-/max-height situations.
Few tests depend on bug 659828, but thats only a minor part of the whole.

Comment 44

[Daniel.S]

Attachment: Wave 5a [checkin: comment 49]

This is one set of tests that should be fine.

 * dynamic and != tests of negative margins (siblings)

I hope it is big enough to be worthwhile and small enough to be reviewed quickly.

Comment 45

[Daniel.S]

(In reply to Daniel.S (mostly offline during the week) from comment #44)
> Created attachment 554098 [details] [diff] [review] [diff] [details] [review]
> Wave 5a (for review)

Sorry, but ping?

Anything I can do to speed up the process?

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, so I think waiting on me to test the patch wasn't the best idea.  That said, I just did that, and I get the following failures:

REFTEST TEST-UNEXPECTED-FAIL | file:///home/dbaron/builds/ssd/mozilla-central/mozilla/layout/reftests/margin-collapsing/block-clear-5c.html | image comparison (==), max difference: 255, number of differing pixels: 15680
REFTEST TEST-UNEXPECTED-FAIL | file:///home/dbaron/builds/ssd/mozilla-central/mozilla/layout/reftests/margin-collapsing/block-clear-5d.html | image comparison (==), max difference: 255, number of differing pixels: 39200
REFTEST TEST-UNEXPECTED-FAIL | file:///home/dbaron/builds/ssd/mozilla-central/mozilla/layout/reftests/margin-collapsing/block-clear-5g.html | image comparison (==), max difference: 255, number of differing pixels: 15680
REFTEST TEST-UNEXPECTED-FAIL | file:///home/dbaron/builds/ssd/mozilla-central/mozilla/layout/reftests/margin-collapsing/block-clear-5h.html | image comparison (==), max difference: 255, number of differing pixels: 54880

In the future, I think you should try asking on #developers if someone less busy than me could test the patch for you, or help you push to try.  (Or maybe try asking dholbert or sjohnson?)

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, actually, never mind, those are failures related to the patch for bug 376365.

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 554098 [details] [diff] [review]
Wave 5a [checkin: comment 49]

OK, I'll land these; sorry for taking so long to get to looking at this.  I'll try to be better about this in the future or to delegate it to someone else.

Comment 49

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52fa780fc244

Comment 50

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/52fa780fc244

Comment 51

[Daniel.S]

(In reply to David Baron [:dbaron] from comment #46)
> In the future, I think you should try asking on #developers if someone less
> busy than me could test the patch for you, or help you push to try.  (Or
> maybe try asking dholbert or sjohnson?)

I'll try :)

(In reply to David Baron [:dbaron] from comment #48)
> OK, I'll land these; sorry for taking so long to get to looking at this. 
> I'll try to be better about this in the future or to delegate it to someone
> else.

It's ok, I know you're busy. I just have the problem that I'm no longer familiar with the tests I've written so long ago.

I think I'll have some time in mid february to check some of the other tests I've written (which aren't checked in yet) and whether the changes to CSS 2.1 had any impact on the expected rendering.

Comment 52

[Daniel.S]

Attachment: Wave 5b (for review)

Here are the tests that still need to get landed:

 * margins of html and body in HTML
 * margins of html, body and an generic 'root' element in XHTML
 * margins of XML root elements
 * dynamic and != tests of non-sibling element margins
   * new tests for parent and first-child top margins
   * new tests for parent and last-child bottom margins

(In reply to David Baron [:dbaron] from comment #46)
> In the future, I think you should try asking on #developers if someone less
> busy than me could test the patch for you, or help you push to try.  (Or
> maybe try asking dholbert or sjohnson?)

Sorry, I requested your review again. Should I r? dholbert or sjohnson instead or should I just ask them to test and push a patch once its written (or something else)?

Comment 53

[Daniel.S]

Attachment: Wave 5b, rev 1 (for review)

Sorry, this is the correct patch >.<

Comment 54

[Daniel.S]

(In reply to David Baron [:dbaron] from comment #46)
> In the future, I think you should try asking on #developers if someone less
> busy than me could test the patch for you, or help you push to try.  (Or
> maybe try asking dholbert or sjohnson?)

CC'd the two (I hope I've got the right sjohnson :). Please forgive the noise.

Comment 55

[Daniel Holbert [:dholbert]]

A few observations / questions, from a very quick skim of the patch:

 a) Overall, some comments (in the manifest and/or the tests) would be very helpful for explaining what the various tests (or groups of tests) are intended to cover.

 b) Some of the XHTML tests have a "<root>" node at the top level. What's the purpose of that? (It looks like it's intentional, given that it's in the test name -- maybe add a comment to those tests and/or to the manifest file explaining what's going on there?)

 c) You probably want to be listening for MozReftestInvalidate instead of DOMContentLoaded (That guarantees that your dynamic modifications do actually happen after our initial layout has completed)

 d) Some of the tests have multiple references. That's kind of backwards from how reftests are generally set up -- the reference cases are (generally) assumed to be simpler and to be pretty likely to render correctly.  From that viewpoint, multiple reference cases are sort of redundant, by definition. So it's generally better do structure this as multiple testcases sharing the same reference, rather than multiple references sharing a single testcase, if you can.

 e) Some of the tests have a reference case (or several) **as well as** a "!=" "not-reference" case (or several).  That seems unnecessary.  If we're already checking that the testcase renders like we expect it to, then we shouldn't *also* need to test that it *doesn't* render in some other way.  The only way that makes sense is if you're really testing the not-reference case, I'd imagine -- and if that's the case, you should just convert it into an additional testcase, and compare it against the reference as well. (as a "!=" I suppose)  Otherwise, I don't really see the point of having those.

It might be possible to ameliorate my concerns on (d) and (e) by just adding some comments explaining what you're checking for.  Right now, I'm just going off of the reftest "best practices" that I've picked up over the years.

Comment 56

[Daniel.S]

(In reply to Daniel Holbert [:dholbert] from comment #55)
> A few observations / questions, from a very quick skim of the patch:

Thanks for your quick reply.

>  a) Overall, some comments (in the manifest and/or the tests) would be very
> helpful for explaining what the various tests (or groups of tests) are
> intended to cover.

I can (and will) add comments to the manifest. I thought about adding comments before, but there are almost no manifest files with comments, so I didn't bother. I wont comment single tests, except if it's really necessary, there are simply too many of them. I estimate that about a third of all tests written is currently checked in.

>  b) Some of the XHTML tests have a "<root>" node at the top level. What's
> the purpose of that? (It looks like it's intentional, given that it's in the
> test name -- maybe add a comment to those tests and/or to the manifest file
> explaining what's going on there?)

The margins of a root element don't collapse. In XML, the html element is not necessarily the root element of a document. These tests check whether margins on the html element correctly collapse in that case.
I'll add a comment to the manifest for these tests.

>  c) You probably want to be listening for MozReftestInvalidate instead of
> DOMContentLoaded (That guarantees that your dynamic modifications do
> actually happen after our initial layout has completed)

That's right. I used DOMContentLoaded for local testing. I forgot to replace it before posting the patch. Sorry.

Please note that I wrote these tests 18 months ago. If something looks strange, please don't hesitate to ask.

>  d) Some of the tests have multiple references. That's kind of backwards
> from how reftests are generally set up -- the reference cases are
> (generally) assumed to be simpler and to be pretty likely to render
> correctly.  From that viewpoint, multiple reference cases are sort of
> redundant, by definition. So it's generally better do structure this as
> multiple testcases sharing the same reference, rather than multiple
> references sharing a single testcase, if you can.
> 
>  e) Some of the tests have a reference case (or several) **as well as** a
> "!=" "not-reference" case (or several).  That seems unnecessary.  If we're
> already checking that the testcase renders like we expect it to, then we
> shouldn't *also* need to test that it *doesn't* render in some other way. 
> The only way that makes sense is if you're really testing the not-reference
> case, I'd imagine -- and if that's the case, you should just convert it into
> an additional testcase, and compare it against the reference as well. (as a
> "!=" I suppose)  Otherwise, I don't really see the point of having those.
> 
> It might be possible to ameliorate my concerns on (d) and (e) by just adding
> some comments explaining what you're checking for.  Right now, I'm just
> going off of the reftest "best practices" that I've picked up over the years.

When I wrote these tests, I tried to test every possible case I could think of. You'll notice that many test series simply differ by the height of a margin. I don't think I can write much more tests for those cases, i.e. I don't think more tests written in my "style" of writing will add much to the test suite.
However, if you can suggest particular cases that should be tested, I'll gladly try to embrace your proposal.

Maybe I overdid it, but there are multiple == and != tests for some tests because I think David suggested it when I started writing these tests. See

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #12)
> You might want to make a few of the references even more explicit, e.g.,
> change block-sibling-1-ref.html (or add a second reference) to have
> three divs, all 20px high, where the first and third have a background
> and the middle one is transparent.

and

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #12)
It might also be good to get into the habit of adding some != tests to
make sure that things that you expect to be unequal to be unequal, just
to verify that the tests are still being useful.

So some of the != references portray how incorrect renderings could look like. I didn't come up with a much better method.

Would you prefer if I left out some of the additional references and !references?

Comment 57

[Daniel Holbert [:dholbert]]

(In reply to Daniel.S (mostly offline during the week) from comment #56)
> Maybe I overdid it, but there are multiple == and != tests for some tests
> because I think David suggested it when I started writing these tests.

Gotcha -- sorry, didn't mean to send mixed messages about that :)

> Would you prefer if I left out some of the additional references and !references?

Not necessarily -- I haven't actually read them in detail (partly because there were no comments, so I wasn't sure what I should be looking for when comparing them. ;) )

If you think the additional references / !references are useful & legitimately increase our test-coverage (which it sounds like they do?), then by all means keep them, but add a brief explanatory comment (in the manifest and/or test) to explain why they're there / what they're intended to test.

Comment 58

[Daniel.S]

Attachment: Wave 5b, rev 2 (for review)

Sorry for the delay.

Here's revision 2: I'm using MozReftestInvalidate again and I added quotes from CSS 2.1 and comments to the manifest. I hope they help to understand which parts are actually tested.

Some comments will improve as the test suite grows. When I started I just wanted to have a little bit of everything, so there are sections which aren't exhaustivly tested yet.

I'm unsure about all the skip-if(B2G) tests that are in the manifest now. I'm not able to test B2G and the bugs mentioned in this regard aren't clear why some tests are skipped and others are not.

Comment 59

[Daniel Holbert [:dholbert]]

(In reply to Daniel.S (mostly offline during the week) from comment #58)
> Here's revision 2: I'm using MozReftestInvalidate again and I added quotes
> from CSS 2.1 and comments to the manifest. I hope they help to understand
> which parts are actually tested.

The comments are very helpful -- thanks for those!

> I'm unsure about all the skip-if(B2G) tests that are in the manifest now.
> I'm not able to test B2G and the bugs mentioned in this regard aren't clear
> why some tests are skipped and others are not.

I wouldn't worry about these for the purposes of this patch. If any of the new tests fail on B2G, we can find out in a TryServer run and annotate them appropriately.

(Incidentally, though, our "hg blame" web interface is generally the best way to investigate why a given reftest is annotated in a certain way -- it'll tell you the commit that marked it that way.  View the file in MXR, and click the "hg blame" link at upper-right. That points to https://hg.mozilla.org/mozilla-central/rev/d932f2172ce2 , which marked *tons* of tests tree-wide as "skip-if(B2G)" as part of getting reftests working at all on B2G.  Bug 773482 apparently tracks the process of investigating & cleaning up these B2G failures. Anyway -- that's not super-helpful for figuring out what these failures look like & what was causing them -- but at first glance, it looks like the failures were for tests with dynamic modifications, plus block-overflow-3.html and  block-overflow-4.html. It's also possible that these tests pass on B2G now, and we just haven't noticed yet.)

Comment 60

[Daniel Holbert [:dholbert]]

Comment on attachment 723548 [details] [diff] [review]
Wave 5b, rev 2 (for review)

Haven't gotten through the tests yet, but here are my notes on the reftest.list tweaks:

>+++ b/layout/reftests/margin-collapsing/reftest.list
>+# "When two or more margins collapse,
>+#  the resulting margin width is the maximum of the collapsing margins' widths.
>+# The margins of two in-flow siblings should collapse.
>+# These tests feature margins of different or equal sizes on each box.
>+# The norefs depict incorrect results where the margins did not collapse.
> == block-sibling-1a.html block-sibling-1-ref.html

There's a missing closing-quotation mark, for the spec-quote there.

>+# These tests check, whether margins are correctly collapsed even when the

remove comma after "check"

>+# The first-child series tests cases where the top margin of a box collapses
>+# with the top margin of its parent element.
>+# This series is more extensive than the non-sibling series, because
>+# various combinations of positive and negative margins are tested.
>+# The norefs especially try to depic cases where the combination
>+# of positive and negative margins are incorrectly calulated.

s/depic/depict/

>+# Two basic tests for margins of floats and block siblings/descendants.

Drop the word "Two" -- it's not really correct, and not future-proof besides. :)  You've technically got four tests (1a, 1b, 2a, 2b), and even more if you include all of the various references / notreference combinations that you're checking. (And when someone adds more tests here in the future, we don't want them to have to remember to update this count in the comment.)

>+# Basic tests for margins of boxes, that have the overflow porperty applied
>+# (block formatting context) and their block siblings/descendants.

s/porperty/property/
s/(block formatting context)/(hence forming block formatting contexts)/ or something like that

>+# The margins of a table-caption do not collapse with the margins
>+# of the table wrapper box. They can collapse with the margins of other
>+# table-captions, though, if they share the same caption-side.
>+# Multiple captions are currently not implemented, see Bug 144517

This comment seems to imply that we're testing multiple-caption margin-collapsing, up until the last sentence where we say we don't have that feature implemented. So it ends up being a bit confusing as to whether we're trying to test it.

To clarify this, add a line just before that last line saying:
  # We don't test the multiple-caption behavior here, though, because
(and that'll be followed by the existing text "...multiple captions are currently not implemented, see Bug 144517")

>+# Bug 451791 is hard to perceive in the block-clear-1 tests.

Remove the "hard to perceive" line -- that sounds like there's some visually-imperceptible fuzziness on some antialiased pixels or something (we have a lot of other *really* "hard to perceive" tests like that), but really it looks like this whole testcase is rendered 1px-lower-down-the-page with respect to the reference case. (on my machine at least) It's quite easy to perceive, if you open them in two tabs and switch back and forth. :)

Comment 61

[Daniel Holbert [:dholbert]]

Comment on attachment 723548 [details] [diff] [review]
Wave 5b, rev 2 (for review)

So -- following up on the usefulness of noref & multiple reference-cases thing that I brought up in earlier comments -- so from your reftest.list-commenting, it looks like the explanation for the noref files is generally something like this:
>+# The norefs illustrate incorrectly collapsed or non-collapsing margins.
[...]
>+!= block-non-sibling-1a.html block-non-sibling-1-noref1.html
>+!= block-non-sibling-1a.html block-non-sibling-1-noref2.html
>+!= block-non-sibling-1a.html block-non-sibling-1-noref3.html
[same for 1b, 1c, etc.]

These sorts of illustrative "noref" files could definitely be useful for educational / explanatory purposes, e.g. for teaching someone how margins work.

However, from a *testsuite* perspective, I think the cost of these "noref" files outweighs their value.  We're already checking whether the test matches its reference. If it doesn't match, it'll *already* be flagged as a failure, and we'll back out whatever commit broke it. We generally don't really get any benefit from *also* checking it against an alternate formulation of the reference case, or checking it against some bonus known-incorrect renderings.

For a one-off test here and there, this wouldn't be a huge deal (though it'd still probably be redundant).  In this case, though, I really feel like we should avoid it, because this patch is adding *so many* of these redundant checks.

Here are some metrics for layout/reftests/margin-collapsing/reftest.list, after this patch is applied:

 Total number of non-comment lines:
  $ cat reftest.list | grep "^[^#]" | wc -l
  820
 Number of lines involving a "noref" test:
  $ cat reftest.list | grep "^[^#]" | grep noref | wc -l
  371
 Number of lines involving a secondary reference case:
  $ cat reftest.list | grep "^[^#]" | grep ref2 | wc -l
  273

That adds up to 644 lines -- nearly 80% of the checks in this manifest -- that are arguably redundant.

(Before the test is applied, the results of those commands are 410, 124, and 102.  So this is already a bit of a problem -- there are already 226 out of 410 arguably-redundant checks in this directory -- but this patch nearly triples that.

I'm bringing this up because there's a nonzero cost to each line in reftest.list.  These tests have to all be run on on *every* commit, for *every* platform that we support, from now until forever. So while we want to add tests, I think we need to be very careful not to add otherwise-unnecessary tests just for illustration purposes (at least, not in a large-scale way).

I feel bad saying this, because you clearly put a lot of time into these tests and they generally look great, if I suppress this anti-redundancy-instinct. :) But I do think we should probably remove the added ref2/notref files in this patch, except in any situations where there are *really* good reasons for keeping them. (e.g. in cases where it's impossible to make a reference case, but we can provide a few known-bad noref cases)

Comment 62

[Daniel Holbert [:dholbert]]

(Also: you should follow the steps at https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F to get your name into your patches' heders, to be sure your patches get attributed to you.)

Comment 63

[Daniel Holbert [:dholbert]]

I pushed the latest patch to TryServer, too, to give it some cross-platform testing:
 https://tbpl.mozilla.org/?tree=Try&rev=e40ec0fa502b

Comment 64

[Daniel.S]

(In reply to Daniel Holbert [:dholbert] from comment #60)
Sorry for all the mistakes.

> >+# The margins of a table-caption do not collapse with the margins
> >+# of the table wrapper box. They can collapse with the margins of other
> >+# table-captions, though, if they share the same caption-side.
> >+# Multiple captions are currently not implemented, see Bug 144517
> 
> This comment seems to imply that we're testing multiple-caption
> margin-collapsing, up until the last sentence where we say we don't have
> that feature implemented. So it ends up being a bit confusing as to whether
> we're trying to test it.
> 
> To clarify this, add a line just before that last line saying:
>   # We don't test the multiple-caption behavior here, though, because
> (and that'll be followed by the existing text "...multiple captions are
> currently not implemented, see Bug 144517")

Actually, there are tests for those, they just fail.
Not implemented is the actual rendering of multiple captions.
I think I'll just remove that last line.

> >+# Bug 451791 is hard to perceive in the block-clear-1 tests.
> 
> Remove the "hard to perceive" line -- that sounds like there's some
> visually-imperceptible fuzziness on some antialiased pixels or something (we
> have a lot of other *really* "hard to perceive" tests like that), but really
> it looks like this whole testcase is rendered 1px-lower-down-the-page with
> respect to the reference case. (on my machine at least) It's quite easy to
> perceive, if you open them in two tabs and switch back and forth. :)

Done. I wasn't sure if I should or should not add the line, so I've added it just to be sure :)

(In reply to Daniel Holbert [:dholbert] from comment #61)
> [...] We generally don't really get any benefit from *also* checking
> it against an alternate formulation of the reference case,
> or checking it against some bonus known-incorrect renderings.
> 
> For a one-off test here and there, this wouldn't be a huge deal (though it'd
> still probably be redundant).  In this case, though, I really feel like we
> should avoid it, because this patch is adding *so many* of these redundant
> checks.

Just to make sure I understand you correctly. Do you say noref cases are usually not necessary and could be left out? It should be easy to get rid of them.
If necessary, I could also comment out the existing ones (after looking through them).

> I feel bad saying this, because you clearly put a lot of time into these
> tests and they generally look great, if I suppress this
> anti-redundancy-instinct. :) But I do think we should probably remove the
> added ref2/notref files in this patch, except in any situations where there
> are *really* good reasons for keeping them. (e.g. in cases where it's
> impossible to make a reference case, but we can provide a few known-bad
> noref cases)

Please don't worry. I don't have a strong opinion on them. My primary goal is to have the tests checked in so roc can rewrite and fix margin-collapsing. Haha.

I will just need some time to look through the references to choose which ones are best to sort out.

(In reply to Daniel Holbert [:dholbert] from comment #62)
> (Also: you should follow the steps at
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F to get your name into your patches' heders, to be sure your
> patches get attributed to you.)

Done - I think.

Comment 65

[Daniel Holbert [:dholbert]]

(In reply to Daniel.S (mostly offline during the week) from comment #64)
> (In reply to Daniel Holbert [:dholbert] from comment #60)
> Sorry for all the mistakes.

No worries!

> Actually, there are tests for those, they just fail.
> Not implemented is the actual rendering of multiple captions.
> I think I'll just remove that last line.

Ah -- I see -- those are the "caption-sibling" tests?  You could maybe just tweak the multiple-caption-not-implemented line to say
  # Those tests (caption-sibling-*) currently fail, because we don't
  # yet support multiple captions (bug NNN).

Or you could drop it and let anyone who cares just see the "fails"-annotated lines; either way.

> (In reply to Daniel Holbert [:dholbert] from comment #61)
> > [...] We generally don't really get any benefit from *also* checking
> > it against an alternate formulation of the reference case,
> > or checking it against some bonus known-incorrect renderings.
> > 
> > For a one-off test here and there, this wouldn't be a huge deal (though it'd
> > still probably be redundant).  In this case, though, I really feel like we
> > should avoid it, because this patch is adding *so many* of these redundant
> > checks.
> 
> Just to make sure I understand you correctly. Do you say noref cases are
> usually not necessary and could be left out? It should be easy to get rid of
> them.

Yes. The "-notref" files and their corresponding lines in reftest.list are (in principle) unnecessary, when we've already got a functioning reference case.  So, they should be removed.

(That's the general rule, at least. I could imagine there being rare situations where they might add value, but those would be exceptions.)

Same goes for the "-ref2" references (though I guess you'd want to pick between ref1 and ref2 -- whichever is simpler/cleaner -- and rename whichever one you're keeping to just -ref.html)

> If necessary, I could also comment out the existing ones (after looking
> through them).

That's less important, since those are already in and there aren't as many of them.  But that'd be nice to do -- maybe in a followup bug, to keep this bug from getting too complex. 

(Also: It'd be better to remove them than to comment them out.)

> Please don't worry. I don't have a strong opinion on them. My primary goal
> is to have the tests checked in so roc can rewrite and fix
> margin-collapsing. Haha.

Phew, thanks for being such a good sport! :)

> I will just need some time to look through the references to choose which
> ones are best to sort out.

Sounds good.

Comment 66

[Daniel Holbert [:dholbert]]

Comment on attachment 723548 [details] [diff] [review]
Wave 5b, rev 2 (for review)

[Marking as feedback+/review- after my last wave of review comments, so that bugzilla will stop nagging me about having a pending review request here.]

Comment 67

[Daniel.S]

Attachment: Wave 5b, rev 3 (for review)

(In reply to Daniel Holbert [:dholbert] from comment #66)
> [Marking as feedback+/review- after my last wave of review comments, so that
> bugzilla will stop nagging me about having a pending review request here.]

I should've cleared the request, sorry.
Here's my apology: A new patch ;)

It includes all your comments so far.

(In reply to Daniel Holbert [:dholbert] from comment #65)
> Or you could drop it and let anyone who cares just see the "fails"-annotated
> lines; either way.

I decided for this one.

> (That's the general rule, at least. I could imagine there being rare
> situations where they might add value, but those would be exceptions.)

I have removed both ref2s and norefs. Considering your point of view they didn't seem to add any value to the test suite. The references I used were all ref2s. They are less complex and should not depend on the margin property (expcept for margin:0;).

Now, I'll cross my fingers.

Comment 68

[Daniel Holbert [:dholbert]]

(In reply to Daniel.S (mostly offline during the week) from comment #67)
> I should've cleared the request, sorry.
> Here's my apology: A new patch ;)

No worries! You didn't need to clear the request; that's on me. Bugzilla has just gotten naggier recently (which is mostly a good thing), so I cleared it to quiet it down. :)

Comment 69

[Daniel Holbert [:dholbert]]

Comment on attachment 739268 [details] [diff] [review]
Wave 5b, rev 3 (for review)

Thanks for addressing all of that!  The tests generally look good -- comments below -- mostly consistency / stylistic things, with just a few actual test-bugs.

(Note: Parts of this may seem nitpicky, since some of these issues are things that reviewers might miss or not bother mentioning in a single one-off test.  But in this case, since these affect many new test files, I think it's worth correcting them things before landing, rather than introducing lots of tiny issues at once.)

>+++ b/layout/reftests/margin-collapsing/block-auto-height-last-child-1-ref.html
>@@ -0,0 +1,19 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<style type="text/css">
>+#parent, #separator {
>+ height: 20px;
>+ background-color: green;
>+}
>+#last-child {
>+ height: 20px;
>+}
>+</style>
>+</head>
>+<body>
>+<div id="parent"></div>
>+<div id="last-child"></div>
>+<div id="separator"></div>

The structure of this reference case is odd, as compared to the corresponding testcases. In particular, "parent" is not a parent here, and it's got a colorful background when it didn't in the testcases. (and "last-child" *doesn't* have a colorful background, while it *does* in the testcases)

Basically: the reference uses the same element-IDs as its testcases, but the roles and coloring are mysteriously swapped around here.

The following would make more sense to me, in this reference case (and would render the same).  It'd also be closer to what you've got in the next reference case, for the "-2" version of this reftest):
---
<style type="text/css">
#last-child, #separator {
 height: 20px;
 background-color: green;
}
#parent {
 height: 40px;
}
</style>
</head>
<body>
<div id="parent">
 <div id="last-child"></div>
</div>
<div id="separator"></div>
---

This would make it identical in structure to the testcases -- just using "height" in place of the testcase's "margin-bottom" (on the 'parent' element).

ALTERNATELY: if you want to keep the existing structure, then that's fine, but at least tweak the IDs a bit. e.g. make "last-child" the colorful thing (to match the testcase), and insert a div after it called "spacer"?  (And just drop "parent" altogether in this file.)  So you'd still have 3 divs, but they'd be "last-child[green]","spacer","separator[green]" instead of "parent[green]","last-child","separator[green]".  Then things would be more analogous to the testcase.

>+++ b/layout/reftests/margin-collapsing/block-auto-height-last-child-3-dyn.html
>+<style type="text/css">
>+#last-child, #separator {
>+ height: 20px;
>+ background-color: green;
>+}
[...]
>+#last-child {
>+ margin-bottom: 20px;
>+ background-color: blue;
>+}

nit: You've got #last-child's background-color defined twice, to two different colors (green vs blue). This is confusing & unnecessary, and it's probably worth avoiding.

It's be better to just have separate declarations for #separator and #last-child. So: you could replace "#last-child, #separator {" with "#separator {", and then add "height: 20px;" to the existing "#last-child {" rule. Or something like this.

Looks like this issue applies to a lot of the test files. :( In particular, this applies to:
 - block-auto-height-last-child-3*
 - block-auto-height-last-child-4* (but not -ref)
 - block-auto-height-last-child-6*
 - block-auto-height-last-child-7* (but not -ref)
 - block-auto-height-last-child-8* (but not -ref)
 - block-first-child-3*
 - block-first-child-4* (but not -ref)
 - block-first-child-6*
 - block-first-child-7* (but not -ref)
 - block-first-child-8*

(When fixing this, ideally it'd be great if you could also double-check that the reference-case's styles end up in approximately the same order as the testcase's styles. This particulary applies to the ones labeled "but not -ref" above, since their reference case's styles are already a bit rearranged w.r.t. the testcase, and they'll likely be more similar after the testcases are tweaked to only set the child color once.)

>+++ b/layout/reftests/margin-collapsing/block-first-child-1-ref.html
>@@ -0,0 +1,18 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<style type="text/css">
>+#separator, #parent, #first-child {
>+ height: 20px;
>+}
>+#separator, #first-child {
>+ background-color: green;
>+}
>+</style>
>+</head>
>+<body>
>+<div id="separator"></div>
>+<div id="parent"></div>
>+<div id="first-child"></div>
>+</body>
>+</html>

This has a similar issue to the first chunk that I quoted in this comment -- 'parent' isn't actually a parent, because you moved first-child out of it, so it's serving a completely different role in the testcase vs. reference case. Maybe just rename "parent" to "spacer" here? I think that would make things clearer.

This applies to:
 - block-first-child-1-ref.html
 - block-first-child-2-ref.html
 - block-first-child-5-ref.html

>+++ b/layout/reftests/margin-collapsing/block-first-child-2-ref.html
>@@ -0,0 +1,19 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<style type="text/css">
>+#separator, #first-child {
>+ background-color: green;
>+ height: 20px;

nit: Those last 2 lines are reversed in the reference case vs. the testcases. Flip 'em.

This applies to just 2 files:
 - block-first-child-2-ref.html
 - block-first-child-5-ref.html

>+++ b/layout/reftests/margin-collapsing/block-html-html-1-ref.html
>@@ -0,0 +1,23 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+ <style type="text/css">
>+ html, body {
>+  background-color: #fff;

"#fff" is out of place here -- replace with "white". (You use color names, not hex codes, everywhere else in this patch, aside from a few stray #fff's.)

(You can easily fix this by e.g. opening the patch file in a text editor and doing a search-and-replace operation.)

>+++ b/layout/reftests/margin-collapsing/block-html-html-1.html
>@@ -0,0 +1,22 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+ <style type="text/css">
>+ html, body, div {
>+  display: block;
>+  margin: 0; padding: 0;
>+ }

Drop the ", div" from this selector -- there are no <div>s at all in this file.

This applies to:
 - block-html-html-1.html
 - block-xhtml-html-1.xhtml

ALSO: the "display:block" is unnecessary. That's already the default value of display, for html body and div. Drop that line.
That applies to:
 - block-html-html-*
 - block-xhtml-html-*

(The "margin: 0; padding: 0;" is also technically unnecessary, too, at least in this file, so you could even drop this rule entirely, in some this file & some of its related files.  But I'm fine with leaving the margin/padding bit it, since it seems to be present in most of the related test files and it serves a purpose in at least some of them.)

>+++ b/layout/reftests/margin-collapsing/block-html-html-2-dyn.html
>@@ -0,0 +1,30 @@
>+<!DOCTYPE html>
>+<html class="reftest-wait">
[...]
>+ div {
>+  background-color: green;
>+  margin: 10px 0;
>+  height: 100px;
>+ }
>+ </style>
>+ <script type="text/javascript">
>+ function test() {
>+  document.getElementsByTagName('div')[0].style.display = 'block';
>+  document.documentElement.removeAttribute('class');

This testcase needs "display: none" in the "div {...}" rule.  Without that, it's not actually testing any dynamic changes!

This applies to:
 - block-html-html-2-dyn.html
 - block-html-html-4-dyn.html
 - block-xhtml-html-2-dyn.xhtml
 - block-xhtml-html-4-dyn.xhtml

Also: This testcase (numbered "2") is compared against the "-1" reference case, as shown from this snippet from reftest.list:
 +== block-html-html-1.html block-html-html-1-ref.html
 +== block-html-html-2.html block-html-html-1-ref.html
 [...]
 +== block-html-html-1-dyn.html block-html-html-1-ref.html
 +== block-html-html-2-dyn.html block-html-html-1-ref.html

That technically means these tests would be better-named "-1a" and "-1b" instead of "-1" and "-2". (and then the later tests renamed to fill in the missing "2" gap)
(This applies to the "xhtml" versions of these tests, too.)

If you don't have time to get to this part, though, don't worry about it -- it's not the end of the world if we have a "-2" compared against "-1-ref".

>+++ b/layout/reftests/margin-collapsing/block-non-sibling-1a-dyn.html
>+#a div {
>+ display: none;
[...]
>+<script type="text/javascript">
>+function test() {
>+ document.getElementsByTagName('div')[1].style.display = 'block';
[...]
>+<div id="a">
>+ <div></div>

In this file and the other block-non-sibling-*-dyn tests, the expressions
 document.getElementsByTagName('div')[1]
 document.getElementsByTagName('div')[2]
 document.getElementsByTagName('div')[3]
are a bit fragile & hard to read.  We've got IDs that we can use instead, so we should just use those.

So -- I'd prefer that we replace these with the following:
 document.getElementById('a').firstChild
 document.getElementById('b').firstChild
which are are less fragile and much more human-readable.

(You can probably do search-and-replace in the patch file to fix this.. I suspect the following string-replacements will suffice:
  s/sByTagName('div')[1]/ById('a').firstChild/
  s/sByTagName('div')[2]/ById('b').firstChild/
  s/sByTagName('div')[3]/ById('b').firstChild/
)

(This applies to all of the block-non-sibling-*-dyn.html tests.)

I think this will be r=me with those things addressed, but since that's a lot to change, I'll mark it feedback+ for now and take a glance at the final patch before granting r+.  Thanks!

Comment 70

[Daniel.S]

(In reply to Daniel Holbert [:dholbert] from comment #69)
> So -- I'd prefer that we replace these with the following:
>  document.getElementById('a').firstChild
>  document.getElementById('b').firstChild
> which are are less fragile and much more human-readable.

The first child of #a is a text node, so this won't work w/o changing the code.
Do you want me to introduce a #hidden for the -dyn cases instead?

Thanks so far. I should be able to address all your "nitpicky" :)

Comment 71

[Daniel Holbert [:dholbert]]

d'oh! ok, never mind about that part then.

Thanks!

Comment 72

[Zack Weinberg (:zwol)]

What remains to be done for this bug?

Comment 73

[Daniel.S]

(In reply to Zack Weinberg (:zwol) from comment #72)
> What remains to be done for this bug?

At the moment I have a big to-do list from :dholbert. I'm working on it, but I had to work on another project the last few months, so there hasn't been any progress since last time.

I know it's unfortunate that this is taking me so long. On the other hand, I don't think time is a critical factor for this bug and all of it dependencies, is it?

Comment 74

[Zack Weinberg (:zwol)]

These bugs have been around for a very long time, I think we can live with them a bit longer. :)

The todo list you refer to, is that just comment 69, or is there more?  I'm trying to figure out whether other people can usefully help.

Comment 75

[Daniel.S]

(In reply to Zack Weinberg (:zwol) from comment #74)
> The todo list you refer to, is that just comment 69, or is there more?  I'm
> trying to figure out whether other people can usefully help.

Well it's comment 69 and comment 69 again for 7-8 other batches of tests, since most of the tests have already been written.
Most tests require checking and removing their duplicate references (there are a lot of those). Some tests (like those for quirks mode) would have to be checked against latest specs and latest browser versions.

The current set of tests (wave 5) only needs a little less than the work named in comment 69. This is just a bit more difficult, because I have to make the changes in the .diff file. But that doesn't apply to all the other tests.

When I'm done with Wave 5 - and that's what I have planned for the following two months - I could upload the whole set of tests, so that other's may have a look at the "raw material".

Comment 76

[Daniel.S]

Attachment: Wave 5b, rev 4 (for review) [Checkin: comment 81]

(In reply to Daniel Holbert [:dholbert] from comment #69)
> The following would make more sense to me, in this reference case (and would
> render the same).  It'd also be closer to what you've got in the next
> reference case, for the "-2" version of this reftest):

Switched to your suggestion.


> >+++ b/layout/reftests/margin-collapsing/block-auto-height-last-child-3-dyn.html
> nit: You've got #last-child's background-color defined twice, to two
> different colors (green vs blue). This is confusing & unnecessary, and it's
> probably worth avoiding.

Fixed. I think I got just what you wanted.


> >+++ b/layout/reftests/margin-collapsing/block-first-child-1-ref.html
> This has a similar issue to the first chunk that I quoted in this comment --
> 'parent' isn't actually a parent, because you moved first-child out of it,
> so it's serving a completely different role in the testcase vs. reference
> case. Maybe just rename "parent" to "spacer" here? I think that would make
> things clearer.

Renamed parent to spacer. I think this is the best choice regarding these tests.


> nit: Those last 2 lines are reversed in the reference case vs. the
> testcases. Flip 'em.

Fixed.

> "#fff" is out of place here -- replace with "white". (You use color names,
> not hex codes, everywhere else in this patch, aside from a few stray #fff's.)

Fixed.

> Drop the ", div" from this selector -- there are no <div>s at all in this
> file.

Fixed.


> ALSO: the "display:block" is unnecessary. That's already the default value
> of display, for html body and div. Drop that line.
> That applies to:
>  - block-html-html-*
>  - block-xhtml-html-*

Dropped.

> (The "margin: 0; padding: 0;" is also technically unnecessary, too, at least
> in this file, so you could even drop this rule entirely, in some this file &
> some of its related files.  But I'm fine with leaving the margin/padding bit
> it, since it seems to be present in most of the related test files and it
> serves a purpose in at least some of them.)

I kept this line because at least body has a default margin of 8px. I removed some unnecessary div selectors though.


> >+++ b/layout/reftests/margin-collapsing/block-html-html-2-dyn.html
> This testcase needs "display: none" in the "div {...}" rule.  Without that,
> it's not actually testing any dynamic changes!

Fixed.

> Also: This testcase (numbered "2") is compared against the "-1" reference
> case, as shown from this snippet from reftest.list: [...]
> 
> If you don't have time to get to this part, though, don't worry about it --
> it's not the end of the world if we have a "-2" compared against "-1-ref".

You dare to say? It's fixed :)
The problem also appeared in block-xhtml-root-*


I think this one's done. Please have a look.

Comment 77

[Daniel Holbert [:dholbert]]

Comment on attachment 795030 [details] [diff] [review]
Wave 5b, rev 4 (for review) [Checkin: comment 81]

Looks great!  Thanks for addressing all of that!

One final header-related nit: So your patch starts out with this:
># User Daniel Schattenkirchner <[e-mail]>
>
>Bug 477462 - margin-collapsing test suite, wave 5b; root, html, body margins and non-sibling margins; r=REVIEWER

...which *looks* right, *but*: when I import it with "hg qimport", mercurial thinks the "# User" line is the commit message. (It doesn't recognize that that's the author line)

I believe that's because you're missing one header line -- please add the following single line at the top of your patch:
# HG changeset patch

(at least, that's what my patches all have, and that fixes your patch so that I can import it correctly.

I'm not sure why your patch is missing that (or what you need to fix to get it for future patches) -- maybe double-check that you've followed the steps here:
 https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

But anyway -- r=me, and I'll push this to Try server to test it. (I verified that the tests pass on my local machine.)

Comment 78

[Daniel Holbert [:dholbert]]

Try run with Wave 5b rev 4: https://tbpl.mozilla.org/?tree=Try&rev=9ff9d0b3829f

Comment 79

[Daniel Holbert [:dholbert]]

Try run looks good! Rather than make you address the nit in comment 77, I just went ahead and added the "#HG changeset patch" line & updated the commit message to note r=dholbert & landed:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/47068f470e9b

(For the benefit of those importing & landing your future patches, it'd be nice to see if you can figure out how to get that HG header auto-included.)

Thanks!

Comment 80

[Daniel Holbert [:dholbert]]

(I just realized that the Try run didn't include Android reftests in the try run, since they require a different Try invocation. Hopefully that doesn't matter; I guess we'll see, now that this landed on inbound. :))

Comment 81

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/47068f470e9b

Comment 82

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Have we looked at upstreaming these tests to the W3C?

Comment 83

[Daniel.S]

(In reply to :Ms2ger from comment #82)
> Have we looked at upstreaming these tests to the W3C?

No. At least, I did not.

There are a few cases which are wrong in other browsers and I've reported these issues (and test cases) separately.

Adding the tests of this bug to the CSS 2.1 Test Suite would require a lot of work, e.g. adding comments and explanations to each test. Also, because the CSS 2.1 Test Suite is not based on reftests, most tests probably would have to be rewritten, so that pass or fail could be perceived instantly.


(In reply to Daniel Holbert [:dholbert] (vacation through 8/31) from comment #77)
> Looks great!  Thanks for addressing all of that!

Yay! Thanks for fixing the last few nits.

> I believe that's because you're missing one header line -- please add the
> following single line at the top of your patch:
> # HG changeset patch

Sorry. My environment has always had some problems. I'll try to look into the issue.

Comment 84

[Daniel.S]

Attachment: Wave 6 (for review)

Since I had some free time, I've also prepared a couple of new tests.

This set is all about the bottom margins of a parent and its last child. These margins are tricky, because you have to take CSS 2.1 §10.7 (Minimum and maximum heights) [1] into account. But there's actually only one rule to remember: If (and only if) the parents' height is 'auto' then the two margins collapse.

I've also updated the date of the errata, but there were no new changes related to margin collapsing.

[1] http://www.w3.org/TR/2011/REC-CSS2-20110607/visudet.html#min-max-heights

Comment 85

[Daniel Holbert [:dholbert]]

(Just a heads-up: I probably won't be able to get to this for a week or so -- I'm on vacation and will have a good bit of catching up to do after I'm back.)

Comment 86

[Daniel Holbert [:dholbert]]

Comment on attachment 796814 [details] [diff] [review]
Wave 6 (for review)

>diff --git a/layout/reftests/margin-collapsing/block-fix-height-last-child-4a.html b/layout/reftests/margin-collapsing/block-fix-height-last-child-4a.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/margin-collapsing/block-fix-height-last-child-4a.html
>+#parent {
>+ height: 100px;
>+ min-height: 50px;
>+ background-color: green;
>+}
>+#last-child {
>+ height: 10px;
>+ margin-bottom: 20px;
>+}

In block-fix-height-last-child-1* through -3*, you have a lightgreen #parent and a green #last-child.

But in block-fix-height-last-child-4*, you change it to a _green #parent_, and _no color at all_ on #last-child. That seems like an arbitrary change, and I think it's quite useful to be able to see the boundaries of #last-child (as you can in your -1* through -3* tests, but can't in -4* because it has no background).

So: I'd prefer that you changed the -4* tests to use the same background scheme that you use in -1* through -3*.  i.e. make #parent have background-color: lightgreen (instead of green), and make #last-child have background-color: green (instead of no background) in your block-fix-height-last-child-4* tests.  (And add a #last-child to your reference case, like in your reference case for the earlier tests)

Some quick local testing on my machine says that these tests will still pass, after that change.

More comments coming...

Comment 87

[Daniel Holbert [:dholbert]]

Comment on attachment 796814 [details] [diff] [review]
Wave 6 (for review)

>diff --git a/layout/reftests/margin-collapsing/block-min-height-last-child-1-ref.html b/layout/reftests/margin-collapsing/block-min-height-last-child-1-ref.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/margin-collapsing/block-min-height-last-child-1-ref.html
>@@ -0,0 +1,29 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<style type="text/css">
>+#parent {
>+ min-height: 100px;

In the reference cases where "min-height" makes a difference, it'd be a little more readable (and guaranteed-to-work-the-same-everywhere) if you just use "height" instead of "min-height".

That applies to:
 block-min-height-last-child-1-ref.html
 block-min-height-last-child-2-ref.html
 block-min-height-last-child-3-ref.html

>diff --git a/layout/reftests/margin-collapsing/block-min-height-last-child-4-ref.html b/layout/reftests/margin-collapsing/block-min-height-last-child-4-ref.html
>+#parent {
>+ min-height: 100px;
>+ background-color: lightgreen;
>+}
>+#last-child {
>+ height: 100px;

In this and the subsequent reference cases, "min-height" on the parent has no effect, since the child is already at least as tall as the min-height.  So, just drop the "min-height" declaration.

This applies to:
 block-min-height-last-child-4-ref.html
 block-min-height-last-child-5-ref.html
 block-min-height-last-child-7-ref.html
 block-min-height-last-child-8-ref.html

(not #6 or #9 -- looks like you already removed it from those, which is good.)

>+++ b/layout/reftests/margin-collapsing/reftest.list
>+# Note: The block-auto-height-last-child series automatically covers
>+# all cases where 'min-height' is '0' and 'max-height' is 'none' as
>+# these are the default values of those properties.
>+# However, there's the older block-zero-min-height series which explicitly
>+# adds min-height: 0; This should have no effect on margin-collapsing.
>+== block-zero-min-height-1a.html block-zero-min-height-1-ref.html

Could you do this "block-zero-min-height" chunk-moving in a separate patch?  When patches get this big, it's nice to make them as modular as possible, and split off logically-separate chunks into their own patch (especially if they're non-functional changes). (And in this case, "add new tests" vs. "reorder existing tests in reftest.list" are logically separate, and functional (introducing new tests to be run) vs. essentially non-functional (reordering existing tests)).

This makes it easier for hg-blame archeologists in the future to see what's changing & why.

(Feel free to layer the reordering-patch either before or after the main patch -- whatever's easier for you / makes more sense to you.)

>+# According to CSS 2.1 §10.7 (Minimum and maximum heights) this is true,
>+# even if the decendants height is bigger than the defined max-height.
>+== block-max-height-last-child-1a.html block-max-height-last-child-1-ref.html

Thank you for the excellent explanations of what you're testing!

Just one typo nit on this chunk: s/decendants/descendant's/  (That's 2 changes: add an "s" before the "c", and also add an apostrophe)

r=me with the above & previous comment (comment 86) addressed.

Comment 88

[Daniel.S]

Attachment: Wave 6, rev 1 (for checkin) [checkin: comment 91]

(In reply to Daniel Holbert [:dholbert] from comment #86)
> So: I'd prefer that you changed the -4* tests to use the same background
> scheme that you use in -1* through -3*.  i.e. make #parent have
> background-color: lightgreen (instead of green), and make #last-child have
> background-color: green (instead of no background) in your
> block-fix-height-last-child-4* tests.  (And add a #last-child to your
> reference case, like in your reference case for the earlier tests)

Fixed.

(In reply to Daniel Holbert [:dholbert] from comment #87)
> In the reference cases where "min-height" makes a difference, it'd be a
> little more readable (and guaranteed-to-work-the-same-everywhere) if you
> just use "height" instead of "min-height".

Fixed.

> In this and the subsequent reference cases, "min-height" on the parent has
> no effect, since the child is already at least as tall as the min-height. 
> So, just drop the "min-height" declaration.

Fixed. I think I've started this, but forgot to finish.

> (Feel free to layer the reordering-patch either before or after the main
> patch -- whatever's easier for you / makes more sense to you.)

I think I'll do some more re-arranging soon, so I have left this part out for now. If it desn't make more sense to split re-arranging in several patches, I'd like to do them all at once.

> Just one typo nit on this chunk: s/decendants/descendant's/  (That's 2
> changes: add an "s" before the "c", and also add an apostrophe)

Ew.. Fixed.

> r=me with the above & previous comment (comment 86) addressed.

I hope it's ok to carry over your r+. I'm unsure though, whether I should add the checkin-needed keyword, because I don't know if that includes try server testing.

Comment 89

[Daniel Holbert [:dholbert]]

(In reply to Daniel.S from comment #88)
> Created attachment 801101 [details] [diff] [review]
> Wave 6, rev 1 (for checkin)
> > r=me with the above & previous comment (comment 86) addressed.
> 
> I hope it's ok to carry over your r+.

Absolutely! (You could replace r=REVIEWER with r=dholbert in the commit message, too, if you like; but whoever ends up landing it will probably know to do that, too.)

> I'm unsure though, whether I should
> add the checkin-needed keyword, because I don't know if that includes try
> server testing.

Thanks for your caution on that -- I didn't run it through Try yet. I'll do that now.
 https://tbpl.mozilla.org/?tree=Try&rev=b9efe473a15a

Comment 90

[Daniel.S]

(In reply to Daniel Holbert [:dholbert] from comment #89)
> Absolutely! (You could replace r=REVIEWER with r=dholbert in the commit
> message, too, if you like; but whoever ends up landing it will probably know
> to do that, too.)

I thought I had changed that line already, that's why I asked. I wasn't sure if it makes any sense to add a simple r+ while attaching the patch.

> Thanks for your caution on that -- I didn't run it through Try yet. I'll do
> that now.
>  https://tbpl.mozilla.org/?tree=Try&rev=b9efe473a15a

Looks good. :)

Comment 91

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd8abf7ff87f

Comment 92

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dd8abf7ff87f

Comment 93

[Daniel.S]

Attachment: Wave 7 (for review)

With this patch I'd like to improve some of the existing tests.

I have added better ID names, changed some colors and re-arranged some styles. I've also rewritten a few tests because some of them didn't test anything at all (sorry).

Among the issues fixed are:
* Made a comment about absolute positioning more correct.
* Removed the margins in block-overflow-3-ref and -4-ref that were mistakenly added in bug 665597.
* Made table-caption tests actually test margin-collapsing
* Some tests for inline-blocks and fieldsets were testing both vertical and horizontal margins. I have removed the horizontal bits because the inline-block margins are already tested and for block elements they are are better placed in a separate test.
* The table-caption-2 series should've been part of the 1 series as well because they share the same reference (visually). In order to make a (weak) argument for having both -1 and -2 I simplified the -1-ref and kept the -2-ref intact (i.e. it still uses margins). So the two series differ at least by the reference code they're compared with.
* Unfortunately, the same is true for table-caption-bottom/top-1/2. The -1 tests didn't really test for margin-collapsing and if I change them (like I did) they share the same reference with -2.

Note: The lenghts in inline-block-sibling-1-ref have been changed to em by bug 518357.

I hope this won't cause too much trouble. I just wanted to do this because I will add dynamic and new tests to some of the existing series.

Comment 94

[Daniel Holbert [:dholbert]]

Would it be possible to file a new bug for this patch & subsequent patches? (bug title e.g. "improve some of the tests from bug 477462")

Bugs get a bit unmanageable when they have ~100 comments and patches that are spread out across many months / releases.

Comment 95

[Daniel.S]

Comment on attachment 811545 [details] [diff] [review]
Wave 7 (for review)

(In reply to Daniel Holbert [:dholbert] from comment #94)
> Would it be possible to file a new bug for this patch & subsequent patches?
> (bug title e.g. "improve some of the tests from bug 477462")

Sure, please see bug 921761.

> Bugs get a bit unmanageable when they have ~100 comments and patches that
> are spread out across many months / releases.

I can see the issue.

However, please leave this bug open.
There are a lot of dependencies and I don't want to wake sleeping dogs before I'm done with the tests I still have to get in. Thanks.

Comment 96

[Daniel Holbert [:dholbert]]

(In reply to Daniel.S from comment #95)
> However, please leave this bug open.
> There are a lot of dependencies and I don't want to wake sleeping dogs
> before I'm done with the tests I still have to get in. Thanks.

Sounds good. (I appreciate the concern for not wanting to let things get lost)

Let's consider this bug a tracking bug at this point, and land (most/all) future patches via helper-bugs that block this one.

(& I'll look into bug 921761 in the next few days - thanks for spinning that off.)

Comment 97

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.