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)

defect

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
Blocks: 137995
*** Bug 176685 has been marked as a duplicate of this bug. ***
I think this really belongs in Layout: Tables
Assignee: mkaply → table
Component: BiDi Hebrew & Arabic → Layout: Tables
QA Contact: zach → amar
 Confirming the cell borders are not correctly displayed with build:
2002-11-05-08-trunk on WINXP.
Keywords: testcase
OS: Windows 2000 → All
Priority: -- → P3
Target Milestone: --- → Future
the cells in an rtl table merge for some reason
The border collapse code paints the table borders as they are ltr over the table.
This is a big problem for BiDi users. 
Can someone tell me where I should look for the code for collapsed table borders?
*** Bug 216445 has been marked as a duplicate of this bug. ***
*** Bug 221935 has been marked as a duplicate of this bug. ***
(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.
Attached patch proposed fix (obsolete) — Splinter Review
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.
Attached patch proposed fix (obsolete) — Splinter Review
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.
Attachment #141450 - Attachment is obsolete: true
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 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?
Assignee: core.layout.tables → nobody
QA Contact: amar → core.layout.tables
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
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.
Attached file some more tests
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
*** Bug 236884 has been marked as a duplicate of this bug. ***
Bug 139627 ("Table rules drawn ignoring dir=rtl setting") is related.

Prog.
Depends on: 203686
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
Blocks: 240501
*** Bug 242855 has been marked as a duplicate of this bug. ***
Depends on: 139627
*** Bug 246244 has been marked as a duplicate of this bug. ***
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.
(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
*** Bug 260612 has been marked as a duplicate of this bug. ***
> 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.
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.)
Whiteboard: [oracle-nls]
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.
>  'border-left' and 'border-right' being backwards for border-collapsed tables.

what?
Sorry, for any table elements in RTL border-collapsed tables.
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.
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: nobody → bernd_mozilla
Attachment #162090 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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+
I checked in the fix, I will keep this bug open till I file all the followups
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
*** Bug 139627 has been marked as a duplicate of this bug. ***
border-left acts like border-right with this fix (build 2004-11-04 06).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
this is exactly what bug 267420 is about
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
*** Bug 119882 has been marked as a duplicate of this bug. ***
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
One more case of the bug:
http://www.twinadv.com/setlanguage.php?id=2&langpath=/index.php

I hope this will help ;)
(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)
This is fixed on trunk, get either a ff trunk build or a Mozilla 1.8.5a before
further commenting.
*** Bug 275498 has been marked as a duplicate of this bug. ***
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
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
Yep happens here too. See this:
http://www.twinadv.com/setlanguage.php?id=2
*** Bug 294946 has been marked as a duplicate of this bug. ***
*** Bug 302997 has been marked as a duplicate of this bug. ***
*** Bug 305251 has been marked as a duplicate of this bug. ***
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

Did you test with a recent build http://www.mozilla.org/projects/firefox/ like
Firefox 1.5 Beta 2 ?
That wikipedia page looks fine to me in the latest trunk build.
Attached image solved problem
Great !

with Beta 2
*** Bug 312622 has been marked as a duplicate of this bug. ***
Blocks: Persian
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.

Attachment

General

Creator:
Created:
Updated:
Size: