1375518 - Border radius is not painted correctly on border-collapsed table-cell elements

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Antwan]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170619071954

Steps to reproduce:

Apply border-radius to an element displayed as table-cell, contained within a border-collapsed table-row element.

https://jsfiddle.net/Lut9zqrr/


Actual results:

The border-radius doesnt apply


Expected results:

The border radius applies

Comment 1

[Antwan]

This was introduced with Firefox 55b

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]: Layout regression


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

Regressed by: Bug 929484

Comment 3

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

I'll take this.

Comment 4

[Astley Chen (inactive)]

Ya-Chieh, would you like to give it a try?

Comment 5

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Don't override GetBorderRadii in nsBCTableCellFrame.

Background image or color calculates clip region based on GetBorderRadii. But nsBCTableCellFrame always return no border radii which makes calculation wrong. The result looks good after removing the GetBorderRadii override funtion. But I don't know there are other side effects.

MozReview-Commit-ID: 7CSTDwuxm21

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Trigger a try to see if any problems occur.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2204eba3070f74bc447bea5e36c01ad502235fd2

Comment 7

[Astley Chen (inactive)]

Tested on HTML table and CSS table, looks good so far. Test cases are expected.

Comment 8

[Ya-Chieh Wu (inactive)]

For the reftest, I used a div to draw a border-radius element and compare it to a border-collapsed with border-radius table element.
push a try to see the result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0e454033a57b8b4a619d07b8ee959f9eaf11074

Comment 9

[Astley Chen (inactive)]

