480238 - Double-clicking tree column separator opens highlighted link

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Brian Carpenter [:geeknik]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090225 Shiretoko/3.1b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090225 Shiretoko/3.1b3pre

When you open the Library and Show All History in 3.1b3pre and you double click on one of the column separators, instead of resizing the column to match the text width, it opens the highlighted link instead.

Reproducible: Always

Steps to Reproduce:
1. Click History -> Show All History
2. Double click a column separator
Actual Results:  
Highlighted link is opened in a new tab

Expected Results:  
A double click on a column separator has the default behavior of resizing the column to match the text width of that column.

Comment 1

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090225 Minefield/3.2a1pre

Confirmed.  For me, the page opens in an existing tab, not a new tab, but this may be due to a difference in settings.

Comment 3

[Alice0775 White]

I found a regression range as follows.
Regression range:
Works Fine:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022005 Minefield/3.0b4pre

Broken:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022104 Minefield/3.0b4pre

Bonsai query between the above regression range:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-20+05%3A00%3A00&maxdate=2008-02-21+04%3A59%3A59&cvsroot=%2Fcvsroot

Doubtful one of regression is 
Bug 418120 -  Double click on bookmark item in places library does not open in main window .

Comment 4

[Alice0775 White]

Workaround fix of the issue:(content\browser\places\places.xul)

- ondblclick="PlacesOrganizer.openSelectedNode(event);"
+ ondblclick="if (event.target.localName == 'treechildren') PlacesOrganizer.openSelectedNode(event);"

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

Thanks Alice.
This is something like that, i preferred unifying the behavior in one single method, and fixed a problem with columns sorting on dbl click for Windows.

Btw, actually xul trees don't support resizing double clicking the column splitters, so i'll file a followup to fix that in toolkit widget.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

oh looks like resize to content on double click is bug 155510, quite old.

Comment 7

[Neil Deakin]

Comment on attachment 374168 [details] [diff] [review]
patch v1.0

