267420 - implement bc rtl computation

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Bernd]

with the fix of bug 1744070 we paint the borders somehow correct in the rtl
case. But we during the computation we assume that a vertical border between one
table cell and its next sibling is on the right hand side of the table cell.
Under rtl conditions this should be the left border.

Comment 1

[Bernd]

argh,  that is bug 174470

Comment 2

[Bernd]

Attachment: patch

Comment 3

[Bernd]

Attachment: revised patch

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

testcase: attachment 164726 [details]

Comment 5

[Eyal Rozenberg]

I think the patch should include an explanation that this parameter exists, and
that this side reversal is taking place, because of the reversal caused by the
fix of bug 174470 - and no other reason.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It wasn't caused by bug 174470.  We've always collapsed the right border of the
first cell with the left border of the second cell, independent of direction,
and bug 174470 did nothing to change that.

Comment 7

[Eyal Rozenberg]

But IIRC until the fix for 174470 was checked in border-left and border-right
basically worked for RTL tables, whereas now they don't. Or is there something
I'm missing?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Before the fix for bug 174470, for RTL tables, a border specified for the cell
at the left edge of the table showed up at the right edge of the table.

Comment 9

[Bernd]

This discussion is fruitless, we need to toggle the direction as the border
collapse computation stores the results in a top and a left edge
http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/celldata.h#226 and
a corner specification. What this patch does, it redefines the meaning of left
edge as first edge, so that it works for both ltr and rtl conditions. Its not
very elegant but it a) works b) creates a patch which is reviewable c) frees me
from a complete rewrite, which I have no intention to do in a near future.

Comment 10

[fantasai]

Sorry for the delay~ Been having computer issues on top of everything else.
(including but not limited to hard disk failure)

Bernd, your patch utterly fails to apply. I have no idea why. (Perhaps I am
doing something stupid, it's "patch -p0 < bernd.patch", no?) I just updated my
tree, too. I would prefer to test it myself as well, but if it passes all the
tests (including proper handling of corners) in bug 174470, then r+ fantasai.

Comment 11

[fantasai]

Comment on attachment 165035 [details] [diff] [review]
revised patch

yep, stupid mistake: I forgot to strip CRs :)

Build with attached patch fails test in attachment 141471 [details] ; border widths in
tests 1,2,5,&6 rtl are not correct.

Comment 12

[Bernd]

Attachment: next version

Comment 13

[Bernd]

Comment on attachment 167455 [details] [diff] [review]
next version

fantasai could you try this?

Comment 14

[fantasai]

Bernd, take a close look at the third and fifth tables in the rtl column on that
test case. Do you see the border overlapping the text like I do? It doesn't
happen in the ltr tests.

Comment 15

[Bernd]

It happens also without this patch, so it should not block the review ;-), its
pixel alignment question

Comment 16

[Eyal Rozenberg]

Shouldn't there be some comments explaining the fact that this is a 'double
inversion' that is due to the fact the the whole border layout is pre-inverted?
I mean, eventually someone is going to re-write the border code and it would be
best to add as little confusion as possible to what is already quite a mess...

Comment 17

[Bernd]

Attachment: revised patch with more correct celloffset

the change to the previous version is 
info.cell->SetBorderWidth(firstSide, PR_MAX(smallHalf,
info.cell->GetBorderWidth(firstSide)));
replaced
info.cell->SetBorderWidth(NS_SIDE_LEFT, PR_MAX(smallHalf,
info.cell->GetBorderWidth(NS_SIDE_LEFT)));

and the corresponding secondSide NS_SIDE_RIGHT.

and a introducing comment:
XXX the BC-RTL hack - The correct fix would be a rewrite as described in bug
203686.
   In order to draw borders in rtl conditions somehow correct, the existing
structure which relies
   heavily on the assumption that the next cell sibling will be on the right
side, has been modified.
   We flip the border during painting and during style lookup. Look for
tableIsLTR for places where
   the flipping is done.

Comment 18

[Bernd]

Comment on attachment 169424 [details] [diff] [review]
revised patch with more correct celloffset

Could you please look at it again

Comment 19

[fantasai]

Bernd, it passes all the tests and the changes to the border collapse code look
all right, but what's the stuff for changing height reflow for?

Comment 20

[Bernd]

the first two sections are the patch for bug 272830 ;-), just ignore them

Comment 21

[fantasai]

Comment on attachment 169424 [details] [diff] [review]
revised patch with more correct celloffset

Ok, then, r+ fantasai on the part of the patch that's for /this/ bug. ;)
Thanks for fixing it!~

Comment 22

[Bernd]

Comment on attachment 169424 [details] [diff] [review]
revised patch with more correct celloffset

David could you sr (minus the first two chunks)?

Comment 23

[Bernd]

fix checked in

Comment 24

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).