If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[FEATURE] Click link with key modifier and open URL in new chromed window

VERIFIED FIXED in mozilla0.9

Status

()

Core
Keyboard: Navigation
P1
normal
VERIFIED FIXED
18 years ago
15 years ago

People

(Reporter: don, Assigned: Blake Ross)

Tracking

({helpwanted})

Trunk
mozilla0.9
helpwanted
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: trunk-only fix)

Attachments

(10 attachments)

(Reporter)

Updated

18 years ago
Priority: P3 → P1
Summary: Click link with key modifier and open URL in new window → Click link with key modifier and open URL in new chromed window
Target Milestone: M10
(Reporter)

Description

18 years ago
Bill, find out what key modifiers we should be using for the various platforms
and let me know whether this is actually possible.
(Reporter)

Comment 1

18 years ago
Bill, do you also need notifications on nsILinkHandler for a right mouse click
(see Radha's bug #6085) for this to work?
(Reporter)

Updated

18 years ago
Summary: Click link with key modifier and open URL in new chromed window → [FEATURE] Click link with key modifier and open URL in new chromed window
(Reporter)

Updated

18 years ago
Blocks: 10575

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 2

18 years ago
This one does require some support from underlying nsWebShell (and perhaps lower
down, in nsHTMLAnchorElement since it is doing much of the work).  I think the
problem is deeper than getting nsWebShell to pass link click notifications.  The
nsHTMLAnchorElement seems to be deciding what the "verb" is (replace, new, ?)
but that seems a bit presumptious.  First, it decides to use eLinkVerb_Replace
regardless of any key modifiers.  But even if it somehow tested for key
modifiiers, doing it at this level makes it difficult to then have an embedding
app that might have different ideas about what those modifiers should be.

I'm cc'ing scc on this, since he is the webshell guru now.  This complication
should be addressed within scope of the general link notification issue (which
I'll assume is covered by bug #6085, et al).

Updated

18 years ago
QA Contact: beppe → cpratt

Comment 3

18 years ago
Sounds like a great feature request. Going by comments on the UI newsgroup,
ideally we'd so something like this:
Linux: middle-click
Mac: shift-click
Win32: not sure yet, but possibly change it to shift-click (shift-click
currently saves the link to disk)

Updated

18 years ago
Target Milestone: M11 → M12

Comment 4

18 years ago
Adding travis to cc: list (and removing scc).  The new and improved webshell's
link handling interfaces need to provide us a means of dealing with this.

Moving out to M12, too.
(Reporter)

Updated

18 years ago
Target Milestone: M12 → M13
(Reporter)

Comment 5

18 years ago
Move this to M13 because it isn't really dogfood.
(Reporter)

Updated

18 years ago
No longer blocks: 10575
(Reporter)

Updated

18 years ago
Target Milestone: M13 → M15
(Reporter)

Comment 6

18 years ago
Not required for beta 1.

Comment 7

18 years ago
*** Bug 31728 has been marked as a duplicate of this bug. ***

Comment 8

18 years ago
I could imagine users wanting to be able to do any of the following without 
right-clicking:
- open new normal-sized window in front
- open new maximized window in front (bug 17921)
- open new maximized window in back, or minimized (in addition to being 
maximized)
- copy url

So this feature should be customizable:
- Clicking on a link does: [dropdown, default: open in same window]
- Shift-clicking on a link does: [dropdown, default: open in new normal window]
- Ctrl-clicking on a link does: [dropdown, default: nothing special]

Also, the context menu for the links this would affect (file, http, finger, 
ftp, what else?) should show the modifiers, for example:
- Open link in new window    (shift-click)
- Save link as                (ctrl-click)

Comment 9

18 years ago
I totally agree with davidr8@home.com. It should be customized. I wish to be 
able open a new maximized window (I think it's the most windows peoples use) 
in foreground and maximized in background.

Comment 10

18 years ago
Bug 17754 says this should work on form submissions too.

Comment 11

18 years ago
*** Bug 22761 has been marked as a duplicate of this bug. ***

Comment 12

18 years ago
Suggested modifier keys ...

For opening link in new window:
* On MacOS, use Cmd+click. This is not only 4xp, but also IEp, and iCabp.
* On X, use the middle mouse button. This is 4xp.
* On Windows (and also on X, assuming that some X users won't have three mouse
  buttons), use Ctrl+click. This is the logical equivalent on PCs to Cmd+click on
  Macs, making life easier for those who will use Mozilla on multiple platforms.

For saving link:
* On MacOS, use Option+click. This is 4xp.
* On Windows and X, use Alt+click. This may be 4xp, I don't have a PC in front of
  me to be sure. But it's the PC equivalent of Option+click, anyway, which will
  make life easier for those who will use Mozilla on multiple platforms.

For open link in maximized window:
* Don't bother. This won't be useful for:
  - those users who normally browse in maximized windows anyway (since the new
    window should inherit the maximized state of the previous one);
  - those users who don't normally browse in maximized windows anyway (since
    their displays are too big for a maximized window to be useful);
  - those users for whom pressing Ctrl+click and then clicking maximize is easier
    than remembering and using the modifier key for opening in a maximized
    window.
  And I think those three criteria together would cover at least 99% of users.

Shift+click can not be used, because it's the 4xp (and OtherApps-p) modifier key 
for allowing selecting text in links (i.e. temporarily turning links off for text 
selection purposes). See bug 15665.

As for customization of these key-bindings ... allow customization if you like, 
but don't do so in the prefs dialog, do so with editing raw prefs.js. Customizing 
something like this is definitely an extreme power-user feature.
Keywords: 4xp

Comment 13

18 years ago
Everything in the last comment makes sense, which is why it's annoying that
inconsistent reality once again intrudes. On Win32 (and for that matter, on
Win16) Shift+click has been overloaded three ways since at least NN 2.0. 

1. With NN 4.7, if shift is pressed and held, a simple click on a link brings 
   up a Save-as dialog for that link. This comes in handy if the mime type
   is set wrong on the server end, or if the default action for that mime type
   on the browser end is something other than save.

2. If shift is pressed and held, and a click-drag is begun elsewhere on the 
   page and continues partway into the link text, the selection includes that
   part of the link text and no action is taken on the link.

3. For both 1 and 2, if a click has been made anywhere else in the text of the
   page, the shift+click creates a selection extending from the initial click
   to the shift-click, exactly as expected for a normal selection not including
   part of the text of a link - but 1 and 2 also happen as above, although 
   in case 2, this will feel the same. All of this is bug 15665 territory.
   In case 1, the selection is odd-looking, but not a problem.

Case 1 is 4xp; there is no particular reasons not to also support ALT+click
on win32 other than the wastefulness of using two key-click-combos for the same 
action, and the chance that the ALT could be let up before the click, 
activating the menubar. ALT+click does nothing with NN 4.7 on WinNT.

I'd hesitate to change the existing key-click-combo for saving a link on Win32
for the reason put forward by Tognazzini: shortcut keys should be kept the most
consistent of any part of a UI - see
http://www.asktog.com/basics/firstPrinciples.html#consistency

Updated

18 years ago
QA Contact: cpratt → elig
(Reporter)

Comment 14

18 years ago
Move to M16 for now ...
Target Milestone: M15 → M16

Comment 15

18 years ago
*** Bug 36199 has been marked as a duplicate of this bug. ***

Updated

18 years ago
Keywords: nsbeta2
Target Milestone: M16 → M17

Comment 16

18 years ago
*** Bug 37807 has been marked as a duplicate of this bug. ***

Comment 17

18 years ago
Putting on [nsbeta2+][5/16] radar.  This is a feature MUST complete work by 
05/16 or we may pull this feature for PR2.
Whiteboard: [nsbeta2+][5/16]
(Reporter)

Comment 18

18 years ago
Move to M19 target milestone.
Target Milestone: M17 → M19

Comment 19

18 years ago
Putting on [nsbeta2-] radar. Removing [5/16]. Missed the Netscape 6 feature 
train.  Please set to MFuture.
Whiteboard: [nsbeta2+][5/16] → [nsbeta2-]

Comment 20

18 years ago
Open In New Window" left-click modifier... what a great idea! I can't
speak for anyone else, but include it, document it, and I'll use it :)

Discussion on this has also been raised in #22529

Comment 21

17 years ago
Nav triage team: [nsbeta3-]
Whiteboard: [nsbeta2-] → [nsbeta2-][nsbeta3-]
perhaps this should go under Keybd Nav? (i know, it deals with the mouse, too,
but this seems to be a better place than the Basket That Is XP Apps. ;-)
Component: XP Apps → Keyboard Navigation

Comment 23

17 years ago
Oh, ye gods ... helpwanted.
Keywords: helpwanted
For the record, command-clicking works on the Mac as of 2000073120 (M18 trunk).
(It needs performance tuning, but it works, nonetheless.) What is the status of
ctrl-clicking on Windows and Linux?

Comment 25

17 years ago
Testing with the 2000-07-31-08-M18 nightly binary on WinNT, 
Ctrl-click, Shift-click, and Alt-click on a link all work the same as
a plain click; no new window, no save as, they just open the new page
in the same window.
hi Henri, i've been looking at the branch [2000.08.01.04-m17, commercial to be
exact] bit this week, and on winNT and linux the behavior of Ctrl+click is [over
content] to select the entire paragraph --or, rather, select the entired
contents of a table cell. (i tested by going the http://www.mozilla.org, and
clicking over the cell with "This status update contains information on...") and
i also got the same result doing Command+click on the Mac [using same bits as
winNT and linux].
QA Contact: elig → sairuh

Comment 27

17 years ago
> on winNT and linux the behavior of Ctrl+click is [over content] to select the
> entire paragraph --or, rather, select the entired contents of a table cell.

That shouldn't be happening. Selecting a paragraph/cell should be triple-click 
(bug 32807); opening link in a new frontmost window should be Cmd+/Ctrl+click 
(this bug); opening a link in a new second-from-front window should be
Cmd+/Ctrl+Shift+click (some bug yet to be filed:-).
Adding nsbeta3 keyword to bugs which already have nsbeta3 status markings so 
the queries don't get all screwed up.
Keywords: nsbeta3
(Assignee)

Comment 29

17 years ago
seems easy enough. so what's the decision -- shift or ctrl or win32/linux ?
Assignee: law → BlakeR1234
Status: ASSIGNED → NEW

Comment 30

17 years ago
Personally I don't want shift-click overloaded.  We already have a case where 
shift or ctrl wheel overloading was vetoed due to conflicts.

ctrl or alt is fine. As long as mpt doesn't disagree w/ himself elsewhere i'll 
support him here.
(Assignee)

Comment 31

17 years ago
Created attachment 13589 [details] [diff] [review]
patch to make links clicked with control modifier or middle mouse button open in new window
(Assignee)

Comment 32

17 years ago
attached a patch, please review.

The only caveat I see right now is that it seems to start resolving the URL in 
the old window before a new window is opened which then starts loading the URL. 
This doesn't seem to have any adverse effects, though.
Status: NEW → ASSIGNED
Keywords: review
(Assignee)

Comment 33

17 years ago
Created attachment 13590 [details] [diff] [review]
patch - just some visual touchups
(Assignee)

Comment 34

17 years ago
ahhh...I see now why this wasn't done sooner (see law's old comment re: 
embedding purposes at http://bugzilla.mozilla.org/show_bug.cgi?id=6085).  I'd 
rather have a C++ fix at least, than nothing.  Of course I'd much prefer it in 
JS if possible.

(fwiw, people are telling me that middle click still doesn't work to open a 
link in a new window, and it doesn't work for me either without my patch...)

Comment 35

17 years ago
>The only caveat I see right now is that it seems to start resolving the URL in 
>the old window before a new window is opened which then starts loading the 
>URL. 

This already happens with target= links (but not "open in new window"), so 
there's probably a bug on it somewhere.

Comment 36

17 years ago
And Blake Ross continues weaving UI magic wherever he goes ...

Jesse, I think the bug you are looking for is bug 9805.

Hey Blake, I don't suppose you could do {new-window-modifier}+Shift+click to open 
in a second-from-frontmost window at the same time, could you? (I'm not 
contradicting myself here, really I'm not: this wouldn't interfere with the use 
of Shift to select link text, because it's using another modifier key at the same 
time as Shift. And IIRC this is both IEp and iCabp, too.)

Comment 37

17 years ago
Windows:
* shift+click has a defined behavior: make a selection.
* Netscape4's behavior for shift+click[link] is select and open link for 
saving.
* IE's behavior is somewhat similar.
If you have a selection and use shift+click you get the selection extended to 
where you click and then a new window for the click.
(Assignee)

Comment 38

17 years ago
Bill, any word on this?  I'd like to get the patch in before beta3 if it's 
acceptable to do it that way.

Comment 39

17 years ago
Sorry, I didn't realize it was up to me to OK this one.  Architecturally 
speaking, and in my humble opinion, this is the wrong place for this logic.  
That said, it appears that it would work, so I have no problem with it.

However, I'm not the owner of this layout code so you really need approval from 
somebody else.  You can tell them I reviewed it and said it was OK, if that 
helps.
Please put a big "XXX move this policy code up into the XUL/JS/other-embedding
layer" comment and attach the revised patch.

What stands in the way of doing the right thing now?  What open bug describes
the lack of scriptable interfaces (or whatever the problem is) that blocks us
from moving this code out of low-level "mechanism" code and into "policy" land?

/be

Comment 41

17 years ago
I'll echo Brendan in wondering why this isn't in navigator.js.  Putting complex
navigator UI behavior in the low-level C++ seems like the wrong place.

See the middle click handler in navigator.{xul,js} to see how easy it is to do
this sort of thing.  Open in new window is certainly scriptable (middle mouse is
already doing that); save link as certainly should be.
(Assignee)

Comment 42

17 years ago
Bill, Brendan, Akkana: I am not disagreeing with you; I hate the patch and the 
method it uses to fix it also.  So let's trash it.

Akkana, people are telling me that middle click still doesn't work for them to 
open in a new window... indeed, mousewheel button don't work for me.  Any idea 
why?

Comment 43

17 years ago
I've heard there are driver issues with wheel mice (we don't work with all mouse
drivers, for reasons not clear to me).  The functionality is still working
(works for me in today's build) so I have to guess that it's a driver problem. 
Does middle-mouse paste work for you?  You are on Unix, right?  Middle mouse
functions (other than scrolling) are off by default on nonunix platforms, and
you have to turn them on with 
user_pref("middlemouse.paste", true);
user_pref("middlemouse.openNewWindow", true);
(Assignee)

Comment 44

17 years ago
Oh.  Nope, I'm on windows.  So I'll try those prefs.  Thanks.
Adding bryner for wheel-mouse expertise.

/be
cc'ing janc who tests mousewheel stuff.
middle button may be getting intercepted by your mouse driver on windows to go
into scroll pan mode.

Comment 48

17 years ago
Currently, clicking on a link with mouse button 1 (left button, I guess), 
navigates to the link, regardless of any key state.  You cannot, I don't 
believe, fix that in navigator.xul/.js, because the click never gets there 
(unlike middel button clic and right-mouse click).

I'm not sure that the right answer is for the C++ code to do nothing with mouse 
clicks on links (i.e., let navigator.xul/.js handle the whole thing).  That 
wouldn't work too well for other sorts of embedding applications (e.g., viewer).

My belief was that part of the embedding API would include an interface by which 
the embedee would notify the embeder that such events had transpired.  The 
embeder would then handle them as it saw fit (with perhaps some canonical 
default handling by the embedee as a last resort).

That's the code that I thought should have been written when this bug first 
appeared.  As it stands, I don't have a clue as to which of the umpteen 
nsIDocShell/nsIWebNavigation/et al interfaces would have to be changed to get 
the "link-has-been-clicked-on" event from the bowels of nsIHTMLAnchorElement.cpp 
 to where they need to be.  I suspect we don't want to change those interfaces 
at this point in time.

We could modify the patch so that it simply ignores the link click if the 
inputEvent isMeta or isControl.  These situations could then be handled in 
navigator.xul/.js.

That would change the behavior of other "embedding" applications (e.g., viewer, 
mail, sidebar(?), etc.).  But so would the original patch (although the other 
embedders would get a new "feature" without any extra effort if we go that 
route).

So the choice is either of two one-off solutions:

1. One that works for Navigator but is perhaps slightly less broken.
2. One that works for viewer/mail/etc. but is perhaps broken worse, 
architecturally speaking (but for which the code is already written).

Updated

17 years ago
Blocks: 25530

Comment 49

17 years ago
I found this bug whilst thinking of the following feature, and I was wondering
if it is directly related to this one, if not a direct restatement of it. It
sounds to me like it is at least dependent upon it:

Find a page where you want to click multiple links from the same page (say the
download links at the Themes Park the day it opens). Ordinarily this won't work,
because clicking one link takes you to that link and you have to click Back to
revisit that page.

Answer: Shift+Click (or whatever key combo) would disable the standard behavior,
and open the window silently up in a new browser window that does NOT come to
the front, but stays obediently behind. In this fashion you could open 8 links
from the same page and not have to go to each page and click Back each time;
instead you could just open multiple pages and sort through them one at a time.

Comment 50

17 years ago
Kovu, that's what I suggested in my 2000-08-28 comment.

[Blake, given your 2000-09-11 comment, is this really waiting for review, or can 
that keyword be cleared?]
(Assignee)

Comment 51

17 years ago
yeah, removing `review' keyword.  This is my top priority for next milestone, 
though.
Keywords: review
Target Milestone: M19 → mozilla0.9
(Assignee)

Comment 52

17 years ago
I tried what Bill suggested about ignoring the left click if the modifiers were 
down and then handling the click in js, and it seemed to work very nicely.  If 
no one has any objections, I'm going to take that route.  I think the best way 
to go, since embedding should be the bare minimum (e.g. we shouldn't be adding 
anything that isn't necessary).  I'll attach a patch in a little while.
(Assignee)

Comment 53

17 years ago
Then again, if we go this route we're disallowing ctrl/meta modifiers for links 
in embedding...Thoughts, anyone?

(Matthew/Kovu, please file a separate bug for the window layering issue.  I'll 
have to talk to danm about how to do that)

Comment 54

17 years ago
Filed bug 56690 for opening links in a new non-frontmost window.
>Then again, if we go this route we're disallowing ctrl/meta modifiers for links 
>in embedding...Thoughts, anyone?

cc'ing valeski.  I repeat that nsGenericHTMLElement (or generic anything) is not
the place to wire up UI policy and high-level mechanism.  Embeddings that want
to elaborate and customize beyond left-click-without-modifier-keys should use
XUL, or should use the DOM as XUL does in a non-XUL, embedding specific way.

/be
(Assignee)

Comment 56

17 years ago
Brendan: OK, then.  Attaching a patch for alt+click (save file) and ctrl+click 
(open in new window) in XUL/JS, per Matthew's 3/27 comment...
(Assignee)

Comment 57

17 years ago
Created attachment 17159 [details] [diff] [review]
patch - js/xul implementation of alt+ and ctrl+ click
(Assignee)

Comment 58

17 years ago
Created attachment 17160 [details] [diff] [review]
the correct patch, sorry
(Assignee)

Comment 59

17 years ago
Created attachment 17162 [details] [diff] [review]
new patch - should ignore shift also, and return false; in linkClick()
(Assignee)

Comment 60

17 years ago
sorry about all the spam this has generated tonight.

jag says he'll review after we work out the issue of whether to use alt or 
shift for saving the link.  I'm starting a discussion in n.p.m.ui about this, 
please join me there.  Thanks!
+            if ( inputEvent->isControl || inputEvent->isMeta ||
inputEvent->isAlt )

Nit: pink didn't follow "When in Rome" and use the prevailing parens style, but
you should -- no need for gratuitous space after ( and before ).

+              break;  // let the click go through so we can handle it in JS/XUL
             else

Nit: else after break is a non-sequitur, it overindents the next statement:

               ret = TriggerLink(aPresContext, eLinkVerb_Replace,
                                       baseURL, href, target, PR_TRUE);

yet misleadingly suggests (by indentation) that control could reach the
successor statements to this TriggerLink, even from the break ("then" part),
which of course can't happen because the break makes control leave this case of
the switch.

Get rid of the "arbitrary else" here, and unindent the TriggerLink call.  Looks
good otherwise.  On to the JS:

+    var node = enclosingLink(event.originalTarget)
+    if (node)
+      var href = node.href;
+    if (href != "") {

If node converts to false (is null), href defaults to undefined, not "".  How
about:

+    var node = enclosingLink(event.originalTarget);
+    var href = node ? node.href : "";
+    if (href != "") {

(I added a semicolon at the end of the var node = ... too.)  But if you were to
test 'if (href) {' instead, you wouldn't need to worry.  "" and undefined both
convert to false.  I think ?: is better, but I wonder why you want to check for
the empty string here -- doesn't a link with an empty href load the directory of
the current document?  Indeed it does (Nav4.x says): "" is a URL relative to the
document base.

Maybe what you really mean here is:

+    var node = enclosingLink(event.originalTarget)
+    if (node) {
+      var href = node.href;
+      if (event.button == 1) {

In which case, why load href into a local var at all?

This linkClick function should have a return false at the bottom -- are you not
running with strict warnings enabled?  Oh, just mid-air collided with your fixed
update -- no worries, but don't let those warnings pile up.

I realize it was already there, but brace style is "when vandalizing Rome", not
"When in Rome" here (after the if (pref...)):

-  function browserHandleMiddleClick(event)
+  function browserHandleMiddleClick(event, linkURL)
   {
-    var target = event.originalTarget;
     if (pref.GetBoolPref("middlemouse.openNewWindow"))
     {

Should this function, too, return a value (true, presumably)?

-      onclick="if (event.button==2) browserHandleMiddleClick(event);"
+      onclick="linkClick(event);"

Should these return the value returned by the function called? Otherwise, what
good are the return statements in the navigator.js functions called from onclick
handler bodies?

/be
(Assignee)

Comment 62

17 years ago
Created attachment 17165 [details] [diff] [review]
patch with suggested cleanup

Comment 63

17 years ago
Don't use Alt+S, please, you'll (kind of) squash my other bug 55679 and 47708,
where I argue that Alt+S should send a message in Mail/News (as in Office and
ICQ), and that front-end buttons should have keyboard shortcuts in general. I
know that this is technically a browser bug right now, but Mail/News handles
HTML and links, too, so it would be a likely candidate in Mail/News, too, if I
didn't speak up and ask that it be reserved for Alt+Send. It's too sweet of a
shortcut to be for anything that you do as often as send e-mail messages. Finish
typing, a roll of the thumb and forefinger and bam, sent. No mouse, no obscure
menu shortcut Alt+F+D or whatever. Anyway, I just thought I'd snivel a bit
before you used up Alt+S.

Comment 64

17 years ago
@$%&, you said Alt+Click above, not Alt+S. Retracting above due to blissful
stupidity. I have no issues with +Clicks. <cowering>
(Assignee)

Comment 65

17 years ago
Created attachment 17185 [details] [diff] [review]
newest patch - linkClick needs to always return true; for non-modified clicks to work
(Assignee)

Comment 66

17 years ago
Created attachment 17186 [details] [diff] [review]
grr.  THIS is the real patch. don't hate me because i'm dumb.

Comment 67

17 years ago
Blake: generally I think that we prefer
if (cond) {
  doFn()
}
over
if (cond)
  doFn()

Brendan: am I correct? [this is not a block to an r=, nor is it an r=]
timeless: I don't know, the file is a mess, so I'll give you my style rules:

  if (foo)
    bar;

no braces if each part is one line.  If either part (if condition or then part)
is multiline, then brace:

  if (foo) {
    bar1;
    bar2;
  }

or

  if (very_long_foo1 &&
      foo2) {
    bar;
  }

Rationale: when reading code quickly to find a certain statement, it's easier to
avoid having to find "continuation" operators such as && above, and simply scan
for closing braces underhanging if, for, while, etc.

Blake: if you're gonna fix:


-   function isScrollbar(node)
-   {
+  function isScrollbar(node)
+  {
      while (node)
      {

why not rectify the while loop body's brace to be on the same line as the while?

Ok, enough style mongering (shaver is whacking this file hard, too).

-      if (url && url.length > 0)
-      {
+      if (url && url.length > 0) {

Why the > 0 length check here?  Don't we want to allow the relative URL ""?

Argh, else-after-return non-sequitur added back here:


+  function linkClick(event) 
+  {
+    var node = enclosingLink(event.originalTarget);
+    if (node) {
+      if (event.button == 1) {
+        if (event.metaKey || event.ctrlKey) {               // if meta+ or
ctrl+click
+          openNewWindowWith(node.href);                     // open link in new
window
+          event.preventBubble();
+          return true;
+        } else if (event.altKey)                              // if alt+click
+          return savePage(node.href);                       // save the link

The last should be

+          return true;
+        }
+        if (event.altKey)                              // if alt+click
+          return savePage(node.href);                  // save the link

(I made the // if alt+click line up better here, too.)


+            if (inputEvent->isControl || inputEvent->isMeta ||
inputEvent->isAlt || inputEvent->isShift)
+              break;  // let the click go through so we can handle it in JS/XUL
+            ret = TriggerLink(aPresContext, eLinkVerb_Replace,
                                       baseURL, href, target, PR_TRUE);

Looks good, but nit: how about putting as many actual args on the same line as
the TriggerLink(aPresContext, ..., and then underhanging the rest so they start
in the same column as the 'a' in aPresContext?

/be
I just wrote:

>Looks good, but nit: how about putting as many actual args on the same line as

but left out "as the sacred 80th column allows" after "as many actual args".

Also, in reply to timeless, my rationale seemingly made the case for his style
of always bracing then parts, even if single-line:

  if (foo) {
    bar;
  }

I don't mind this when reading, but find it tedious when coding, and of little
value when scanning because

  if (foo)
    bar;

is as easy (for me) to scan -- the single indented-to-next-level line of the
then part requires no further checking, and neither does the one-line if (foo)
part.  Any multilining in either part, however, begs for braces (the closing
one, really) to aid scanning.  My 2 cents.

/be

Comment 70

17 years ago
personally i don't mind [because we all like laziness]
if (foo)
  bar;

but as brendan said after thought
> Any multilining in either part, however, begs for braces (the closing
> one, really) to aid scanning.

I would never withhold an r= for that, but it was worth thinking about. Thank 
you brendan. blake: have fun, and thanks for working on mozilla.
(Assignee)

Comment 71

17 years ago
Created attachment 17256 [details] [diff] [review]
patch containing new file cleanup suggestions
r=brendan@mozilla.org, thanks.

/be
Looking at the diff for nsGenericHTMLElement.cpp, if "inputEvent->isControl ||
inputEvent->isMeta || inputEvent->isAlt ||inputEvent->isShift" is true, we'll
leak 'baseURL', no?

Making 'baseURL' a 'nsCOMPtr<nsIURI>' is probably the easiest way to fix that.
Other than that the changes to nsGenericHTMLElement.cpp look good to me (appart
from the missing whitespace before 'inputEvent->isShift' :-), if you wanna be
even more careful, have joki@netscape.com look at the diff.
Shoot me!

jst's right.  My NS_IF_RELEASE radar was malfunctioning (NS_RELEASE radar still
working).  Luckily, we haven't got r= from module owners or a= from reviewers
for the areas listed in http://mozilla.org/hacking/reviewers.html.  I'm going to
skulk away now for a while....

/be
(Assignee)

Comment 75

17 years ago
In case anyone's wondering what's up with this bug:

The patch for this needs to wait until the giant navigator.js smacking in bug 
55798 lands so we don't have merge hell.

Also, there are some build problems with changing baseURL to an nsCOMPtr that 
I'll need to work out with scc.
(Assignee)

Comment 76

17 years ago
Created attachment 18168 [details] [diff] [review]
new patch to use nsCOMPtr's - please review
The new patch looks good to me. r=jst
sr=brendan@mozilla.org.  I talked to blake about it on IRC.  He's tested the 
patch thoroughly.

I asked about a separate issue that depends on this fix, namely what should 
shift+left-click do (I'm used to save-to-file, which Mozilla has done forever, 
since Netscape 1.0 on Unix anyway).  He said he'd restart the thread about that 
on m.ui.

/be

Comment 79

17 years ago
ok, I think this should now go in, given brendan's intense scrutiny. We can
figure out the shift-click issue in another bug, after it's been decided on m.ui
a=alecf
(Assignee)

Comment 80

17 years ago
Sorry, had to wait until the tree cleared.

Fix checked in to trunk.

* Bug 58283 is the alt+click / shift+click issue.
* I'll file new bugs to make these modifiers work throughout the suite (e.g. in 
mailnews)

( http://bonsai.mozilla.org/cvsquery.cgi?who=%5BBb%5Dlake%5BRr%
5D&whotype=regexp&date=all )
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: [nsbeta2-][nsbeta3-] → trunk-only fix
vrfy fixed using 2000.11.1508-trunk bits on linux, mac and winnt.
Ctrl+click+link opens a new browser window [Command+click+link on mac] and
Shift+click+link saves the target link page.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.