Closed Bug 140577 Opened 22 years ago Closed 20 years ago

radio buttons and check boxes aligned to the left when UI aligned to the right

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: tsahi_75, Assigned: smontagu)

References

(Blocks 1 open bug)

Details

(4 keywords)

Attachments

(4 files, 4 obsolete files)

Build ID: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0rc1) Gecko/20020417
(2002041711)

Description: when the interface is aligned to the right, e.g. when using a RTL
language (like hebrew or arabic), some radio buttons and checkboxes in the
preferences dialog are aligned to the left instead of to the right (but the
directionality is ok).

Steps to reproduce:
1. aligning the interface to the right: add these lines to the file intl.css, in
the locale\en-US\global, in the en-US.jar file (the language pack file, in the
chrome folder):

window,dialog,wizard,page {
  direction: rtl;
}

menu { direction: rtl; }

outliner { direction: rtl; }

2. start mozilla
3. open Edit|Preferences|Navigator or Edit|Preferences|Tabbed Browsing. look at
the radio buttons and checkboxes.

Actual Results: these elements are sometimes aligned to the left (not on all
categories), but the direction is ok (i.e. the boxes/buttons are to the right of
the text)

Expected results: elements should be aligned to the right.
Attached image check boxes
Attached image radio buttons
This seems to be a bug in align="start"...
umm...right. does that mean it's easy to solve?
Blocks: 137995
Status: UNCONFIRMED → NEW
Ever confirmed: true
any work being done here?
where does this bug stand?
is it possible to make a temporary fix using the following line:

#generalStartupBrowser { align: right !important; }

or

#generalStartupBrowser { text-align: right !important; }

in intl.css, like in bug 157607 comment 9 , to fix for example the checkbox
labeled "Navigator", in Edit>preferences>appearance? i tried it, but it didn't work.
pay attention to the definition of start in xulplanet
http://www.xulplanet.com/references/elemref/ref_XULElement.html "start: Child
elements are aligned starting from the left or top edge of the box. If the box
is larger than the total size of the children, the extra space is placed on the
right or bottom side."

So there is no wonder that all the boxes aligned to "start" cause the
misaligment when RTL is used. This is a simple code works as designed.

IMHO there are two ways to fix this:
1. Change the definition of start to say that start aligns to the right in RTL
boxes this probably involves changing the code here
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsBoxFrame.cpp#422
2. Remove the start aligment from all places (actually deprecate it ?). AFAIK by
default hboxes are aligned to the left, and hboxes without start are aligning
perfectly to the right when RTL is used.

BTW, can this bug be reassigned to someone that actually involved with mozilla
development.
markk5@bezeqint.net: i could assign it to you, mkaply *is* involved in mozilla
development. 

that said, i wasted a few minutes on this.
Assignee: mkaply → timeless
Attached patch fix (obsolete) — Splinter Review
Attached image patched view
/*I added:*/
page{direction:rtl;}
/*to userChrome.css*/
Attachment #134664 - Flags: superreview?(dbaron)
Attachment #134664 - Flags: review?(varga)
note that the local variable for center is really not needed but was done for
consistency (especially since i did a lazy find and replace). i'm more than
willing to undo those bits if any reviewer challenges me on them.
Status: NEW → ASSIGNED
Sorry and thanks.
timeless, in an RTL locale, the whole interface is flipped. the tree pane in the
prefs panel is on the right side of the window. try adding the css in the bug
description to userCrome.css to see the full effect.
tsahi: i'm well aware of that, i didn't need that to test or verify my change.
(if i wanted to do that, i'd have at least picked a localization). It's one
thing for me to see Hebrew going rtl, it's another to see English going rtl (it
gives me a headache).

I'm likely to file bugs about some of the other panels behaving badly, so i
suppose i'll attach them to bug 137995's tree.
Comment on attachment 134664 [details] [diff] [review]
fix

>+  Halignment align_Left = nsBoxFrame::hAlign_Left,
>+             align_Center = nsBoxFrame::hAlign_Center,
>+             align_Right = nsBoxFrame::hAlign_Right;

I would call these align_Start, align_Center and align_End.

