921761 - improve some of the tests from bug 477462, improve their reftest.list and add some new tests

Status: RESOLVED FIXED

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

Description

[Daniel.S]

This bug is an extension to bug 477462 and covers three steps:

1. Improve some of the existing tests
2. Improve the reftest.list of the m-c test suite
3. Add new tests to the test suite

#1 and #2 can be reviewed and landed separately.

After that I'll have to take a break from bug 477462 for a while again.

Comment 1

[Daniel.S]

Attachment: Wave 7 (for review)

Here's the comment for #1, as already posted in bug 477462, comment 93.

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 any thing 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 2

[Daniel.S]

Attachment: improve reftest.list for margin-collapsing tests

Here's the update to reftest.list

I simply moved some tests around and added more comments. The main focus was to group tests of elements/boxes that establish a block formatting context.

Comment 3

[Daniel Holbert [:dholbert]]

Sorry for the delay on this - I'm planning on reviewing this tomorrow.

Comment 4

[Daniel Holbert [:dholbert]]

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

FIRSTLY: the improvements to ID / class names are awesome for readability - thanks for those!

SECONDLY: so, I noticed that a lot of these reftests (e.g. block-abs-pos-1*, inline-block-child*, inline-block-sibling-1*) just use shades of green for coloring. This is a bit undesirable, since an entirely-green rendering can be misinterpreted (by casual human viewers) as a "pass" condition, when the testcase is viewed directly.

In particular, the CSS testsuite guidelines say:
  # In the absence of any red, green indicates success.
http://www.w3.org/Style/CSS/Test/guidelines.html#color:

Now, we don't really adhere to those guidelines when writing reftests, but it's nice to keep that coloring distinction in mind, in case we decide to submit these tests to a w3C testsuite in the future.

So perhaps at some point (definitely doesn't need to be now), it'd be worth doing a search-and-replace to switch away from "green" and "lightgreen" in these tests to some other arbitrary (non-red) colors.  (It looks like the guidelines linked above suggest using Fuchsia, Yellow, Teal, Orange for general-purpose coloring.)

Again, definitely not something that has to happen now; just a heads up.

Anyway - now, on to specifics:

>Bug 921761 - margin-collapsing test suite, wave 7; improving tests by adding readable IDs, re-arranging styles, changing colors; r=REVIEWER

commit message nit: s/re-arranging/rearranging/

>+++ b/layout/reftests/margin-collapsing/block-abs-pos-1-ref.html
>+<!-- Note: CSS 2.1 allows guessing the static position of an absolutely
>+     positioned element whose offsets are 'auto' (initial value), i.e.
>+     the reference of this test is just one of many possible outcomes. -->

Optional: It might be worth linking to the relevant chunk of the spec here, for justification / further-reading. The relevant link would be:
 http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-width

If you add this, probably best to add it in both instances of the comment (testcase + reference case).

>+++ b/layout/reftests/margin-collapsing/block-float-1a-noref.html
>@@ -6,18 +6,18 @@
>  height: 20px;
>  background-color: green;
> }
[...]
> #float {
>  height: 100px; width: 50px;
>- background-color: blue;
>  margin-right: auto;
>+ background-color: green;
> }

I don't think there's any benefit in switching from Green/Blue to Green/Green here (for 2 pieces of content in the testcase) -- and in fact, it's better as Green/Blue.

Tests are easier to understand & debug when different content uses different colors (to make bounds clearer, and to make it easier to immediately tell which part is mismatching when the test fails).

So, I'd rather we don't make the "change blue to green" tweaks to these "block-float-1*" tests.  (The other cleanups to these tests look good, though.)

This also applies to the table-caption-* tests. (they've got similar s/blue/green/ tweaks that I think we should leave as blue, or some distinct color)

>+++ b/layout/reftests/margin-collapsing/block-overflow-5-ref.html
[...]
>+++ b/layout/reftests/margin-collapsing/block-overflow-5-ref2.html
[...]
>+++ b/layout/reftests/margin-collapsing/block-overflow-5c-ref.html
[...]
>+++ b/layout/reftests/margin-collapsing/block-overflow-5c-ref2.html

Observation (probably not to be addressed in this patch):
These four different "block-overflow-5" reference cases are a bit confusing...
  i) As noted elsewhere, multiple identically-rendering reference cases are redundant, so we can (& probably should) drop one out of each "ref,ref2" pair.
 ii) For the "5c" test/references, it'd be clearer to move block-overflow-5c* to its own differently-numbered test, given that it doesn't match the #5 reference case.

