Closed Bug 185270 Opened 20 years ago Closed 16 years ago

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

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

Details

(Keywords: fixed-seamonkey1.1)

Attachments

(4 files, 2 obsolete files)

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.
Attachment #109261 - Flags: review?(akkana)
Attachment #109261 - Flags: review?(akkana) → review?(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.
Summary: Shortcut key to open in new tab, Insert (background) or Shift+insert (foreground) → Shortcut key to open link in new tab, Insert (background) or Shift+insert (foreground)
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.
Attachment #109261 - Flags: review?(kyle.yuan) → review+
Attachment #109261 - Flags: superreview?(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?
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?
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 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.
Attachment #109261 - Flags: superreview?(jst) → superreview-
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.
Attachment #109261 - Attachment is obsolete: true
Attached patch Much simpler patch (obsolete) — Splinter Review
Attachment #109812 - Flags: superreview?(sfraser)
Attachment #109812 - Flags: review?(kyle.yuan)
Patch looks better to me. Needs review from some front-end JS person.
Attachment #109812 - Flags: review?(kyle.yuan) → review?(sgehani)
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?
Ctrl+Enter is the default open in new window key. Two different operations.
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.)
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?
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.
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.
Alt+Enter also means Full Screen in many Windows programs.
Attachment #109812 - Flags: superreview?(sfraser) → superreview?(jaggernaut)
Attachment #109812 - Flags: review?(sgehani) → review?(andreww)
Attachment #109812 - Flags: review?(andreww) → review+
Comment on attachment 109812 [details] [diff] [review]
Much simpler patch

sr=jag, asking caillon for r=
Attachment #109812 - Flags: superreview?(jaggernaut)
Attachment #109812 - Flags: superreview+
Attachment #109812 - Flags: review?(caillon)
Attachment #109812 - Flags: review+
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
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!
Attachment #109812 - Flags: review?(caillon) → review+
Comment on attachment 109812 [details] [diff] [review]
Much simpler patch

I think aaronl found some problems with this patch.
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
Attachment #111121 - Flags: superreview?(jaggernaut)
Attachment #111121 - Flags: review?(caillon)
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.
Attachment #111121 - Flags: review?(caillon) → review+
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)
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 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?
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 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
Attachment #111121 - Flags: superreview?(jaggernaut) → superreview+
checked in
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
was this tested on mac before it was checked in? did it ever work in a mac debug
build?
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).
I filed bug 231151 about the problem described in comment #34.
Since Shift+Insert doesn't work on a Mac, perhaps someone could remove it?
Keywords: helpwanted
(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
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"
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.
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.
Attachment #245967 - Flags: superreview?(neil)
Attachment #245967 - Flags: review?(cst)
(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.
Attachment #245967 - Flags: superreview?(neil) → superreview+
Branch diff of my patch, same code as trunk patch.
Attachment #246014 - Flags: approval1.8.1.1?
Attachment #246014 - Flags: approval1.8.1.1?
Comment on attachment 245967 [details] [diff] [review]
Adaption of the code to the command event (Checked into trunk)

The patch works.
Attachment #245967 - Flags: superreview?(neil)
Attachment #245967 - Flags: superreview+
Attachment #245967 - Flags: review?(cst)
Attachment #245967 - Flags: review+
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...
Attachment #245967 - Flags: superreview?(neil) → superreview+
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 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
Attachment #245967 - Attachment description: Adaption of the code to the command event → Adaption of the code to the command event (Checked into trunk)
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
/suite/ is relatively new. I doubt that anyone else is using this file (except possibly K-Meleon/Galeon/Ephiphany).
(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).

I think this bug can by closed -> RESOLVED FIXED
Status: REOPENED → RESOLVED
Closed: 20 years ago16 years ago
Keywords: helpwanted
Resolution: --- → FIXED
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.