Bug 79272 - REGRESSION: Wrong collapsing border resolution on RTL table cells
Summary: REGRESSION: Wrong collapsing border resolution on RTL table cells
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Julien Chaffraix
URL:
Keywords: HasReduction, Regression
Depends on: 96710 101060
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-22 13:14 PST by Ebrahim Byagowi
Modified: 2021-01-21 15:58 PST (History)
19 users (show)

See Also:


Attachments
testcase (761 bytes, text/html)
2012-02-22 13:14 PST, Ebrahim Byagowi
no flags Details
modified testcase (635 bytes, text/html)
2012-04-17 14:49 PDT, Shezan Baig
no flags Details
Safari 5.0 vs Chrome (158.35 KB, image/png)
2012-08-01 10:39 PDT, Ebrahim Byagowi
no flags Details
Safari 5.1.7 vs Chrome (140.36 KB, image/png)
2012-08-01 10:44 PDT, Ebrahim Byagowi
no flags Details
Proposed fix: handle mixed directions at the cell level. (26.20 KB, patch)
2012-11-05 01:13 PST, Julien Chaffraix
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ebrahim Byagowi 2012-02-22 13:14:37 PST
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.
Comment 1 Shane Stephens 2012-02-22 19:57:23 PST
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.
Comment 2 Ebrahim Byagowi 2012-03-30 15:52:17 PDT
Hi! Please be more serious about this bug :D
Comment 3 Shezan Baig 2012-04-16 12:46:57 PDT
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?
Comment 4 Shezan Baig 2012-04-16 12:52:55 PDT
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."
Comment 5 Shezan Baig 2012-04-17 14:49:41 PDT
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)
Comment 6 Ebrahim Byagowi 2012-07-27 08:14:46 PDT
@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 :)
Comment 7 Shezan Baig 2012-07-27 08:18:51 PDT
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.
Comment 8 Ebrahim Byagowi 2012-07-27 08:33:48 PDT
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 :)
Comment 9 Julien Chaffraix 2012-07-27 09:43:00 PDT
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.
Comment 10 Ebrahim Byagowi 2012-08-01 10:39:37 PDT
Created attachment 155836 [details]
Safari 5.0 vs Chrome
Comment 11 Ebrahim Byagowi 2012-08-01 10:44:21 PDT
Created attachment 155839 [details]
Safari 5.1.7 vs Chrome
Comment 13 Julien Chaffraix 2012-09-07 19:19:59 PDT
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).
Comment 14 Shezan Baig 2012-09-10 06:39:19 PDT
(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
Comment 15 Julien Chaffraix 2012-11-05 01:13:58 PST
Created attachment 172290 [details]
Proposed fix: handle mixed directions at the cell level.
Comment 16 Build Bot 2012-11-05 02:10:29 PST
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
Comment 17 WebKit Review Bot 2012-11-05 03:17:09 PST
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
Comment 18 Julien Chaffraix 2012-11-05 05:38:09 PST
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.
Comment 19 Ebrahim Byagowi 2021-01-21 15:58:48 PST
Still exists, let's see what we can do about it now.