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

Status: VERIFIED FIXED

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

Description

[Tsahi Asher]

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.

Comment 1

[Tsahi Asher]

Attachment: check boxes

Comment 2

[Tsahi Asher]

Attachment: radio buttons

Comment 3

[Boris Zbarsky [:bzbarsky]]

This seems to be a bug in align="start"...

Comment 4

[Tsahi Asher]

umm...right. does that mean it's easy to solve?

Comment 5

[Tsahi Asher]

any work being done here?

Comment 6

[Tsahi Asher]

where does this bug stand?

Comment 7

[Tsahi Asher]

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

[Mark Kaplun]

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

[timeless]

[e-mail]: i could assign it to you, mkaply *is* involved in mozilla
development. 

that said, i wasted a few minutes on this.

Comment 10

[timeless]

Attachment: fix

Comment 11

[timeless]

Attachment: patched view

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

Comment 12

[timeless]

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.

Comment 13

[Mark Kaplun]

Sorry and thanks.

Comment 14

[Tsahi Asher]

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

[timeless]

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 16

[Simon Montagu :smontagu]

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

Comment 17

[timeless]

Attachment: only localize left/right, use IsNormalDirection, and only honor it for non deprecated consumers

Comment 18

[Tsahi Asher]

smontagu, can you review so this get's in to 1.6?

Comment 19

[Simon Montagu :smontagu]

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.

Comment 20

[Simon Montagu :smontagu]

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.

Comment 21

[Simon Montagu :smontagu]

I've got the beginnings of a handle on this.

Comment 22

[Simon Montagu :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.

Comment 23

[Simon Montagu :smontagu]

Attachment: New patch

Comment 24

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

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

Comment 25

[Simon Montagu :smontagu]

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

Comment 26

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

ahhhhhhh

Comment 27

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

   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?

Comment 28

[Simon Montagu :smontagu]

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

Comment 29

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

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

Comment 30

[Simon Montagu :smontagu]

Attachment: With the suggested correction

Comment 31

[Simon Montagu :smontagu]

Patch checked in.

Comment 32

[Asaf Romano (gone)]

(In reply to comment #31)
> Patch checked in.

What about aviary and 1.7.3 branches?

Comment 33

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

ONLY if they're critical for some reason. We are not backporting all Gecko fixes
from 1.8 to 1.7 and Aviary.

Comment 34

[Asaf Romano (gone)]

(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

[Reuven Gonen]

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

[Asa Dotzler [:asa]]

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

not this close to PR. maybe after

Comment 37

[Benjamin Smedberg]

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.

Comment 38

[Benjamin Smedberg]

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

re-nominating for aviary and 1.7

Comment 39

[Asa Dotzler [:asa]]

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

a=asa for branch checkins.