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

RESOLVED FIXED

Status

()

Core
Keyboard: Navigation
--
enhancement
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

({fixed-seamonkey1.1})

Trunk
fixed-seamonkey1.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

15 years ago
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.
(Assignee)

Comment 1

15 years ago
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.
(Assignee)

Updated

15 years ago
Attachment #109261 - Flags: review?(akkana)
(Assignee)

Updated

15 years ago
Attachment #109261 - Flags: review?(akkana) → review?(kyle.yuan)

Comment 2

15 years ago
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)
(Assignee)

Comment 3

15 years ago
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.

Updated

15 years ago
Attachment #109261 - Flags: review?(kyle.yuan) → review+
(Assignee)

Updated

15 years ago
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?
(Assignee)

Comment 5

15 years ago
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?
(Assignee)

Comment 6

15 years ago
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

15 years ago
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-
(Assignee)

Comment 8

15 years ago
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
(Assignee)

Comment 9

15 years ago
Created attachment 109812 [details] [diff] [review]
Much simpler patch
(Assignee)

Updated

15 years ago
Attachment #109812 - Flags: superreview?(sfraser)
Attachment #109812 - Flags: review?(kyle.yuan)

Comment 10

15 years ago
Patch looks better to me. Needs review from some front-end JS person.
(Assignee)

Updated

15 years ago
Attachment #109812 - Flags: review?(kyle.yuan) → review?(sgehani)

Comment 11

15 years ago
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?
(Assignee)

Comment 12

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

Comment 13

15 years ago
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.)
(Assignee)

Comment 14

15 years ago
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

15 years ago
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.
(Assignee)

Comment 16

15 years ago
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

15 years ago
Alt+Enter also means Full Screen in many Windows programs.
(Assignee)

Updated

15 years ago
Attachment #109812 - Flags: superreview?(sfraser) → superreview?(jaggernaut)
(Assignee)

Updated

15 years ago
Attachment #109812 - Flags: review?(sgehani) → review?(andreww)

Updated

15 years ago
Attachment #109812 - Flags: review?(andreww) → review+

Comment 18

15 years ago
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+
(Assignee)

Comment 19

15 years ago
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!

Updated

15 years ago
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.
(Assignee)

Comment 22

15 years ago
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
Attachment #109812 - Attachment is obsolete: true
(Assignee)

Comment 23

15 years ago
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 25

15 years ago
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)
(Assignee)

Comment 26

15 years ago
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

15 years ago
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?
(Assignee)

Comment 28

15 years ago
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

15 years ago
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+
(Assignee)

Comment 30

15 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 15 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 → ---
(Assignee)

Comment 32

15 years ago
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?

Comment 34

14 years ago
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

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

Comment 36

14 years ago
Since Shift+Insert doesn't work on a Mac, perhaps someone could remove it?
Keywords: helpwanted

Comment 37

13 years ago
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

11 years ago
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

11 years ago
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.
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.
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.

Updated

11 years ago
Attachment #245967 - Flags: superreview?(neil) → superreview+
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.
Attachment #246014 - Flags: approval1.8.1.1?

Updated

11 years ago
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 44

11 years ago
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 45

11 years ago
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

11 years ago
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 47

11 years ago
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

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

Updated

11 years ago
Keywords: fixed-seamonkey1.1
(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
Last Resolved: 15 years ago11 years ago
Keywords: helpwanted
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.