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

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout
P1
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: Antwan, Assigned: Ya-Chieh Wu (inactive))

Tracking

({regression})

55 Branch
mozilla56
regression
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55+ fixed, firefox56+ fixed)

Details

Attachments

(7 attachments, 5 obsolete attachments)

2.25 KB, patch
Ya-Chieh Wu (inactive)
: review+
Details | Diff | Splinter Review
3.08 KB, patch
Ya-Chieh Wu (inactive)
: review+
Details | Diff | Splinter Review
369 bytes, text/html; charset=UTF-8
Details
1.08 KB, text/html
Details
1.09 KB, text/html
Details
3.30 KB, patch
Ya-Chieh Wu (inactive)
: review+
Details | Diff | Splinter Review
7.63 KB, patch
Ya-Chieh Wu (inactive)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 months ago
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
(Reporter)

Comment 1

11 months ago
This was introduced with Firefox 55b

Updated

11 months ago
Component: Untriaged → Layout
Product: Firefox → Core

Updated

11 months ago
Keywords: regressionwindow-wanted

Comment 2

11 months ago
[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
Blocks: 929484
Status: UNCONFIRMED → NEW
status-firefox54: --- → unaffected
status-firefox55: --- → affected
status-firefox56: --- → affected
status-firefox-esr52: --- → unaffected
tracking-firefox55: --- → ?
tracking-firefox56: --- → ?
Ever confirmed: true
Flags: needinfo?(mtseng)
Keywords: regressionwindow-wanted → regression

Updated

11 months ago
Priority: -- → P1
I'll take this.
Assignee: nobody → mtseng
Flags: needinfo?(mtseng)

Comment 4

11 months ago
Ya-Chieh, would you like to give it a try?
Flags: needinfo?(ywu)
Created attachment 8881495 [details] [diff] [review]
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

Updated

11 months ago
Assignee: mtseng → ywu
Status: NEW → ASSIGNED
Flags: needinfo?(ywu)

Comment 7

11 months ago
Tested on HTML table and CSS table, looks good so far. Test cases are expected.
(Assignee)

Comment 8

11 months ago
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

11 months ago
(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.
Sounds like a web compat regression we don't want to ship with; tracking for 55.
tracking-firefox55: ? → +
tracking-firefox56: ? → +
(Assignee)

Comment 11

11 months ago
Created attachment 8882031 [details] [diff] [review]
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
(Assignee)

Updated

11 months ago
Attachment #8881495 - Flags: review?(matt.woodrow)
(Assignee)

Updated

11 months ago
Attachment #8882031 - Flags: review?(matt.woodrow)
(Assignee)

Comment 12

11 months ago
hi matt,

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

thank you!
(Assignee)

Comment 13

11 months ago
Created attachment 8882558 [details] [diff] [review]
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
Attachment #8882031 - Attachment is obsolete: true
Attachment #8882031 - Flags: review?(matt.woodrow)
Attachment #8882558 - Flags: review?(matt.woodrow)
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
Attachment #8882558 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8881495 - Flags: review?(matt.woodrow) → review+

Comment 16

11 months ago
(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?
Flags: needinfo?(dbaron)
(Assignee)

Comment 17

11 months ago
Created attachment 8882765 [details] [diff] [review]
0001-Bug-1375518-Don-t-override-GetBorderRadii-in-nsBCTab.patch
Attachment #8881495 - Attachment is obsolete: true
Attachment #8882765 - Flags: review+
(Assignee)

Comment 18

11 months ago
Created attachment 8882766 [details] [diff] [review]
0002-Bug-1375518-reftest-to-check-border-radius-is-painte.patch
Attachment #8882558 - Attachment is obsolete: true
Attachment #8882766 - Flags: review+
(Assignee)

Comment 19

11 months ago
(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.
Created attachment 8883713 [details]
reporter's testcase

This is a cleaned-up version of https://jsfiddle.net/Lut9zqrr/ , attached for proper archiving.
Attachment #8883713 - Attachment is patch: false
Attachment #8883713 - Attachment mime type: text/plain → text/html; charset=UTF-8
Created attachment 8883717 [details]
more examples of border-radius on border-collapse tables
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.
Flags: needinfo?(dbaron)
ni? ywu for the questions in Comment #23.
Flags: needinfo?(ywu)
(Assignee)

Comment 25

11 months ago
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.
Flags: needinfo?(ywu)
(Assignee)

Comment 26

10 months ago
Created attachment 8887760 [details] [diff] [review]
0001-Bug-1375518-Fix-border-radius-on-table-row-groups-ro.patch
Attachment #8887760 - Flags: review?(matt.woodrow)
(Assignee)

Comment 27

10 months ago
Created attachment 8887762 [details] [diff] [review]
0004-Bug-1375518-one-more-reftest-to-check-border-radius-.patch
Attachment #8887762 - Flags: review?(matt.woodrow)
(Assignee)

Comment 28

10 months ago
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

10 months ago
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.
Flags: needinfo?(matt.woodrow)
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.
Flags: needinfo?(matt.woodrow)
Attachment #8887760 - Flags: review?(matt.woodrow) → review?(dbaron)
Attachment #8887762 - Flags: review?(matt.woodrow) → review?(dbaron)
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(...);
Created attachment 8889756 [details]
examples of border-radius on separated border model tables
Based on that testcase, it looks like bug 929484 caused the same behavior change for both border models.
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.
Attachment #8887760 - Flags: review?(dbaron) → review+
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
Attachment #8887762 - Flags: review?(dbaron) → review+
(Assignee)

Comment 36

10 months ago
(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!
(Assignee)

Comment 37

10 months ago
Created attachment 8889784 [details] [diff] [review]
Fix border-radius on table row groups, rows, column groups, or columns
Attachment #8887760 - Attachment is obsolete: true
Attachment #8889784 - Flags: review+
(Assignee)

Comment 38

10 months ago
Created attachment 8889785 [details] [diff] [review]
more reftests to check border-radius on both separate and collapse table
Attachment #8887762 - Attachment is obsolete: true
Attachment #8889785 - Flags: review+
(Assignee)

Updated

10 months ago
See Also: → bug 1384031

Comment 40

10 months ago
(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.
(Assignee)

Comment 41

10 months ago
(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.
(Assignee)

Comment 42

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

I am going to rebase to beta and request to uplift later.
Keywords: checkin-needed

Comment 43

10 months ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
(Assignee)

Comment 44

10 months ago
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
Attachment #8882765 - Flags: approval-mozilla-beta?
(Assignee)

Comment 45

10 months ago
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
Attachment #8882766 - Flags: approval-mozilla-beta?
(Assignee)

Comment 46

10 months ago
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
Attachment #8889784 - Flags: approval-mozilla-beta?
(Assignee)

Comment 47

10 months ago
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
Attachment #8889785 - Flags: approval-mozilla-beta?
(Assignee)

Comment 48

10 months ago
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

10 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Comment 50

10 months ago
(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 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
Attachment #8882765 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8882766 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8889784 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8889785 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.