about:config column dividers behave incorrectly in hebrew rtl

[nadavkav]

Binary package hint: firefox-3.0

i use an hebrew (rtl) UI for firefox 3.0.3 and when i open a new tab and navigate to... about:config
and try to move around the columns dividers they behave incorrectly as if i am in ltr (english) UI

hope i was clear ;-)

ProblemType: Bug
Architecture: i386
Date: Thu Oct 23 14:39:20 2008
DistroRelease: Ubuntu 8.04
Package: firefox-3.0 3.0.3+build1+nobinonly-0ubuntu0.8.04.1
PackageArchitecture: i386
ProcEnviron:
 PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games
 LANG=he_IL.UTF-8
 SHELL=/bin/bash
SourcePackage: firefox-3.0
Uname: Linux 2.6.24-21-generic i686

[Tsahi-75 (tsahi-75)]

i don't know if this requiers a new bug: another problem is that when you click
a column header to reorder the messages, mozilla crashes.

[Jan-varga (jan-varga)]

I think, smontagu has a fix for that in a different bug.

[Tsahi-75 (tsahi-75)]

yes, at bug 206437

[Zipo13 (zipo13)]

was this really fixed in bug 206437

[Tsahi-75 (tsahi-75)]

it seems so. i don't have that problem in mozilla 1.4 with hebrew pack. but this
(176244) bug is still here.

[Mano-mozilla (mano-mozilla)]

1. This is an ALL/ALL bug.
2. I will check this issue soon.

[Mano-mozilla (mano-mozilla)]

btw, after the patch for bug 140759 will be checked in, the resize problem is
the only issue that is still there.

Sorry for bugspam...

[Ehsan-mozilla (ehsan-mozilla)]

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

[nadavkav (nadavkav)]

Binary package hint: firefox-3.0

i use an hebrew (rtl) UI for firefox 3.0.3 and when i open a new tab and navigate to... about:config
and try to move around the columns dividers they behave incorrectly as if i am in ltr (english) UI

hope i was clear ;-)

ProblemType: Bug
Architecture: i386
Date: Thu Oct 23 14:39:20 2008
DistroRelease: Ubuntu 8.04
Package: firefox-3.0 3.0.3+build1+nobinonly-0ubuntu0.8.04.1
PackageArchitecture: i386
ProcEnviron:
 PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games
 LANG=he_IL.UTF-8
 SHELL=/bin/bash
SourcePackage: firefox-3.0
Uname: Linux 2.6.24-21-generic i686

[Alexander Sack (asac)]

please attach a screenshot and describe your particular problem. Also try to disable your extensions to see if this is caused by any extension you have installed.

[nadavkav (nadavkav)]

when i move the divider (1) the other divider (2) moves

see (new) attachment (in hebrew)

[Martin Mai (mrkanister-deactivatedaccount-deactivatedaccount)]

 Thank you for taking the time to report this bug and helping to make Ubuntu better. You reported this bug a while ago and there hasn't been any activity in it recently. We were wondering if this is still an issue for you. Can you try with the latest Ubuntu release? Thanks in advance.

[Yaron (sh-yaron)]

It is still in issue
Checked with Ubuntu 8.04 and it still occurs

[Yaron (sh-yaron)]

Forgot to mention my Firefox version is 3.0.6

[Tomer Cohen (tomer-gmx)]

Bug still exists. Please fix...

[Tomer Cohen (tomerc)]

Reporter: As Ubuntu are not the best place to report Firefox bugs, I recommend you next time to report the bug where it should be reported or at least link it to Mozilla. CC me if you need help.

[nadavkav (nadavkav)]

i also confirm it is still an issue with latest ubuntu and firefox 3.0.6

tomer,
my sister pressed report bug (or something) on the firefox help menu and got redirected here for the bug issue report
so i think it is ubuntu's responsibility to make it transparent for her (and for any normal/casual and unfamiliar user with the origin of applications) to report what ever bug she see and expect it to be handled here and not have to go upstream :-)

i wrote this note to you since i am a developer and i see your similar remarks from time to time. if you want it reported properly to you or upstream. please make sure the firefox' ubuntu link redirect the bug report to mozilla's bugzilla with some predefined tags you can follow :-) ir work somthing with the ubuntu people ?

please please please try to consider the users ;-)

[Smontagu (smontagu)]

