Closed
Bug 480238
Opened 16 years ago
Closed 16 years ago
Double-clicking tree column separator opens highlighted link
Categories
(Firefox :: Bookmarks & History, defect)
Tracking
()
VERIFIED
FIXED
Firefox 3.6a1
People
(Reporter: geeknik, Assigned: mak)
References
Details
(Keywords: regression, verified1.9.1)
Attachments
(1 file, 2 obsolete files)
6.36 KB,
patch
|
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•16 years ago
|
Version: unspecified → 3.1 Branch
Comment 1•16 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Double clicking separator opens highlighted link in new tab → Double-clicking tree column separator opens highlighted link instead of resizing column
Comment 3•16 years ago
|
||
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•16 years ago
|
||
Workaround fix of the issue:(content\browser\places\places.xul)
- ondblclick="PlacesOrganizer.openSelectedNode(event);"
+ ondblclick="if (event.target.localName == 'treechildren') PlacesOrganizer.openSelectedNode(event);"
Reporter | ||
Updated•16 years ago
|
Keywords: regression
Assignee | ||
Comment 5•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Attachment #374168 -
Flags: review? → review?(enndeakin)
Assignee | ||
Comment 6•16 years ago
|
||
oh looks like resize to content on double click is bug 155510, quite old.
Comment 7•16 years ago
|
||
Comment on attachment 374168 [details] [diff] [review]
patch v1.0
># HG changeset patch
># User Marco Bonardo <mbonardo@mozilla.com>
># 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
Attachment #374168 -
Flags: review?(enndeakin) → review-
Assignee | ||
Comment 8•16 years ago
|
||
(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.
Assignee | ||
Comment 9•16 years ago
|
||
morphing title since resize is covered by another bug
Summary: Double-clicking tree column separator opens highlighted link instead of resizing column → Double-clicking tree column separator opens highlighted link
Assignee | ||
Comment 10•16 years ago
|
||
Attachment #374168 -
Attachment is obsolete: true
Attachment #375188 -
Flags: review?(enndeakin)
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite?
Comment 11•16 years ago
|
||
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.
Attachment #375188 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 12•16 years ago
|
||
Attachment #375188 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
Assignee | ||
Comment 13•16 years ago
|
||
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.
Attachment #377394 -
Flags: approval1.9.1?
Comment 14•16 years ago
|
||
Comment on attachment 377394 [details] [diff] [review]
patch v1.2
a191=beltzner
Attachment #377394 -
Flags: approval1.9.1? → approval1.9.1+
Comment 15•16 years ago
|
||
(why is the test not included in the patch I just approved? that MUST land on 191 with this)
Assignee | ||
Comment 16•16 years ago
|
||
the test IS included: /toolkit/content/tests/widgets/tree_shared.js
Assignee | ||
Comment 17•16 years ago
|
||
Keywords: fixed1.9.1
Comment 18•16 years ago
|
||
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•