(In reply to Ya-Chieh Wu from comment #8)
> For the reftest, I used a div to draw a border-radius element and compare it
> to a border-collapsed with border-radius table element.
> push a try to see the result:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e0e454033a57b8b4a619d07b8ee959f9eaf11074

We probably need 2 tries here:
#1 Shows added test cases can catch the failures with intended failure type(fails ==) in specified reftest.list.
#2 Shows added test cases can pass whenever we remove the failure type(==) from that reftest.list.

Comment 10

[Julien Cristau [:jcristau]]

Sounds like a web compat regression we don't want to ship with; tracking for 55.

Comment 11

[Ya-Chieh Wu (inactive)]

Attachment: 0002-Bug-1375518-reftest-to-check-border-radius-is-painte.patch

In this reftest, I used div element to compare table element.

push tries to see the result:

without patch "Don't override GetBorderRadii in nsBCTableCellFrame" :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e626abfaf6fabdf24bcbb6fef61e5b7f631459e2

with patch "Don't override GetBorderRadii in nsBCTableCellFrame" :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25eb3e292da520f14c77ec47a8da3a363e3c9d86

Comment 12

[Ya-Chieh Wu (inactive)]

hi matt,

the try result looks find. could you take a look at this?

thank you!

Comment 13

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug-1375518-reftest-to-check-border-radius-is-painte.patch

Hi matt, 
In this patch, I add one more test case for "display:table". 
thank you!


try looks ok.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d53e513ae5bb266bbe4347bcab1fbf142187b2bb

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8882558 [details] [diff] [review]
0001-Bug-1375518-reftest-to-check-border-radius-is-painte.patch

Review of attachment 8882558 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/table-bordercollapse/reftest.list
@@ +1,1 @@
> +== Bug1375518.html Bug1375518-ref.html

Standard naming style is bug1375518-2.html

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8881495 [details] [diff] [review]
Don't override GetBorderRadii in nsBCTableCellFrame.

Review of attachment 8881495 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like this was added in bug 459144 because BC table cells didn't support border-radius.

Apparently that's no longer true, since this is a regression, but I can't find out where we started drawing these correctly.

Seems like this worth trying, hopefully nothing regresses.

Comment 16

[Astley Chen (inactive)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #15)
> Apparently that's no longer true, since this is a regression, but I can't
> find out where we started drawing these correctly.
> 
> Seems like this worth trying, hopefully nothing regresses.

Spec[1] said CSS3 UAs should ignore border-radius properties applied to internal table elements when ‘border-collapse’ is ‘collapse’.

[1] https://www.w3.org/TR/css3-background/#border-radius-tables

As it's a regression and Webkit/Blink/Edge borwsers all support it, I don't see it a risk as it was.
But it's definitely a discussion topic worth bringing to CSSWG meetings or actually it did.

David, do you know some more background or status of this issue?

Comment 17

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug-1375518-Don-t-override-GetBorderRadii-in-nsBCTab.patch

Comment 18

[Ya-Chieh Wu (inactive)]

Attachment: 0002-Bug-1375518-reftest-to-check-border-radius-is-painte.patch

Comment 19

[Ya-Chieh Wu (inactive)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #15)
> Looks like this was added in bug 459144 because BC table cells didn't
> support border-radius. 
> Apparently that's no longer true, since this is a regression, but I can't
> find out where we started drawing these correctly.
> 
> Seems like this worth trying, hopefully nothing regresses.

thank you matt to note this.

Comment 20

[Ya-Chieh Wu (inactive)]

try:
https://hg.mozilla.org/try/rev/4ba14e66d97b0773d8ca60c0298d31f83a2cb16f

Comment 21

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

Attachment: reporter's testcase

This is a cleaned-up version of https://jsfiddle.net/Lut9zqrr/ , attached for proper archiving.

Comment 22

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

Attachment: more examples of border-radius on border-collapse tables

Comment 23

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

It seems like this is likely to change the behavior of:
 * mouse event targeting
 * overflow:hidden
 * probably other things.

It also seems like the existing Gecko behavior doesn't make much sense.  Why does border-radius on row groups, rows, column groups, or columns apply to the background of each cell, yet the border-radius on the cell itself doesn't?  Was that also what happened before bug 929484?  If it was, why can't the "regression" be fixed by making border-radius on the cell work the same way as border-radius on the other internal table parts, at least until we can actually get a sensible behavior?

This seems like it requires more investigation before it's ready to land.

Comment 24

[Mike Taylor [:miketaylr]]

ni? ywu for the questions in Comment #23.

Comment 25

[Ya-Chieh Wu (inactive)]

Before bug 929484, border-radius on row groups, rows, column groups, or columns don't apply to the background of each cell, yet the border-radius on the table and the cell itself do.

After bug 929484, the border-radius on the cell itself do not apply to the background of the cell, yet the others do.

Comment 26

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug-1375518-Fix-border-radius-on-table-row-groups-ro.patch

Comment 27

[Ya-Chieh Wu (inactive)]

Attachment: 0004-Bug-1375518-one-more-reftest-to-check-border-radius-.patch

Comment 28

[Ya-Chieh Wu (inactive)]

Hi matt,

The patch is trying to revert the behaviors of border-radius on table row groups, table rows, table column groups, or table columns back to what happened before bug 929484 and I also add a reftest on them.  

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=437a00a14842260f1449acd2315db6869139c4ac&selectedJob=115105361

Comment 29

[Astley Chen (inactive)]

Matt, could you help to review these 2 patches? As we are approaching merge day(8/2), it'd be great if you are able to do it sooner. Thanks.

Comment 30

[Matt Woodrow (:mattwoodrow)]

Sorry for the delay, was on PTO. I think dbaron would be a better reviewer here as he's already looked into this in some detail.

Comment 31

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

Comment on attachment 8887760 [details] [diff] [review]
0001-Bug-1375518-Fix-border-radius-on-table-row-groups-ro.patch

So a few thoughts here:

before bug 929484 we rounded the background on the table and table-cell, but not for row, row group, column, or column-group backgrounds, for border-collapsed tables.  That behavior isn't particularly good and we should probably file a bug on fixing it, but fixing and backporting the regression fix first is sensible.

There aren't any testcases here for non-border-collapsed tables, but the behavior there definitely needs to be examined.

>+  bool haveRoundedCorners = false;
>+  if(aForFrame->Type() != LayoutFrameType::TableColGroup &&
>+     aForFrame->Type() != LayoutFrameType::TableCol &&
>+     aForFrame->Type() != LayoutFrameType::TableRow &&
>+     aForFrame->Type() != LayoutFrameType::TableRowGroup) {
>+    haveRoundedCorners = GetRadii(aForFrame, aBorder, aBorderArea,
>+                                  clipBorderArea, aClipState->mRadii);
>+  }

You should put aForFrame->Type() in a variable (probably "fType") rather than calling it four times.  "if(" should be "if (", although really it might be clearer to just write the whole thing as:

  bool haveRoundedCorners =
    fType != LayoutFrameType::TableColGroup &&
    ... &&
    GetRadii(...);

Comment 32

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

Attachment: examples of border-radius on separated border model tables

Comment 33

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

Based on that testcase, it looks like bug 929484 caused the same behavior change for both border models.

Comment 34

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

Comment on attachment 8887760 [details] [diff] [review]
0001-Bug-1375518-Fix-border-radius-on-table-row-groups-ro.patch

OK, assuming that your intent is to land all 4 patches, then r=dbaron given the changes in comment 31.

Comment 35

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

Comment on attachment 8887762 [details] [diff] [review]
0004-Bug-1375518-one-more-reftest-to-check-border-radius-.patch

>+<!DOCTYPE html>
>+<!DOCTYPE HTML>

Just use one of these lines, not both.  It doesn't matter which.  (In both test and reference.)

Also, could you add the same test for border-collapse: separate as well (note that for that you should change the "td { border: 1px solid black }" line to use the selector "table, td".  And change both the <title> and the <h1> (I forgot to do the latter in my version!).

r=dbaron with that

Comment 36

[Ya-Chieh Wu (inactive)]

(In reply to David Baron :dbaron: ⌚️UTC+1 from comment #31)
> before bug 929484 we rounded the background on the table and table-cell, but
> not for row, row group, column, or column-group backgrounds, for
> border-collapsed tables.  That behavior isn't particularly good and we
> should probably file a bug on fixing it, but fixing and backporting the
> regression fix first is sensible.

Thank you David to note this and help with the reviewing. I will file a bug to discuss the behaviors of border-radius on table in general. thx!

Comment 37

[Ya-Chieh Wu (inactive)]

Attachment: Fix border-radius on table row groups, rows, column groups, or columns

Comment 38

[Ya-Chieh Wu (inactive)]

Attachment: more reftests to check border-radius on both separate and collapse table

Comment 39

[Ya-Chieh Wu (inactive)]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=597a25d112213c9670d48f530002d1472d97b374

Comment 40

[Astley Chen (inactive)]

(In reply to Ya-Chieh Wu from comment #39)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=597a25d112213c9670d48f530002d1472d97b374

https://treeherder.mozilla.org/#/jobs?repo=try&revision=597a25d112213c9670d48f530002d1472d97b374&selectedJob=117378293

Just noticed this weird failure as it seemed loading a wrong test and compared to ref html.

Comment 41

[Ya-Chieh Wu (inactive)]

(In reply to Astley Chen [:astley] from comment #40)
> (In reply to Ya-Chieh Wu from comment #39)
> > try:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=597a25d112213c9670d48f530002d1472d97b374
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=597a25d112213c9670d48f530002d1472d97b374&selectedJob=1
> 17378293
> 
> Just noticed this weird failure as it seemed loading a wrong test and
> compared to ref html.

This wasn't related to us as you can see *R3* passing later.

Comment 42

[Ya-Chieh Wu (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=597a25d112213c9670d48f530002d1472d97b374
try looks ok!

I am going to rebase to beta and request to uplift later.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c414e653408
Fix border-radius on table row groups, rows, column groups, or columns. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/7338ea5eca93
Don't override GetBorderRadii in nsBCTableCellFrame. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d37da744549
Add reftest to check that border radius is painted correctly on border-collapsed table. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/32a63be3c80c
Add reftest to check that border-radius is painted correctly on table. r=dbaron

Comment 44

[Ya-Chieh Wu (inactive)]

Comment on attachment 8882765 [details] [diff] [review]
0001-Bug-1375518-Don-t-override-GetBorderRadii-in-nsBCTab.patch

Approval Request Comment
[Bug causing the regression]: bug 929484
[User impact if declined]: The border-radius on table row, row group, column, column group and cell background changes on FF55.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: Nightly automated tests.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: all the patches in this bug.
[Is the change risky?]: no
[Why is the change risky/not risky?]: only change the table code.
[String changes made/needed]: no

Comment 45

[Ya-Chieh Wu (inactive)]

Comment on attachment 8882766 [details] [diff] [review]
0002-Bug-1375518-reftest-to-check-border-radius-is-painte.patch

Approval Request Comment
[Bug causing the regression]: bug 929484
[User impact if declined]: The border-radius on table row, row group, column, column group and cell background changes on FF55.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: Nightly automated tests.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: all the patches in this bug.
[Is the change risky?]: no
[Why is the change risky/not risky?]: only change the table code.
[String changes made/needed]: no

Comment 46

[Ya-Chieh Wu (inactive)]

Comment on attachment 8889784 [details] [diff] [review]
Fix border-radius on table row groups, rows, column groups, or columns

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Approval Request Comment
[Bug causing the regression]: bug 929484
[User impact if declined]: The border-radius on table row, row group, column, column group and cell background changes on FF55.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: Nightly automated tests.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: all the patches in this bug.
[Is the change risky?]: no
[Why is the change risky/not risky?]: only change the table code.
[String changes made/needed]: no

Comment 47

[Ya-Chieh Wu (inactive)]

Comment on attachment 8889785 [details] [diff] [review]
more reftests to check border-radius on both separate and collapse table

Approval Request Comment
[Bug causing the regression]: bug 929484
[User impact if declined]: The border-radius on table row, row group, column, column group and cell background changes on FF55.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: Nightly automated tests.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: all the patches in this bug.
[Is the change risky?]: no
[Why is the change risky/not risky?]: only change the table code.
[String changes made/needed]: no

Comment 48

[Ya-Chieh Wu (inactive)]

I had tested on beta locally and submit a try to see if it breaks anything:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75345368e7e29e9c2f624ec4d276239f9fa20859

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7c414e653408
https://hg.mozilla.org/mozilla-central/rev/7338ea5eca93
https://hg.mozilla.org/mozilla-central/rev/8d37da744549
https://hg.mozilla.org/mozilla-central/rev/32a63be3c80c

Comment 50

[Ya-Chieh Wu (inactive)]

(In reply to Ya-Chieh Wu from comment #48)
> I had tested on beta locally and submit a try to see if it breaks anything:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=75345368e7e29e9c2f624ec4d276239f9fa20859

beta on try looks ok!

Comment 51

[Julien Cristau [:jcristau]]

Comment on attachment 8882765 [details] [diff] [review]
0001-Bug-1375518-Don-t-override-GetBorderRadii-in-nsBCTab.patch

fix table layout regression in 55.  should be in 55.0b13

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/939de7ddf178
https://hg.mozilla.org/releases/mozilla-beta/rev/c725f89f4a7a
https://hg.mozilla.org/releases/mozilla-beta/rev/d2f6b2ba1715
https://hg.mozilla.org/releases/mozilla-beta/rev/04dac9bf8b3e

Comment 53

[Andrei Vaida [:avaida]]

(In reply to Ya-Chieh Wu from comment #47)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: Nightly automated tests.
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Ya-Chieh Wu's assessment on manual testing needs and the fact that this fix has automated coverage.