Closed
Bug 174470
Opened 22 years ago
Closed 20 years ago
[BC] Bidi : Border-collapse property in RTL direction is not working correctly
Categories
(Core :: Layout: Tables, defect, P3)
Core
Layout: Tables
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: kyae-young.kim, Assigned: bernd_mozilla)
References
(Depends on 1 open bug, Blocks 2 open bugs)
Details
(Keywords: rtl, testcase, Whiteboard: [oracle-nls])
Attachments
(11 files, 3 obsolete files)
431 bytes,
text/html
|
Details | |
31.26 KB,
image/gif
|
Details | |
7.50 KB,
text/html
|
Details | |
3.28 KB,
text/html; charset=UTF-8
|
Details | |
126.08 KB,
image/png
|
Details | |
11.48 KB,
patch
|
Details | Diff | Splinter Review | |
2.29 KB,
patch
|
Details | Diff | Splinter Review | |
32.41 KB,
image/png
|
Details | |
16.84 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
627 bytes,
text/html
|
Details | |
78.19 KB,
image/jpeg
|
Details |
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.1) Gecko/20020826 Bidi : Border-collapse property in RTL direction is not working correctly Des : 1) Load the test case 2) Cell borders in RTL direction are not displayed correctly 3) It works fine for LTR direction in Mozilla 4) Check the attached images between Mozilla 1.0.1 and IE 5.5
Reporter | ||
Comment 1•22 years ago
|
||
Reporter | ||
Comment 2•22 years ago
|
||
Comment 3•22 years ago
|
||
*** Bug 176685 has been marked as a duplicate of this bug. ***
Comment 4•22 years ago
|
||
I think this really belongs in Layout: Tables
Assignee: mkaply → table
Component: BiDi Hebrew & Arabic → Layout: Tables
QA Contact: zach → amar
Comment 5•22 years ago
|
||
Confirming the cell borders are not correctly displayed with build: 2002-11-05-08-trunk on WINXP.
Updated•22 years ago
|
Target Milestone: --- → Future
The border collapse code paints the table borders as they are ltr over the table.
Comment 8•21 years ago
|
||
This is a big problem for BiDi users. Can someone tell me where I should look for the code for collapsed table borders?
Comment 9•21 years ago
|
||
The bug is somewhere in this code: http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSRendering.cpp#2252
Assignee | ||
Comment 10•21 years ago
|
||
*** Bug 216445 has been marked as a duplicate of this bug. ***
Comment 11•21 years ago
|
||
*** Bug 221935 has been marked as a duplicate of this bug. ***
Comment 12•20 years ago
|
||
(In reply to comment #9) Actually, my guess would be that the bug is in the code which _calls_ the special border-collapsed code (i.e. calls nsCSSRendering::DrawTableBorderSegment). Using the LXR this may be http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableFrame.cpp with the conspicuously-named function 'CalcBCBorders' at http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableFrame.cpp#5647 one would think that function would check the 'mLeftToRight' member, which it doesn't, apparently.
Comment 13•20 years ago
|
||
I nailed it... although I think the code for border-collapsed border drawing needs some rewriting so as to make less assumptions about the table, to encapsulate and generalize the iteration better, etc. I can't do this since I know epsilon << 1 about the layout; I was simply able to make it get the correct table columns for border segment x-coordinate calculations.
Comment 14•20 years ago
|
||
I nailed it... although I think the code for border-collapsed border drawing needs some rewriting so as to make less assumptions about the table, to encapsulate and generalize the iteration better, etc. I can't do this since I know epsilon << 1 about the layout; I was simply able to make it get the correct table columns for border segment x-coordinate calculations.
Updated•20 years ago
|
Attachment #141450 -
Attachment is obsolete: true
Comment 15•20 years ago
|
||
Comment on attachment 141451 [details] [diff] [review] proposed fix I'm requesting both review types since I believe this fix can have no itegration issues - it's only about drawing pixels at different X coordinates.
Attachment #141451 -
Flags: superreview?
Attachment #141451 -
Flags: review?
Comment 16•20 years ago
|
||
Comment on attachment 141451 [details] [diff] [review] proposed fix On further testing, it seems some more tweaking is necessary to properly handle cases more complicated than the bug testcase shown here. So consider this patch a partial fix. Updates to follow.
Attachment #141451 -
Flags: superreview?
Attachment #141451 -
Flags: review?
Updated•20 years ago
|
Assignee: core.layout.tables → nobody
QA Contact: amar → core.layout.tables
Comment 17•20 years ago
|
||
Sorry for the spam, but talking to bernd and looking at bug 229883 (and to some extent also 4510) made me realize I can't really squeeze a fix in without a major rewrite, of hundreds if not thousands of lines of code, for RTL compatibility - and this would not be a good idea before at least the patch for 229883 lands (or so it seems to me anyway), so I am tentatively marking this bug as depending on 229883. PS - The relevant files are nsTableFrame.* , nsCellMap.* and possibly also nsTableRowGroupFrame.* The main 'issues' AFAICT are the order of iteration over columns, the order of iteration over cells in a row, the determining of which borders/cells/columns are rightmost, the decision who owns which border and the decision of when to sort-of-duplicate borders to prevent overlapping the border of one cell with the cell content of the next one.
Depends on: 229883
Assignee | ||
Comment 18•20 years ago
|
||
Eyal, as you guessed this will be anything but a small patch. So its good if you start to study the code in CalcBCBorder. Its full of the ltr assumption. Furthermore the testcase in this bug demonstrates the wrong behaviour but for a fix we would need a testsuite that verifies the correctness of the border computations especially when the left and the right border are asymmetric in width and style.
Comment 19•20 years ago
|
||
This is not a comprehensive testsuit, but it does have some pitfalls which are circumvented in the original testcase, which my fix-attempt fails to avoid.
Attachment #141451 -
Attachment is obsolete: true
Comment 20•20 years ago
|
||
*** Bug 236884 has been marked as a duplicate of this bug. ***
Comment 21•20 years ago
|
||
Bug 139627 ("Table rules drawn ignoring dir=rtl setting") is related. Prog.
Hardware: PC → All
Summary: Bidi : Border-collapse property in RTL direction is not working correctly → [BC] Bidi : Border-collapse property in RTL direction is not working correctly
Comment 22•20 years ago
|
||
*** Bug 242855 has been marked as a duplicate of this bug. ***
Comment 23•20 years ago
|
||
*** Bug 246244 has been marked as a duplicate of this bug. ***
Comment 25•20 years ago
|
||
I found out that something the previous example looks OK. Most of the times it looks like the bug is there. Maybe the cause isn't only in the places people thought it will be.
Comment 26•20 years ago
|
||
(In reply to comment #25) > I found out that something the previous example looks OK. Most of the times it > looks like the bug is there. something = sometimes I got it right more than 5 times... Another test site: http://starcomsystems.com/hu/mozbug001.htm
Assignee | ||
Comment 27•20 years ago
|
||
*** Bug 260612 has been marked as a duplicate of this bug. ***
Comment 28•20 years ago
|
||
> I found out that something the previous example looks OK. Most of the times it
> looks like the bug is there.
But in the png the two tables are not mirrors of each other. What do you mean
when you say it "looks ok"? The bottom table structure is not a mirroring of the
top table structure.
These are two approaches to solving one of the two halves of this problem -- the painting half, rather than the collapsing half. The first approach tries to correct things manually, the second approach is simply to translate and scale the rendering context. The second could probably be made to work with a bit of GFX hacking; the first could probably be made to work by making the code a good bit uglier still.
Comment 32•20 years ago
|
||
My limited intuition says that the eventual solution to this problem would be to draw each cell's border separately (see bug 203686). In the general case there is no way of avoiding this (e.g. cells with position: relative), plus it has been suggested that some careful coding would make this not-so-expensive. Still, a temporary fix for the specific issue of RTL tables is still very welcome. As for the two approaches - do they both not work at the moment?
I think in the collapsing border model, cells and rows with 'position: relative' shouldn't move the borders.
And, FWIW, I suspect it would be easier to get the inverse of approach 1 working fully -- that is, paint the borders going from left-most to right-most rather than from start-most to end-most.
(Note that I think that's what I was doing for the vertical ones, but not the horizontal ones -- and it's the horizontal ones that have most of the problems.)
Updated•20 years ago
|
Whiteboard: [oracle-nls]
Assignee | ||
Comment 36•20 years ago
|
||
Assignee | ||
Comment 37•20 years ago
|
||
Don't forget to remove the printfs, and also don't forget to file a separate bug on 'border-left' and 'border-right' being backwards for border-collapsed tables.
Comment 39•20 years ago
|
||
> 'border-left' and 'border-right' being backwards for border-collapsed tables.
what?
Sorry, for any table elements in RTL border-collapsed tables.
Comment 41•20 years ago
|
||
So if I set table { border-left: solid red; } the border will be on the right side of the table??
Yep (with this patch). It should be pretty easy to fix, though.
Assignee | ||
Comment 43•20 years ago
|
||
The patch is more a demonstration that we could do it the dirty way now instead of waiting for the big and more correct solution. It fixes the wrong painting (there are still some pixel alignment issues - see the border close to the 2). It still asserts on Davids testcase (during scroll) and has a invalidation problem. I would like to split the problem into the painting issue here, and the rtl border computation in another bug, for 95% percent of the cases I guess this will help our users already, I mean all those excel generated tables. I hope I will have it review ready on sunday.
Assignee | ||
Comment 44•20 years ago
|
||
Assignee: nobody → bernd_mozilla
Attachment #162090 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee | ||
Comment 45•20 years ago
|
||
Comment on attachment 162208 [details] [diff] [review] patch with working invalidation David, could you please review it. The patch is modified from your first approach. I am thankfull for your help as I am weak at architectural problems. The main differences are some pixel aligment, side reversal and the startColX computation. Once this goes in I will file the bugs for the remaining issues (left to righ reversion, the testcase asserts even in the ltr case, some borders are wrong, the 2 is covered by the border)
Attachment #162208 -
Flags: superreview?(dbaron)
Attachment #162208 -
Flags: review?(dbaron)
Comment on attachment 162208 [details] [diff] [review] patch with working invalidation r+sr=dbaron, although I didn't go through all the details. But please rename |PRBool aTableDir| (what do true and false mean?) to something clearer like |PRBool aTableIsLTR| (which I think is what it means).
Attachment #162208 -
Flags: superreview?(dbaron)
Attachment #162208 -
Flags: superreview+
Attachment #162208 -
Flags: review?(dbaron)
Attachment #162208 -
Flags: review+
Assignee | ||
Comment 47•20 years ago
|
||
I checked in the fix, I will keep this bug open till I file all the followups
Assignee | ||
Comment 48•20 years ago
|
||
I filed: - bug 267418 on the assert that I see when parts of the table (even ltr) are painted. - bug 267419 on the rendering of the testcase in ltr conditions - bug 267420 on the correct computation of bc borders under rtl conditions.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 49•20 years ago
|
||
*** Bug 139627 has been marked as a duplicate of this bug. ***
Comment 50•20 years ago
|
||
border-left acts like border-right with this fix (build 2004-11-04 06).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 51•20 years ago
|
||
Assignee | ||
Comment 52•20 years ago
|
||
this is exactly what bug 267420 is about
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Comment 53•20 years ago
|
||
*** Bug 119882 has been marked as a duplicate of this bug. ***
Comment 54•20 years ago
|
||
Unfortunatly this hasn't been fixed yet. Table borders are still incorrect on "rtl" direction, while "border-collapse: collapse" is set. Here is an example: http://nomaed.evil.co.il/moz_bugs/rtl_collapse_mistake.html
Comment 55•20 years ago
|
||
One more case of the bug: http://www.twinadv.com/setlanguage.php?id=2&langpath=/index.php I hope this will help ;)
Comment 56•20 years ago
|
||
(In reply to comment #55) > One more case of the bug: > http://www.twinadv.com/setlanguage.php?id=2&langpath=/index.php > I hope this will help ;) As a solution for meanwhile, just set in the table itself style="border- collapse: separate" That's what I did, and it helped (http://e-dent.msn.co.il - the main menu)
Assignee | ||
Comment 57•20 years ago
|
||
This is fixed on trunk, get either a ff trunk build or a Mozilla 1.8.5a before further commenting.
Comment 58•20 years ago
|
||
*** Bug 275498 has been marked as a duplicate of this bug. ***
Comment 59•20 years ago
|
||
Hello, Problem with collapsed borders in left to right is not completely over. When you first load the page, table borders are not correct. If you press the Refresh it will be rendered correctly this time. (I am not sure what can be the reason but I have tested it several times and every time refresh has fixed my table). http://cloob.com/bug/fire1.jpg and http://cloob.com/bug/fire1.jpg Please see the pictures (before and after refresh). Regards, Mac
Comment 60•20 years ago
|
||
Sorry, Links are : http://cloob.com/bug/fire1.jpg and http://cloob.com/bug/fire2.jpg and my version is : Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a6) Gecko/20041227 Firefox/1.0+ Regards, Mac
Comment 61•20 years ago
|
||
Yep happens here too. See this: http://www.twinadv.com/setlanguage.php?id=2
Comment 62•19 years ago
|
||
*** Bug 294946 has been marked as a duplicate of this bug. ***
Comment 63•19 years ago
|
||
*** Bug 302997 has been marked as a duplicate of this bug. ***
Comment 64•19 years ago
|
||
*** Bug 305251 has been marked as a duplicate of this bug. ***
Comment 65•19 years ago
|
||
This is not fixed!! take a look at http://ar.wikipedia.org/wiki/%D8%A5%D9%86%D8%B8%D9%8A%D9%85 for example, eaven reloading the page doesnt fix the table!! Please reopen this bug
Assignee | ||
Comment 66•19 years ago
|
||
Did you test with a recent build http://www.mozilla.org/projects/firefox/ like Firefox 1.5 Beta 2 ?
Comment 67•19 years ago
|
||
That wikipedia page looks fine to me in the latest trunk build.
Comment 68•19 years ago
|
||
Great ! with Beta 2
Comment 69•19 years ago
|
||
*** Bug 312622 has been marked as a duplicate of this bug. ***
Comment 70•16 years ago
|
||
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
You need to log in
before you can comment on or make changes to this bug.
Description
•