Last Comment Bug 185270 - Shortcut key to open link in new tab, Insert (background) or Shift+insert (foreground)
: Shortcut key to open link in new tab, Insert (background) or Shift+insert (fo...
Status: RESOLVED FIXED
: fixed-seamonkey1.1
Product: Core
Classification: Components
Component: Keyboard: Navigation (show other bugs)
: Trunk
: All All
-- enhancement (vote)
: ---
Assigned To: Aaron Leventhal
: sairuh (rarely reading bugmail)
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-12-13 14:47 PST by Aaron Leventhal
Modified: 2007-01-12 09:21 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add key bindings and support in nsGlobalWindow.cpp, navigator.js. Don't need Shift+Insert paste binding in browser context. (9.83 KB, patch)
2002-12-13 15:23 PST, Aaron Leventhal
yuanyi21: review+
sfraser_bugs: superreview-
Details | Diff | Splinter Review
Much simpler patch (3.24 KB, patch)
2002-12-19 20:56 PST, Aaron Leventhal
andreww: review+
jag-mozilla: superreview+
Details | Diff | Splinter Review
Doesn't implement Insert for mac (just Shift+insert there which is still useful), because Insert means help on mac (7.71 KB, patch)
2003-01-09 18:09 PST, Aaron Leventhal
caillon: review+
jag-mozilla: superreview+
Details | Diff | Splinter Review
Remove Mac Shift + Insert (1.01 KB, patch)
2004-08-28 06:05 PDT, Stefan [:stefanh]
no flags Details | Diff | Splinter Review
Adaption of the code to the command event (Checked into trunk) (2.83 KB, patch)
2006-11-19 12:51 PST, Bruno 'Aqualon' Escherl
csthomas: review+
neil: superreview+
Details | Diff | Splinter Review
Adaption of the code to the command event - MOZILLA_1_8_BRANCH (2.87 KB, patch)
2006-11-20 05:32 PST, Bruno 'Aqualon' Escherl
no flags Details | Diff | Splinter Review

Description User image Aaron Leventhal 2002-12-13 14:47:02 PST
I'd like to see these new shortcut keys:
Insert to open in a new tab in the background
Shift+insert to open in a new tab in the foreground.
Comment 1 User image Aaron Leventhal 2002-12-13 15:23:40 PST
Created attachment 109261 [details] [diff] [review]
Add key bindings and support in nsGlobalWindow.cpp, navigator.js. Don't need Shift+Insert paste binding in browser context.
Comment 2 User image Kyle Yuan 2002-12-17 01:48:46 PST
changing summary.

We already have Ctrl+click to open link in new tab, why don't use Ctrl+Enter for
keyboard? Preferences/Navigator/Tabbed Browsing can control whether the new tab
is background or foreground.
Comment 3 User image Aaron Leventhal 2002-12-17 10:14:20 PST
Ctrl+Enter is tied to the behavior of Ctrl+Click and middle-click, which default
to open in new window unless a pref is changed.

I want a separate shortcut for using tabs -- that way the keyboard user can
choose whether they want a new window or tab.
Comment 4 User image Johnny Stenback (:jst, jst@mozilla.com) 2002-12-19 06:54:20 PST
Couldn't this be dealt with completely in chrome JS? I.e. look at all insert key
events and see if they're over a link and do the right thing there. Why involve
the DOM command code here?
Comment 5 User image Aaron Leventhal 2002-12-19 11:32:55 PST
Shoot, I can't remember why I decided against that. I must have been concerned
that looking at every keypress event just to pick out 1 or 2 would be considered
evil. No?
Comment 6 User image Aaron Leventhal 2002-12-19 12:28:23 PST
Spoke with jst, we decided that having a method in nsDOMWindowController to
DoCommandWithDOMEvent() is useful generally, and that it's a good idea to avoid
looking at every keystroke in chrome. Still seeking sr= on same patch.
Comment 7 User image Simon Fraser 2002-12-19 12:43:44 PST
Comment on attachment 109261 [details] [diff] [review]
Add key bindings and support in nsGlobalWindow.cpp, navigator.js. Don't need Shift+Insert paste binding in browser context.

I really don't like that patch, for several reasons.

+const char * const sNewForegroundTabString = "cmd_newForegroundTabWithTarget";
+const char * const sNewBackgroundTabString = "cmd_newBackgroundTabWithTarget";

Evil, evil, evil. GlobalWindow.cpp should not know anything about tabs. Tabs
are a front-end thing, and GlobalWindow is a backend thing.

+nsresult
+nsDOMWindowController::DoCommandWithDOMEvent(const char *aCommandName)

Having a command fire off an event seems weird. Why not fire off the event from
the frontend? Or maybe you should be using command params, if you need to feed
info along with the command.

I'm going to have to veto this patch.
Comment 8 User image Aaron Leventhal 2002-12-19 20:49:45 PST
Comment on attachment 109261 [details] [diff] [review]
Add key bindings and support in nsGlobalWindow.cpp, navigator.js. Don't need Shift+Insert paste binding in browser context.

Hmm... I guess this was overkill.
Comment 9 User image Aaron Leventhal 2002-12-19 20:56:23 PST
Created attachment 109812 [details] [diff] [review]
Much simpler patch
Comment 10 User image Simon Fraser 2002-12-19 21:28:07 PST
Patch looks better to me. Needs review from some front-end JS person.
Comment 11 User image Jesse Ruderman 2003-01-05 02:28:03 PST
I disagree about shift+insert not being needed for paste.  During typeahead
find, shift+insert should add the contents of the clipboard to the typeahead
find string.  And during typeahead find, a link often has focus.

What's wrong with Phoenix's Ctrl+Enter for opening in a new tab?
Comment 12 User image Aaron Leventhal 2003-01-06 09:24:49 PST
Ctrl+Enter is the default open in new window key. Two different operations.
Comment 13 User image Jesse Ruderman 2003-01-06 15:02:44 PST
In Phoenix, Shift+Enter opens a link in a new window, Ctrl+Enter opens a link in
a new tab, and there isn't a pref to switch them.  (Wontfixed for Mozilla in bug
85169.)
Comment 14 User image Aaron Leventhal 2003-01-06 15:24:35 PST
Okay then. As keyboard component owner I need to know these things. Where is the
list of Phoenix keyboard differences, so that I can plan?

