131023 - adding dir=rtl to a div breaks the layout

Status: VERIFIED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Shoshannah Forbes]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 6.0; Windows 98)
BuildID:    2002031106

I am attaching a file that I am working on that has a css based layout. the 
first file look OK in mozilla. However, when I add a dir=rtl to the main div, 
the alignment of the text in it gets messed up, and the text in the nested div 
moves outside it's block

Reproducible: Always
Steps to Reproduce:
1. open layout number 1 in mozilla
2. add a dir=rtl to the main div
3. compare display

Actual Results:  when the dir is added, the layout is broken

Expected Results:  the dir should not brake the layout of the page

Comment 1

[Shoshannah Forbes]

Attachment: the layout without the dir

Comment 2

[Shoshannah Forbes]

Attachment: the layout with the dir added. notice how it now brakes in mozila

Comment 3

[Shoshannah Forbes]

Changing to ALL.
here is a screen shot of  the bug in linux:
http://forums.mishkei.org.il/ATTACHMENTS/666/75/1314c7554/linuxlayout.png

Comment 4

[Matthew Blanchard]

Attachment: RTL div with "left-margin: 100px"

Note the yellow box in the previous screenshot, where the text moves outside
its block.

I believe this can be reduced to the following simple testcase:

<div dir="rtl" style="margin-left: 100px">
x x x x x x x x x x x x x x x x x x x x x x x x x x x
x x x x x x x x x x x x x x x x x x x x x x x x x x x
x x x x x x x x x x x x x x x x x x x x x x x x x x x
x x x x x x x x x x x x x x x x x x x x x x x x x x x
x x x x x x x x x x x x x x x x x x x x x x x x x x x
</div>

We should get a block with 100px left margin, but instead we get 100px _right_
margin. If we draw a border around the DIV (as in the attached testcase), we
see that it is placed correctly (the border). Only the text is misplaced.

Also, note that if we add an inner block element, another DIV, for example, as
in:

<div dir="rtl" style="margin-left: 100px">
<DIV>
x x x x x x x x x x x x x x x x x x x x x x x x x x x
x x x x x x x x x x x x x x x x x x x x x x x x x x x
x x x x x x x x x x x x x x x x x x x x x x x x x x x
x x x x x x x x x x x x x x x x x x x x x x x x x x x
x x x x x x x x x x x x x x x x x x x x x x x x x x x
</DIV>
</div>

the text is placed correctly.

This is why the text in the central column of the main testcase, titled "main
content area", is placed inside the borders: because it's inside <P> elements.

Comment 5

[timeless]

personally, i've relied on this when using DIR="RTL" to turn around chrome, but 
i have no idea if this is correct per spec.

Comment 6

[Shoshannah Forbes]

reversing chrome is a thing IE does to RTL pages, which is not per spec (and i 
tend to find it very annoying).
However, dir=rtl is essential to hebrew pages. And since my site will be in 
Hebrew, it looks like until this bug is fixed, I won't be able to use a css 
based layout.

Comment 7

[Hixie (not reading bugmail)]

margin-left (-right) controls the physically left (right) margin. CSS3 will
introduce properties using the BASE naming scheme, so that you would use
margin-start (-end) to put a margin on the left (right) side on left-to-right
text, on the right (left) side in right-to-left text, and on the top (bottom)
side in top-to-bottom text.

Comment 8

[timeless]

.

Comment 9

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

You say "the alignment of the text in it gets messed up".  Changing the
direction of the page should change the default text alignment unless you
explicitly specify otherwise:  see the "Initial" line in the description of
'text-align': http://www.w3.org/TR/REC-CSS2/text.html#alignment-prop .

Comment 10

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

Attachment: patch fixing problem

I'm not sure what this code was supposed to be doing, but it doesn't make any
sense, since it's offsetting the line's bounds, which are in coordinates
relative to the block's origin, by the offset of the block's coordinate space
relative to the block's parent's coordinate space (at least if I'm remembering
correctly the way all these coordinates work).	Removing it fixes the problem.

