WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
View All
Add attachment
proposed patch, testcase, etc.
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
Ebrahim Byagowi
Comment 12
2012-08-30 00:02:37 PDT
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
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
<
rdar://problem/96918171
>
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.
Top of Page
Format For Printing
XML
Clone This Bug