Make various bookmarks menus middle-clickable

VERIFIED FIXED

Status

VERIFIED FIXED
14 years ago
13 years ago

People

(Reporter: csthomas, Assigned: csthomas)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.
Created attachment 167650 [details] [diff] [review]
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.
Attachment #167650 - Flags: superreview?(jag)
Attachment #167650 - Flags: review?(neil.parkwaycc.co.uk)

Updated

14 years ago
Attachment #167650 - Flags: superreview?(jag)
Attachment #167650 - Flags: superreview+
Attachment #167650 - Flags: review?(trev)
Attachment #167650 - Flags: review?(neil.parkwaycc.co.uk)

Comment 2

14 years ago
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...
I changed my mind about the scope of this bug :).  New patch coming shortly.
Summary: Make personal toolbar overflow items middle-clickable → Make various bookmarks menus middle-clickable
Created attachment 167664 [details] [diff] [review]
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.
Attachment #167650 - Attachment is obsolete: true
Attachment #167664 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #167664 - Flags: review?(trev)

Comment 5

14 years ago
(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...
Whiteboard: active
(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).
Jag?  Timeless?  Anyone else disagree with comment #2?  I cc'ed you to hopefully
get opinions from other xpfe people ;)

Comment 8

14 years ago
i expect to be able to right click on bookmarks menus, this isn't unusual
behavior. but it will almost never work on macosx.
Whiteboard: active → active, r?

Comment 9

14 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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

14 years ago
(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...
Attachment #167664 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #167664 - Flags: review?(trev)
Status: NEW → ASSIGNED
QA Contact: seamonkey.bookmarks → stephend
Whiteboard: active, r? → active
Created attachment 169314 [details] [diff] [review]
patch

Make menus close once you middle-click a bookmark.
Attachment #167664 - Attachment is obsolete: true
Attachment #169314 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169314 - Flags: review?(trev)

Comment 13

14 years ago
Comment on attachment 169314 [details] [diff] [review]
patch

I don't think menuitem has a property open. This doesn't work.
Attachment #169314 - Flags: review?(trev) → review-
Attachment #169314 - Attachment is obsolete: true
Attachment #169314 - Flags: superreview?(neil.parkwaycc.co.uk)
Created attachment 169608 [details] [diff] [review]
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).
Attachment #169608 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169608 - Flags: review?(timeless)

Comment 15

14 years ago
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.
Whiteboard: active → active, r?
Target Milestone: --- → mozilla1.8alpha6
(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

14 years ago
(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.
(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

14 years ago
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.
Attachment #169608 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169608 - Flags: superreview-
Attachment #169608 - Flags: review?(timeless)
Whiteboard: active, r? → [cst:active]
Keywords: helpwanted
Whiteboard: [cst:active]
Target Milestone: mozilla1.8alpha6 → mozilla1.9alpha1
Created attachment 172759 [details] [diff] [review]
patch

Closes popups as Wladimir suggested, and uses loadBookmarMiddleClick for
everything.
Attachment #169608 - Attachment is obsolete: true
Attachment #172759 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #172759 - Flags: review?(trev)

Updated

14 years ago
Attachment #172759 - Flags: review?(trev) → review+
Keywords: helpwanted
Whiteboard: [cst: sr?]

Comment 21

14 years ago
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.
Created attachment 172766 [details] [diff] [review]
patch
Attachment #172759 - Attachment is obsolete: true
Attachment #172766 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #172766 - Flags: review?(trev)

Updated

14 years ago
Attachment #172766 - Flags: review?(trev) → review+
Attachment #172759 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 23

14 years ago
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.
Attachment #172766 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-

Comment 24

14 years ago
(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();
}
Whiteboard: [cst: sr?] → [cst: active]
Created attachment 174415 [details] [diff] [review]
patch

Let's see how many more tries it takes me to get this right.
Attachment #172766 - Attachment is obsolete: true
Attachment #174415 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #174415 - Flags: review?(timeless)

Updated

14 years ago
Attachment #174415 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+

Updated

14 years ago
Attachment #174415 - Flags: review?(timeless) → review+
Whiteboard: [cst: active] → checkin
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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Whiteboard: checkin

Comment 27

14 years ago
So did this finally fix bug 33761?

Comment 28

14 years ago
*** Bug 33761 has been marked as a duplicate of this bug. ***
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.
Status: RESOLVED → VERIFIED

Comment 30

14 years ago
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?

Updated

14 years ago
Target Milestone: mozilla1.9alpha1 → ---
Can anyone confirm comment #30?
Keywords: qawanted

Comment 32

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

Comment 33

13 years ago
(In reply to comment #30)
With current SeaMonkey Branch and Trunk builds on WinXP it works now for me. Thanks.
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.