272800 - Make various bookmarks menus middle-clickable

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

This is related to bug 238964.  The suggestion in bug 238964 comment #3 did not
work (it caused other side effects), so I'm not going to incorporate this into a
patch for that bug.

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch

This works, though I'm not entirely sure it's ideal.  The overflow popup stays
open, so you could middle-click multiple links conveniently, but if you only
want to open one, you'd have to click away to get rid of the overflow popup.  I
don't think changing this would be ideal in other situations.

Comment 2

[Wladimir Palant]

Do we really want this? When I fixed bug 72361 I intentionally left out the
menus (see bug 72361 comment 45 and bug 72361 comment 60). Menus aren't supposed
to react on anything but normal clicks (the command event). Firefox has an
implementation for middle- and right-clicks on menus but this caused some
problems and is IMHO an usability issue in itself. There is still Ctrl-Click to
open a bookmark in a new tab from a menu...

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

I changed my mind about the scope of this bug :).  New patch coming shortly.

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Patch

Make main bookmarks menu, bookmarks on PTF, and bookmarks in PTF overflow all
middle-clickable.  Middle-clicking the top items ("Bookmark this page", "File
bookmark", etc) does nothing, and doesn't give any JS errors, so this doesn't
seem to break anything.

Comment 5

[[e-mail]]

(In reply to comment #2)
>When I fixed bug 72361 I intentionally left out the menus
But (un?)fortunately the menus on the personal toolbar do react...

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #5)
> (In reply to comment #2)
> >When I fixed bug 72361 I intentionally left out the menus
> But (un?)fortunately the menus on the personal toolbar do react...

Either we should remove that (please don't!), or we should make the overflow
area behave the same way.  I don't know whether or not the bookmarks menus
should respond to middle clicks, but I don't see any downside.

The one bookmarks menu that lives in the PTF probably should be consistent with
the rest of the PTF, but in my opinion the main bookmarks menu doesn't *have* to
behave exactly the same way (though as I said, I see no downside).

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Jag?  Timeless?  Anyone else disagree with comment #2?  I cc'ed you to hopefully
get opinions from other xpfe people ;)

Comment 8

[timeless]

i expect to be able to right click on bookmarks menus, this isn't unusual
behavior. but it will almost never work on macosx.

Comment 9

[Wladimir Palant]

Sorry for the late response, I had lots to do.

Ok, looking at the other responses, seems that I am outnumbered. I noticed three
issues with this patch so far:

1. "Error: uncaught exception: 2147500035" when middle-clicking a bookmark
folder (for some reason Ctrl-Click on the folder doesn't throw an exception).

2. The bookmarks menu doesn't close after a middle-click. Patch to bug 72361
backed out a function called loadBookmarkMiddleClick() that used
BookmarksMenuDNDObserver.onDragCloseTarget() to close the menu. I think this is
a very hacky way to do it so we shouldn't revive this function. Instead we might
insert following code into BookmarksUtils.loadBookmarkBrowser():

    // Close any open popups
    if (aEvent.type == "click" && aEvent.button == 1) {
      var node = aEvent.target;
      while (node) {
        if (node.nodeType == node.ELEMENT_NODE && node.tagName == "menupopup")
          node.hidePopup();

        node = (node == node.parentNode ? null : node.parentNode);
      }
    }

3. The overflow menu for the personal toolbar hasn't been fixed. Isn't that what
this bug was originally about? FYI, the overflow menu appears on the right side
if the browser window isn't wide enough for all personal toolbar items to fit
into it.

Comment 10

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #9)
> 1. "Error: uncaught exception: 2147500035" when middle-clicking a bookmark
> folder (for some reason Ctrl-Click on the folder doesn't throw an exception).
WFM - do you get a line number?

> 2. The bookmarks menu doesn't close after a middle-click.
I thought about it, and did it by design.  This way, I can open the menu and
open garfield, dilbert, and foxtrot with 4 clicks instead of 6.  Maybe that
should get a pref (since I can imagine many people wouldn't like this).

> 3. The overflow menu for the personal toolbar hasn't been fixed. Isn't that what
> this bug was originally about?
WFM - as you noted, that was the main point of this bug.

I just diffed my tree, and I still match the patch as far as I can tell, so I'm
not sure why it doesn't work for you.  What OS are you using?

Comment 11

[Wladimir Palant]

(In reply to comment #10)
> 1. WFM - do you get a line number?

No, that's the whole error message. Maybe I should try a more recent nightly...

> 2. I thought about it, and did it by design.  This way, I can open the menu and
> open garfield, dilbert, and foxtrot with 4 clicks instead of 6.  Maybe that
> should get a pref (since I can imagine many people wouldn't like this).

A menu definitely needs to close when you click something. Here, the annoyance
of closing the menu manually (which is a non-standard behaviour, thus even more
annoying) by far outweights the advantages in a rather uncommon case where you
want to open more than one bookmark. If you want to open a dozen of bookmarks -
that's what the Bookmarks dialog is good for.

> 3. WFM - as you noted, that was the main point of this bug.

Yes, it works, my fault...

Comment 12

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Make menus close once you middle-click a bookmark.

Comment 13

[Wladimir Palant]

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

I don't think menuitem has a property open. This doesn't work.

Comment 14

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: Fixed patch

Hopefully I got it right this time.  Neil wanted me to close the menus before
opening the bookmarks, so this code does that.	For the main bookmarks menu, I
used the same method as Firefox (drag'n'drop to close menus).

Comment 15

[[e-mail]]

Comment on attachment 169608 [details] [diff] [review]
Fixed patch

>+                       onclick="if (event.button == 1) {this.open=false; BookmarksUtils.loadBookmarkBrowser(event, this.database);}">
I doubt this works for nested folders. Mind you I feel sure that there exist
cases where onDragCloseTarget doesn't look as if it does what at first glance
it should.

Comment 16

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #15)
> (From update of attachment 169608 [details] [diff] [review] [edit])
> >+                       onclick="if (event.button == 1) {this.open=false;
BookmarksUtils.loadBookmarkBrowser(event, this.database);}">
> I doubt this works for nested folders. 

Has anyone had the opportunity to look into this?  As far as I can tell, it does
in fact work.

Comment 17

[Wladimir Palant]

(In reply to comment #16)
> Has anyone had the opportunity to look into this?  As far as I can tell, it does
> in fact work.

As I already wrote - I tried it and |this.open = false| doesn't work at all, why
should it? Look for leftovers of previous experiments in your tree... And again,
it is IMHO a very bad idea to revive loadBookmarkMiddleClick() in this form -
using onDragCloseTarget() here really is bad style. The code from comment 9
should be more obvious and reliable.

Comment 18

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #17)
> (In reply to comment #16)
> > Has anyone had the opportunity to look into this?  As far as I can tell, it does
> > in fact work.
> 
> As I already wrote - I tried it and |this.open = false| doesn't work at all, why
> should it? Look for leftovers of previous experiments in your tree...

I pulled a clean tree and applied attachment 169608 [details] [diff] [review], and everything seems to
work.  If it's not working for you, I need access to whatever platform you're
testing on.

I will consider eliminating the use of onDragCloseTarget, though that should be
fixed for Firefox at the same time (I used it as a reference).

Comment 19

[[e-mail]]

Comment on attachment 169608 [details] [diff] [review]
Fixed patch

As discussed on IRC, call loadBookmarkMiddleClick on all clicks and in there
call hidePopup in a loop.

Comment 20

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Closes popups as Wladimir suggested, and uses loadBookmarMiddleClick for
everything.

Comment 21

[Wladimir Palant]

timeless: it seems, you really were in a hurry to give r+ here...

cst:

+                       onclick="if (event.button == 1)
BookmarksMenu.loadBookmarkMiddleClick(event, this.database);">

Please be consistent. Either you always check event.button or never. I would
prefer you relying on the check in loadBookmarkMiddleClick() and removing the
event.button check in all the other places.

Other than that the last patch looks good.

Comment 22

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Comment 23

[[e-mail]]

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

You'll need to patch the existing middle-click handler so that middle-clicks on
ptf folder menuitems work.

>+    // unlike for command events, we have to close the menus manually
>+    var node = aEvent.target;
>+    while (node) {
>+      if (node.nodeType == node.ELEMENT_NODE && node.tagName == "menupopup")
>+        node.hidePopup();
>+      
>+      node = (node == node.parentNode ? null : node.parentNode);
>+    }
node is never going to equal its own parent. Also, you can stop the loop at
aEvent.currentTarget (which you know never has any menupopup ancestors). And it
would be better if you used a for loop instead of a while loop.

Comment 24

[Wladimir Palant]

(In reply to comment #23)
> node is never going to equal its own parent.

Yes, I mixed it up with the parent propery. The correct loop will be then:

for (var node = aEvent.target; node && node != aEvent.currentTarget; node =
node.parentNode) {
  if (node.nodeType == node.ELEMENT_NODE && node.tagName == "menupopup")
    node.hidePopup();
}

Comment 25

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Let's see how many more tries it takes me to get this right.

Comment 26

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Checking in mozilla/xpfe/browser/resources/content/navigator.xul;
/cvsroot/mozilla/xpfe/browser/resources/content/navigator.xul,v  <--  navigator.
xul
new revision: 1.427; previous revision: 1.426
done
Checking in mozilla/xpfe/browser/resources/content/navigatorOverlay.xul;
/cvsroot/mozilla/xpfe/browser/resources/content/navigatorOverlay.xul,v  <--  nav
igatorOverlay.xul
new revision: 1.307; previous revision: 1.306
done
Checking in mozilla/xpfe/components/bookmarks/resources/bookmarksMenu.js;
/cvsroot/mozilla/xpfe/components/bookmarks/resources/bookmarksMenu.js,v  <--  bo
okmarksMenu.js
new revision: 1.21; previous revision: 1.20
done

Comment 27

[R.K.Aa.]

So did this finally fix bug 33761?

Comment 28

[[e-mail]]

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

Comment 29

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED with build 2005-03-03-05 on Windows XP using Seamonkey.

A top-level click in the Bookmarks menu or in the Manage Bookmarks window will
open the website URLs in the subfolder (the exception being that clicking a
top-level folder which contains _multiple_ sub-levels will just open the default
homepage).

Looks good.

Comment 30

[OstGote!]

Hmm ... with Mozilla 1.8b2 2005031304 on WinNT4 it works for me only in the
bookmark manager. Middle-Click (for a new tab) does not work with the bookmarks
menu or the  bookmark pulldown menu from the personal toolbar. Ctrl+Click does
work for all. Can anyone confirm this?

Comment 31

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Can anyone confirm comment #30?

Comment 32

[Andrew Schultz]

middle-click on the bookmarks menu (and via personal toolbar) worksforme (opens
 in a new tab) with linux trunk 20050403 and 20050313

Comment 33

[OstGote!]

(In reply to comment #30)
With current SeaMonkey Branch and Trunk builds on WinXP it works now for me. Thanks.