Closed Bug 176244 Opened 20 years ago Closed 13 years ago

Fix column resize and reorder issues when direction is rtl

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1
Tracking Status
status1.9.1 --- ?

People

(Reporter: janv, Assigned: ehsan.akhgari)

References

(Blocks 1 open bug)

Details

(Keywords: rtl)

Attachments

(2 files, 1 obsolete file)

 
Blocks: 135272
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.
I think, smontagu has a fix for that in a different bug.
yes, at bug 206437
was this really fixed in bug 206437
it seems so. i don't have that problem in mozilla 1.4 with hebrew pack. but this
(176244) bug is still here.
1. This is an ALL/ALL bug.
2. I will check this issue soon.
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...
OS: Linux → All
Hardware: PC → All
Blocks: 306980
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: shrir → xptoolkit.widgets
Bug still exists. Please fix...
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?
(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.
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.
Assignee: Jan.Varga → ehsan.akhgari
Status: NEW → ASSIGNED
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...
You're looking for the hit-testing code for tree-column splitters, aren't you?
Ehsan, will bug 479540 (which will probably a more general bug for all trees) be fixed by this bug?
Attached patch Resize Patch (obsolete) — Splinter Review
Please ignore my previous comment.  :-)

This patch fixes the resize issue, but I still don't know where the drag/drop code resides.
Attachment #364647 - Flags: superreview?(roc)
Attachment #364647 - Flags: review?(roc)
(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.
(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.
Duplicate of this bug: 479540
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.
Attachment #364647 - Attachment is obsolete: true
Attachment #364717 - Flags: superreview?(roc)
Attachment #364717 - Flags: review?(roc)
Attachment #364717 - Flags: review?(enndeakin)
Attachment #364647 - Flags: superreview?(roc)
Attachment #364647 - Flags: review?(roc)
Whiteboard: [has patch][needs review enndeakin][needs review roc]
Try server builds available at: <https://build.mozilla.org/tryserver-builds/2009-02-28_14:49-ehsan.akhgari@gmail.com-rtltreeresizereorder/>

Henrik, can you give these builds a test please?
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?
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.
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
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.
Attachment #364717 - Flags: superreview?(roc)
Attachment #364717 - Flags: superreview+
Attachment #364717 - Flags: review?(roc)
Attachment #364717 - Flags: review+
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)
Attachment #364717 - Flags: review?(enndeakin) → review-
Comment on attachment 364717 [details] [diff] [review]
Resize & Reorder Patch

Minus for now due to questions
(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?
Whiteboard: [has patch][needs review enndeakin][needs review roc] → [has patch]
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 on attachment 364717 [details] [diff] [review]
Resize & Reorder Patch

Restoring the review request based on comment 28.
Attachment #364717 - Flags: review- → review?(enndeakin)
Whiteboard: [has patch] → [has patch][needs review enndeakin]
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.
Attachment #364717 - Flags: review?(enndeakin) → review+
(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...
Whiteboard: [has patch][needs review enndeakin] → [has patch]
Keywords: checkin-needed
Whiteboard: [has patch]
Flags: wanted1.9.1.x?
http://hg.mozilla.org/mozilla-central/rev/864ce87ea96f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
blocking1.9.1: --- → ?
Ehsan: why do you think this should block a dot release? Please renominate with rationale.
blocking1.9.1: ? → ---
Flags: wanted1.9.1.x?
(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.
status1.9.1: --- → ?
You need to log in before you can comment on or make changes to this bug.