Again, no need to fix this in *this* patch, since this patch is just tweaking rather than adding those files.

>+++ b/layout/reftests/margin-collapsing/table-caption-1a.html
>@@ -1,28 +1,32 @@
> <!DOCTYPE html>
> <html>
> <head>
> <style type="text/css">
>-.separator {
>+#block1, #caption, #block2 {
>  height: 20px;
>  background-color: green;
> }
[...]
> #caption {
>  display: table-caption;
>- margin: 30px 0;
>- height: 20px;
>- background-color: blue;
>+ margin: 20px 0;
>+}

In most of the table-caption-* tests, this patch merges some of the #caption style with #block or #block1.

I'd rather not do that merge - I think it's clearer with #caption having its own block of style (so you don't have to jump around the style block to find all of its style-bits), and also as noted above, it's better to keep #caption having its own distinct color (e.g. blue, as it currently stands) rather than making it share green.

>+++ b/layout/reftests/margin-collapsing/table-caption-bottom-1.html
>-.separator {
>+#block1, #caption, #block2 {
>  height: 20px;
>  background-color: green;
> }
>+#block1 {
>+ margin-bottom: 10px;
>+}
[...~10 lines omitted...]
>+#block2 {
>+ margin-top: 10px;
> }

Might be worth bumping that final #block2 rule up to just below the #block1 rule, so all of the #block2 styling is approximately in the same place. It's also fine as-is, though. (Looks like you're trying to match content-order, which I guess makes sense, aside from the fact that the #block2 styling is divided between the very beginning & very end.)

SO: r=me with...
 (a) the commit message nit
 (b) the s/blue/green/ changes reverted and the #caption-rule-merging reverted
and with followups filed for the de-green-ification and for the 5/5c reference-case cleanup.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 811567 [details] [diff] [review]
improve reftest.list for margin-collapsing tests