Comment 11

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

Well, except the odd thing is, HorizontalAlignFrames has significant
modifications |#ifdef IBMBIDI|, so it wouldn't surprise me if the semantics were
different.  Furthermore, it modifies the rect parameter, but in the case of the
|#ifdef IBMBIDI| caller that modified rect is thrown away since it's a copy.  I
think this needs some major cleanup.

Comment 12

[Shoshannah Forbes]

Another example of this bug, which tottaly broke this site:
http://www25.brinkster.com/shaharar/

Comment 13

[Shoshannah Forbes]

Attachment: a screen shot of the above site in my mozilla

Comment 14

[Shoshannah Forbes]

as more and more web sites move to CSS based layouts, this is becmoing more and
more of a problem. See what happend in the above screen shot: the site is
totally unreadble

Comment 15

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

Attachment: patch with a little more cleanup

This patch attempts to clean things up a little bit more.

Comment 16

[Chris Waterson]

Looks okay, to me, but it's not clear that the nasty #ifdef situation is getting
any better towards the end of nsLineLayout.cpp. ;-)

Comment 17

[Simon Montagu :smontagu]

I am cc-ing lkemmel to this bug and dbaron to bug 74880, since you seem to be
patching some of the same code.

Comment 18

[Sagie Maoz]

Is Bug 150364 a dupe, or is it blocked by this one?

Comment 19

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

Attachment: revised patch

Comment 20

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

*** Bug 100082 has been marked as a duplicate of this bug. ***

Comment 21

[Simon Montagu :smontagu]

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

>-      PerFrameData* pfd = psd->mFirstFrame;
>+      for (PerFrameData* pfd = psd->mFirstFrame; pfd
> #ifdef IBMBIDI
>-      while ( (nsnull != pfd) && (bulletPfd != pfd) ) {
>-#else
>-      while (nsnull != pfd) {
>-#endif // IBMBIDI
>+           && bulletPfd != pfd
>+#endif
>+           ; pfd = pfd->mNext) {

I don't like having the for statement split across the #ifdef like that. Change
it to something like

#ifdef IBMBIDI
	for (PerFrameData* pfd = psd->mFirstFrame; pfd && bulletPfd != pfd; pfd
= pfd->mNext)
#else
	for (PerFrameData* pfd = psd->mFirstFrame; pfd; pfd = pfd->mNext)
#endif

Have you run the layout regression tests?

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Actually, I sort of suspect that two separate for() statements will just be
prone to the two codepaths diverging unnecessarily...

Comment 23

[Brendan Eich [:brendan]]

bz's right, duplicating code like that invites unintended divergence.  ifdef'ing
only the different part may be ugly, but it's safe and sound.

/be

Comment 24

[Simon Montagu :smontagu]

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

OK, I'm convinced. r=smontagu, if dbaron has run layout regression tests and is
happy with the results. I'm sure he knows how to assess the results better than
I.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should just get rid of the #ifdef IBMBIDI stuff. We're never going to turn it
off. Of course that doesn't need to be done in this patch but it could be, if it
helps.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

sr=roc+moz

Comment 27

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

Fix checked in to trunk, 2002-12-10 18:38 PDT.

Comment 28

[Simon Montagu :smontagu]

Other bugs fixed by this checkin:
bug 96658
bug 135113 (not 100% sure about this one)
bug 138681
bug 148866

Very nice too.

Comment 29

[Simon Montagu :smontagu]

Sorry for the typo, the one I'm not 100% sure about is bug 135115.

Comment 30

[Ossama Khayat]

The same problem is probably here in this site. It's using DIV tags for positioning.
http://www.islamway.com/

Comment 31

[Madhur Bhatia]

verifying with attachment 74180 [details] and attachment 74589 [details]

Comment 32

[(no longer active)]

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