RESOLVED FIXED Bug 79272
REGRESSION: Wrong collapsing border resolution on RTL table cells
https://bugs.webkit.org/show_bug.cgi?id=79272
Summary REGRESSION: Wrong collapsing border resolution on RTL table cells
Ebrahim Byagowi
Reported 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.
Attachments
testcase (761 bytes, text/html)
2012-02-22 13:14 PST, Ebrahim Byagowi
no flags
modified testcase (635 bytes, text/html)
2012-04-17 14:49 PDT, Shezan Baig
no flags
Safari 5.0 vs Chrome (158.35 KB, image/png)
2012-08-01 10:39 PDT, Ebrahim Byagowi
no flags
Safari 5.1.7 vs Chrome (140.36 KB, image/png)
2012-08-01 10:44 PDT, Ebrahim Byagowi
no flags
Proposed fix: handle mixed directions at the cell level. (26.20 KB, patch)
2012-11-05 01:13 PST, Julien Chaffraix
buildbot: commit-queue-
Shane Stephens
Comment 1 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.
Ebrahim Byagowi
Comment 2 2012-03-30 15:52:17 PDT
Hi! Please be more serious about this bug :D
Shezan Baig
Comment 3 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?
Shezan Baig
Comment 4 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."
Shezan Baig
Comment 5 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)
Ebrahim Byagowi
Comment 6 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 :)
Shezan Baig
Comment 7 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.
Ebrahim Byagowi
Comment 8 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 :)
Julien Chaffraix
Comment 9 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.
Ebrahim Byagowi
Comment 10 2012-08-01 10:39:37 PDT
Created attachment 155836 [details] Safari 5.0 vs Chrome
Ebrahim Byagowi
Comment 11 2012-08-01 10:44:21 PDT
Created attachment 155839 [details] Safari 5.1.7 vs Chrome
Julien Chaffraix
Comment 13 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).
Shezan Baig
Comment 14 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
Julien Chaffraix
Comment 15 2012-11-05 01:13:58 PST
Created attachment 172290 [details] Proposed fix: handle mixed directions at the cell level.
Build Bot
Comment 16 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
WebKit Review Bot
Comment 17 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
Julien Chaffraix
Comment 18 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.
Ebrahim Byagowi
Comment 19 2021-01-21 15:58:48 PST
Still exists, let's see what we can do about it now.
Brent Fulgham
Comment 20 2022-07-12 16:52:40 PDT
WebKit still flails on this test. Blink and Gecko produce valid results.
Radar WebKit Bug Importer
Comment 21 2022-07-12 16:52:59 PDT
Ebrahim Byagowi
Comment 22 2024-07-16 10:56:09 PDT
Still exists in the wild, compare https://commons.wikimedia.org/wiki/Template:Mbox?uselang=fa#مثال‌ها in Safari vs Chrome/Firefox
Ebrahim Byagowi
Comment 23 2024-07-17 16:55:28 PDT
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
Comment 24 2024-07-18 03:29:52 PDT
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
Comment 25 2024-09-18 15:23:31 PDT
I think 283875@main - addressed this as well. @Elika - can we close this one as well?
Ebrahim Byagowi
Comment 26 2024-09-20 00:22:56 PDT
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>
Note You need to log in before you can comment on or make changes to this bug.