140759 - tree cell content is not aligned to the right when tree direction is right to left

Status: VERIFIED FIXED

(Core :: XUL, defect, P3)

Description

[Tsahi Asher]

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

Description: when the interface is aligned to the right, e.g. when using a RTL
language (like hebrew or arabic), trees (like the preferences category tree or
the accounts tree in the Messenger) are not aligned to the right of the text,
and remain at the left side of the tree window.

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
4. observe the category tree

Actual Results: tree is on the left edge of the categories pane

Expected Results: tree should be on the right side of the categories pane

Comment 1

[Tsahi Asher]

Attachment: screen shot

Comment 2

[Boris Zbarsky [:bzbarsky]]

This is at least partly because you have

outliner { direction: rtl; }

The outliner tag is no more; it is now named tree.  So you want

tree { direction: rtl; }

Comment 3

[Tsahi Asher]

see news://news.mozilla.org:119/[e-mail] by Simon montagu.
also, replacing 'outliners' with 'tree' doesn't change anything (actually,
having or not having 'outliners' doesn't change anything either)

here is my intl.css after adding the 'tree' element:

/*
 * This file contains all localizable skin settings such as 
 *   font, layout, and geometry
 */
window { 
  font: 3mm tahoma,arial,helvetica,sans-serif;
}

/* align UI to the right */

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

menu { direction: rtl; } 

outliner { direction: rtl; } 

tree { direction: rtl; }  

Comment 4

[Simon Montagu :smontagu]

See also bug 135272

Comment 5

[Jason M. Abels]

Reporter: can you repeat this with a modern mozilla build? If not, mark WFM.

Comment 6

[Tsahi Asher]

this bug exists in mozilla 1.3. didn't check 1.4 yet, but i have no reason to
believe it is fixed there.

Comment 7

[Jason M. Abels]

confirming.

Comment 8

[Asaf Romano (gone)]

Problematic code is here: 
http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2350

Comment 9

[Asaf Romano (gone)]

Tsahi, also change hardware to all ;). I also think that it isn't a "Latout:
BiDi Hebrew & Arabic", but i'm not sure what it does  (you are even welcome to
assign it to me, so i can handle it by myself...).

Comment 10

[Tsahi Asher]

i wasn't sure how it is on Mac.
almost all bugs with RTL UI are under this component. perhaps Localization is
better? (in this case, all bugs with RTL UI should be moved).
i'm trying to reassign this to you. if it's not reassigned, then i don't have
enough priviliges here.

Comment 11

[Asaf Romano (gone)]

Attachment: Fix

Patch for painting RTL trees

Comment 12

[Asaf Romano (gone)]

Attachment: Fix for RTL trees

(Replacing hard tabs with spaces)

Comment 13

[Asaf Romano (gone)]

Comment on attachment 149839 [details] [diff] [review]
Fix for RTL trees

Sorry for bug spam..

Comment 14

[Asaf Romano (gone)]

One thing I forgot to mention: In order to not take risks, i didn't rewrite the
draw_connecting_lines code so it will be prefect as for RTL as it is for LTR,
but it's fixed in a way that the RTL themes can solve it via their css (treeline
class).

Comment 15

[Simon Montagu :smontagu]

Comment on attachment 149839 [details] [diff] [review]
Fix for RTL trees

This is very nearly there, but still needs a bit more polish. I'll attach an
image with comments in a second.

Thanks very much for taking this.

Comment 16

[Simon Montagu :smontagu]

Attachment: screenshot with comments

I see from your last comment that some of the issues can be fixed with css. Can
you attach an example?

Comment 17

[Asaf Romano (gone)]

(In reply to comment #16)
> Created an attachment (id=149854)
> screenshot with comments
> 
> I see from your last comment that some of the issues can be fixed with css. Can
> you attach an example?

I'm uploading a new patch in a couple of minutes

Comment 18

[Asaf Romano (gone)]

Attachment: fix

My mistkae, correcting the connecting lines, now it is OK.

I will be back in an hour to check the other problems, Simon you are welcome to
help :-)

Comment 19

[[e-mail]]

Comment on attachment 149837 [details] [diff] [review]
Fix

I've only been skimming this patch but I've got a few nits: Firstly, you keep
on converting the style direction to a bidi direction although I don't see the
point, why not just compare the style direction everywhere? Secondly, I think
you should make PaintTwisty and PaintImage paint at the appropriate side of the
rectangle (and shrink it appropriately), rather than split the logic into
PaintCell. Finally, GetTextAlignment() already handles RTL which explains why
you only saw fit to adjust right aligned text, although it is possible to
specify centered or reversed alignment which you do not seem to have accounted
for.

Comment 20

[Asaf Romano (gone)]