(In reply to Daniel.S from comment #2)
> I simply moved some tests around and added more comments. The main focus was
> to group tests of elements/boxes that establish a block formatting context.

That would be worth stating at the top of the reftest.list file. Otherwise, it's kinda unclear why we go from "block-" tests to "fieldset-" tests to "table-" tests and then back to "block-" again.

e.g. "This manifest is roughly organized such that tests of elements/boxes that establish block formatting contexts are grouped together." 

> == block-auto-height-last-child-8c-dyn.html block-auto-height-last-child-8-ref.html
>+== block-zero-min-height-1a.html block-zero-min-height-1-ref.html
[...]
>+== block-zero-min-height-3d.html block-zero-min-height-3-ref.html
> # The last-child series is an older variant of the auto-height-last-child tests.
> == block-last-child-1a.html block-last-child-1-ref.html

Per that last comment there, maybe we should keep the block-last-child-* tests adjacent to the block-auto-height-* tests (since they're an "older variant of those tests" and hence quite related), which would mean that the "block-zero-min-height" series would go just a bit further down.

Also, one note on that [preexisting] comment: this patch adds a "block-" prefix to "auto-height-last-child", in a comment elsewhere. Maybe you want to make that change in this comment, too?

r=me

Comment 6

[Daniel.S]

Attachment: Wave 7, rev 1 (for checkin), r=dholbert

No problem, I'm late as well.

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Comment on attachment 811566 [details] [diff] [review]
> Wave 7 (for review)
> 
> So perhaps at some point (definitely doesn't need to be now), it'd be worth
> doing a search-and-replace to switch away from "green" and "lightgreen" in
> these tests to some other arbitrary (non-red) colors.  (It looks like the
> guidelines linked above suggest using Fuchsia, Yellow, Teal, Orange for
> general-purpose coloring.)

I've filed bug 926614 for this purpose.

> >Bug 921761 - margin-collapsing test suite, wave 7; improving tests by adding readable IDs, re-arranging styles, changing colors; r=REVIEWER
> 
> commit message nit: s/re-arranging/rearranging/

Fixed.

> >+++ b/layout/reftests/margin-collapsing/block-abs-pos-1-ref.html
> >+<!-- Note: CSS 2.1 allows guessing the static position of an absolutely
> >+     positioned element whose offsets are 'auto' (initial value), i.e.
> >+     the reference of this test is just one of many possible outcomes. -->
> 
> Optional: It might be worth linking to the relevant chunk of the spec here,
> for justification / further-reading. The relevant link would be:
>  http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-width

Added the link to both cases.

> I don't think there's any benefit in switching from Green/Blue to
> Green/Green here (for 2 pieces of content in the testcase) -- and in fact,
> it's better as Green/Blue.
> 
> Tests are easier to understand & debug when different content uses different
> colors (to make bounds clearer, and to make it easier to immediately tell
> which part is mismatching when the test fails).
> 
> So, I'd rather we don't make the "change blue to green" tweaks to these
> "block-float-1*" tests.  (The other cleanups to these tests look good,
> though.)
> 
> This also applies to the table-caption-* tests. (they've got similar
> s/blue/green/ tweaks that I think we should leave as blue, or some distinct
> color)

Reverted (well, changed) both series.

> Observation (probably not to be addressed in this patch):
> These four different "block-overflow-5" reference cases are a bit
> confusing...
>   i) As noted elsewhere, multiple identically-rendering reference cases are
> redundant, so we can (& probably should) drop one out of each "ref,ref2"
> pair.

Last time we agreed on keeping the redundant cases for now, so I didn't bother. We can always work on this issue when the next set of tests is ready.

>  ii) For the "5c" test/references, it'd be clearer to move
> block-overflow-5c* to its own differently-numbered test, given that it
> doesn't match the #5 reference case.

I've filed bug 926615 for this.

> In most of the table-caption-* tests, this patch merges some of the #caption
> style with #block or #block1.
> 
> I'd rather not do that merge - I think it's clearer with #caption having its
> own block of style (so you don't have to jump around the style block to find
> all of its style-bits), and also as noted above, it's better to keep
> #caption having its own distinct color (e.g. blue, as it currently stands)
> rather than making it share green.

Unmerged and recolored.

> Might be worth bumping that final #block2 rule up to just below the #block1
> rule, so all of the #block2 styling is approximately in the same place. It's
> also fine as-is, though. (Looks like you're trying to match content-order,
> which I guess makes sense, aside from the fact that the #block2 styling is
> divided between the very beginning & very end.)

I've moved the rule up. I think this way it makes just a little more sense.

Comment 7

[Daniel.S]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Comment on attachment 811567 [details] [diff] [review]
> improve reftest.list for margin-collapsing tests
> 
> (In reply to Daniel.S from comment #2)
> > I simply moved some tests around and added more comments. The main focus was
> > to group tests of elements/boxes that establish a block formatting context.
> 
> That would be worth stating at the top of the reftest.list file. Otherwise,
> it's kinda unclear why we go from "block-" tests to "fieldset-" tests to
> "table-" tests and then back to "block-" again.
> 
> e.g. "This manifest is roughly organized such that tests of elements/boxes
> that establish block formatting contexts are grouped together." 

I've added a more general statement at the top of the list.

> > == block-auto-height-last-child-8c-dyn.html block-auto-height-last-child-8-ref.html
> >+== block-zero-min-height-1a.html block-zero-min-height-1-ref.html
> [...]
> >+== block-zero-min-height-3d.html block-zero-min-height-3-ref.html
> > # The last-child series is an older variant of the auto-height-last-child tests.
> > == block-last-child-1a.html block-last-child-1-ref.html
> 
> Per that last comment there, maybe we should keep the block-last-child-*
> tests adjacent to the block-auto-height-* tests (since they're an "older
> variant of those tests" and hence quite related), which would mean that the
> "block-zero-min-height" series would go just a bit further down.

Moved. And I rewrote the comment a little, so that the block-zero-min-height series also has its own comment.

> Also, one note on that [preexisting] comment: this patch adds a "block-"
> prefix to "auto-height-last-child", in a comment elsewhere. Maybe you want
> to make that change in this comment, too?

Fixed.

It would be nice, if you could have a quick look over the changes here and, if everything is alright, have the test tried and checked in. Thank you very much.

Comment 8

[Daniel.S]

Attachment: improve reftest.list for margin-collapsing tests, rev 1

The previous comment should have gone into this box. Sorry.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 816782 [details] [diff] [review]
improve reftest.list for margin-collapsing tests, rev 1

Looks good to me. Thanks!

I'll give these a local sanity-check test-run in a few minutes, and if that passes, I'll trigger a Try run.

Comment 10

[Daniel Holbert [:dholbert]]

Local reftest run was good.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=b289b74db72b

Comment 11

[Daniel Holbert [:dholbert]]

Try run looks good. I'll go ahead and push this to inbound.

Comment 12

[Daniel Holbert [:dholbert]]

Landed!
 https://hg.mozilla.org/integration/mozilla-inbound/rev/558a4c956738
 https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5ad1f42a32

(Daniel, is there a reason this needs to be [leave open]?)

Comment 13

[Daniel.S]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> Landed!

Thanks!

> (Daniel, is there a reason this needs to be [leave open]?)

Yes, there is. I have a bunch of new tests that enhance those we just updated with the first patch of this bug.
I'd like to add them in this bug because they make the updated tests complete. Also, these tests will be ready in a few days, so I didn't bother to open a new bug. After that, this bug can be closed.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/558a4c956738
https://hg.mozilla.org/mozilla-central/rev/4f5ad1f42a32

Comment 15

[Daniel.S]

Attachment: Wave 8 (for review)

Here's the last part of this bug.

* This will enhance existing tests by adding dynamic test cases as well as a few additional tests.
* I have also added tests for multi-column elements and
* a few tests for margins given in percent and em.

Note: The block-no-content-3 and -4 cases now have additonal b and c variants. I hope this is ok, since the original cases are not marked with an 'a'.

I have noticed that block-overflow-3 and -4 are marked skip-if(B2G), so I've added that part to the dynamic cases as well.

I have also tried new colors for tests that are completely new (there are only a few of those).

This bug is fixed if this patch is checked in.

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Daniel.S from comment #15)
> * This will enhance existing tests by adding dynamic test cases as well as a
> few additional tests.

So -- for these cases where you're adding a variant of an existing test: for future reference, you really should be creating these new files using "hg cp $oldfile $newfile" (and then making modifications).

That'd make the patch much easier to review -- (since it'd just show the diffs between the original file and the new copy, rather than showing the entirety of each new copy) -- and it has other under-the-hood benefits like preserving hg blame for the lines that you don't modify in $newfile.

I won't ask you to make that change in this case, though, since it'd be more trouble than it's worth at this point.

> Note: The block-no-content-3 and -4 cases now have additonal b and c
> variants. I hope this is ok, since the original cases are not marked with an
> 'a'.

We should probably just rename the original case as "a" for these (easy enough; just "hg mv block-no-content-3.html block-no-content-3a.html", with a corresponding tweak to reftest.list); however, that doesn't necessarily have to happen here.

> I have noticed that block-overflow-3 and -4 are marked skip-if(B2G), so I've
> added that part to the dynamic cases as well.

Makes sense.

> This bug is fixed if this patch is checked in.

Cool.
/me removes [leave open] so that this bug gets closed when this patch is landed & merged to central.

OK, looking at the patch now.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 822923 [details] [diff] [review]
Wave 8 (for review)

> I won't ask you to make that change in this case, though, since it'd be more
> trouble than it's worth at this point.

Actually... after looking a bit more at the patch, it looks like this file-copying make up a sizeable chunk of this patch, so I think this change is actually worth doing after all. (both for my sanity when reviewing, as well as for a more sane change history in mozilla-central)

(From an quick series of bash commands, it looks like this patch adds 46 new files that are "-dyn" variants of preexisting tests, and it'd be much easier to review this by skimming the diffs rather than skimming 46 new tests. I could, of course, diff them manually myself, but sorting that out is about as complex as just fixing the patch, so I think we might as well just fix the patch.)

Basically, what you'd want to do is the following:
 hg qimport existingPatch.patch # (get the patch in your queue,
                                #  if it's not already)
 hg qpush   # apply the patch

...and then for each file "foo-dyn.html" in this patch that's really a duplicate:
 mv foo-dyn.html /backupdir/
 hg forget foo-dyn.html
 hg qref
 hg cp foo.html foo-dyn.html
 cp /backupdir/foo-dyn.html  ./
 hg qref

So the end result is that you get the same "foo-dyn.html" file, except now hg will be aware that it's a copy of an existing file, and it will represent it as such in the patch. (It'll say "copy from... copy to..." instead of "new file", and it'll only mark the modified lines as being added/removed with +/-.)

(I tried the above commands (with backupdir replaced with an actual path) on a single testcase on my system, and they worked.  You'll probably want to try them on a single file and then inspect the resulting patch file by hand to be sure it does what you expect, before doing it on all of the files, so you can catch any bugs in your process early on. It's a fairly scriptable process. I'm happy to help if you'd like tips - feel free to ping me on IRC.)

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: list of new files that should probably be represented as "hg cp"

For reference, here's a list of all the "-dyn" files that appear to be copies of preexisting non-dyn files, which probably want to be created with "hg cp".

I generated this list by running the command below, in a tree that had the existing patch qimported. $OTHERTREE is the path to another mozilla-central tree without the patch applied (for use in determining whether a non-dyn variant predates this patch).

for x in `hg qstat | grep dyn.html | cut -f4 -d'/'`; do \
  orig=`echo $x | sed s/-dyn//`; \
  ls $OTHERTREE/layout/reftests/margin-collapsing/$orig \
    1> /dev/null 2>/dev/null;
  if [ $? == 0 ]; then echo $x; fi;
done

If you'd like to script the process that I outlined in my previous comment, then you probably want to use a list like this one.

Comment 19

[Daniel Holbert [:dholbert]]

(sorry, I forgot that "hg qstat" is an alias in my .hgrc; it's equivalent to "hg status --rev -2:.")

Comment 20

[Daniel.S]

Attachment: Wave 8, rev 1 (for review; using hg cp)

Here's the updated patch. Sorry I couldn't post it earlier.

As you have already noticed, there are 46 tests which are now copies of existing files instead of completely new files. Thanks for the list you made.
I'll keep in mind for future tests.

(In reply to Daniel Holbert [:dholbert] from comment #16)
> (In reply to Daniel.S from comment #15)
> > Note: The block-no-content-3 and -4 cases now have additonal b and c
> > variants. I hope this is ok, since the original cases are not marked with an
> > 'a'.
> 
> We should probably just rename the original case as "a" for these (easy
> enough; just "hg mv block-no-content-3.html block-no-content-3a.html", with
> a corresponding tweak to reftest.list); however, that doesn't necessarily
> have to happen here.

There could be more cases like this in the future. I'll create a follow-up bug.

Comment 21

[Daniel Holbert [:dholbert]]

Thanks for doing the 'hg cp' rewrite! Review feedback, part 1 of 2:

>diff --git a/layout/reftests/margin-collapsing/block-no-content-1a.html b/layout/reftests/margin-collapsing/block-no-content-1a-dyn.html
>copy from layout/reftests/margin-collapsing/block-no-content-1a.html
>copy to layout/reftests/margin-collapsing/block-no-content-1a-dyn.html
> .separator {
>+ background-color: green;
>  height: 20px;
>- background-color: green;

Nit: In the three block-no-content-1[abc]-dyn copies created in this patch, this background-color line is moving, probably for no reason.

Could you revert that change, to reduce unnecessary differences between the dyn & non-dyn versions?

This also goes for block-no-content-1d.html vs. block-no-content-1d-dyn.html (both of which are added in this patch) -- those new files have that same background-color line in swapped positions, with respect to each other.

>diff --git a/layout/reftests/margin-collapsing/block-no-content-3.html b/layout/reftests/margin-collapsing/block-no-content-3-dyn.html
>copy from layout/reftests/margin-collapsing/block-no-content-3.html
>copy to layout/reftests/margin-collapsing/block-no-content-3-dyn.html

Per the comments I just posted over in bug 978729, It'd ideally be nice for block-no-content-3, -3-dyn, -4, -4-dyn to be renamed to have "a" in the name as part of this patch, since this is the patch that creates the need for that rename. (If we just fix it here, then there's never any inconsistency, which is nice.)

>diff --git a/layout/reftests/margin-collapsing/block-percent-1.html b/layout/reftests/margin-collapsing/block-percent-1-dyn.html
>copy from layout/reftests/margin-collapsing/block-percent-1.html
>copy to layout/reftests/margin-collapsing/block-percent-1-dyn.html
[snip]
> #parent {
>- width: 200px; height: 200px;
>+ width: 200px;
[snip]
> #block2 {
>  height: 40px;
>- margin-top: 25%;
>  background-color: green;
> }
> </style>
>+<script type="text/javascript">
>+function test() {
>+ document.getElementById('block2').style.marginTop = '25%';

Looks like the "height: 200px" removal on #parent isn't related to the dynamic-change nature of this -dyn variant.

Could you either revert that change, or just make the same change in the "block-percent-1.html" original file as well, to minimize unnecessary differences between the two versions of this test? (It looks like it doesn't affect rendering, so I'd probably lean towards changing the original file.)

>diff --git a/layout/reftests/margin-collapsing/fieldset-sibling-1c.html b/layout/reftests/margin-collapsing/fieldset-sibling-1c-dyn.html
>copy from layout/reftests/margin-collapsing/fieldset-sibling-1c.html
>copy to layout/reftests/margin-collapsing/fieldset-sibling-1c-dyn.html
> #separator-top {
>  height: 20px;
>- margin-bottom: 10px;
>+ margin-bottom: 20px;
>  background-color: green;
> }
> fieldset {
>  height: 20px;
>- margin: 20px 0;
>+ margin: 0;
[...]
>+function test() {
>+ document.getElementsByTagName('fieldset')[0].style.margin = '20px 0';
>+ document.documentElement.removeAttribute('class');
>+}
>+document.addEventListener('MozReftestInvalidate', test, false);
>+</script>

Looks like the change to #separator-top was probably unintentional? If so, revert it.

>diff --git a/layout/reftests/margin-collapsing/table-caption-bottom-1.html b/layout/reftests/margin-collapsing/table-caption-bottom-1-dyn.html
>copy from layout/reftests/margin-collapsing/table-caption-bottom-1.html
>copy to layout/reftests/margin-collapsing/table-caption-bottom-1-dyn.html
[snip]
> #block1 {
>  margin-bottom: 10px;
> }
>-#block2 {
>- margin-top: 10px;
>-}
[snip]
>+#block2 {
>+ margin-top: 10px;
>+}

This patch moves this #block2 chunk of style to a different place in the <style> block, for table-caption-bottom-1-dyn.html.

Could you revert that part, unless there's a reason for it?

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8384552 [details] [diff] [review]
Wave 8, rev 1 (for review; using hg cp)

Review feedback, part 2 of 2:

>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/margin-collapsing/table-caption-bottom-outside-1.html
>+#block1 {
>+ margin-bottom: 10px;
>+}
>+#table {
>+ display: table;
>+}
>+#caption {
>+ display: table-cell;
>+ caption-side: bottom-outside;
[snip]
>+#block2 {
>+ margin-top: 10px;
>+}
>+</style>

