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

VERIFIED FIXED

Status

()

Core
Layout: Text
VERIFIED FIXED
15 years ago
9 years ago

People

(Reporter: Tsahi Asher, Assigned: smontagu)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
fixed-aviary1.0, fixed1.7, l12y, polish
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
Created attachment 81288 [details]
check boxes
(Reporter)

Comment 2

15 years ago
Created attachment 81289 [details]
radio buttons
This seems to be a bug in align="start"...
(Reporter)

Comment 4

15 years ago
umm...right. does that mean it's easy to solve?
(Assignee)

Updated

15 years ago
Blocks: 137995

Updated

15 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 5

15 years ago
any work being done here?
(Reporter)

Comment 6

15 years ago
where does this bug stand?
(Reporter)

Comment 7

15 years ago
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.

Comment 8

14 years ago
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.

Comment 9

14 years ago
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

Comment 10

14 years ago
Created attachment 134664 [details] [diff] [review]
fix

Comment 11

14 years ago
Created attachment 134665 [details]
patched view

/*I added:*/
page{direction:rtl;}
/*to userChrome.css*/

Updated

14 years ago
Attachment #134664 - Flags: superreview?(dbaron)
Attachment #134664 - Flags: review?(varga)

Comment 12

14 years ago
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

Comment 13

14 years ago
Sorry and thanks.
(Reporter)

Comment 14

14 years ago
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.

Comment 15

14 years ago
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.
(Assignee)

Comment 16

14 years ago
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-

Updated

14 years ago
Attachment #134664 - Flags: superreview?(dbaron)

Comment 17

14 years ago
Created attachment 135246 [details] [diff] [review]
only localize left/right, use IsNormalDirection, and only honor it for non deprecated consumers

Updated

14 years ago
Attachment #134664 - Attachment is obsolete: true

Updated

14 years ago
Attachment #135246 - Flags: review?(smontagu)
(Reporter)

Comment 18

14 years ago
smontagu, can you review so this get's in to 1.6?
(Assignee)

Comment 19

14 years ago
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-
(Assignee)

Comment 20

14 years ago
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
(Assignee)

Comment 21

13 years ago
I've got the beginnings of a handle on this.
Assignee: nobody → smontagu
(Assignee)

Comment 22

13 years ago
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.
(Assignee)

Comment 23

13 years ago
Created attachment 157361 [details] [diff] [review]
New patch
(Assignee)

Updated

13 years ago
Attachment #135246 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
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.
(Assignee)

Comment 25

13 years ago
(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.
ahhhhhhh
   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?
(Assignee)

Comment 28

13 years ago
Created attachment 157951 [details] [diff] [review]
Revised patch

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
(Assignee)

Updated

13 years ago
Attachment #157951 - Flags: superreview?(roc)
Attachment #157951 - Flags: review?(roc)
(Assignee)

Updated

13 years ago
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.
(Assignee)

Comment 30

13 years ago
Created attachment 157998 [details] [diff] [review]
With the suggested correction
Attachment #157951 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #157998 - Flags: superreview?(roc)
Attachment #157998 - Flags: review?(roc)
(Assignee)

Updated

13 years ago
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+
(Assignee)

Comment 31

13 years ago
Patch checked in.
Status: NEW → RESOLVED
Last Resolved: 13 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).

Comment 35

13 years ago
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 36

13 years ago
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 39

13 years ago
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+
(Assignee)

Updated

13 years ago
Keywords: fixed-aviary1.0, fixed1.7

Updated

13 years ago
Flags: blocking-aviary1.0?
Status: RESOLVED → VERIFIED

Updated

9 years ago
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.