User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0 Build ID: 20170816210634 Steps to reproduce: A table is nested inside another table. The borders of all tables are collapsed. All rows have a background color. All cells have a border. The outer table has a caption. Actual results: The borders of the cells of the inner table are not visible if the outer table has a caption (not OK). The borders of the cells of the inner table are visible if the outer table doesn't have a caption (OK). Expected results: The borders of the cells of the inner table should be visible even if the outer table has a caption. This works as expected in previous versions of Firefox, in Google Chrome and in Internet Explorer.

[Tracking Requested - why for this release]: regression table layout since 55

Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=33df8c04309cf792b214c5c6c903a391048f5516&tochange=2a15fd71cf351e12c1286139e53b42b4cc486141 Regressed by: Bug 929484

@:mtseng Your patch seems to cause the regression. Can you look this?

Track 56+ as regression.

Comment on attachment 8902168 [details] Bug 1394226 - Correct z-ordering for some table parts. https://reviewboard.mozilla.org/r/173640/#review179356 Sorry, I really don't understand the table code well enough for this, but I'm not sure who does. Maybe dbaron? Is it correct that we create the display item for the caption at the end of creating items for the wrapper (on the outer table?), but the caption might belong to a content for something in the middle, so we need to sort it to the right z-position? What position is the BC item in, and why is that right, even though it's apparently not the content order? Dumping the display lists before/after sorting (PrintDisplayListTo) as well as the frame tree might help me here.

If my understanding of this code is correct, it might be easier to build the caption items into a separate display list, and then insert them into the right spot, rather than appending and resorting the whole thing?

I have no idea which approach is better either. ni dbaron for some suggestion.

(In reply to Matt Woodrow (:mattwoodrow) from comment #9) > Is it correct that we create the display item for the caption at the end of > creating items for the wrapper (on the outer table?), but the caption might > belong to a content for something in the middle, so we need to sort it to > the right z-position? Based on a literal reading of https://drafts.csswg.org/css2/zindex.html#painting-order it is not correct for backgrounds and borders (where the caption should paint on top of all parts of the table, down to the cells), but it is correct for other things. It's not clear to me that that's a sensible behavior to implement, though. It should be possible to test other implementations to see if that's what they do, although I'm not sure what I'd do with the results of such testing. It's possible this should be raised with the CSS WG. > What position is the BC item in, and why is that right, even though it's > apparently not the content order? I agree that the BC border display item needs to be on top of the descendants. I'd note that this code: https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/layout/tables/nsTableFrame.cpp#1589-1610 paints three things on top of the descendants (i.e., out of content order): * the border-collapse borders of the entire table * the table's border in the separated border mode * the table's outline This code then modifies the sorting only when there's a caption frame present: https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/layout/tables/nsTableWrapperFrame.cpp#176-196 which seems quite inconsistent. In general, the z-ordering of all of these things should be the same whether or not there is a caption frame. So there's clearly a bug here that should be fixed.

I was going to suggest that I think the most straightforward fix here is: 1. move the aFrame->DisplayOutline() call in DisplayGenericTablePart to before the aTraversal call 2. change nsTableWrapperFrame::BuildDisplayList to not sort the BorderBackgrounds list except then I noticed that it doesn't *currently* sort the BorderBackgrounds list (but it does sort the BlockBorderBackgrounds list -- but it looks like that's not the list the nsDisplayTableBorderCollapse goes on). So I admit I don't actually understand why the bug is happening today given that it doesn't look like the current code sorts the BorderBackgrounds list. (Does something somewhere move things from the BorderBackgrounds list to the BlockBorderBackgrounds list?)

(In reply to David Baron :dbaron: ⌚️UTC-7 (away September 4-8) from comment #13) > So I admit I > don't actually understand why the bug is happening today given that it > doesn't look like the current code sorts the BorderBackgrounds list. (Does > something somewhere move things from the BorderBackgrounds list to the > BlockBorderBackgrounds list?) Function DisplayLine (https://searchfox.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#6698-6699) would replace BorderBackground with BlockBorderBackground. Because of the nested table, all BorderBackground in the nested table move to BlockBorderBackground.

Because of comment 14, we still change the order of collapsed border in nested border. Do you have any thought for this? Thanks.

In that case, I think what I suggested in comment 13 should be correct, except that it shouldn't sort the BlockBorderBackgrounds list either. (And there should be a comment explaining why not, rather than just missing code!)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b854be8df7fbeb17d46a1aa54bb84620f508ab7

Comment on attachment 8902168 [details] Bug 1394226 - Correct z-ordering for some table parts. https://reviewboard.mozilla.org/r/173640/#review184730 This looks good, but you should have another patch adding a reftest. ::: layout/tables/nsTableWrapperFrame.cpp:191 (Diff revision 3) > - set.BlockBorderBackgrounds()->SortByContentOrder(GetContent()); > + // We don't sort BlockBorderBackgrounds and BorderBackgrounds because the > + // display items in those lists should stay out of content order. I'd add to the end of this sentence: "in order to follow the rules in https://www.w3.org/TR/CSS21/zindex.html#painting-order and paint the caption background after all of the rest".

Comment on attachment 8907924 [details] Bug 1394226 - Add reftests. https://reviewboard.mozilla.org/r/179602/#review184784 While this is useful, I think you could have a stronger test by also having a bug1394226-notref.html file, that has the inner borders on the inner table in a different color, and assert that it is != to the test file, to assert that the borders are showing up (rather than the borders disappearing both with and without the caption). r=dbaron with that addition (of one new file and one line in the reftest.list)... though I'd like to look at the revised patch.

Comment on attachment 8907924 [details] Bug 1394226 - Add reftests. https://reviewboard.mozilla.org/r/179602/#review185284 This isn't right, because the -notref has the red border around the edge of the inner table too, which means the test passes even without the fix. Only the inner borders should be red.

After discussing with Astley, it's unlikely to get fix in 56. Mark 56 as won't fix.

Comment on attachment 8907924 [details] Bug 1394226 - Add reftests. https://reviewboard.mozilla.org/r/179602/#review187246 r=dbaron

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3aaf5bbaaae40c2910d00f9bde12d8a19a6a84d8

Pushed by mtseng@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0de55d6a505d Correct z-ordering for some table parts. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/9c1445f35fbf Add reftests. r=dbaron

https://hg.mozilla.org/mozilla-central/rev/0de55d6a505d https://hg.mozilla.org/mozilla-central/rev/9c1445f35fbf

This fix seems very similar to what I proposed in bug 604270 a while back. Roc pointed out that it can cause problems wrt relative ordering of descendants of the cells and the table-caption. See bug 604270 comment 7. Does this patch avoid that problem?