>+  if (GetStyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL) {

I think the test should be |if (IsNormalDirection())| to cover all the bases.

>   if (NS_CONTENT_ATTR_HAS_VALUE == content->GetAttr(kNameSpaceID_None, nsHTMLAtoms::align, value)) {
>     // XXXdwh Everything inside this if statement is deprecated code.
>     if (value.EqualsIgnoreCase("left")) {
>-        aHalign = nsBoxFrame::hAlign_Left;
>+        aHalign = align_Left;
>         return PR_TRUE;
>     } else if (value.EqualsIgnoreCase("right")) {
>-        aHalign = nsBoxFrame::hAlign_Right;
>+        aHalign = align_Right;
>         return PR_TRUE;
>     }
>   }

Don't change the deprecated code. "left" shouldn't be used, but if it is, it
should mean "left" and not be overloaded to mean "start".
Attachment #134664 - Flags: review?(varga) → review-
Attachment #134664 - Flags: superreview?(dbaron)
Attachment #134664 - Attachment is obsolete: true
Attachment #135246 - Flags: review?(smontagu)
smontagu, can you review so this get's in to 1.6?
Comment on attachment 135246 [details] [diff] [review]
only localize left/right, use IsNormalDirection, and only honor it for non deprecated consumers

Sorry, I didn't mark this r- at the time, though I discussed it with timeless
on IRC. The patch causes lots of regressions.
Attachment #135246 - Flags: review?(smontagu) → review-
Comment on attachment 135246 [details] [diff] [review]
only localize left/right, use IsNormalDirection, and only honor it for non deprecated consumers

Sorry, I didn't mark this r- at the time, though I discussed it with timeless
on IRC. The patch causes lots of regressions.
Assignee: timeless → nobody
Status: ASSIGNED → NEW
OS: Windows 98 → All
Hardware: PC → All
I've got the beginnings of a handle on this.
Assignee: nobody → smontagu
Here's what is happening: at
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsBoxFrame.cpp#611
right-to-left horizontal boxes clear the NS_STATE_IS_DIRECTION_NORMAL bit, which
is then used in nsSprocketLayout.cpp to control the positioning of the children
inside the parent box.

The problem is that we can't clear the same bit in vertical boxes (if we do they
lay out their children from bottom to top) so the children of right-to-left
vertical boxes don't get aligned to the right.

The cleanest way to fix this is probably to have separate state bits for
HORIZONTAL_DIRECTION_NORMAL and VERTICAL_DIRECTION_NORMAL, but of course there
are no free bits.
Attached patch New patch (obsolete) — Splinter Review
Attachment #135246 - Attachment is obsolete: true
Attachment #157361 - Flags: superreview?(roc)
Attachment #157361 - Flags: review?(roc)
(In reply to comment #22)
> Here's what is happening: at
> http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsBoxFrame.cpp#611
> right-to-left horizontal boxes clear the NS_STATE_IS_DIRECTION_NORMAL bit,
> which is then used in nsSprocketLayout.cpp to control the positioning of the
> children inside the parent box.
> 
> The problem is that we can't clear the same bit in vertical boxes (if we do
> they lay out their children from bottom to top) so the children of
> right-to-left vertical boxes don't get aligned to the right.

