Middle click in places bar doesnt open bookmarks

RESOLVED FIXED in Firefox 2 beta1

Status

()

P4
normal
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Trunk
Firefox 2 beta1
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

13 years ago
Similar to the personal toolbar, middle clicking a bookmark in the places bar should open the bookmark in a new tab. This does not currently happen either in the places bar or the bookmarks menu. Middle clicking seems to do nothing.
(Assignee)

Comment 1

13 years ago
Forgot build:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060203 Firefox/1.6a1 ID:2006020309

Updated

13 years ago
Assignee: nobody → annie.sullivan
Priority: -- → P4
Target Milestone: --- → Firefox 2 beta1
(Assignee)

Comment 2

13 years ago
Created attachment 211203 [details] [diff] [review]
Check for middle clicks in getLoadFunctionForEvent

This patch provides the middle click functionality for both bookmarks menu and places bar.

_getLoadFunctionForEvent is modified to notice the middle clicks.

The three functions that perform the link opening have the event added to their arguments, this allows us to pass openNewTabWith the event which will then correctly open new tabs in the foreground and background depending on the shift key.

The click handlers for the menupopups and toolbar have extra logic added so they only call mouseLoadURI for clicks on items directly within them, previously it would get called for eveny parent menupopup of the bookmark.

Finally the menupopup closes itself and it's parents if a middle click was pressed since menu's don't automatically close in that case.

Comment 3

13 years ago
> The three functions that perform the link opening have the event added to their
> arguments, this allows us to pass openNewTabWith the event which will then
> correctly open new tabs in the foreground and background depending on the shift
> key.

This makes sense when I read your explanation and look at the code, but it's kind of confusing why there are event arguments if I'm just looking at the code.  Can you add @param comments to the comments above the functions you added the event arguments to, explaining this?
(Assignee)

Comment 4

13 years ago
Created attachment 211245 [details] [diff] [review]
Updated to use standard openUILink

I could add the comments if you prefer that way, but having thought on this a bit more I don't see why places is creating it's own functions to determine where to open links when there is already a standard function there to do it (standardised in bug 246719).

This patch replaces the places functions and just calls the standard function to open the link.
Attachment #211203 - Attachment is obsolete: true

Comment 5

13 years ago
> Updated to use standard openUILink

I'm looking at the code for openUILink, and it seems like the link will be opened even if the user has right-clicked.  Is this the case?  If so, can you add a check and not open the link on right-clicks?
(Assignee)

Comment 6

13 years ago
Created attachment 211305 [details] [diff] [review]
Ignores right clicks

Sorry about that, yes openUILink seems to treat a right click the same as a left click. This patch filters out anything above a middle click.

Another thing that comes to mind is that from my experience most of the browser's interface is set to use the click handler for detecting middle clicks, and then the command handler for left clicks and keyboard selection. I don't know if is a good idea to follow the same standard here, if so I could put together a patch for that.
Attachment #211245 - Attachment is obsolete: true

Updated

13 years ago
Attachment #211305 - Flags: review+
(Assignee)

Updated

13 years ago
Assignee: annie.sullivan → mossop
(Assignee)

Comment 7

13 years ago
Do I need to request super review here?
Status: NEW → ASSIGNED
(Assignee)

Comment 8

13 years ago
Comment on attachment 211305 [details] [diff] [review]
Ignores right clicks

This regresses much of the functionality from the bookmarks context menu. Will post an updated patch later.
Attachment #211305 - Attachment is obsolete: true
(Assignee)

Comment 9

13 years ago
Created attachment 211380 [details] [diff] [review]
Reverts some of the changes to fix previous functionality

In comparison with the previous patch, this brings back the three functions to open links since the bookmarks context menu uses though. Also menu's were not setting themselves as the active view on right click so move the test for that to later.
Attachment #211380 - Flags: review?(annie.sullivan)

Updated

13 years ago
Attachment #211380 - Flags: review?(annie.sullivan) → review+
(Assignee)

Comment 10

