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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
XUL
P3
normal
VERIFIED FIXED
16 years ago
2 years ago

People

(Reporter: Tsahi Asher, Assigned: Away for a while)

Tracking

(Blocks: 2 bugs, {rtl, verified1.9.1})

Trunk
mozilla1.9.2a1
rtl, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8a2 -
wanted-next +
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 19 obsolete attachments)

41.06 KB, image/gif
Details
22.07 KB, image/png
Details
39.84 KB, image/jpeg
Details
30.02 KB, image/png
Details
4.06 KB, application/vnd.mozilla.xul+xml
Details
15.66 KB, patch
Details | Diff | Splinter Review
128.29 KB, image/png
Details
(Reporter)

Description

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

Updated

16 years ago
Summary: trees are not aligne → trees are not aligned to the right when UI aligned to the right
(Reporter)

Comment 1

16 years ago
Created attachment 81393 [details]
screen shot
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; }
(Reporter)

Comment 3

16 years ago
see news://news.mozilla.org:119/3CA10A02.C4316EAF@netscape.com 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; }  

See also bug 135272
Blocks: 137995

Comment 5

15 years ago
Reporter: can you repeat this with a modern mozilla build? If not, mark WFM.
(Reporter)

Comment 6

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

15 years ago
confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Problematic code is here: 
http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2350
(Reporter)

Updated

14 years ago
OS: Windows 98 → All
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...).
(Reporter)

Comment 10

14 years ago
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.
Assignee: mkaply → romano_a
Hardware: PC → All
Created attachment 149837 [details] [diff] [review]
Fix

Patch for painting RTL trees
Attachment #149837 - Flags: superreview?(mkaply)
Attachment #149837 - Flags: review?(smontagu)
Status: NEW → ASSIGNED
Attachment #149837 - Flags: superreview?(mkaply)
Attachment #149837 - Flags: review?(smontagu)
Created attachment 149839 [details] [diff] [review]
Fix for RTL trees

(Replacing hard tabs with spaces)
Attachment #149837 - Attachment is obsolete: true
Comment on attachment 149839 [details] [diff] [review]
Fix for RTL trees

Sorry for bug spam..
Attachment #149839 - Flags: superreview?(mkaply)
Attachment #149839 - Flags: review?(smontagu)
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 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.
Attachment #149839 - Flags: review?(smontagu) → review-
Created attachment 149854 [details]
screenshot with comments

I see from your last comment that some of the issues can be fixed with css. Can
you attach an example?
(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
Attachment #149839 - Flags: superreview?(mkaply)
Created attachment 149856 [details] [diff] [review]
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 :-)
Attachment #149839 - Attachment is obsolete: true

Comment 19

