455668 - table cells have border-radius from the row/column/etc. with the background applied to every cell

Status: NEW

(Core :: Layout: Tables, defect)

Description

[Sander]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1 FirePHP/0.1.2
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1 FirePHP/0.1.2

Using "tr:first-child" I'd expect to target the first table row. But instead, all the table-cells within this row are targetted.

Reproducible: Always

Steps to Reproduce:
1. Create a table and target the first tr with "tr:first-child".
2. Add style rule to this selector, i.e. "-moz-border-radius-topleft: 0.5em;"
3. Only the topleft corner of the first table row should have a border-radius. Instead, all table cells get a topleft border-radius.

See http://home.react.nl/~sandervl/firefox-first-child-selector-bug.html .

Comment 1

[Jesse Ruderman]

Not a selector bug.  The same thing happens if you use #firsttr instead of tr:first-child.

Comment 2

[Jesse Ruderman]

Attachment: clearer testcase

Comment 3

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

Probably a bug in nsTablePainter.cpp:  TableBackgroundPainter::PaintCell uses the border data for the element that the background comes from when calling PaintBackgroundWithSC.  That's wrong in this case, and it looks like things might work OK if the nsStyleBorder for the cell were passed, although I haven't looked closely enough to see if that would break something else.

Comment 4

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

Also note that this would lead to incorrect handling of -moz-background-clip:border if the row has a specified solid border but the cell does not, due to the "solid border" optimizations that roc added recently.

Comment 5

[Bernd]

Jesse the testcase is wfm?

Comment 6

[Jesse Ruderman]

I think so.  It seems strange that the behavior of border-radius depends on whether the background is specified on the <tr> or the <td>, since without border-radius, it doesn't matter where the background is specified.  Can you look at this new testcase and tell me whether the current behavior is correct?

Comment 7

[Jesse Ruderman]

Attachment: testcase 2

Comment 8

[Bernd]

testcase 2 is wfm,  due to bug 43178 checkin?

Comment 9

[Zack Weinberg (:zwol)]

Attachment: screenshot of testcase 2 with current trunk (3.5 is the same)

I'm not at all sure how these test cases are supposed to behave.  This is what case 2 looks like for me with Firefox 3.5 and trunk (the screenshot was taken from trunk, but 3.5 is visually identical).  I do not see *any* curvature applied except when it is "on each td".  If that's what is supposed to happen, this bug should be closed; if not, perhaps it needs a new title, since I'm not seeing the effect described in the original bug description.

Comment 10

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

Attachment: screenshot from Firefox 3.0

Comment 11

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

Fix range on mozilla-central is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=670a3b50dfe0&tochange=28488df9e75e
which probably means bug 456219, but I don't see why.

In any case, we should definitely add a test given that this was a bug that got fixed by accident.

Comment 12

[Adam Casey]

Attachment: 455668-1.html

Updated testcase to use the new/correct border-radius type

I guess it looked like it was passing because FF stopped supporting -moz-border-radius-topleft.

Comment 13

[Adam Casey]

However I think you were spot on with comment 3, with the patch in bug 921341 applied, this renders correctly.

Is it worth creating a separate reftest for this, or is using the one in 921341 acceptable?

Comment 14

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

I guess that depends on whether the reftest from bug 921341 gives reasonable coverage for this case.

In hindsight, bug 921341 was probably a duplicate of this bug, but given that there's work going on there, it should probably stay separate.