But those RTL child boxes should get NS_STATE_IS_DIRECTION_NORMAL cleared when
we call GetInitialDirection on them, right? Because the CSS direction will be
inherited into their frames.
(In reply to comment #24)

> But those RTL child boxes should get NS_STATE_IS_DIRECTION_NORMAL cleared when
> we call GetInitialDirection on them, right? Because the CSS direction will be
> inherited into their frames.

Yes, I think they do, because the child boxes themselves are laid out RTL as
they should be (the radio buttons or checkboxes are to the right of the text).
The bug is in where the parent box positions its children.
   if (aFrameState & NS_STATE_IS_DIRECTION_NORMAL) {
-    aX = aClientRect.x; // The normal direction. |x| and |y| increase as we
move through our children.
+    if (frameDirection == NS_STYLE_DIRECTION_LTR)
+      aX = aClientRect.x; // The normal direction. |x| and |y| increase as we
move through our children.
+    else
+      aX = aClientRect.x + aOriginalRect.width;
     aY = aClientRect.y;
   }
   else {
     aX = aClientRect.x + aOriginalRect.width;  // The reverse direction. |x|
and |y| decrease as we
     aY = aClientRect.y + aOriginalRect.height; // move through our children.
   }

I'm suspicious of this code. If it's a vertical box with reversed direction,
i.e. going from bottom to top, then shouldn't you still consider the RTL
direction in some way?
Attached patch Revised patch (obsolete) — Splinter Review
Good point. I'm not 100% sure what is the Right Thing to do. The new patch has
y-order always normal in horizontal boxes, even when reversed, and x-order
always normal in vertical boxes unless they have rtl direction.
Attachment #157361 - Attachment is obsolete: true
Attachment #157951 - Flags: superreview?(roc)
Attachment #157951 - Flags: review?(roc)
Attachment #157361 - Flags: superreview?(roc)
Attachment #157361 - Flags: review?(roc)
+  if (aFrameState & NS_STATE_IS_HORIZONTAL) {
+    if ((aFrameState & NS_STATE_IS_DIRECTION_NORMAL) &&
+       frameDirection == NS_STYLE_DIRECTION_LTR) {
+      // The normal direction. |x| increases as we move through our children.
+      aX = aClientRect.x;
+    }
+    else {
+      // The reverse direction. |x| decreases as we move through our children.
+      aX = aClientRect.x + aOriginalRect.width;
+    }

For this case, I think you should just check NS_STATE_IS_DIRECTION_NORMAL.
GetInitalDirection already takes the CSS direction setting into account; RTL
boxes will unset NS_STATE_IS_DIRECTION_NORMAL.
http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/nsBoxFrame.cpp#602

Other than that it looks good.
Attachment #157951 - Attachment is obsolete: true
Attachment #157998 - Flags: superreview?(roc)
Attachment #157998 - Flags: review?(roc)
Attachment #157951 - Flags: superreview?(roc)
Attachment #157951 - Flags: review?(roc)
Attachment #157998 - Flags: superreview?(roc)
Attachment #157998 - Flags: superreview+
Attachment #157998 - Flags: review?(roc)
Attachment #157998 - Flags: review+
Patch checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
(In reply to comment #31)
> Patch checked in.

What about aviary and 1.7.3 branches?
Attachment #157998 - Flags: approval1.7.x?
Attachment #157998 - Flags: approval-aviary?
ONLY if they're critical for some reason. We are not backporting all Gecko fixes
from 1.8 to 1.7 and Aviary.
(In reply to comment #33)
> ONLY if they're critical for some reason. We are not backporting all Gecko fixes
> from 1.8 to 1.7 and Aviary.

At least for Aviary, we do - we can no more change xul files to reduce the
appearance of this bug  in the hebrew l10n release
(http://www.mozilla.org/foundation/trademarks/l10n-policy.html).
1. was this bug fixed in thunderbird as well? while this bug appears in few
places in firefox, in thunderbird the problem appears constantly through the
interface.

2. is it possible to check in the fix for bug 180628? this is another
modification i added to the xul files, and if we can't do any changes for ff 1.0
this bug will remain unfixed. it seems that the only thing needed is to review
the patch.



Comment on attachment 157998 [details] [diff] [review]
With the suggested correction

not this close to PR. maybe after
Attachment #157998 - Flags: approval-aviary? → approval-aviary-
Asa, this is a localizability polish bug, I would like to see it land for the RC
if at all possible. In any case, we should make it so that official builds using
RTL locales include this patch.
Flags: blocking-aviary1.0?
Keywords: l12y, polish
Comment on attachment 157998 [details] [diff] [review]
With the suggested correction

re-nominating for aviary and 1.7
Attachment #157998 - Flags: approval-aviary- → approval-aviary?
Comment on attachment 157998 [details] [diff] [review]
With the suggested correction

a=asa for branch checkins.
Attachment #157998 - Flags: approval1.7.x?
Attachment #157998 - Flags: approval1.7.x+
Attachment #157998 - Flags: approval-aviary?
Attachment #157998 - Flags: approval-aviary+
Flags: blocking-aviary1.0?
Status: RESOLVED → VERIFIED
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.