Implementing Insert/Shift+Insert won't hurt Phoenix's Ctrl+Enter/Shift+Enter
stuff. I think we should try to be consistent between the 2 apps. What's the
keyboard shortcut for Save As in Phoenix? Who makes these decisions for Phoenix?
Comment 15 User image Jesse Ruderman 2003-01-06 19:42:03 PST
Alt+Click is "save link as" in Phoenix, but Alt+Enter doesn't work (I don't know
why).  http://texturizer.net/phoenix/keyboard.html lists most of the shortcuts
in Phoenix.  I think Blake and Hyatt make most of the keyboard and other UI
decisions for Phoenix.

Adding Insert/Shift+Insert for "open link in new tab" wouldn't break Phoenix's
Ctrl+Enter/Ctrl+Shift+Enter for "open link in new tab", but it would break
Shift+Insert for paste during typeahead find.
Comment 16 User image Aaron Leventhal 2003-01-06 22:28:17 PST
Alt+Enter is stolen by Windows, because it means properties. It can't be used
for anything -- I've tried. Isn't Accel+V the modern way to paste with the
keyboard. I thought Shift+Insert is the old accelertor for that. AFAIK, not many
people know/use that anymore. If I implement paste for typeaehadfind, it won't
be with Shift+Insert, sorry.
Comment 17 User image Jesse Ruderman 2003-01-06 22:50:01 PST
Alt+Enter also means Full Screen in many Windows programs.
Comment 18 User image jag (Peter Annema) 2003-01-09 15:44:36 PST
Comment on attachment 109812 [details] [diff] [review]
Much simpler patch

