All of the new modifier- and middle- click behavior that applies to links should also apply to PT items. I'll attach a patch.

i'm acronym-impaired today, what's PT? physical therapy? psychic therapy?

Sairuh, PT - personal toolbar. Blake, in news message (at least news, do not use mozilla's mail) the CTRL-clisk does not work either.

thx, eugene! cc'ing claudius, mr toolbar...

Eugene, ctrl+click in news messages should work with my latest checkin. Lemme know.

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

Does this patch include context menus for personal toolbar items, or is there another bug for that? "PT" is difficult (and non-obvious) to search for, so adding "personal toolbar" to summary.

context menus for personal toolbar = bug 17920

patch is 2 months old, please take a look. Adding patch keyword.

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

Whats the status of this rfe/bug ?

Although I do want this feature, I wonder how it belongs in "Keyboard Navigation"

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

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

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

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

Blake, any progress on this?

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

If there's a patch, why hasn't it been checked in yet ?

adding myself

Hey... we forgot to celebrate the patches 1st birthday! Please, someone freaking review/check this in if it hasen't bitroted to hell and back by now.

The patch as it stands broke (if I recall correctly) keyboard use of the personal toolbar as well as the use of personal toolbar menus....

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

Since, I'm eager to get this functionality into Mozilla. Here's a patch that I put together with patchmaker. It even works with sub menus on the Personal toolbar. I've never used just the keyboard on the PT, but if someone would explain what broke when applying the previous patch, I'll test that mine does not suffer from the same problem. Tested on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:0.9.9+) Gecko/20020326 Also if anyone is interested I also posted a patch to bug 103834.

Great! Adding mozilla1.0 and patch keyword so this gets looked at. Is this a low risk patch? If not, we won't see it in 1.0 I fear. Have you asked someone for r and sr?

It should be low risk, there is only half a dozen new lines of code, the rest is moving some stuff around. How do I request a review? and who do I request it from?

> +function getURL(node, datasources) //extract a URL This is a pretty generic function name for a function visible in global scope from navigator.xul... Maybe something more like |getURLFromPersonalToolbarItem|? > return; // don't bother loading it This is going to trigger a strict javascript warning ("Function does not always return a value"). Please return |null| or |""| (I'd lean toward the latter). >- if (target) >+ if (target) What's with this change? If this is because you replaced some spaces with a tab, please don't do that. :) > + onclick="handleLinkClick(event,getURL(event.target,document.getElementById('inne rmostBox').database),null)"> Please put some spaces after those commas. I'm assuming you've tested this thoroughly? Tried middle-click, shift-click, ctrl-click and their interaction with the various click-related tabbed browsing preferences with this patch? Drivers will need information on the testing this received to approve it anyway, so you might as well do it... The one thing that gives me pause is mixing oncommand and onclick and I have no tree at the moment to test the behavior on... Fix those and I'll take another look. :) For more info on review, see: http://mozilla.org/hacking/ and http://mozilla.org/hacking/reviewers.html (you'll want blaker@netscape.com or jaggernaut@netscape.com to sr this patch once it has r=). For info on 1.0 approval, you'll want to mail drivers@mozilla.org once the patch has r= and sr=; see the tail end of http://mozilla.org/status/2002-03-13.html for the sort of information that should go into the email.

In the previous patch if a left click or modified left click event occurs, then both the oncommand and onclick methods will run. I found a way around this by looking at the event for each and only running code in the click event to handle modified clicks (alt, cntrl, etc) and middle click, and only running code in the oncommand function if no modifiers were used. This works fine on the PT normally, but in a submenu the menuitem element does not give me any useful information in the event for how the event was fired, so I cannot use the same tactic. If someone knows how to get useful information out of a menuitem event let me know. I need whether any modifiers where used. So far it always returns 53535 as the button and always gives false for the modifiers. So my choices at this point are A. Make only url's on the main PT have full link functionality and let submenu's have just middle click support. or B. Make middle click support work every where including submenus, but hold off on full functionality until its possible on sub menus. Ideas? Also using the regular link handling code for the personal toolbar sets the referrer as the current document. Should the referrer be left out in this situation? After all the current document is *NOT* really how you got to the new site. I'm also trying how to figure out how to make a PT submenu close after someone middle clicks a link. Currently it remains open.

