Bug 79272 – REGRESSION: Wrong collapsing border resolution on RTL table cells

• Component: CSS
• Status: RESOLVED FIXED

[Ebrahim Byagowi]

Created attachment 128273 [details]
testcase

When a table have this "border-collapse: collapse;", table border rendering is getting wrong.
You can see that by comparing attached HTML in webkit and not webkit browsers.

[Shane Stephens]

If rtl is specified along with border-collapse: collapse and border-left, the border appears on the right hand side. If border-collapse is not specified the border appears on the left.

[Ebrahim Byagowi]

Hi! Please be more serious about this bug :D

[Shezan Baig]

CC'ing David Hyatt to this ticket to get some advice.

The root cause of the bug is that the text direction on the table->style() is different from the text direction on the cell style.

In RenderTableCell, 'computeCollapsed[Start/End/Before/After]Border' use the text direction from 'table->style()' to determine the color component of the border, but use the text direction from 'style()' to determine the other components of the border.  Additionally, in 'paintCollapsedBorders', it uses the text direction from 'table->style()' when looking up the cached collapsed borders.

In order to fix this, we need to make sure the text direction is obtained from the same style object in all these places.  There are 2 ways I can think of to fix this bug, but I'm not sure which would be more "correct":

1. Change 'computeCollapsed*Border' to use 'style()' when computing the color
   component of the border.  Also change 'paintCollapsedBorder' to use 
   'style()' when looking up the cached collapsed borders.

2. Change 'computerCollapsed*Border' to use 'table->style()' when determining
   all the other components of the border.  This would involve adding methods
   like:

    const BorderValue& borderBefore(RenderStyle* tableStyle) const;
    const BorderValue& borderAfter(RenderStyle* tableStyle) const;
    const BorderValue& borderStart(RenderStyle* tableStyle) const;
    const BorderValue& borderEnd(RenderStyle* tableStyle) const;

   to the RenderTable[Cell|Col|Section] classes.


I'm leaning towards option (1), but the fact that tableStyle is explicitly used in the cached border lookups makes me wonder whether there is a reason table->style() needs to override the cell's style?

[Shezan Baig]

Also note that rev 71251, where RenderStyle's border[Start|End|Before|After] were introduced, the checkin comment says "This change still doesn’t support cells whose block flow differs from the table’s."

[Shezan Baig]

Created attachment 137610 [details]
modified testcase

Attaching a modified testcase that demonstrates more clearly what is going on.

Notice that when "border-collapse: collapse" is specified:

"10px solid black"   is rendered as  "10px solid green"
"20px dotted green"  is rendered as  "20px dotted black"

(in addition to each of them being on the wrong side of the cell)

[Ebrahim Byagowi]

@Shezan Baig: Thanks for assigning this bug to yourself :) I believe this bug is a regression, can you check very old Safari for it? I think if you find the changes/commit that caused this bug you can fix it easier, thanks in advance :)

[Shezan Baig]

Ebrahim, if you move "direction: rtl;" to the table element instead of the td element, I believe this will fix the problem in your case.

[Ebrahim Byagowi]

Thanks for your propose but I saw this bug somewhere that I had not access to edit, also that table must have mixed RTL and LTR cells because it was a table for translations, one column was in English (message title) and other column was for translated RTL message (and some cell must be English because they was not translated), I know for this case they could use a display: block RTL wrapper but I think this problem worth to be fixed. Thanks again :)

[Julien Chaffraix]

Sorry for the delay, I completely missed this bug.

(In reply to comment #6)
> @Shezan Baig: Thanks for assigning this bug to yourself :) I believe this bug is a regression, can you check very old Safari for it? I think if you find the changes/commit that caused this bug you can fix it easier, thanks in advance :)

I really wonder if it's a regression as we have had issues with mixed directionality for border collapsed computation for quite some time. After bug 87900 we support mixed directionality at the section level but not below it (this was the cause of super nasty bugs where we would pick up an unset border).

@Shezan Baig, your option (1) is only part of the fix and is missing the big picture.

I am not supportive of implementing a band-aid solution and we should do several things:
* disable 'direction' at the row level as it's a lot of complexity for a corner case usage with an easy work-around (just wrap your cells in row-groups). It's possible to implement that but it's pretty hard to do properly.
* add support for mixed directionality at the cell level.

