325781 - Middle click in places bar doesnt open bookmarks

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P4)

Description

[Dave Townsend [:mossop]]

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.

Comment 1

[Dave Townsend [:mossop]]

Forgot build:

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

Comment 2

[Dave Townsend [:mossop]]

Attachment: 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

[Annie Sullivan]

> 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?

Comment 4

[Dave Townsend [:mossop]]

Attachment: 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.

Comment 5

[Annie Sullivan]

> 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?

Comment 6

[Dave Townsend [:mossop]]

Attachment: 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.

Comment 7

[Dave Townsend [:mossop]]

Do I need to request super review here?

Comment 8

[Dave Townsend [:mossop]]

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.

Comment 9

[Dave Townsend [:mossop]]

Attachment: 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.

Comment 10

[Dave Townsend [:mossop]]

Annie, I can't checkin. If this is ready to go in could you land it?

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

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?

Comment 12

[Dave Townsend [:mossop]]

Attachment: 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?

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 211871 [details] [diff] [review]
Added comments

Selected the wrong kind of review...

Comment 14

[Jon Henry]

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?

Comment 15

[Dave Townsend [:mossop]]

(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

[Jon Henry]

(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.

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

*** Bug 327444 has been marked as a duplicate of this bug. ***

Comment 18

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 211871 [details] [diff] [review]
Added comments

sr=[e-mail]

Comment 19

[Dave Townsend [:mossop]]

Attachment: 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.

Comment 20

[Annie Sullivan]

The patch is checked in.

Comment 21

[Gervase Markham [:gerv]]

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