185270 - Shortcut key to open link in new tab, Insert (background) or Shift+insert (foreground)

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, enhancement)

Description

[Aaron Leventhal]

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

[Aaron Leventhal]

Attachment: Add key bindings and support in nsGlobalWindow.cpp, navigator.js. Don't need Shift+Insert paste binding in browser context.

Comment 2

[Kyle Yuan]

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

[Aaron Leventhal]

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

[Johnny Stenback (:jst)]

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

[Aaron Leventhal]

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

[Aaron Leventhal]

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

[Simon Fraser [no longer active]]

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

[Aaron Leventhal]

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

[Aaron Leventhal]

Attachment: Much simpler patch

Comment 10

[Simon Fraser [no longer active]]

Patch looks better to me. Needs review from some front-end JS person.

Comment 11

[Jesse Ruderman]

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

[Aaron Leventhal]

Ctrl+Enter is the default open in new window key. Two different operations.

Comment 13

[Jesse Ruderman]

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

[Aaron Leventhal]

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

[Jesse Ruderman]

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

[Aaron Leventhal]

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

[Jesse Ruderman]

Alt+Enter also means Full Screen in many Windows programs.

Comment 18

[jag (Peter Annema)]

Comment on attachment 109812 [details] [diff] [review]
Much simpler patch

sr=jag, asking caillon for r=

Comment 19

[Aaron Leventhal]

Jag, did you mean to overwrite Andreww's r=?

 <[e-mail]> has granted Aaron Leventhal <[e-mail]>'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

[sairuh (rarely reading bugmail)]

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

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 109812 [details] [diff] [review]
Much simpler patch

I think aaronl found some problems with this patch.

Comment 22

[Aaron Leventhal]

Attachment: Doesn't implement Insert for mac (just Shift+insert there which is still useful), because Insert means help on mac

Comment 23

[Aaron Leventhal]

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

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

[Kathleen :Brade]

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

[Aaron Leventhal]

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

[jag (Peter Annema)]

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

[Aaron Leventhal]

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

[jag (Peter Annema)]

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

[Aaron Leventhal]

checked in

Comment 31

[sairuh (rarely reading bugmail)]

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

[Aaron Leventhal]

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

[sairuh (rarely reading bugmail)]

was this tested on mac before it was checked in? did it ever work in a mac debug
build?

Comment 34

[Isaac Chan]

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

[Stefan Borggraefe]

I filed bug 231151 about the problem described in comment #34.

Comment 36

[[e-mail]]

Since Shift+Insert doesn't work on a Mac, perhaps someone could remove it?

Comment 37

[Stefan [:stefanh]]

Attachment: 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

[Karsten Düsterloh]

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

[[e-mail]]

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

[Bruno 'Aqualon' Escherl]

Attachment: 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

[Bruno 'Aqualon' Escherl]

(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

[Bruno 'Aqualon' Escherl]

Attachment: Adaption of the code to the command event - MOZILLA_1_8_BRANCH

Branch diff of my patch, same code as trunk patch.

Comment 43

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

Comment on attachment 245967 [details] [diff] [review]
Adaption of the code to the command event (Checked into trunk)

The patch works.

Comment 44

[[e-mail]]

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

[Ian Neal]

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

[Ian Neal]

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

[Ian Neal]

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

[Philip Chee]

/suite/ is relatively new. I doubt that anyone else is using this file (except possibly K-Meleon/Galeon/Ephiphany).

Comment 49

[Bruno 'Aqualon' Escherl]

(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

[Bruno 'Aqualon' Escherl]

I think this bug can by closed -> RESOLVED FIXED