This will need testing as there are lots of cases that we need to get right, involving mixed directionality at different levels. It's not a simple bug to fix and I would advise looking at bug 87900 for some context on the issues to overcome. Also feel free to poke me as needed for guidance.

[Ebrahim Byagowi]

Created attachment 155836 [details]
Safari 5.0 vs Chrome

[Ebrahim Byagowi]

Created attachment 155839 [details]
Safari 5.1.7 vs Chrome

[Ebrahim Byagowi]

I tested #5 testcase on http://browsershots.org/http://jsfiddle.net/ebraminio/cfzYt/embedded/result/ difference is available between chrome 8 and 9: 
http://browsershots.org/screenshots/06d410c03a3a36881f066b6d4de4d8e6
http://browsershots.org/screenshots/e53045f432e199ce342757d8a1f9c183

[Julien Chaffraix]

Ebrahim, thanks for the snapshots. It's definitely a regression, bumping the priority. My take would have been from r119594 but it was landed after the bug report.

Shezan, do you feel like fixing this bug soon? If not, I will need to take a stab at it.

As said, the testing is pretty important here and should thoroughly cover mixed directionality between table, table-row-group and table-cell in horizontal and vertical writing-modes. Currently we don't properly handle mixed directionality with table-column / table-column-group. table-row is a hard case and could be postponed to another bug (it could regress painting performance AFAICT).

[Shezan Baig]

(In reply to comment #13)
> Shezan, do you feel like fixing this bug soon? If not, I will need to take a stab at it.
> 


Julien, would love to take a stab at this, but I'm currently tied up with a bunch of other priorities that may take a while.  Feel free to take this if you want.

Thanks,
Shezan

[Julien Chaffraix]

Created attachment 172290 [details]
Proposed fix: handle mixed directions at the cell level.

[Build Bot]

Comment on attachment 172290 [details]
Proposed fix: handle mixed directions at the cell level.

Attachment 172290 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14725676

New failing tests:
css2.1/20110323/border-conflict-element-038.htm
css2.1/20110323/border-conflict-element-029.htm
fast/css/border-conflict-element-002.htm

[WebKit Review Bot]

Comment on attachment 172290 [details]
Proposed fix: handle mixed directions at the cell level.

Attachment 172290 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14680218

New failing tests:
css2.1/20110323/border-conflict-element-038.htm
css2.1/20110323/border-conflict-element-029.htm
fast/css/border-conflict-element-002.htm

[Julien Chaffraix]

Comment on attachment 172290 [details]
Proposed fix: handle mixed directions at the cell level.

The failures are related to handling mixed direction between cells and columns / column-groups - which I though I could get away with postponing to a follow-up bug.

[Ebrahim Byagowi]

Still exists, let's see what we can do about it now.

[Brent Fulgham]

WebKit still flails on this test. Blink and Gecko produce valid results.

[Radar WebKit Bug Importer]

<rdar://problem/96918171>

[Ebrahim Byagowi]

Still exists in the wild, compare 

https://commons.wikimedia.org/wiki/Template:Mbox?uselang=fa#مثال‌ها

in Safari vs Chrome/Firefox

[Ebrahim Byagowi]

Just yet another test case from the link on the previous link,

data:text/html;charset=utf8,<div dir="rtl"><table style="border-style:solid;border-left-width:8px;border-color:black;border-collapse:collapse;"><tbody><tr>%20<td%20dir="ltr">abc</td><td>abc</td></tr></tbody></table>

[Ebrahim Byagowi]

I've create a PR just to rebase Julien Chaffrai patch here, https://github.com/WebKit/WebKit/pull/30950

It fixes the original case in 2012 but fails at

data:text/html;charset=utf8,<div dir="rtl"><table style="border-style:solid;border-left-width:8px;border-color:black;border-collapse:collapse;"><tr><td dir="ltr">abc</td><td>abc</td></tr</table>

Let me see if I can figure this out

[Ahmad Saleem]

I think 283875@main - addressed this as well.

@Elika - can we close this one as well?

[Ebrahim Byagowi]

I just built ToT WebKit (1bef55fa7ec) which had 283875@main also, it fixes both the original case and the other 

https://bugs.webkit.org/attachment.cgi?id=128273

and

data:text/html;charset=utf8,<div dir="rtl"><table style="border-style:solid;border-left-width:8px;border-color:black;border-collapse:collapse;"><tbody><tr>%20<td%20dir="ltr">abc</td><td>abc</td></tr></tbody></table>