Closed Bug 59132 Opened 22 years ago Closed 18 years ago

Modifier- and middle-click on PT items should behave like links/open in new tab (personal toolbar)

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Future

People

(Reporter: bugzilla, Assigned: neil)

References

(Blocks 1 open bug)

Details

(Keywords: polish, Whiteboard: haspatch)

Attachments

(2 files, 10 obsolete files)

All of the new modifier- and middle- click behavior that applies to links 
should also apply to PT items.  I'll attach a patch.
Attached patch patchSplinter Review
i'm acronym-impaired today, what's PT? physical therapy? psychic therapy?
Component: XP Apps: GUI Features → Keyboard Navigation
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.
Status: NEW → ASSIGNED
*** Bug 57526 has been marked as a duplicate of this bug. ***
Priority: P3 → P2
Target Milestone: --- → mozilla0.9
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.
Summary: Modifier- and middle-click on PT items should behave like links → Modifier- and middle-click on PT items should behave like links (personal toolbar)
context menus for personal toolbar = bug 17920
Keywords: patch
patch is 2 months old, please take a look.  Adding patch keyword.
Keywords: patch
*** Bug 65526 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla0.9 → mozilla1.0
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
Target Milestone: mozilla1.0 → Future
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. ***
Attached patch new patch (obsolete) — Splinter Review
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?
Keywords: mozilla1.0, patch
Blocks: 123569
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.
Attached patch even newer patch (obsolete) — Splinter Review
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. ***
Attached patch updated patch (obsolete) — Splinter Review
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. ***
Attached patch updated patch (obsolete) — Splinter Review
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. ***
Attached patch patch (obsolete) — Splinter Review
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.
Attached patch patch (obsolete) — Splinter Review
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;
Attachment #76589 - Attachment is obsolete: true
Attachment #76860 - Attachment is obsolete: true
Attachment #83246 - Attachment is obsolete: true
Attachment #85064 - Attachment is obsolete: true
Attachment #85128 - Attachment is obsolete: true
Attachment #89754 - Attachment is obsolete: true
Attachment #90177 - Attachment is obsolete: true
*** Bug 157665 has been marked as a duplicate of this bug. ***
Attached patch updated (obsolete) — Splinter Review
Better fix.  Seeking reviews
Attachment #90356 - Attachment is obsolete: true
*** 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?)
Attached patch updated for current trunk (obsolete) — Splinter Review
Attachment #91906 - Attachment is obsolete: true
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.
Attachment #97481 - Attachment is obsolete: true
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-
Keywords: nsbeta1nsbeta1-
*** 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. ***
Flags: blocking1.3a?
Flags: blocking1.3a? → blocking1.3a-
*** 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.
Flags: blocking1.4a+
Whiteboard: haspatch
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).
Assignee: blaker → neil
Status: ASSIGNED → NEW
Flags: blocking1.4a+
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. ***
Summary: Modifier- and middle-click on PT items should behave like links (personal toolbar) → Modifier- and middle-click on PT items should behave like links/open in new tab (personal toolbar)
*** Bug 241775 has been marked as a duplicate of this bug. ***
Fixed by patch to bug 72361.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
*** Bug 255190 has been marked as a duplicate of this bug. ***
Status: RESOLVED → VERIFIED
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.