13 years ago
Annie, I can't checkin. If this is ready to go in could you land it?
Comment on attachment 211380 [details] [diff] [review]
Reverts some of the changes to fix previous functionality

>Index: browser/components/places/content/controller.js
>-        if (event.target.localName != "menuitem" &&
>-            event.target.localName != "menu")
>+        if ((event.target.localName != "menuitem" &&
>+             event.target.localName != "menu") ||
>+             event.target.parentNode != this)
>           return;
>         this._selection = event.target.node;
>         PlacesController.activeView = this;
>+        if (event.button > 1)
>+          return;

Please document what these assertions mean. 

>         PlacesController.mouseLoadURI(event);
>+        if (event.button == 1) {
>+          var node = this;
>+          while (node && 
>+                (node.localName == "menu" || 
>+                 node.localName == "menupopup")) {
>+            if (node.localName == "menupopup")
>+              node.hidePopup();
>+            
>+            node = node.parentNode;
>+          }
>+        }

It's not intuitive to me that clicking with the middle button should require the menu to have to be closed manually by client code. Is this a bug? If so, can you find or file one, add a comment and reference it here? 

>+      <handler event="click"><![CDATA[
>+        if (event.target.localName != "toolbarbutton" ||
>+            event.target.parentNode != this ||
>+            event.button > 1)
>+          return;

What case does this prevent against? Add a comment, please!

Can I get one more patch with documentation, so that I can understand what's going on?
Attachment #211380 - Flags: superreview-
(Assignee)

Comment 12

13 years ago
Created attachment 211871 [details] [diff] [review]
Added comments

Added comments as requested. Also made the minor change of removing the test for event.target.parentNode==this from toolbar.xml since it is not necessary.

I have not filed a bug for menus not closing when middle clicked since I don't believe it is a bug. Certainly in any other windows app, middle clicking on a menu does not close it, it's only these few special cases like bookmarks where firefox does do something and then manually closes the menu. Both Seamonkey and Firefox already have code elsewhere that does this:

http://lxr.mozilla.org/mozilla/source/browser/base/content/utilityOverlay.js#263
http://lxr.mozilla.org/mozilla/source/xpfe/components/bookmarks/resources/bookmarksMenu.js#286

Perhaps it would be better to just call closeMenus instead of writing a copy here, since this patch already uses openUILink and so is reliant on utilityOverlay?
Attachment #211380 - Attachment is obsolete: true
Attachment #211871 - Flags: ui-review?(bugs)
Attachment #211871 - Flags: review+
(Assignee)

Updated

13 years ago
Blocks: 327444
(Assignee)

Comment 13

13 years ago
Comment on attachment 211871 [details] [diff] [review]
Added comments

Selected the wrong kind of review...
Attachment #211871 - Flags: ui-review?(bugs) → superreview?(bugs)

Comment 14

13 years ago
Will this patch also resolve the fact that middle-clicking a folder on the Places bar does nothing, or should that be filed as a separate bug?
(Assignee)

Comment 15

13 years ago
(In reply to comment #14)
> Will this patch also resolve the fact that middle-clicking a folder on the
> Places bar does nothing, or should that be filed as a separate bug?

No that wont be resolved here.

Comment 16

13 years ago
(In reply to comment #15)
> No that wont be resolved here.
> 

Ok. I couldn't find an existing bug on that, so I went ahead and filed bug 327464.
*** Bug 327444 has been marked as a duplicate of this bug. ***
Comment on attachment 211871 [details] [diff] [review]
Added comments

sr=ben@mozilla.org
Attachment #211871 - Flags: superreview?(bugs) → superreview+
(Assignee)

Comment 19

13 years ago
Created attachment 212255 [details] [diff] [review]
Un-bitrotted

bug 318052 introduced a part of this. This is just an un-bitrotted patch thats ready for checkin so carrying over the reviews.
Attachment #211871 - Attachment is obsolete: true
Attachment #212255 - Flags: superreview+
Attachment #212255 - Flags: review+

Comment 20

13 years ago
The patch is checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.