> So far it always returns 53535 as the button and always gives false for the > modifiers. That sounds like a (fairly major) bug -- we're leaving the button field uninitialized.... Please check for it and then file a bug (probably on events, but maybe on XUL) if there isn't one already? Oh, and what broke with the first patch is that is was not using oncommand. Triggering the personal toolbar with the keyboard will trigger oncommand but not onclick, so the first patch screwed over keyboard-only users.

Alright stupid question, but how do I get to the Personal toolbar without using the mouse? I tried cntrl+tab and it seems to skip it, or else I just can't see when its selected. Also note that this bug now depends on 126189 for menuentry events.

Much improved patch. I put in a check to prevent modified left click events from being dealt with in menus until bug 126189 is fixed. But you can still use middle click everywhere. Outstanding issues 1. Menu's don't close after a middle click. 2. After testing heavily, I've run into cases when handlelink fails because it cannot get a referrer. The getReferrer function is just throwing an exeption because there seems to be no valid focused content window. The exeption says "focusedWindow._content has no properties". I can bullet proof the getReferrer error handling so it won't die and just return null, but that means the referrer will be sent erratically. Which again begs the question, should I be sending a referrer in the first place? 3. I still don't know how to test keyboard only access to the personal toolbar.

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

It would be nice if this behavior were also applied to bookmark menus. After all, a Personal Toolbar item is also a bookmark. (This suggestion is not the same as Bug 72361 which wants Ctrl+Click to open menus in new windows by default. I want opening in a new tab to be the result of the Tabbed Browsing configuration for links.)

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

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

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

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

