176244 - Fix column resize and reorder issues when direction is rtl

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jan Varga [:janv]]

Comment 1

[Tsahi Asher]

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.

Comment 2

[Jan Varga [:janv]]

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

Comment 3

[Tsahi Asher]

yes, at bug 206437

Comment 4

[Itamar]

was this really fixed in bug 206437

Comment 5

[Tsahi Asher]

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

Comment 6

[Asaf Romano (gone)]

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

Comment 7

[Asaf Romano (gone)]

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

Comment 8

[(no longer active)]

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

Comment 9

[Tomer Cohen :tomer]

Bug still exists. Please fix...

Comment 10

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

Comment 11

[(no longer active)]

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

Comment 12

[(no longer active)]

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.

Comment 13

[(no longer active)]

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

Comment 14

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

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

Comment 15

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

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

Comment 16

[(no longer active)]

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

Comment 17

[(no longer active)]

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

Comment 18

[(no longer active)]

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

Comment 20

[(no longer active)]

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

Comment 21

[(no longer active)]

Try server builds available at: <https://build.mozilla.org/tryserver-builds/2009-02-28_14:[e-mail]-rtltreeresizereorder/>

Henrik, can you give these builds a test please?

Comment 22

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

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?

Comment 23

[(no longer active)]

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.

Comment 24

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

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

Comment 25

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

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.

Comment 26

[Neil Deakin]

Comment on attachment 364717 [details] [diff] [review]
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)

Comment 27

[Neil Deakin]

Comment on attachment 364717 [details] [diff] [review]
Resize & Reorder Patch

Minus for now due to questions

Comment 28

[(no longer active)]

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

Comment 29

[Tomer Cohen :tomer]

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

Comment 30

[(no longer active)]

Comment on attachment 364717 [details] [diff] [review]
Resize & Reorder Patch

Restoring the review request based on comment 28.

Comment 31

[Neil Deakin]

Comment on attachment 364717 [details] [diff] [review]
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.

Comment 32

[(no longer active)]

(In reply to comment #31)
> (From update of attachment 364717 [details] [diff] [review])
> 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...

Comment 33

[(no longer active)]

Attachment: Possible candidate for check-in

Comment 34

[(no longer active)]

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

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 36

[(no longer active)]

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