Borders of cells of nested table are not visible if borders are collapsed and rows have a background color and outer table has a caption

RESOLVED FIXED in Firefox 57
(Needinfo from 2 people)

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: duke1995, Assigned: mtseng, NeedInfo)

Tracking

(Blocks 1 bug, {regression})

55 Branch
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(thunderbird_esr52 unaffected, firefox-esr52 unaffected, firefox55 wontfix, firefox56+ wontfix, firefox57+ fixed)

Details

Attachments

(5 attachments)

Reporter

Description

2 years ago
Posted file HTML test case
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.
Reporter

Comment 1

2 years ago
Reporter

Comment 2

2 years ago

Comment 3

2 years ago
[Tracking Requested - why for this release]: regression table layout since 55
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression

Updated

2 years ago
Has Regression Range: --- → yes
Has STR: --- → yes

Comment 5

2 years ago
@:mtseng
Your patch seems to cause the regression. Can you look this?
Flags: needinfo?(mtseng)
Track 56+ as regression.
Comment hidden (mozreview-request)
Flags: needinfo?(mtseng)
Attachment #8902168 - Flags: review?(matt.woodrow)
Assignee: nobody → mtseng
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
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.
Flags: needinfo?(dbaron)
(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.
Flags: needinfo?(dbaron)
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.
Flags: needinfo?(dbaron)
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!)
Flags: needinfo?(dbaron)
Comment hidden (mozreview-request)
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".
Attachment #8902168 - Flags: review?(dbaron) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Attachment #8907924 - Flags: review?(dbaron) → review-
Comment hidden (mozreview-request)
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.
Attachment #8907924 - Flags: review?(dbaron) → review-
Priority: -- → P2
After discussing with Astley, it's unlikely to get fix in 56. Mark 56 as won't fix.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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?
Flags: needinfo?(mtseng)
Flags: needinfo?(dbaron)
You need to log in before you can comment on or make changes to this bug.