Closed Bug 131023 Opened 18 years ago Closed 18 years ago

adding dir=rtl to a div breaks the layout

Categories

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

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.3beta

People

(Reporter: xslf, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: rtl, Whiteboard: [Hixie-P2][patch])

Attachments

(5 files, 2 obsolete files)

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
OS: Mac System 9.x → All
Hardware: Macintosh → All
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.
.
Assignee: mkaply → dbaron
Component: BiDi Hebrew & Arabic → Style System
QA Contact: zach → ian
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 .
Component: Style System → Layout
Summary: adding dir=rtl to a div brakes the layout → adding dir=rtl to a div breaks the layout
Attached patch patch fixing problem (obsolete) — Splinter Review
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.
Target Milestone: --- → mozilla1.0
Blocks: 137995
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
Whiteboard: [Hixie-P2]
Attached patch patch with a little more cleanup (obsolete) — Splinter Review
This patch attempts to clean things up a little bit more.
Attachment #74617 - Attachment is obsolete: true
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?
Target Milestone: mozilla1.0 → mozilla1.1beta
Whiteboard: [Hixie-P2] → [Hixie-P2][patch]
Target Milestone: mozilla1.1beta → mozilla1.2alpha
Target Milestone: mozilla1.2alpha → mozilla1.2beta
Component: Layout → Layout: Block & Inline
Attached patch revised patchSplinter Review
Attachment #84764 - Attachment is obsolete: true
Attachment #107862 - Flags: superreview?(bzbarsky)
Attachment #107862 - Flags: review?(smontagu)
*** 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?
Attachment #107862 - Flags: review?(smontagu) → review-
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.
Attachment #107862 - Flags: review- → review+
Attachment #107862 - Flags: superreview?(bzbarsky) → superreview?(roc+moz)
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
Attachment #107862 - Flags: superreview?(roc+moz) → superreview+
Fix checked in to trunk, 2002-12-10 18:38 PDT.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.2beta → mozilla1.3beta
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]
Status: RESOLVED → VERIFIED
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.