[BC] Bidi : Border-collapse property in RTL direction is not working correctly

RESOLVED FIXED in Future

Status

()

defect
P3
normal
RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: kyae-young.kim, Assigned: bernd_mozilla)

Tracking

(Depends on 1 bug, Blocks 2 bugs, {rtl, testcase})

Trunk
Future
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [oracle-nls])

Attachments

(11 attachments, 3 obsolete attachments)

Reporter

Description

17 years ago
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

17 years ago
Reporter

Comment 2

17 years ago
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

Comment 6

16 years ago
the cells in an rtl table merge for some reason
Assignee

Comment 7

16 years ago
The border collapse code paints the table borders as they are ltr over the table.

Comment 8

16 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?
Assignee

Comment 10

16 years ago
*** Bug 216445 has been marked as a duplicate of this bug. ***
*** Bug 221935 has been marked as a duplicate of this bug. ***

Comment 12

16 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

16 years ago
Posted 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.

Comment 14

16 years ago
Posted 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.

Updated

16 years ago
Attachment #141450 - Attachment is obsolete: true

Comment 15

16 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

16 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?
Assignee: core.layout.tables → nobody
QA Contact: amar → core.layout.tables

Comment 17

16 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

16 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

16 years ago
Posted 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

Comment 20

16 years ago
*** Bug 236884 has been marked as a duplicate of this bug. ***

Comment 21

16 years ago
Bug 139627 ("Table rules drawn ignoring dir=rtl setting") is related.

Prog.

Updated

15 years ago
Depends on: 203686

Updated

15 years ago
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

Updated

15 years ago
Blocks: 240501
*** Bug 242855 has been marked as a duplicate of this bug. ***
*** Bug 246244 has been marked as a duplicate of this bug. ***

Comment 25

15 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

15 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

15 years ago
*** Bug 260612 has been marked as a duplicate of this bug. ***

Comment 28

15 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

15 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

15 years ago
Whiteboard: [oracle-nls]
Assignee

Comment 36

15 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

15 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

15 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

15 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

15 years ago
Assignee: nobody → bernd_mozilla
Attachment #162090 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee

Comment 45

15 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

15 years ago
I checked in the fix, I will keep this bug open till I file all the followups
Assignee

Comment 48

15 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: 15 years ago
Resolution: --- → FIXED
Assignee

Comment 49

15 years ago
*** Bug 139627 has been marked as a duplicate of this bug. ***

Comment 50

15 years ago
border-left acts like border-right with this fix (build 2004-11-04 06).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 52

15 years ago
this is exactly what bug 267420 is about
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
*** Bug 119882 has been marked as a duplicate of this bug. ***

Comment 54

15 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

15 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

15 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

15 years ago
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. ***

Comment 59

15 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

15 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

15 years ago
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. ***

Comment 65

14 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

14 years ago
Did you test with a recent build http://www.mozilla.org/projects/firefox/ like
Firefox 1.5 Beta 2 ?

Comment 67

14 years ago
That wikipedia page looks fine to me in the latest trunk build.

Comment 68

14 years ago
Posted image solved problem
Great !

with Beta 2
*** Bug 312622 has been marked as a duplicate of this bug. ***
Blocks: Persian

Comment 70

11 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.