sr=jag, asking caillon for r=
Comment 19 User image Aaron Leventhal 2003-01-09 15:50:29 PST
Jag, did you mean to overwrite Andreww's r=?

 <andreww@netscape.com> has granted Aaron Leventhal <aaronl@netscape.com>'s
request for review:

Bug 185270: Shortcut key to open link in new tab, Insert (background) or
Shift+insert (foreground)
http://bugzilla.mozilla.org/show_bug.cgi?id=185270

Attachment 109812 [details] [diff]: Much simpler patch
http://bugzilla.mozilla.org/attachment.cgi?id=109812&action=edit
Comment 20 User image sairuh (rarely reading bugmail) 2003-01-09 16:10:11 PST
i'm sorry i didn't mention this earlier, but the Insert key on mac keyboards
also serves as the Help key. in mozilla (at least with a mach-o trunk build),
Help is not brought up when hitting this key (so, it'd work fine with this fix).
however, when i hit the key in chimera, the pointer turned from the regular
arrow to the question mark. this raises the question: how would this fix
interact with embed apps where the key relies on other (eg, OS-defined)
operations? perhaps it'd be moot (is this fix easily overriden?), but i'm
wondering what (if any) longterm affects might be (in other apps, other OS's).
thanks!
Comment 21 User image Christopher Aillon (sabbatical, not receiving bugmail) 2003-01-09 17:21:12 PST
Comment on attachment 109812 [details] [diff] [review]
Much simpler patch

I think aaronl found some problems with this patch.
Comment 22 User image Aaron Leventhal 2003-01-09 18:09:37 PST
Created attachment 111121 [details] [diff] [review]
Doesn't implement Insert for mac (just Shift+insert there which is still useful), because Insert means help on mac
Comment 23 User image Aaron Leventhal 2003-01-09 18:10:52 PST
Comment on attachment 111121 [details] [diff] [review]
Doesn't implement Insert for mac (just Shift+insert there which is still useful), because Insert means help on mac

Seeking new reviews because of Mac Insert=Help issue
Comment 24 User image Christopher Aillon (sabbatical, not receiving bugmail) 2003-01-09 18:38:24 PST
Comment on attachment 111121 [details] [diff] [review]
Doesn't implement Insert for mac (just Shift+insert there which is still useful), because Insert means help on mac

>Index: xpfe/communicator/resources/content/contentAreaClick.js
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/communicator/resources/content/contentAreaClick.js,v
>retrieving revision 1.34
>diff -u -r1.34 contentAreaClick.js
>--- xpfe/communicator/resources/content/contentAreaClick.js	14 Nov 2002 01:40:38 -0000	1.34
>+++ xpfe/communicator/resources/content/contentAreaClick.js	10 Jan 2003 02:06:43 -0000
>@@ -124,6 +124,8 @@
>     if (local_name) {
>       local_name = local_name.toLowerCase();
>     }
>+    
>+    var isKeyPress = (event.type == "keypress");

I really wish that our event inheritance wasn't so screwy and you could check
for |event instanceof TextEvent| first.  But oh well.

> 
>     switch (local_name) {
>       case "a":
>@@ -134,6 +136,7 @@
>         break;
>       case "input":
>         if ((event.target.type.toLowerCase() == "text" || event.target.type == "") // text field

I know you didn't write this (or at least not in this patch, not sure if you
did before), but could you change the above line to just be

   if (event.target.type == "text"

please?  The spec mandates we lowercase the type and "" is not a valid return. 
If there is no attribute specified, we return "text".

...

>@@ -148,9 +151,9 @@
>           linkNode = null;
>         break;
>     }
>+    var href;
>     if (linkNode) {
>-      handleLinkClick(event, linkNode.href, linkNode);
>-      return true;
>+      href = linkNode.href;
>     } else {
>       // Try simple XLink
>       var href;

Remove the above |href| declaration.  JS scoping rules will give you a
redeclaration warning.


With those nits addressed, and provided that everyone is happy with the
suggested keybindings, r=caillon.
Comment 25 User image Kathleen Brade 2003-01-10 06:17:15 PST
Comment on attachment 111121 [details] [diff] [review]
Doesn't implement Insert for mac (just Shift+insert there which is still useful), because Insert means help on mac

Please make sure that pressing shift-insert to paste in a textarea/input does
paste and doesn't add the tab.

In xpfe/browser/resources/content/unix/platformNavigationBindings.xul and
corresponding window version, I don't think you need:  modifiers=""
Also, these files seem to put the modifiers after the command (which strikes me
as odd but that may be their consistency)
Comment 26 User image Aaron Leventhal 2003-01-10 10:18:26 PST
Brade and caillon's fixes are in, and I've verified that paste still works in
input fields.

I'll put up the new patch if requested to do so.
Comment 27 User image jag (Peter Annema) 2003-01-10 19:43:25 PST
Comment on attachment 111121 [details] [diff] [review]
Doesn't implement Insert for mac (just Shift+insert there which is still useful), because Insert means help on mac

What was the effect of 
-      <handler event="keypress" keycode="VK_INSERT" modifiers="shift"
command="cmd_paste"/>

in platformHTMLBindings.xml? Did it just end up doing nothing?
Comment 28 User image Aaron Leventhal 2003-01-10 23:04:44 PST
Jag, AFAICT, that binding was erroneous. It was under "content" not in editable
areas. The patch still works if I leave those in, but I thought they should be
removed, because they're not doing anything.
Comment 29 User image jag (Peter Annema) 2003-01-21 11:09:37 PST
Comment on attachment 111121 [details] [diff] [review]
Doesn't implement Insert for mac (just Shift+insert there which is still useful), because Insert means help on mac

sr=jag
Comment 30 User image Aaron Leventhal 2003-01-21 19:27:26 PST
checked in
Comment 31 User image sairuh (rarely reading bugmail) 2003-02-10 17:51:14 PST
insert and shift+insert work fine on win2k and linux rh8.0. as expected, insert
doesn't work on mac --but shift+insert does nothing.

tested with 2003.02.10 comm trunk bits.

reopening since this is non-functional. however, let me know if comment 22 is no
longer valid (due to all the changes to the patch since then, i'm not sure); in
which case, do re-resolve. thanks.
Comment 32 User image Aaron Leventhal 2003-02-10 23:19:56 PST
The keybinding assignment is there for shift+insert on mac. If it's not working,
well ... I don't want to spend any time on figuring that out since Insert
doesn't work either. I suggest we close it.
Comment 33 User image sairuh (rarely reading bugmail) 2003-02-11 09:25:11 PST
was this tested on mac before it was checked in? did it ever work in a mac debug
build?
Comment 34 User image Isaac Chan 2004-01-04 01:26:02 PST
I am using win Xp.
just downloaded, installed mozilla days ago and i am still new to this browser.

When i click a link with 'insert', as expected, it open link in new tab.
however, not one tab open. but many(about 5-7 of them).
Comment 35 User image Stefan Borggraefe 2004-01-16 11:01:39 PST
I filed bug 231151 about the problem described in comment #34.
Comment 36 User image neil@parkwaycc.co.uk 2004-01-18 09:21:24 PST
Since Shift+Insert doesn't work on a Mac, perhaps someone could remove it?
Comment 37 User image Stefan [:stefanh] 2004-08-28 06:05:19 PDT
Created attachment 157253 [details] [diff] [review]
Remove Mac Shift + Insert

(In reply to comment #36)
> Since Shift+Insert doesn't work on a Mac, perhaps someone could remove it?

Here's a simple one-line patch that removes Shift+Insert from
mac/platformNavigationBindings.xul
Comment 38 User image Karsten Düsterloh 2006-11-19 08:18:02 PST
I pondered a bit why this doesn't work in current Windows SM builds:
- in platformNavigationBindings.xul, we have key definitions for VK_INSERT:
    <key ... command="cmd_newTabWithTarget"/>
    <key ... modifiers="shift" command="cmd_newTabWithTarget"/>
- in navigatorOverlay.xul, the executing function is defined:
    <command id="cmd_newTabWithTarget" oncommand="contentAreaClick(event);"/>

But contentAreaClick makes at least two wrong assumptions:
- it expects the event.(original)target to be the focused link node,
  but it's the <command> element
- it expects the event to be of type "keypress",
  but it's of type "command"
Comment 39 User image neil@parkwaycc.co.uk 2006-11-19 09:45:48 PST
Yes, this was "broken" by bug 331290. The new XBL code dispatches a command event on the <key> element, whereas the previous code used to compile the key attribute with an event parameter and pass it the original key event.
Comment 40 User image Bruno 'Aqualon' Escherl 2006-11-19 12:51:46 PST
Created attachment 245967 [details] [diff] [review]
Adaption of the code to the command event (Checked into trunk)

The patch adopts the code to recognize the command event fired by pressing VK_INSERT and sets the target of the event to the current focused element.
Comment 41 User image Bruno 'Aqualon' Escherl 2006-11-19 23:02:17 PST
(In reply to comment #40)
> Created an attachment (id=245967) [edit]
Just to clarify it, the bug is in Branch and Trunk and the patch works in both.
Comment 42 User image Bruno 'Aqualon' Escherl 2006-11-20 05:32:28 PST
Created attachment 246014 [details] [diff] [review]
Adaption of the code to the command event - MOZILLA_1_8_BRANCH

Branch diff of my patch, same code as trunk patch.
Comment 43 User image Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-11-20 15:46:16 PST
Comment on attachment 245967 [details] [diff] [review]
Adaption of the code to the command event (Checked into trunk)

The patch works.
Comment 44 User image neil@parkwaycc.co.uk 2006-11-21 01:38:46 PST
Comment on attachment 245967 [details] [diff] [review]
Adaption of the code to the command event (Checked into trunk)

Resetting sr+ due to mid-air collision...
Comment 45 User image Ian Neal 2006-11-21 15:37:02 PST
Comment on attachment 246014 [details] [diff] [review]
Adaption of the code to the command event - MOZILLA_1_8_BRANCH

a=me for SM1.1
Comment 46 User image Ian Neal 2006-11-21 15:38:05 PST
Comment on attachment 245967 [details] [diff] [review]
Adaption of the code to the command event (Checked into trunk)

Checking in (trunk)
contentAreaClick.js;
new revision: 1.59; previous revision: 1.58
done
Comment 47 User image Ian Neal 2006-11-21 15:50:23 PST
Comment on attachment 246014 [details] [diff] [review]
Adaption of the code to the command event - MOZILLA_1_8_BRANCH

You will have double check that the file is only used by SM otherwise you will also need an 1.8.1.1 approval
Comment 48 User image Philip Chee 2006-11-21 17:56:07 PST
/suite/ is relatively new. I doubt that anyone else is using this file (except possibly K-Meleon/Galeon/Ephiphany).
Comment 49 User image Bruno 'Aqualon' Escherl 2006-11-22 11:37:49 PST
(In reply to comment #48)
> /suite/ is relatively new. I doubt that anyone else is using this file (except
> possibly K-Meleon/Galeon/Ephiphany).
The patched file on branch is in xpfe/communicator/resources/content/, I'm not sure if this file is used by anyone else (I could try to find that out later).

Comment 50 User image Bruno 'Aqualon' Escherl 2007-01-12 09:21:27 PST
I think this bug can by closed -> RESOLVED FIXED

Note You need to log in before you can comment on or make changes to this bug.