Steve Krulewitz Assignee
	Description • 17 years ago

The ordinal attribute on the tree column binding is always returned as a string, which causes comparisons to produce unexpected results when reordering columns which the ordinal value exceeds 9.

	Neil Deakin
	Comment 1 • 17 years ago

Comment on attachment 292170 [details] [diff] [review]
patch

The second change here isn't really needed, as strings versions will compare equal as well.

There's also two cases where 2 is added/subtracted from the ordinal. This should be fixed up as well.

Tests would be nice too, added to the existing tree tests in tree_shared.js

	Steve Krulewitz Assignee
	Comment 2 • 17 years ago

I also added a fix (and test) for an addition problem when moving a column directly after itself (STR: moue down on the left half of a column, drag to the right half of the same column, moue up).

	Steve Krulewitz Assignee
	Comment 3 • 17 years ago

Needed to use tabs and not spaces in that makefile :)

	Neil Deakin
	Comment 4 • 17 years ago

>+function testtag_tree_column_reorder()
>+{
>+  // Make sure the tree is scrolled into the view, otherwise the test will
>+  // fail
>+  window.parent.document.getElementById("testframe").scrollIntoView();

This causes an exception when the test is run separately as there is no testframe.

>+  synthesizeMouse(down, 10, 10, { type: "mousedown"});

Is there a reason 10 was used for these values (and others)?

>+              else {
>+                // If this is an after move and the column we're moving is
>+                // the same as the target column, don't move.
>+                if (!before && col == col.mTargetCol) {
>+                  move = false;
>+                }
>               }

You can just merge this into an 'else if'.

Otherwise, this looks good. Can you describe what the additional problem you mention in comment 2 is in this bug?

	Steve Krulewitz Assignee
	Comment 5 • 17 years ago

(In reply to comment #4)
> >+function testtag_tree_column_reorder()
> >+{
> >+  // Make sure the tree is scrolled into the view, otherwise the test will
> >+  // fail
> >+  window.parent.document.getElementById("testframe").scrollIntoView();
> 
> This causes an exception when the test is run separately as there is no
> testframe.

Fixed.

> 
> >+  synthesizeMouse(down, 10, 10, { type: "mousedown"});
> 
> Is there a reason 10 was used for these values (and others)?

I cleaned this up a bit.  This purpose of the first "10" was to avoid the extra hit test space given to the splitter -- but simply starting the first mousedown in the middle of the column is a more sensible approach.  The second "10" was not needed and I removed it.

> 
> >+              else {
> >+                // If this is an after move and the column we're moving is
> >+                // the same as the target column, don't move.
> >+                if (!before && col == col.mTargetCol) {
> >+                  move = false;
> >+                }
> >               }
> 
> You can just merge this into an 'else if'.

Fixed.

> 
> Otherwise, this looks good. Can you describe what the additional problem you
> mention in comment 2 is in this bug?
> 

It is a problem with dragging a column on to itself.  If you go to about:config, and start your mouse drag on the left hand of the "Preference Name" column header, drag to the right and release on the right hand side of the same column, the column will remain in its drag mode.

I also noticed that this test will fail if the page is zoomed in (ctrl +) -- however, I've found this to be the case with the widget tests in general.

	Reed Loden [:reed]
	Comment 6 • 17 years ago

Checking in toolkit/content/tests/widgets/Makefile.in;
/cvsroot/mozilla/toolkit/content/tests/widgets/Makefile.in,v  <--  Makefile.in
new revision: 1.45; previous revision: 1.44
done
RCS file: /cvsroot/mozilla/toolkit/content/tests/widgets/test_tree_column_reorder.xul,v
done
Checking in toolkit/content/tests/widgets/test_tree_column_reorder.xul;
/cvsroot/mozilla/toolkit/content/tests/widgets/test_tree_column_reorder.xul,v  <--  test_tree_column_reorder.xul
initial revision: 1.1
done
Checking in toolkit/content/tests/widgets/tree_shared.js;
/cvsroot/mozilla/toolkit/content/tests/widgets/tree_shared.js,v  <--  tree_shared.js
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/content/widgets/tree.xml;
/cvsroot/mozilla/toolkit/content/widgets/tree.xml,v  <--  tree.xml
new revision: 1.54; previous revision: 1.53
done

	Stefan Sitter [:ssitter]
	Comment 8 • 16 years ago

The issue mentioned in Comment #2 is most probably the one that was filed as Bug 338889 before. Thanks for fixing.