14 years ago
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.
(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.
Summary: trees are not aligned to the right when UI aligned to the right → Wrong rendering of RTL trees
Created attachment 149900 [details]
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.
(Reporter)

Comment 22

14 years ago
(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.
Summary: Wrong rendering of RTL trees → trees are not aligned to the right when UI aligned to the right
(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.
(Reporter)

Comment 24

14 years ago
how about this?
Summary: trees are not aligned to the right when UI aligned to the right → trees are not aligned to the right when UI direction is right to left
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...
(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.
Great!!! Now, we just need to workaround the space_before_twist problem.
Created attachment 149930 [details] [diff] [review]
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.
Created attachment 150000 [details] [diff] [review]
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.
Attachment #149856 - Attachment is obsolete: true
Blocks: 143344

Comment 30

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

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

Comment 32

14 years ago
(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.
(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).
Oh, my mistkae, i'm not talking about #urlbar, the problem is with the
autocomplete box under it.
Created attachment 150024 [details] [diff] [review]
Another progress.

Clicking problem is behind. This also patches PaintSeperator, but it still
needs work (in PaintRow).
Attachment #150000 - Attachment is obsolete: true
Created attachment 150106 [details] [diff] [review]
(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.
Attachment #150024 - Attachment is obsolete: true
Comment on attachment 150106 [details] [diff] [review]
(Probably) Final Patch

Requesting review from Simon.
Attachment #150106 - Flags: review?(smontagu)
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 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?
Attachment #150106 - Flags: review?(smontagu) → review+
Created attachment 150198 [details] [diff] [review]
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).
Well,the problem is in PaintRow (placement in BRANCH patch), now I need to
workaround it...
Created attachment 150254 [details] [diff] [review]
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.
Attachment #150106 - Attachment is obsolete: true
Comment on attachment 150254 [details] [diff] [review]
Final Patch for trunk, addressing Simon's comments

Asking for r/sr
Attachment #150254 - Flags: superreview?(dbaron)
Attachment #150254 - Flags: review?(smontagu)
(Reporter)

Comment 44

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

Updated

14 years ago
Attachment #150254 - Flags: review?(smontagu) → review+

Comment 45

14 years ago
>     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.
(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 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.
Created attachment 150494 [details] [diff] [review]
Same fix with CodeStyle tweaks
Attachment #150254 - Attachment is obsolete: true
Created attachment 150495 [details] [diff] [review]
Same fix with CodeStyle tweaks

Sorry for bug spam. I forgot something from Simon's comments
Attachment #150494 - Attachment is obsolete: true
Attachment #150254 - Flags: superreview?(dbaron)
Attachment #150495 - Flags: superreview?(dbaron)
Attachment #150495 - Flags: review?

Comment 50

14 years ago
(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?
(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 on attachment 150495 [details] [diff] [review]
Same fix with CodeStyle tweaks

Only style changes, moving review + flag.
Attachment #150495 - Flags: review? → review+
Priority: -- → P2
Target Milestone: --- → mozilla1.8alpha2

Comment 53

14 years ago
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 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.
Created attachment 151196 [details] [diff] [review]
Addressing David's comments

Also removing some no-more-relevant XXX comments (saying we need to handle what
this patch handles...).
Attachment #150495 - Attachment is obsolete: true
Attachment #150495 - Flags: superreview?(dbaron)
Attachment #151196 - Flags: superreview?(dbaron)
Attachment #151196 - Flags: review?(dbaron)
Created attachment 151197 [details] [diff] [review]
-b diff

Also attaching -b diff
Flags: blocking1.8a2?

Comment 57

14 years ago
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Priority: P2 → --
Resolution: --- → FIXED
Target Milestone: mozilla1.8alpha2 → ---
Status: RESOLVED → REOPENED
Priority: -- → P2
Resolution: FIXED → ---
Target Milestone: --- → mozilla1.8alpha2
neil, there is no reason to close a bug if the patch hasn't checked in.....
Status: REOPENED → ASSIGNED
Priority: P2 → --
Target Milestone: mozilla1.8alpha2 → ---
Priority: -- → P1
Target Milestone: --- → mozilla1.8alpha2
(Reporter)

Comment 59

14 years ago
Asaf, priority was P2.
(In reply to comment #59)
> Asaf, priority was P2

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

Updated

14 years ago
Flags: blocking1.8a2? → blocking1.8a2-
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.
Attachment #151196 - Flags: superreview?(dbaron) → superreview-
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 on attachment 151196 [details] [diff] [review]
Addressing David's comments

Asking for sr=

At this point, i dont know from who...
Attachment #151196 - Flags: superreview- → superreview?
Attachment #151196 - Flags: review?(dbaron) → review?(smontagu)

Comment 64

14 years ago
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? 
(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.
Attachment #151196 - Flags: superreview?
Attachment #151196 - Flags: superreview-
Attachment #151196 - Flags: review?(smontagu)
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.".
(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?
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?
(Reporter)

Comment 69

14 years ago
(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.
(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**.
Created attachment 152435 [details]
Showing the problem to dbraon

Columns rendering is OK. In-the-cell rendering is wrong.
(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.
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).
Giving bug a summary that I think accurately describes the problem.
Summary: trees are not aligned to the right when UI direction is right to left → twistys are not aligned to the right when tree direction is right to left
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.
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).
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?
(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.
(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 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));
(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.
-> 1.8b
Target Milestone: mozilla1.8alpha2 → mozilla1.8beta

Comment 83

13 years ago
FYI: This bug also makes the threaded msg view in the email pretty much 
unusable.
Summary: twistys are not aligned to the right when tree direction is right to left → tree cell content is not aligned to the right when tree direction is right to left
Priority: P1 → P3
Target Milestone: mozilla1.8beta1 → mozilla1.9beta

Updated

12 years ago
Blocks: 306980
Assignee: bugs.mano → Jan.Varga
Status: ASSIGNED → NEW
Component: Layout: BiDi Hebrew & Arabic → XP Toolkit/Widgets: Trees
QA Contact: zach → xptoolkit.trees
Target Milestone: mozilla1.9beta → ---
Created attachment 245013 [details] [diff] [review]
Simpler approach
Assignee: Jan.Varga → smontagu
Attachment #149930 - Attachment is obsolete: true
Attachment #150198 - Attachment is obsolete: true
Attachment #151196 - Attachment is obsolete: true
Attachment #151197 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #245013 - Flags: review?

Updated

11 years ago
Attachment #245013 - Flags: review? → review?(mano)

Comment 85

11 years ago
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.
(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 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
Attachment #245013 - Flags: review?(mano) → review-

Updated

11 years ago
Duplicate of this bug: 386073
(Assignee)

Comment 89

10 years ago
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.
Blocks: 412273, 415606
Flags: blocking1.9?
Flags: wanted-next+
Flags: blocking1.9?
Flags: blocking1.9-

Comment 90

10 years ago
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.
Blocks: 421603
(Assignee)

Updated

10 years ago
Blocks: 415597
(Assignee)

Updated

10 years ago
No longer blocks: 412273
(Assignee)

Comment 91

10 years ago
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
(Assignee)

Updated

10 years ago
Duplicate of this bug: 421603
(Assignee)

Comment 93

10 years ago
Created attachment 318026 [details] [diff] [review]
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).
Assignee: smontagu → ehsan.akhgari
Attachment #245013 - Attachment is obsolete: true

Updated

10 years ago
Blocks: 412273

Updated

10 years ago
No longer blocks: 415597

Comment 94

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

Comment 95

10 years ago
(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.

Updated

9 years ago
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
(Assignee)

Comment 96

9 years ago
Created attachment 355575 [details] [diff] [review]
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.
Attachment #318026 - Attachment is obsolete: true
(Assignee)

Comment 97

9 years ago
Try server builds available at <https://build.mozilla.org/tryserver-builds/2009-01-06_06:57-ehsan.akhgari@gmail.com-rtltree/>
(Assignee)

Comment 98

9 years ago
Created attachment 356029 [details] [diff] [review]
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.
Attachment #355575 - Attachment is obsolete: true
Attachment #356029 - Flags: review?(roc)
Attachment #356029 - Flags: review?(gavin.sharp)
(Assignee)

Comment 99

9 years ago
Created attachment 356030 [details]
XUL test case
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
Attachment #356029 - Flags: superreview+
Attachment #356029 - Flags: review?(roc)
Attachment #356029 - Flags: review+
(Assignee)

Comment 101

9 years ago
(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...
Flags: in-litmus?
OK, don't worry about it then
Attachment #356029 - Flags: review?(gavin.sharp) → review-
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?
(Assignee)

Comment 104

9 years ago
Created attachment 356137 [details] [diff] [review]
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).
Attachment #356029 - Attachment is obsolete: true
Attachment #356137 - Flags: review?(gavin.sharp)
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...
Attachment #356137 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 106

9 years ago
Created attachment 356344 [details] [diff] [review]
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
Attachment #356137 - Attachment is obsolete: true
Attachment #356344 - Flags: review?(gavin.sharp)
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.
Attachment #356344 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 108

9 years ago
Created attachment 356666 [details] [diff] [review]
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.
Attachment #356344 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Comment 109

9 years ago
http://hg.mozilla.org/mozilla-central/rev/a77147340730
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 110

9 years ago
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.
Attachment #356666 - Attachment description: Patch (v4) for check-in → Checked in
Attachment #356666 - Flags: approval1.9.1?
(Assignee)

Comment 111

9 years ago
Created attachment 356748 [details]
Screenshot of the library window with this patch

Comment 112

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

Updated

9 years ago
Duplicate of this bug: 402267
(Assignee)

Updated

9 years ago
Duplicate of this bug: 421608
(Assignee)

Updated

9 years ago
Depends on: 476346
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.
Status: RESOLVED → VERIFIED
Comment on attachment 356666 [details] [diff] [review]
Checked in

a1.9.1=dbaron
Attachment #356666 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Comment 117

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/475218f630f8
Keywords: fixed1.9.1

Comment 118

9 years ago
(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. :)
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.
Keywords: fixed1.9.1 → verified1.9.1
(Assignee)

Comment 120

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

Comment 122

9 years ago
(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?
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.
Duplicate of this bug: 455351
Depends on: 569994
Duplicate of this bug: 176243
(Assignee)

Updated

5 years ago
Flags: in-litmus?
(Assignee)

Updated

2 years ago
Depends on: 1194842
You need to log in before you can comment on or make changes to this bug.