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

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

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.

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.

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.

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.

.

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 .

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.

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.

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

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

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

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

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

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

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

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

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

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 on attachment 107862 [details] [diff] [review] revised patch sr=roc+moz

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

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

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

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

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

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