(In reply to comment #19)
> (From update of attachment 149837 [details] [diff] [review])
> I've only been skimming this patch but I've got a few nits: Firstly, you keep
> on converting the style direction to a bidi direction although I don't see the
> point, why not just compare the style direction everywhere?
That's right, We can just take the mDirection from nsStyleVisibility. I'll change it

> Secondly, I think
> you should make PaintTwisty and PaintImage paint at the appropriate side of the
> rectangle (and shrink it appropriately), rather than split the logic into
> PaintCell.
In my code (still working on it..) I moved some of the logic from PaintCell to
PaintTwisty, and i can probably do the same fo PaintImage, BUT I  can't move the
 connecting lines drawing code into other method.
> Finally, GetTextAlignment() already handles RTL which explains why
> you only saw fit to adjust right aligned text, although it is possible to
> specify centered or reversed alignment which you do not seem to have accounted
> for.
> 
I saw that, but i was wondering what do we mean "Right text-aligned tree",
Alignment and Direction are different things. I'm realy not sure (and the
current code "not sure" too) how we should render
----
tree
{
  text-align: right;
}
----
But that is realy an edge case, I can't see the case when someone would do that
(except of checking if it's buggy ;) )

I'm changing the bug summary since I know what the bug reporter meant.

Comment 21

[Asaf Romano (gone)]

Attachment: Right-aligned tree (NOT RTL DIRECTION)

That's how we render
-----
tree
{
  text-align: right;
}
----

IMO it should stay like that. reverse rendering order should be done only for
RTL trees.

Comment 22

[Tsahi Asher]

(In reply to comment #20)
> 
> I'm changing the bug summary since I know what the bug reporter meant.
> 

you may know what i mean, but not everyone knows. until this is fixed, i think
it would be best to leave the summary as it is.

summaries are usually changed only if the bug is not exactly what it seemed at
first.

Comment 23

[Asaf Romano (gone)]

(In reply to comment #22)
> (In reply to comment #20)
> > 
> > I'm changing the bug summary since I know what the bug reporter meant.
> > 
> 
> you may know what i mean, but not everyone knows. until this is fixed, i think
> it would be best to leave the summary as it is.
> 
> summaries are usually changed only if the bug is not exactly what it seemed at
> first.

But this is the case. we should not reverse the rendering oreder of *just* right
aligned trees.

Comment 24

[Tsahi Asher]

how about this?

Comment 25

[Asaf Romano (gone)]

OK Tsahi.

One major problem: Opening a tree node via the mouse is still by clicking on the
*orginal* place of the twisity (the arrow).

Srill working on that...

Comment 26

[Simon Montagu :smontagu]

(In reply to comment #25)
> OK Tsahi.
> 
> One major problem: Opening a tree node via the mouse is still by clicking on the
> *orginal* place of the twisity (the arrow).
> 
> Srill working on that...

I believe that you need to patch nsTreeBodyFrame::GetItemWithinCellAt to solve
that problem.

Comment 27

[Asaf Romano (gone)]

Great!!! Now, we just need to workaround the space_before_twist problem.

Comment 28

[Simon Montagu :smontagu]

Attachment: w-i-p patch

(In reply to comment #27)
> Great!!! Now, we just need to workaround the space_before_twist problem.

I have a fix for that too :)
You just need to make the adjustment to the x coordinate right before the
twisty is drawn, as in this version of your patch.

Comment 29

[Asaf Romano (gone)]

Attachment: More progrees

1. I forgot to handle progrees meteres, this patch handle them. The good news:
This completely solves bug 143344. :-) :-)
2. I removed all the places where I converted nsStyleVisibility:mDirection to
nsBidiDirection.
3. Start to solve the click on twist problem, but it is still there :-\

I also found a very wired behaviour of #urlbar, when it's RTLed, although It
will never be so (The hebrew LPack force-LTR it), we have to check why.

Simon, this patch created after applying your last patch, if you are going to
add something, please continue from this point, 10x.

Comment 30

[[e-mail]]

Comment on attachment 149900 [details]
Right-aligned tree (NOT RTL DIRECTION)

I couldn't figure out how to duplicate this, does this need your patch to work?
Only I thought that GetTextAlignment considers both align and ltr.

Comment 31

[[e-mail]]

(In reply to comment #30)
>(From update of attachment 149900 [details])
>I couldn't figure out how to duplicate this, does this need your patch to work?
>Only I thought that GetTextAlignment considers both align and ltr.
OK, so I managed to get right-aligned text in a tree via a css file;
I found that both text-align: right; and direction: rtl; worked.

Comment 32

[Tsahi Asher]

(In reply to comment #29)
> Created an attachment (id=150000)
> More progrees
> 
hey, that's a nice round number!

> 
> I also found a very wired behaviour of #urlbar, when it's RTLed, although It
> will never be so (The hebrew LPack force-LTR it), we have to check why.
> 

not accurate. the address bar is LTRed from bug 157607.

Comment 33

[Asaf Romano (gone)]

(In reply to comment #31)
> (In reply to comment #30)
> >(From update of attachment 149900 [details])
> >I couldn't figure out how to duplicate this, does this need your patch to work?
> >Only I thought that GetTextAlignment considers both align and ltr.
> OK, so I managed to get right-aligned text in a tree via a css file;
> I found that both text-align: right; and direction: rtl; worked.

*you need* the patch to see the difference between text-align: right and
direction:rtl.  I will upload a screenshot later.

Tsahi(In reply to comment #32)
> (In reply to comment #29)

> > I also found a very wired behaviour of #urlbar, when it's RTLed, although It
> > will never be so (The hebrew LPack force-LTR it), we have to check why.
> > 
> 
> not accurate. the address bar is LTRed from bug 157607.

Not for me (if idont forrc ltr the urlbar, it's RTLed).

Comment 34

[Asaf Romano (gone)]

Oh, my mistkae, i'm not talking about #urlbar, the problem is with the
autocomplete box under it.

Comment 35

[Asaf Romano (gone)]

Attachment: Another progress.

Clicking problem is behind. This also patches PaintSeperator, but it still
needs work (in PaintRow).

Comment 36

[Asaf Romano (gone)]

Attachment: (Probably) Final Patch

AutoComplete Problem -> Solve (It has happened becauee i'm an idiot).
Seperator -> Solved.

BTW, according to the AutoComplete field, The BiDi versions (if they don't
force ltr it) need to define a margin-right to its class. In the current classs
it has 0px, so there is no space between the address to the border.

Comment 37

[Asaf Romano (gone)]

Comment on attachment 150106 [details] [diff] [review]
(Probably) Final Patch

Requesting review from Simon.

Comment 38

[Asaf Romano (gone)]

some comment for r/sr:
1. If you see text which has no right margin as you excpect (e.g. AddressBar
Autocomplete), this is because they don't have one in the stylesheets, in this
cases the BiDi LPs should change their class.
2. In PaintRow, I made a little change, in the orginal code seperatorRect has
been defined after if (isPre...{...} now it's before (still created fro the
values of rowRect) and the changes to its width and x are being done inside the
if block, that's because at first. if aColumn->IsPrimary() was FALSE the changes
to seperatorRect were -0 to the width and -0 to x...

Comment 39

[Simon Montagu :smontagu]

Comment on attachment 150106 [details] [diff] [review]
(Probably) Final Patch

-      nsRect cellRect(primaryCol->GetX(), rowRect.y, primaryCol->GetWidth(),
rowRect.height);
+      nsRect cellRect(primaryCol->GetX(), rowRect.y, primaryCol->GetWidth(),
rowRect.height);    

Please remove this whitespace change.

>-        currX = previousCol->GetX() + previousCol->GetWidth();
>+      {
>+        currX = previousCol->GetX();
>+        if (vis->mDirection==NS_STYLE_DIRECTION_LTR)
>+            currX+=previousCol->GetWidth();
>+      }

Nit: please make the curly-bracket-positioning style consistent. The original
file uses K+R style, so stick to that throughout the patch.

Please also add a comment here explaining why currX doesn't need to be modified
when direction is RTL.

>+    // Because I didn't want to cause breaks, I'm reimplemnting nsRect::Deflate.

"reimplementing" everywhere this comment occurs.

>@@ -2477,9 +2531,10 @@
>                 remainingWidth, currX);
>   }
>   
>-  // Now paint the icon for our cell.
>-  nsRect iconRect(currX, cellRect.y, remainingWidth, cellRect.height);
>   nsRect dirtyRect;
>+  nsRect iconRect(currX, cellRect.y, remainingWidth, cellRect.height);
>+  
>+  // Now paint the icon for our cell.

Since this change doesn't do anything, you might as well remove it.

r=smontagu with these changes, but I'd like to see some more QA. Shosh, can you
help out?

Comment 40

[Asaf Romano (gone)]

Attachment: W.I.P. patch for 1.7 branch

Simon, there is a very strange problem with this patch (which doen't happen
with the trunk patch), when there is more than one column, all the tree is too
much lefted, and i (still) can't find out why, more strange is that all
functions get give the same behaviour so nsTreeBodyFrame::GetItemWithinCellAt
hasn't been braked (meaning it returns the right place where the twisky does
appear).

Comment 41

[Asaf Romano (gone)]

Well,the problem is in PaintRow (placement in BRANCH patch), now I need to
workaround it...

Comment 42

[Asaf Romano (gone)]

Attachment: Final Patch for trunk, addressing Simon's comments

btw, there is another problem with rtl trees where resizing of a column is done
via the previous column you would like to resize, I will open a seperate bug
for this later.

Comment 43

[Asaf Romano (gone)]

Comment on attachment 150254 [details] [diff] [review]
Final Patch for trunk, addressing Simon's comments

Asking for r/sr

Comment 44

[Tsahi Asher]

(In reply to comment #42)
> btw, there is another problem with rtl trees where resizing of a column is done
> via the previous column you would like to resize, I will open a seperate bug
> for this later.

this is bug 176244.

Comment 45

[[e-mail]]

>     switch (aColumn->GetTextAlignment()) {
>       case NS_STYLE_TEXT_ALIGN_RIGHT: {
>-        textRect.x += textRect.width - width;
>+        if (vis->mDirection==NS_STYLE_DIRECTION_RTL) {
>+            aRenderingContext.GetWidth(text, width);
>+            textRect.x -= width;
>+        }
>+        else
>+            textRect.x += textRect.width - width;
>       }
>       break;
>       case NS_STYLE_TEXT_ALIGN_CENTER: {
Using a plain tree (i.e. without twisties, icons or lines) then direction: ltr;
and text-align: right; both automatically gave the same apparently correct
display. Now obviously the rest of your patch is needed to fix the twisties and
icons to draw in the right place, but I don't see why you need this change.

Comment 46

[Asaf Romano (gone)]

(In reply to comment #45)
> >     switch (aColumn->GetTextAlignment()) {
> >       case NS_STYLE_TEXT_ALIGN_RIGHT: {
> >-        textRect.x += textRect.width - width;
> >+        if (vis->mDirection==NS_STYLE_DIRECTION_RTL) {
> >+            aRenderingContext.GetWidth(text, width);
> >+            textRect.x -= width;
> >+        }
> >+        else
> >+            textRect.x += textRect.width - width;
> >       }
> >       break;
> >       case NS_STYLE_TEXT_ALIGN_CENTER: {
> Using a plain tree (i.e. without twisties, icons or lines) then direction: ltr;
> and text-align: right; both automatically gave the same apparently correct
> display. Now obviously the rest of your patch is needed to fix the twisties and
> icons to draw in the right place, but I don't see why you need this change.

That's because after applying the path PaintCell behaves different for RTL
(adding remaingWidth).

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 150254 [details] [diff] [review]
Final Patch for trunk, addressing Simon's comments

Any chance you could produce a patch revised so that it follows the coding
style of the code around it.  I'd rather not have to point out all the places
where you don't put space around =, ==, +=, etc., where you switch from
two-space to four-space indent, and where you over-parenthesize the right side
of an assignment.

Comment 48

[Asaf Romano (gone)]

Attachment: Same fix with CodeStyle tweaks

Comment 49

[Asaf Romano (gone)]

Attachment: Same fix with CodeStyle tweaks

Sorry for bug spam. I forgot something from Simon's comments

Comment 50

[[e-mail]]

(In reply to comment #46)
>(In reply to comment #45)
>>>     switch (aColumn->GetTextAlignment()) {
>>>       case NS_STYLE_TEXT_ALIGN_RIGHT: {
>>I don't see why you need this change.
>That's because after applying the path PaintCell behaves different for RTL
>(adding remaingWidth).
Then why did you only change the NS_STYLE_TEXT_ALIGN_RIGHT case?

Comment 51

[Asaf Romano (gone)]

(In reply to comment #50)
> (In reply to comment #46)
> >(In reply to comment #45)
> >>>     switch (aColumn->GetTextAlignment()) {
> >>>       case NS_STYLE_TEXT_ALIGN_RIGHT: {
> >>I don't see why you need this change.
> >That's because after applying the path PaintCell behaves different for RTL
> >(adding remaingWidth).
> Then why did you only change the NS_STYLE_TEXT_ALIGN_RIGHT case?

I don't see the tree which will be RTL and NOT RIGHT aligned. If it bothers you,
I will open a seperate bug for it, after this will be fixed, but this is a case
which will probably never be tested.

Comment 52

[Asaf Romano (gone)]

Comment on attachment 150495 [details] [diff] [review]
Same fix with CodeStyle tweaks

Only style changes, moving review + flag.

Comment 53

[Boris Hajduk]

This bug looks very similar to bugs #140636 and #185233
I suppose they are closely related, and probably the same problem cause the 3 bugs.

Comment 54

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 150495 [details] [diff] [review]
Same fix with CodeStyle tweaks

>+  // In order to check the tree direction
>+  const nsStyleVisibility* vis = GetStyleVisibility();

Rather than constantly dereferencing vis to get the direction, why not just
have either:

PRUint8 direction = GetStyleVisibility()->mDirection;

or

PRBool isLTR = GetStyleVisibility()->mDirection == NS_STYLE_DIRECTION_LTR;

and using that instead.

>+      if ((vis->mDirection == NS_STYLE_DIRECTION_LTR? primaryX : cellRect.x+cellRect.width-primaryX) > rowRect.x) {
>+        separatorRect.width -= (vis->mDirection == NS_STYLE_DIRECTION_LTR? primaryX : cellRect.x+cellRect.width-primaryX) - rowRect.x;

You're repeating the same calculation twice here.  Put it in a variable, test
if it's greater than zero, and if so subtract it from separatorRect.width.

>+      nscoord srcX;
>       for (PRInt32 i = level; i > 0; i--) {
>         if (i <= maxLevel) {
>-          lineX = currX + twistySize.width + mIndentation / 2;
>-
>-          nscoord srcX = lineX - (level - i + 1) * mIndentation;

Why move this out of the loop and the if?

I'm up to here, but it's probably best if you attach another patch addressing
these comments (particularly the first) that I can look at.  And please attach
a diff ignoring whitespace (i.e., add -b to the diff options) *as well* as the
normal one.

Comment 55

[Asaf Romano (gone)]

Attachment: Addressing David's comments

Also removing some no-more-relevant XXX comments (saying we need to handle what
this patch handles...).

Comment 56

[Asaf Romano (gone)]

Attachment: -b diff

Also attaching -b diff

Comment 57

[[e-mail]]

(In reply to comment #51)
>I don't see the tree which will be RTL and NOT RIGHT aligned. If it bothers
>you, I will open a seperate bug for it, after this will be fixed, but this
>is a case which will probably never be tested.

In mail's folder pane, the (normally hidden?) columns for unread, total and size
are right aligned in LTR, and therefore left aligned in RTL, no?

So much for a case which will probably never be tested :-P

Comment 58

[Asaf Romano (gone)]

neil, there is no reason to close a bug if the patch hasn't checked in.....

Comment 59

[Tsahi Asher]

Asaf, priority was P2.

Comment 60

[Asaf Romano (gone)]

(In reply to comment #59)
> Asaf, priority was P2

Look at the bug activity page, who marked is as p2-bug? :-)

Comment 61

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 151196 [details] [diff] [review]
Addressing David's comments

So I've been trying to avoid reviewing this patch for a while, which wasn't
really the right thing to do, since this patch seems very difficult to
understand.  I think it's time I just marked it superreview- since I think it's
unlikely that I'd approve of what it's doing if I did understand it.

Messing with lots of complicated math seems like the wrong approach here.  I'm
not even sure what you're doing to that math -- it sometimes looks like you're
inverting the meaning of nsRect, though without any comments in the bug or in
the patch explaining what you're doing, I really can't tell.

My understanding of what you're trying to fix here (correct me if I'm wrong),
is that when trees have 'direction: rtl', they need to display from the first
column at the right to the last column at the left.  Is that what this bug is
fixing, or is it fixing other issues?

If that's what it's fixing, can't you just do something (perhaps near or in
nsTreeColumns) that just considers the columns in the opposite order.

The code here is difficult to understand and poorly documented, and I fully
expect that if anyone ever changes the rest of the tree code the RTL code here
would break.

Comment 62

[Asaf Romano (gone)]

No David! What this patch fixes is the order of rendering in *each* column, the
point is that when your'e drawing through the x ruller, it breaks RTL casess (x
is x... rtl first point is the totoal width instead of 0, and the last point is
in x=0).

IF you can't sr it, let someone else do that, these calcuations are very logic
from the BiDi point of view (bascicly, it's just width-x, instead of x).


sigh

Comment 63

[Asaf Romano (gone)]

Comment on attachment 151196 [details] [diff] [review]
Addressing David's comments

Asking for sr=

At this point, i dont know from who...

Comment 64

[Jungshik Shin]

Asking 'nobody' for sr would never work especially after being denied sr.  Why
don't you at least address dbaron's concern about the poor documentation before
trying again if you think it's not a code issue but a communication problem? 

Comment 65

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #62)
> No David! What this patch fixes is the order of rendering in *each* column, the
> point is that when your'e drawing through the x ruller, it breaks RTL casess (x
> is x... rtl first point is the totoal width instead of 0, and the last point is
> in x=0).

I have no idea what this comment means.  What's an "x ruler"?  What's currently
broken about the rendering within columns?  This bug should have a testcase with
expected and actual results described.

Comment 66

[Asaf Romano (gone)]

I don't see the missing documentation, for example:

+    if (treeDir == NS_STYLE_DIRECTION_LTR)
+      AdjustForBorderPadding(meterContext, meterRect);
+    else
+    {
+      // Because I didn't want to cause breaks, I'm reimplementing
AdjustForBorderPadding and nsRect::Deflate.
+      nsMargin borderPadding(0, 0, 0, 0);
+      GetBorderPadding(meterContext, borderPadding);
+      meterRect.x -= borderPadding.right;
+      meterRect.y += borderPadding.top;
+      meterRect.width -= borderPadding.right+borderPadding.left;
+      meterRect.height -= borderPadding.top+borderPadding.bottom;
+    }

Take a minute to see that AdjustForBorderPadding will be broken for rtl, because
of this:
http://lxr.mozilla.org/seamonkey/source/gfx/src/nsRect.cpp#174 (x+=marginLeft
should be x-=marginright)

There are no magic in this code that should be over documented, just reverting
the drawing order for RTL by (over the x-ruller (i.e. RTL :) )).

Finally, i have a problem with SRing without ask first
"Sorry, i'm not sure what is this bug": 
"My understanding of what you're trying to fix here (correct me if I'm wrong),
is that when trees have 'direction: rtl', they need to display from the first
column at the right to the last column at the left.".

Comment 67

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #66)
> Take a minute to see that AdjustForBorderPadding will be broken for rtl, because
> of this:
> http://lxr.mozilla.org/seamonkey/source/gfx/src/nsRect.cpp#174 (x+=marginLeft
> should be x-=marginright)

No, this is exactly what I was talking about as something that I'd object to. 
In nsRect, x is always the left edge, no matter what direction is.  That makes
some RTL stuff a little difficult, but it's much easier than having nsRect mean
different things in different contexts.

> Finally, i have a problem with SRing without ask first
> "Sorry, i'm not sure what is this bug": 
> "My understanding of what you're trying to fix here (correct me if I'm wrong),
> is that when trees have 'direction: rtl', they need to display from the first
> column at the right to the last column at the left.".

OK, so what is the bug?  Since you claim that what I thought was broken and what
the screenshots show as broken isn't what you're fixing, what is it that you're
fixing?

Comment 68

[Asaf Romano (gone)]

x ruller means horizontal coordinate (in other words .x property of each element)

well, when your'e drawing rtl elements, code like this:
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2398

ignores rtl (you are "going" (x+=)) through the ltr direction.

anf the suggested fix:
+    currX +=(treeDir == NS_STYLE_DIRECTION_LTR? mIndentation * level:
-mIndentation * level);

what is so not clear?

Comment 69

[Tsahi Asher]

(In reply to comment #65)

> I have no idea what this comment means.  What's an "x ruler"?  What's currently
> broken about the rendering within columns?  This bug should have a testcase with
> expected and actual results described.
> 

what's currently broken is that the trees are not aligned right (or should i
say, "don't take RTL direction") when UI direction is RTL. attachment 81393 [details]
shows exactly the problem: the UI is RTL (using CSS), and the trees are where
they are when the UI is LTR.

i expect the trees in a RTL locale to be on the right side of the tree pane,
with the "branches" pointing left instead of right.

Comment 70

[Asaf Romano (gone)]

(In reply to comment #67)
> (In reply to comment #66)
> > Take a minute to see that AdjustForBorderPadding will be broken for rtl, because
> > of this:
> > http://lxr.mozilla.org/seamonkey/source/gfx/src/nsRect.cpp#174 (x+=marginLeft
> > should be x-=marginright)
> 
> No, this is exactly what I was talking about as something that I'd object to. 
> In nsRect, x is always the left edge, no matter what direction is.  That makes
> some RTL stuff a little difficult, but it's much easier than having nsRect mean
> different things in different contexts.
> 
You will see that when your'e
http://lxr.mozilla.org/seamonkey/source/gfx/src/nsRect.cpp#174
for the progress bar (download manager tree) it is sifted out instead of shifted in.

> > Finally, i have a problem with SRing without ask first
> > "Sorry, i'm not sure what is this bug": 
> > "My understanding of what you're trying to fix here (correct me if I'm wrong),
> > is that when trees have 'direction: rtl', they need to display from the first
> > column at the right to the last column at the left.".
> 
> OK, so what is the bug?  Since you claim that what I thought was broken and what
> the screenshots show as broken isn't what you're fixing, what is it that you're
> fixing?

The screenshot show the problem...the rendering order is wrong **in the cell**.

Comment 71

[Asaf Romano (gone)]

Attachment: Showing the problem to dbraon

Columns rendering is OK. In-the-cell rendering is wrong.

Comment 72

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #68)
> well, when your'e drawing rtl elements, code like this:
>
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2398
> 
> ignores rtl (you are "going" (x+=)) through the ltr direction.

Code like that would be fine if you start with the last column and proceed to
the first column, which seems like it would be much simpler than what this patch
does.

(In reply to comment #69)
> what's currently broken is that the trees are not aligned right (or should i
> say, "don't take RTL direction") when UI direction is RTL. attachment 81393 [details]
> shows exactly the problem: the UI is RTL (using CSS), and the trees are where
> they are when the UI is LTR.
> 
> i expect the trees in a RTL locale to be on the right side of the tree pane,
> with the "branches" pointing left instead of right.

I think you're getting all the terms wrong, which makes it pretty much
impossible to have a useful discussion of what the problem is.  I think we call
the tree the whole table-like thing (what you call the "tree pane").  The little
triangles or pluses at the edge are twistys.

Comment 73

[Asaf Romano (gone)]

David, see last screenshot pleaee, there is no problem in the order of columns
(thas what you said in comment 72, correct me if i'm wrong).

Comment 74

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Giving bug a summary that I think accurately describes the problem.

Comment 75

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If all you're changing is in-cell stuff, then the general approach is
acceptable. However, nsRect should be used without changing the coordinate
space, like it is in all the rest of Mozilla's code, so my sr- still stands.

Comment 76

[Asaf Romano (gone)]

This is not just the twisy, it's also the indatation, image (if there is one),
and other stuff (e.g placing of the text).

Comment 77

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, if you move the twisty from the left to the right, you should make room
for it on the right rather than on the left.  Are there other things this fixes
other than just the side-effects of moving it to the opposite side?

Comment 78

[Asaf Romano (gone)]

(In reply to comment #75)
> If all you're changing is in-cell stuff, then the general approach is
> acceptable. However, nsRect should be used without changing the coordinate
> space, like it is in all the rest of Mozilla's code, so my sr- still stands.

What's the deal? ha?
When nsRect +marginLeft instead of -marginRight (that's what shpuld logicly
happen .. you want it to be shifted in (going to the left side, not to right
one), do your realy think it's the right behavior for rtl!?

I nearly sure that's the reason for other bugs such as bug 74880 (not whole
issue, try to look at even not nested list in mozilla 1.6, bullet is sifted out
instead of shifted in), this has been fixed with some padding css hacks.

Comment 79

[Asaf Romano (gone)]

(In reply to comment #77)
> Well, if you move the twisty from the left to the right, you should make room
> for it on the right rather than on the left.  Are there other things this fixes
> other than just the side-effects of moving it to the opposite side?

and...that's what happens after patching.. (i'm adding the coulmn with in PaintCell), also see 
PaintTwisty, PaintCell, PaintImgee...

Comment 80

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 151196 [details] [diff] [review]
Addressing David's comments

Perhaps it's not that anything is particularly wrong, just that the logic is
much more confused than it should be.  As an example, your changes to
PaintProgressMeter are the following:

>@@ -2993,17 +3088,36 @@
>   nsMargin meterMargin;
>   meterContext->GetStyleMargin()->GetMargin(meterMargin);
>   meterRect.Deflate(meterMargin);
>+  
>+  // Checking the tree direction
>+  PRUint8 treeDir = GetStyleVisibility()->mDirection;
>+   
>+  if (treeDir == NS_STYLE_DIRECTION_RTL)
>+    meterRect.x -= meterRect.width;
> 
>   // Paint our borders and background for our progress meter rect.
>   PaintBackgroundLayer(meterContext, aPresContext, aRenderingContext, meterRect, aDirtyRect);
> 
>+  if (treeDir == NS_STYLE_DIRECTION_RTL)
>+    meterRect.x += meterRect.width;    // That's because the width will be changed later.
>+
>   // Time to paint our progress. 
>   PRInt32 state;
>   mView->GetProgressMode(aRowIndex, aColumn, &state);
>   if (state == nsITreeView::PROGRESS_NORMAL) {
>     // Adjust the rect for its border and padding.
>-    AdjustForBorderPadding(meterContext, meterRect);
>-
>+    if (treeDir == NS_STYLE_DIRECTION_LTR)
>+      AdjustForBorderPadding(meterContext, meterRect);
>+    else
>+    {
>+      // Because I didn't want to cause breaks, I'm reimplementing AdjustForBorderPadding and nsRect::Deflate.
>+      nsMargin borderPadding(0, 0, 0, 0);
>+      GetBorderPadding(meterContext, borderPadding);
>+      meterRect.x -= borderPadding.right;
>+      meterRect.y += borderPadding.top;
>+      meterRect.width -= borderPadding.right+borderPadding.left;
>+      meterRect.height -= borderPadding.top+borderPadding.bottom;
>+    }
>     // Set our color.
>     aRenderingContext.SetColor(meterContext->GetStyleColor()->mColor);
> 
>@@ -3019,6 +3133,10 @@
>       intValue = 100;
> 
>     meterRect.width = NSToCoordRound((float)intValue / 100 * meterRect.width);
>+    
>+    if (treeDir == NS_STYLE_DIRECTION_RTL)
>+      meterRect.x -= meterRect.width;
>+
>     PRBool useImageRegion = PR_TRUE;
>     nsCOMPtr<imgIContainer> image;
>     GetImage(aRowIndex, aColumn, PR_TRUE, meterContext, useImageRegion, getter_AddRefs(image));
>@@ -3030,7 +3148,9 @@
>   else if (state == nsITreeView::PROGRESS_UNDETERMINED) {
>     // Adjust the rect for its border and padding.
>     AdjustForBorderPadding(meterContext, meterRect);
>-
>+    if (treeDir == NS_STYLE_DIRECTION_RTL)
>+      meterRect.x -= meterRect.width;
>+      
>     PRBool useImageRegion = PR_TRUE;
>     nsCOMPtr<imgIContainer> image;
>     GetImage(aRowIndex, aColumn, PR_TRUE, meterContext, useImageRegion, getter_AddRefs(image));


Instead, I think you should only need to do the following (which involves
having the correct meterRect.x passed in instead of the right edge that I
presume you change it to elsewhere):

>       intValue = 100;
> 
>-    meterRect.width = NSToCoordRound((float)intValue / 100 * meterRect.width);
>+    nscoord meterWidth = NSToCoordRound((float)intValue / 100 * meterRect.width);
>+    if (GetStyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL)
>+      meterRect.x += meterRect.width - meterWidth; // right align
>+    meterRect.width = meterWidth;
>+
>     PRBool useImageRegion = PR_TRUE;
>     nsCOMPtr<imgIContainer> image;
>     GetImage(aRowIndex, aColumn, PR_TRUE, meterContext, useImageRegion, getter_AddRefs(image));

Comment 81

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #80)
> Perhaps it's not that anything is particularly wrong, just that the logic is
> much more confused than it should be.  As an example, your changes to

Oops, I wrote that before I realized that you'd changed PaintProgressMeter to
accept an nsRect whose nsRect::x was referring to the right edge, which is
wrong.  So this shows how much simpler it would be if you didn't do that.

Comment 82

[Asaf Romano (gone)]

-> 1.8b

Comment 83

[Shoshannah Forbes]

FYI: This bug also makes the threaded msg view in the email pretty much 
unusable.

Comment 84

[Simon Montagu :smontagu]

Attachment: Simpler approach

Comment 85

[[e-mail]]

Comment on attachment 245013 [details] [diff] [review]
Simpler approach

>   nsRect iconRect(currX, cellRect.y, remainingWidth, cellRect.height);
>+  if (treeDir == NS_STYLE_DIRECTION_RTL)
>+    iconRect.x = currX + remainingWidth - iconRect.width;

>     nsRect elementRect(currX, cellRect.y, remainingWidth, cellRect.height);
>+    if (treeDir == NS_STYLE_DIRECTION_RTL)
>+      elementRect.x = currX + remainingWidth - elementRect.width;

There are three cases of this pattern.
The first case, which looks right, I can see the rect width is set first.
The second case, I can't see what the rect width is set to.
The third case, above, the rect width is the remaining width.
This means that the added code serves no purpose.

Comment 86

[Simon Montagu :smontagu]

(In reply to comment #85)
> This means that the added code serves no purpose.

You're right, the real repositioning is happening in PaintImage(), PaintText() etc.

Which is the case where you don't see what the rect width is set to? 

Comment 87

[Asaf Romano (gone)]

Comment on attachment 245013 [details] [diff] [review]
Simpler approach

Looks good except:
 * each column still seems to be few pixels off compared to his header.
 * RTL Editable trees are still very broken, see the "testcase" at http://wiki.mozilla.org/XUL:Tree
 * comment 85

Comment 89

[(no longer active)]

Nominating as blocking1.9.  This is crucial for RTL languages.  Without this, many of the places in the UI for various Gecko-based apps look broken, for example, the Library in Firefox 3.0.

Comment 90

[Tomer Cohen :tomer]

We are year and half since the last bug activity to solve this issue, and we need this 6-year-old-bug fixed in order to ship fully RTL-compatible user interfaces for Firefox, Thunderbird, and any other Gecko-based software which has an RTL interface. 

Please fix before Firefox 3 final release.

Comment 91

[(no longer active)]

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

Comment 93

[(no longer active)]

Attachment: Updated Patch

I updated the patch to trunk, did some cleanup, and addressed comment 85.  The first and third problems in comment 87 seem to be fixed now.  What I need help with is:

1. Testing the patch.  Behnam, Tomer, anyone else interested: please give this patch a try with the latest trunk and make sure everything is looking good.

2. I'm not sure where to start in order to fix the editable trees.

Would this patch at this point be considered with leaving the fix for editable trees to a followup bug?  It would be great to have this for 1.9 for RTL locales (ar, he and fa).

Comment 94

[Tomer Cohen :tomer]

(In reply to comment #93)
> 1. Testing the patch.  Behnam, Tomer, anyone else interested: please give this
> patch a try with the latest trunk and make sure everything is looking good.

Ehsan - I've seen you didn't checked in yet your patch. Do you mind creating binaries for the changes? I think we can use the tryserver for that.

Comment 95

[(no longer active)]

(In reply to comment #94)
> (In reply to comment #93)
> > 1. Testing the patch.  Behnam, Tomer, anyone else interested: please give this
> > patch a try with the latest trunk and make sure everything is looking good.
> 
> Ehsan - I've seen you didn't checked in yet your patch. Do you mind creating
> binaries for the changes? I think we can use the tryserver for that.

I don't have access to the tryserver yet.  If someone can post this patch to the tryserver, I'd appreciate it.  If not, please let me know which platform would you like to test, so that I can make a build ready for you to try.  I can build on Windows and Linux at the moment.

Comment 96

[(no longer active)]

Attachment: Updated to trunk

The same patch as attachment 318026 [details] [diff] [review], updated to trunk.

I've submitted it to the try server.  I'll update here with a link to the try server build.

Comment 97

[(no longer active)]

Try server builds available at <https://build.mozilla.org/tryserver-builds/2009-01-06_06:[e-mail]-rtltree/>

Comment 98

[(no longer active)]

Attachment: Patch (v1)

This is the final patch, ready for review (request from roc on layout parts, and gavin on toolkit parts).

This fixes a number of problems with the previous patch (progressmeter support, as well as editable support) and also corrects the cell position calculations in presence of the columnpicker.

This is *finally* ready for review  :-)

I'll be attaching the test case I've been using shortly.  I have also submitted this patch to the try server.  The Force RTL extension <https://addons.mozilla.org/en-US/firefox/addon/7438> can also be used to test this on real XUL trees in Firefox UI.

Comment 99

[(no longer active)]

Attachment: XUL test case

Comment 100

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

Comment on attachment 356029 [details] [diff] [review]
Patch (v1)

I wonder if you could write a reftest for this using -moz-transfom to horizontally flip an LTR tree

Comment 101

[(no longer active)]

(In reply to comment #100)
> I wonder if you could write a reftest for this using -moz-transfom to
> horizontally flip an LTR tree

I gave that a shot, and it was failing because of a 1 pixel per column header difference (in Windows at least).  I poked around the code a bit, but didn't find out where the column headers are painted.  Can you give me a hint?

But overall, I'm not sure how much value such a test would have.  In real-world usage, we don't need an RTL tree layout to match a mirrored LTR layout 100%.  And such a test would be too fragile (because anything inside the tree needs to be 100% symmetric.

Maybe it'd be better to test some of the properties in RTL mode itself (I'm not sure what the desired properties to test here would be).

Anyway, this will definitely need Litmus tests...

Comment 102

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

OK, don't worry about it then

Comment 103

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 356029 [details] [diff] [review]
Patch (v1)

>diff --git a/toolkit/content/widgets/tree.xml b/toolkit/content/widgets/tree.xml

>+      <constructor><![CDATA[
>+          var style = window.getComputedStyle(this, "");
>+          this.setAttribute("chromedir", style.direction);
>+        ]]>
>+      </constructor>

chromedir is already set on the <content> using locale.dir from global.dtd. If you need to ensure that bindings that extend this binding and override it's <content> also get the attribute, I would prefer the trick from attachment 351009 [details] [diff] [review]. I see a couple of extenders of tree.xml#tree that specify <content> (in autocomplete.xml most notably, and a few in comm-central), so I guess that's probably best.

>             var left = outx.value;
>+            var height = outheight.value;
>+
>+            coords = box.getCoordsForCellItem(row, column, "cell",
>+                                              outx, outy, outwidth, outheight);
>+
>+            if (style.direction == "rtl")
>+              left = outx.value;
>+
>             input.left = left;
>             input.height = outheight.value + topadj +
>                            parseInt(style.borderBottomWidth) +
>                            parseInt(style.paddingBottom);
>-
>-            coords = box.getCoordsForCellItem(row, column, "cell",
>-                                              outx, outy, outwidth, outheight);
>             input.width = outwidth.value - (left - outx.value);
>+            input.left = outx.value;

I'm not sure I really understand the logic here, the variable re-use is kind of confusing. Can you explain?

Comment 104

[(no longer active)]

Attachment: Patch (v2)

(In reply to comment #103)
> (From update of attachment 356029 [details] [diff] [review])
> >diff --git a/toolkit/content/widgets/tree.xml b/toolkit/content/widgets/tree.xml
> 
> >+      <constructor><![CDATA[
> >+          var style = window.getComputedStyle(this, "");
> >+          this.setAttribute("chromedir", style.direction);
> >+        ]]>
> >+      </constructor>
> 
> chromedir is already set on the <content> using locale.dir from global.dtd. If
> you need to ensure that bindings that extend this binding and override it's
> <content> also get the attribute, I would prefer the trick from attachment
> 351009 [details]. I see a couple of extenders of tree.xml#tree that specify <content> (in
> autocomplete.xml most notably, and a few in comm-central), so I guess that's
> probably best.

Actually that was not what I had in mind.  :-)

I wanted to make sure that XUL trees can be made RTL by just specifying the CSS stylesheet rule.  Without that code, if you want a tree to be RTL (in an LTR build) you would have to set both style="direction: rtl" and chromedir="rtl" on it.

But now that I know about this trick, I think it would be good to use it here as well.  I did in this patch (which eliminates the behavior that I described from the previous patch).  Now that I'm thinking about it, that behavior may not really be that useful anyway.

> >             var left = outx.value;
> >+            var height = outheight.value;
> >+
> >+            coords = box.getCoordsForCellItem(row, column, "cell",
> >+                                              outx, outy, outwidth, outheight);
> >+
> >+            if (style.direction == "rtl")
> >+              left = outx.value;
> >+
> >             input.left = left;
> >             input.height = outheight.value + topadj +
> >                            parseInt(style.borderBottomWidth) +
> >                            parseInt(style.paddingBottom);
> >-
> >-            coords = box.getCoordsForCellItem(row, column, "cell",
> >-                                              outx, outy, outwidth, outheight);
> >             input.width = outwidth.value - (left - outx.value);
> >+            input.left = outx.value;
> 
> I'm not sure I really understand the logic here, the variable re-use is kind of
> confusing. Can you explain?

Sure.  In an LTR tree, the left-side of the textbox can be aligned to the left side of the text.  In an RTL tree, however, the left side of the text is somewhere along the middle of the cell, unless the text has overflown the cell width.  The |left| variable contains the left position of the textbox, which for LTR trees would be the left side of the text rect, and for RTL trees would be the left side of the cell rect.  The |height| variable's value is not changed -- I just save it before getting the cell rect, because otherwide |outheight| would be overwritten.  For RTL trees, I set the |left| variable to the left position of the cell rect.  From there on, everything moves forward like before.  (The |input.left = outx.value;| line was a mistake, and I removed it in this patch).

Comment 105

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 356137 [details] [diff] [review]
Patch (v2)

In this version you're not using the "height" variable you're adding. Can you just rewrite this to use different out variables for the "cell" and "text", to avoid all of this confusion? You can just pass {} for parameters you're not going to use.

I'm not really familiar with trees in general, so a comment explaining what exactly the "cell" coordinates represent vs. "text" coordinates would be useful too, unless that's already documented somewhere...

Comment 106

[(no longer active)]

Attachment: Patch (v3)

(In reply to comment #105)
> In this version you're not using the "height" variable you're adding. Can you
> just rewrite this to use different out variables for the "cell" and "text", to
> avoid all of this confusion? You can just pass {} for parameters you're not
> going to use.

Done.

> I'm not really familiar with trees in general, so a comment explaining what
> exactly the "cell" coordinates represent vs. "text" coordinates would be useful
> too, unless that's already documented somewhere...

I added a comment.  Here is some more info:

-+-----------------------+------
 | some text             |
-+-----------------------+------
   ^^^^^^^^^  => text coordinates
 ^^^^^^^^^^^^^^^^^^^^^^^^^  => cell coordinates

Comment 107

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 356344 [details] [diff] [review]
Patch (v3)

(In reply to comment #104)
> The |left| variable contains the left position of the textbox, which for LTR
> trees would be the left side of the text rect, and for RTL trees would be the
> left side of the cell rect.

Ok, so with this patch, the input width is the same as the cell width for RTL, since you end up substracting (cellx.value - cellx.value) from cellwidth.value to set input.width. Shouldn't that calculation always use textx.value rather than "left", so that there's an appropriate amount of padding where the text starts (either on the right or the left)?

r=me with that change or a suitable explanation.

Comment 108

[(no longer active)]

Attachment: Checked in

(In reply to comment #107)
> Ok, so with this patch, the input width is the same as the cell width for RTL,
> since you end up substracting (cellx.value - cellx.value) from cellwidth.value
> to set input.width. Shouldn't that calculation always use textx.value rather
> than "left", so that there's an appropriate amount of padding where the text
> starts (either on the right or the left)?

Actually, this needs a little more complex calculation for RTL, because the purpose of this calculation is to account for the space reserved for things like cell image or the twisty.  I made this correction.  Thanks for catching this!

I'd appreciate if someone can land this for me.

Comment 109

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/a77147340730

Comment 110

[(no longer active)]

Comment on attachment 356666 [details] [diff] [review]
Checked in

This bug has been there for too long and has bugged quite a few users of RTL locales (and possibly XUL application developers as well).  Requesting approval to land on 1.9.1.

Comment 111

[(no longer active)]

Attachment: Screenshot of the library window with this patch

Comment 112

[Tomer Cohen :tomer]

(In reply to comment #111)
> Created an attachment (id=356748) [details]
> Screenshot of the library window with this patch

Since this bug is highly welcome on Thunderbird (for the folders view), I think it worth checking.

Comment 115

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090208 Minefield/3.2a1pre and the identical build on OS X.

Comment 116

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 356666 [details] [diff] [review]
Checked in

a1.9.1=dbaron

Comment 117

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/475218f630f8

Comment 118

[Tomer Cohen :tomer]

(In reply to comment #117)
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/475218f630f8

Thanks! This issue is seven years old and about to be closed soon. :)

Comment 119

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks fantastic! Cannot find any tree widget in Firefox which behaves wrong anymore. Good work! Verified fixed with builds on OS X and Windows:

Mozilla/5.0 (Windows; U; Windows NT 6.0; fa; rv:1.9.1b3pre) Gecko/20090213 Shiretoko/3.1b3pre

Ehsan, have you already posted the message to the l10n newsgroup? I ask because of the location of those litmus tests. Probably a separate rtl subgroup would be a good idea.

Comment 120

[(no longer active)]

(In reply to comment #119)
> Ehsan, have you already posted the message to the l10n newsgroup? I ask because
> of the location of those litmus tests. Probably a separate rtl subgroup would
> be a good idea.

I'm not quite sure what you mean...  Which Litmus tests?  I can post a note on .l10n if needed, or make a blog post with screenshots, etc., or both.

Comment 121

[Henrik Skupin [:whimboo][⌚️UTC+1]]

See the in-litmus flag on this bug. You have asked for that. We should find a good way to have all of those rtl tests in-place. For me it sounds like to add a rtl subgroup.

Comment 122

[(no longer active)]

(In reply to comment #121)
> See the in-litmus flag on this bug. You have asked for that. We should find a
> good way to have all of those rtl tests in-place. For me it sounds like to add
> a rtl subgroup.

Ah, you mean an RTL subgroup in Litmus, right?  Sure that would be a great idea.  I'm not sure about the process of getting a subgroup added though.  Do you have any ideas?

Comment 123

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This should be discussed in the next l10n meeting too. If Tim doesn't mention it please go ahead and raise your hands. When a final decision was made it shouldn't be a problem to setup a new subgroup. We can file a new bug on that.