A referrer is no longer sent when clicking on a personal toolbar link like it was in the previous patch. So problems getting a referrer are no longer relevant. I'm gonna need some help from some more experienced mozilla developers to fix these still outstanding issues. issues 1. PT Menu's don't close after a middle click. 2. Patch probably doesn't support keyboard only users of PT (Don't know how to test.) 3. Waiting on bug 126189 for full support of PT Menu's.

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

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

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

Works great seeking reviews. resolved issues 1. PT Menu's don't close after a middle click. FIXED 2. keyboard only users of PT. Most likely fixed, uses oncommand for everything now except for middle click. 3. Waiting on bug 126189 for modified left click on PT menus but middle click still works everywhere.

> + var url = getURLFromBookmark(node, datasources); > + > + // Check if we have a browser window Shouldn't this bail if |url| is the empty string here? > + else > - openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no", url); > + { Please follow the prevailing "open-brace-on-same-line" style? > + if (tmpTarget.nodeName == "menupopup") > + tmpTarget.hidePopup(); Comment that you can't |break;| here because you need to close any parent menus too? > + //oncommand always leaves button numbers set to 65535 > + button = (bookmarkItem && event.button == 65535)?0:event.button Point this to the bug involved? And comment in that bug that this should be fixed once that bug is fixed? There is another thing here that bothers me. This will not send "referer" for clicks on links in regular pages that happen to have "bookmark-item" as a class. Perhaps the |bookmarkItem| bool should simply be passed to this function? That would be much better, imo.

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

updated per comments and to work with current trunk Fixed - PT item opened in new window no longer sends referrer

Middle click attempting to open fictionary pages for home button and the bookmarks button (home-button.com and bookmarks-button.com) -FIXED Additionally, middle clicks on the PT bookmarks drop down menu now work after applying the patch. Interestingly, I made no changes to get this working but I hadn't checked its behavior recently.

Fixes middle click on empty space on PT opens innermostBox.com. Could use a suggestion on a better fix. This patch checks for "hbox" which is the innermostBox (bascially the PT itself) if (event.target.nodeName == "hbox") return false;

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

Better fix. Seeking reviews

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

Nominating for nsbeta and mozilla1.2. Blake: any chance of getting this reviewed ?

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

There have been 20 dups for this bug; does it qualify for "mostfreq" status? I should point out that this is a "Galeon parity" issue (is there a keyword for that?) -- Galeon does an excellent job of this and outshines Mozilla here and in some other areas like tab management and crash recovery... (It seems wrong that we have to go to a "lightweight" browser with Mozilla embedded to get valuable features that the "heavyweight" full Mozilla doesn't offer, don't you think?)

Can we get a brief document attached that describes how this patch modifies the current situation?

This patch runs click events on personal toolbar items throught the handleLinkClick function used by normal links. This adds support for all click events that work on links. Additionally, since it is fired through the oncommand handler it should also be fired for keyboard (ctrl+enter) events, but since I don't know how to get focus on the PT (It's always skipped in the Ctrl+Tab rotation), I haven't tested this. + oncommand="PersonalToolBarOpenURL(event, this.database)" + onclick="PersonalToolBarURLClick(event, this.database)" It adds both an onclick handler and an oncommand handler to the PersonalToolbar. The onclick handler is needed because oncommand does not process middle click events and oncommand doesn't handle keyboard events. (A little odd I realize, but I've had no problems in testing.) The seperate functions PersonalToolBarOpenURL and PersonalToolBarURLClick must be used so events like left click are not handled twice. The bookmark parsing code is seperated out into a new getURLFromBookmark function so it is not duplicated. - function handleLinkClick(event, href, linkNode) + function handleLinkClick(event, href, linkNode, bookmarkItem) handleLinkClick is modified with a flag bookmarkItem stating whether the item is from a bookmark. The flag is used to determine whether a referrer should be sent. + var button = (bookmarkItem && event.type == "command")?0:event.button; Additionally, oncommand does not set event.button so it is checked. Limitations: Middle click and modified middle click events both work on PT sub-menu's but modified left clicks won't work until bug 126189 is fixed. This patch does not add extra functionality to other items on the PT (bookmarks, home button), I'll take a look at that in other bugs.

This is identical to attachment 97481 [details] [diff] [review] except that I created it with cvs diff whereas the original used patchmaker (and was difficult to apply). All code is from the original patch--credit/blame goes there, not here.

I think the bug Bug 171792 may be a duplicate, or dpendancy for this bug.

It's not.

We have a patch. Who's gonna review it?

Nav triage team: nsbeta1-

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

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

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

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

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

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

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

I would love to see this working in the latest builds, was going to report it ages ago but decided to do a search now and found this bug reported. Any movement on this little puppy.

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

Plz... 1.4.

neil, any cycles to review (and if necessary, clean up) this patch? note to aedmonds, only drivers can block + bugs. I've cleared your +. You can ? it (I didn't, since I don't think this should block 1.4, but I'd still like to see it fixed).

comments for neil: we'll have to make sure this plays nice with the non bookmark PT buttons (print,go,etc), and bookmark groups, and folders of bookmarks on the PT.

Comment on attachment 97719 [details] [diff] [review] Same patch, now easier to apply Sorry, but I couldn't get this to apply in my nightly build 2003032404, and my cvs build is horked :-( and a big bookmarks patch landed so that might further bitrot this patch :-/

What's the latest status on this one? (Perhaps this bug/rfe would be easier to find (and therefore less dupe-prone) if "new tab" or "tab" was added to the summary?)

Install TabBrowser Extensions (from http://white.sakura.ne.jp/~piro/xul/_tabextensions.html.en) so you'll be able to open new tabs from the TB with a middle click (and much more). Perhaps it redundantizes this patch.

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

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

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

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

Fixed by patch to bug 72361.

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