[as in to the last part of previous comment:]

This "block2" chunk is at the very bottom of the <style> block here (instead of up next to #block1), which is an unnecessary difference between this file & table-caption-bottom-1.html. Aside from that difference, they're identical (except for the 'caption-side' value that you're testing).

Can you bring that #block2 chunk up, to fix that unnecessary difference?

Same applies to:
 table-caption-top-1-dyn.html
 table-caption-top-outside-1.html
 table-caption-top-outside-1-dyn.html
which all have a similar difference vs. the pre-existing file table-caption-top-1.html. 

ALTERNATELY: if you prefer having the #block2 at the bottom in all of these tests, then just fix the original files (table-caption-bottom-1.html, table-caption-top-1.html) so that they're consistent with these new copies.  This family of tests should just be consistent, one way or the other.

r=me with this and comment 21 addressed. Thanks again for the tests!

Comment 23

[Daniel.S]

Attachment: Wave 8, rev 2 (review addressed)

(In reply to Daniel Holbert [:dholbert] from comment #21)
> Nit: In the three block-no-content-1[abc]-dyn copies created in this patch,
> this background-color line is moving, probably for no reason.
> 
> Could you revert that change, to reduce unnecessary differences between the
> dyn & non-dyn versions?
> 
> This also goes for block-no-content-1d.html vs. block-no-content-1d-dyn.html
> (both of which are added in this patch) -- those new files have that same
> background-color line in swapped positions, with respect to each other.

Fixed.

> Per the comments I just posted over in bug 978729, It'd ideally be nice for
> block-no-content-3, -3-dyn, -4, -4-dyn to be renamed to have "a" in the name
> as part of this patch, since this is the patch that creates the need for
> that rename. (If we just fix it here, then there's never any inconsistency,
> which is nice.)

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Oh, I guess I did say "however, that doesn't necessarily have to happen
> here" in the other bug. Sorry for the mixed messages. :) I think I probably
> said that because I was suggesting so much additional work at the same time
> (all of the 'hg cp' consolidation).
> 
> Anyway -- I do think it's probably best to do the rename as part of the same
> patch that creates the need for the rename, if you don't mind. (Shouldn't be
> much trouble; just four hg mv commands [for -3, -3-dyn, -4, -4-dyn] and a
> reftest.list tweak)

Hehe. I made the changes in this patch.
Working with copies was not very difficult until I had to work with both renames and copies. This was a little tricky. I hope I didn't mess it up.

(In reply to Daniel Holbert [:dholbert] from comment #21)
> Looks like the "height: 200px" removal on #parent isn't related to the
> dynamic-change nature of this -dyn variant.
> 
> Could you either revert that change, or just make the same change in the
> "block-percent-1.html" original file as well, to minimize unnecessary
> differences between the two versions of this test? (It looks like it doesn't
> affect rendering, so I'd probably lean towards changing the original file.)

I have removed the height from both tests.

> Looks like the change to #separator-top was probably unintentional? If so,
> revert it.

The change is actually correct. The original test case is wrong. I fixed it.

> This patch moves this #block2 chunk of style to a different place in the
> <style> block, for table-caption-bottom-1-dyn.html.
> 
> Could you revert that part, unless there's a reason for it?

(In reply to Daniel Holbert [:dholbert] from comment #22)
> [as in to the last part of previous comment:]
> 
> This "block2" chunk is at the very bottom of the <style> block here (instead
> of up next to #block1), which is an unnecessary difference between this file
> & table-caption-bottom-1.html. Aside from that difference, they're identical
> (except for the 'caption-side' value that you're testing).
> 
> Can you bring that #block2 chunk up, to fix that unnecessary difference?
> 
> r=me with this and comment 21 addressed. Thanks again for the tests!

Fixed.

Thanks for the quick review!

Please have the patch tried and checked in if there are no more mistakes.

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to Daniel.S from comment #23)
> Hehe. I made the changes in this patch.
> Working with copies was not very difficult until I had to work with both
> renames and copies. This was a little tricky. I hope I didn't mess it up.

You should be able to verify that you didn't mess it up by trying to "hg qimport" the patch into a clean tree, BTW.  And at least on my end, it seems this did mess up the patch a bit -- the patch doesn't apply now. (but I think I can fix it by hand)

In particular, I get:
{
patching file layout/reftests/margin-collapsing/block-no-content-3a-dyn.html
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/margin-collapsing/block-no-content-3a-dyn.html.rej
patching file layout/reftests/margin-collapsing/block-no-content-4a-dyn.html
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/margin-collapsing/block-no-content-4a-dyn.html.rej
patching file layout/reftests/margin-collapsing/block-percent-1-dyn.html
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/margin-collapsing/block-percent-1-dyn.html.rej
patching file layout/reftests/margin-collapsing/fieldset-sibling-1c-dyn.html
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/margin-collapsing/fieldset-sibling-1c-dyn.html.rej
}

In each case, it looks like the "before" source code is expecting that all of the patch up to that point has already been applied, which I don't think is correct. (IIRC the "before" for each patch hunk should be from before this patch has been applied at all).

Anyway, this is a bit odd -- mercurial should be smart enough to handle this correctly. Are you using an old version of mercurial, perhaps? (I'm on 2.7.2)  Or did you make these changes via manual edits to the patch file? (instead of 'hg mv' / 'hg qref')

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8385268 [details] [diff] [review]
Wave 8, rev 2 (review addressed)

(In reply to Daniel Holbert [:dholbert] from comment #24)
> In each case, it looks like the "before" source code is expecting that all
> of the patch up to that point has already been applied, which I don't think
> is correct. (IIRC the "before" for each patch hunk should be from before
> this patch has been applied at all).

For example, this doesn't work:

>diff --git a/layout/reftests/margin-collapsing/block-no-content-3.html b/layout/reftests/margin-collapsing/block-no-content-3a.html
>rename from layout/reftests/margin-collapsing/block-no-content-3.html
>rename to layout/reftests/margin-collapsing/block-no-content-3a.html
>diff --git a/layout/reftests/margin-collapsing/block-no-content-3a.html b/layout/reftests/margin-collapsing/block-no-content-3a-dyn.html
>copy from layout/reftests/margin-collapsing/block-no-content-3a.html
>copy to layout/reftests/margin-collapsing/block-no-content-3a-dyn.html

...but if I change the last 3 lines to drop the "a" in "block-no-content-3a.html", then it works correctly.

Similarly, this looks fishy...
>diff --git a/layout/reftests/margin-collapsing/block-percent-1.html b/layout/reftests/margin-collapsing/block-percent-1.html
>--- a/layout/reftests/margin-collapsing/block-percent-1.html
>+++ b/layout/reftests/margin-collapsing/block-percent-1.html
>@@ -1,14 +1,14 @@
> <!DOCTYPE html>
> <html>
> <head>
> <style type="text/css">
> #parent {
>- width: 200px; height: 200px;
>+ width: 200px;
[SNIP]
>diff --git a/layout/reftests/margin-collapsing/block-percent-1.html b/layout/reftests/margin-collapsing/block-percent-1-dyn.html
>copy from layout/reftests/margin-collapsing/block-percent-1.html
>copy to layout/reftests/margin-collapsing/block-percent-1-dyn.html
>--- a/layout/reftests/margin-collapsing/block-percent-1.html
>+++ b/layout/reftests/margin-collapsing/block-percent-1-dyn.html
>@@ -1,26 +1,32 @@
> <!DOCTYPE html>
>-<html>
>+<html class="reftest-wait">
> <head>
> <style type="text/css">
> #parent {
>  width: 200px;
> }

It looks fishy because the #parent width/height change should be reflected in *both* block-percent-1.html *and* block-percent-1-dyn.html, but it's not. (It looks like the -dyn chunk expects the height to be already gone.)

If you generated this with edits & hg commands & hg qref, then this is probably indicative of a mercurial bug (which might be fixed in newer versions, if you're on an old mercurial).

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: Wave 8, rev 3 (fixed to be importable)

I fixed the issues in comment 24 by hand-editing the patch. Here's the updated version, which I can successfully hg import.

Comment 27

[Daniel Holbert [:dholbert]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=b92eb6c50112

Comment 28

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/0b6d9f0847e9

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0b6d9f0847e9

Comment 30

[Daniel.S]

(In reply to Daniel Holbert [:dholbert] from comment #24)
> In each case, it looks like the "before" source code is expecting that all
> of the patch up to that point has already been applied, which I don't think
> is correct. (IIRC the "before" for each patch hunk should be from before
> this patch has been applied at all).
> 
> Anyway, this is a bit odd -- mercurial should be smart enough to handle this
> correctly. Are you using an old version of mercurial, perhaps? (I'm on
> 2.7.2)  Or did you make these changes via manual edits to the patch file?
> (instead of 'hg mv' / 'hg qref')

Yes, I have made some manual edits. I'm sorry for causing so much trouble.
I'm grateful for your support in this regard. I have learned a lot about hg and I'll study it more before I put up the next patch. I have to admit that I didn't fully recognize the work I had to put into this project. But I'm dedicated to finish it.

Comment 31

[Daniel Holbert [:dholbert]]

No worries; it was relatively easy to fix up. Thanks again for all the work you've put in on this!