So, as comment 7 predicted, the only remaining issue seems to be that when resizing columns in RTL, the column boundary that gets its position moved is the one symmetrically opposite from the one under the mouse. Ehsan, can you look at this?

[Ehsan-mozilla (ehsan-mozilla)]

(In reply to comment #10)
> So, as comment 7 predicted, the only remaining issue seems to be that when
> resizing columns in RTL, the column boundary that gets its position moved is
> the one symmetrically opposite from the one under the mouse. Ehsan, can you
> look at this?

Yeah, it seems so. I'll take a look; I guess it shouldn't be that hard to fix.

I'm also CCing Henrik because he's recently done some QA work on RTL trees.

[Ehsan-mozilla (ehsan-mozilla)]

Hmm, I also see this problem while trying to reorder the columns using drag and drop, so I guess Mano was not quite right in comment 7.

[Ehsan-mozilla (ehsan-mozilla)]

roc: do you know where I should look at in order to find the hit test code for tree column headers? The hit test code in nsTreeColFrame.cpp seems irrelevant...

[Roc-ocallahan (roc-ocallahan)]

You're looking for the hit-testing code for tree-column splitters, aren't you?

[Hskupin (hskupin)]

Ehsan, will bug 479540 (which will probably a more general bug for all trees) be fixed by this bug?

[Ehsan-mozilla (ehsan-mozilla)]

Created attachment 364647
Resize Patch

Please ignore my previous comment. :-)

This patch fixes the resize issue, but I still don't know where the drag/drop code resides.

[Ehsan-mozilla (ehsan-mozilla)]

(In reply to comment #14)
> You're looking for the hit-testing code for tree-column splitters, aren't you?

Yeah, I figured it out after a bit of messing around in the debugger. :-)

What I need to know right know is where the drag/drop is handled for tree columns, so that I could look into the reordering issue.

What happens know is that the correct column gets dragged, but its drop location is the reverse of the correct location (i.e., if you drop a column at the left side of an RTL tree, it will get inserted at the right side). Also, I need to correct the drawing of the "placeholder" lines which should be drawn at each side of the drop target column as the user is performing the drag and drop.

[Ehsan-mozilla (ehsan-mozilla)]