># HG changeset patch
># User Marco Bonardo <[e-mail]>
># Date 1240441517 -7200
># Node ID f4b175170979471a1889a24bfc032fdf6e53b62d
># Parent  d299a5eb0f76b527e617efaf6e10aa3d2dc98927
>[mq]: 48238.diff
>
>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
>--- a/browser/components/places/content/places.js
>+++ b/browser/components/places/content/places.js
>@@ -285,26 +285,34 @@ var PlacesOrganizer = {
>   },
> 
>   /**
>-   * Handle clicks on the tree. If the user middle clicks on a URL, load that
>-   * URL according to rules. Single clicks or modified clicks do not result in
>-   * any special action, since they're related to selection.
>+   * Handle clicks on the tree.
>+   * Single Left click, right click or modified click do not result in any
>+   * special action, since they're related to selection.
>    * @param   aEvent
>    *          The mouse event.
>    */
>   onTreeClick: function PO_onTreeClick(aEvent) {
>+    // Only handle clicks on tree children.
>     if (aEvent.target.localName != "treechildren")
>       return;
> 
>-    var currentView = aEvent.currentTarget;
>-    var selectedNode = currentView.selectedNode;
>-    if (selectedNode && aEvent.button == 1) {
>-      if (PlacesUtils.nodeIsURI(selectedNode))
>-        PlacesUIUtils.openNodeWithEvent(selectedNode, aEvent);
>-      else if (PlacesUtils.nodeIsContainer(selectedNode)) {
>-        // The command execution function will take care of seeing the
>-        // selection is a folder/container and loading its contents in
>-        // tabs for us.
>-        PlacesUIUtils.openContainerNodeInTabs(selectedNode, aEvent);
>+    if (aEvent.button == 0 && aEvent.detail == 2) {
>+      // Double click with left mouse button should open selected node.
>+      PlacesOrganizer.openSelectedNode(aEvent);

What does 'open' mean here? Double-clicking a row should already open it. Or does this mean something different?

>+  var isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes);

Use navigator.platform to check which platform we're on
 
>+  if (isWindows) {
>+    // Windows cycles only on odd clicks, so double click has been ignored.

It shouldn't be ignored. It should just cycle once.


>+    is(columnElement.getAttribute("sortDirection"), "ascending", "double click cycleHeader column sortDirection ascending");
>+    // 2 single clicks should restore natural sorting.
>+    mouseClickOnColumnHeader(tree, columnIndex, 0, 1);
>+    mouseClickOnColumnHeader(tree, columnIndex, 0, 1);
>+  }
>+  else
>+    is(columnElement.getAttribute("sortDirection"), "", "double click cycleHeader column sortDirection");
>+
>+  // Check we ahave gone back to natural sorting.

Fix 'ahave'

I'm not following what the test is doing. Clicking the header should cycle between 'ascending', descending' and ''. Double-clicking should be the same on Windows, and move two values on other platforms.

>+function mouseClickOnColumnHeader(aTree, aColumnIndex, aButton, aClickCount)
>+{
>+  var columns = getSortedColumnArray(aTree);

You may just want to pass columns to the function instead since it's used several times.

>+#ifdef XP_WIN
>+          // On Windows multiple clicking on tree columns only cycles one time
>+          // every 2 clicks.
>+          if (event.detail % 2 == 0)

Why not just compare it to 2? Or use event.detail >= 2

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #7)
> >+      PlacesOrganizer.openSelectedNode(aEvent);
> 
> What does 'open' mean here? Double-clicking a row should already open it. Or
> does this mean something different?

it is different because we also have to sync selection in the left pane when double clicking in the right pane (that is a flat tree list)

> >+  if (isWindows) {
> >+    // Windows cycles only on odd clicks, so double click has been ignored.
> 
> It shouldn't be ignored. It should just cycle once.
> 
> 
> >+    is(columnElement.getAttribute("sortDirection"), "ascending", "double click cycleHeader column sortDirection ascending");
> >+    // 2 single clicks should restore natural sorting.
> >+    mouseClickOnColumnHeader(tree, columnIndex, 0, 1);
> >+    mouseClickOnColumnHeader(tree, columnIndex, 0, 1);
> >+  }
> >+  else
> >+    is(columnElement.getAttribute("sortDirection"), "", "double click cycleHeader column sortDirection");
> >+
> >+  // Check we ahave gone back to natural sorting.
> 
> Fix 'ahave'
> 
> I'm not following what the test is doing. Clicking the header should cycle
> between 'ascending', descending' and ''. Double-clicking should be the same on
> Windows, and move two values on other platforms.

well what i did notice is that the onclick handler on double click is called once with details=1 and once with details=2 (And so on). So what i exactly do i ignoring the second click event.
The test simulates firing a click event with details=2, that is so ignored.
Should i consider that a bug (the fact we do receive 2 events?)

> >+#ifdef XP_WIN
> >+          // On Windows multiple clicking on tree columns only cycles one time
> >+          // every 2 clicks.
> >+          if (event.detail % 2 == 0)
> 
> Why not just compare it to 2? Or use event.detail >= 2

because on Windows if you quickly click 4 times (call that a quad click?)  it will change order 2 times, so it changes order 1 time every 2 clicks. Btw looks like for us is hard (possible?) to go over details=3.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

morphing title since resize is covered by another bug

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

Comment 11

[Neil Deakin]

Comment on attachment 375188 [details] [diff] [review]
patch v1.1

>+      if (PlacesUtils.nodeIsURI(selectedNode) &&
>+          (doubleClickOnFlatList || middleClick)) {
>+        // Open associated uri in the browser.
>+        PlacesOrganizer.openSelectedNode(aEvent);
>+      }
>+      else if (PlacesUtils.nodeIsContainer(selectedNode) &&
>+               middleClick) {

Not that it matters too much but checking the booleans first before checking nodeIsURI/nodeIsContainer in the conditions would be ever so slightly faster.

>+{
>+  var columnHeader = aColumns[aColumnIndex].element;
>+  var columnWidth = columnHeader.boxObject.width;

Use getBoundingClientRect instead of box objects.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.2

http://hg.mozilla.org/mozilla-central/rev/cff0c2442f46

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 377394 [details] [diff] [review]
patch v1.2

asking approval 1.9.1, has a test.

Avoids that when double clicking Library's column headers we load the currently selected item.

Plus on Windows avoids that double clicking the header sorts 2 times instead of one.

Comment 14

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

Comment on attachment 377394 [details] [diff] [review]
patch v1.2

a191=beltzner

Comment 15

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

(why is the test not included in the patch I just approved? that MUST land on 191 with this)

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the test IS included: /toolkit/content/tests/widgets/tree_shared.js

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7ad74cc20eed

Comment 18

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090521 Minefield/3.6a1pre ID:20090521030951

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090521 Shiretoko/3.5pre ID:20090521030957