(In reply to comment #15)
> Ehsan, will bug 479540 (which will probably a more general bug for all trees)
> be fixed by this bug?

Yes, this bug is present in all trees. I didn't understand that bug 479540 is about the resizing issue, but if that's all the problem in that bug, then it's a dupe of this one.

[Hskupin (hskupin)]

*** Bug 479540 has been marked as a duplicate of this bug. ***

[Ehsan-mozilla (ehsan-mozilla)]

Created attachment 364717
Resize & Reorder Patch

Finally I figured that the reordering of tree columns is handled inside tree.xml. This patch fixes the reordering issue in addition to what the previous patch solved for the resize issue.

The logic to fix the reordering is very simple: start looking up the column from end to start in RTL mode (which makes the lookup order to be from physical left to physical right, no matter what the direction of the tree is).

With this change, the only things that were needed were to make sure that "before" and "after" are specified physically (i.e., being equivalent to physical left and right), and adjusting a bunch of CSS in order to draw the drop indicator line correctly for RTL mode.

Requesting r+sr from roc on the layout part, and r from Neil on the toolkit part.

[Ehsan-mozilla (ehsan-mozilla)]

Try server builds available at: <https://build.mozilla.org/tryserver-builds/2009-02-28_14:<email address hidden>/>

Henrik, can you give these builds a test please?

[Hskupin (hskupin)]

Looks good. There is only one small difference I was able to see right now. When I open e.g. the Library while staying in RTL mode and trying to drag the latest column separator (which is at least on OS X directly beneath the window border) the column width isn't changed when dragging to the left side. Is here the LTR mode faulty?

[Ehsan-mozilla (ehsan-mozilla)]

Hmm, I'm not sure what you mean. I can both increase and decrease the last column's width using the column separator after it (which falls right besides the window border. Can you provide a set of steps to reproduce?

Also, this may be Mac specific, but I doubt it, but at any rate I tested it on Windows.

[John Vivirito (gnomefreak)]

marked as triaged since there is no more info needed from us at this time.

[Hskupin (hskupin)]

You have tested it with Force RTL? I ask because I can see this on Windows too. Just drag the latest column slider over to tags or the name. The width of the last column isn't changed as when having LTR active.

Use a fresh profile and

[Hskupin (hskupin)]

drag the column slider for the location to the right. It's reproducible for me with your tryserver build but works with a normal Minefield nightly build.

[Enn (enndeakin)]

Comment on attachment 364717
Resize & Reorder Patch

>+ if (mFrame->GetStyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL) {
>+ PRBool tmp = left;
>+ left = right;
>+ right = tmp;
>+ }
>+

This is OK, but it seems like it could be written more compact with the code about and below it.

>+ var columns = [];
> var col = this.columns.getFirstColumn();
>- var lastCol = null;
>+ while (col) {
>+ columns.push(col);
>+ col = col.getNext();
>+ }
>+ if (isRTL)
>+ columns.reverse();

It would be simpler code to just iterate using getColumnAt()

I don't follow this patch really. It seems that getColumnAtX should already return the right column whether in ltr or rtl mode, especially as you say you are interpreting 'before' or 'after' without respect to the direction.

Perhaps you really want to just iterate the same in either direction but the two lines here just be reversed?

 currentX += cw;
 if (currentX - (cw * aThresh) > adjustedX)

[Enn (enndeakin)]

Comment on attachment 364717
Resize & Reorder Patch

Minus for now due to questions

[Ehsan-mozilla (ehsan-mozilla)]

(In reply to comment #26)
> I don't follow this patch really. It seems that getColumnAtX should already
> return the right column whether in ltr or rtl mode, especially as you say you
> are interpreting 'before' or 'after' without respect to the direction.

OK, let me clarify.

_getColumnAtX's input (aX) is in physical coordinates (i.e., from the left side for both RTL and LTR modes). However, the tree column APIs work in logical ordering (i.e., the first column is the leftmost and rightmost for LTR and RTL, respectively). This is why we have to iterate on the columns from the end to beginning in RTL mode, so that we would effectively iterate over them in physical order. This way the existing formula for calculating the result column would continue to work.

Also, aPos is in logical order, that's why I swap its value for RTL iterations.

> Perhaps you really want to just iterate the same in either direction but the
> two lines here just be reversed?
>
> currentX += cw;
> if (currentX - (cw * aThresh) > adjustedX)

No, that won't work, because for example on the first iteration of the loop, cw is the width of the right-most (i.e., first) column, while currentX is the x position from the left side, so swapping those two lines isn't going to solve anything.

Given this description, what do you think about the patch?

[Tomer Cohen (tomer-gmx)]

Issue still relevant with Firefox 3.5rc2, Ubuntu 9.04

Same issue also reported in Luanchpad. https://bugs.launchpad.net/ubuntu/+source/firefox-3.0/+bug/288111

[Ehsan-mozilla (ehsan-mozilla)]

Comment on attachment 364717
Resize & Reorder Patch

Restoring the review request based on comment 28.

[Enn (enndeakin)]

Comment on attachment 364717
Resize & Reorder Patch

OK, looking at it again and it is clearer now. You could still compact down some of the 'left' and 'right' checks I think.

[Ehsan-mozilla (ehsan-mozilla)]

(In reply to comment #31)
> (From update of attachment 364717 [details])
> OK, looking at it again and it is clearer now. You could still compact down
> some of the 'left' and 'right' checks I think.

Is there anything specific you have in mind? I didn't find out any obvious places to compact the code...

[Ehsan-mozilla (ehsan-mozilla)]

Created attachment 385073
Possible candidate for check-in

[Ehsan-mozilla (ehsan-mozilla)]

http://hg.mozilla.org/mozilla-central/rev/864ce87ea96f

[Beltzner (beltzner)]

Ehsan: why do you think this should block a dot release? Please renominate with rationale.

[Ehsan-mozilla (ehsan-mozilla)]

(In reply to comment #35)
> Ehsan: why do you think this should block a dot release? Please renominate with
> rationale.

Without this patch, the headers for XUL trees cannot be correctly resized or reordered. The reordering issue is especially severe, because once the user drags a column and drops it at any specific location, the column will actually move to another location, and there is practically no way of getting the correct ordering back (unless the user understands the bug, and drops the column in the mirror of the desired location.)

While this doesn't affect LTR builds, it's a serious problem for RTL builds. On second thought, though, maybe it's too severe to ask for blocking here, but I think this is definitely wanted at least.

Also, please note that this affects Mozilla and XULRunner apps, as well as any extensions which use the XUL tree widget.