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

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[don]

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

Comment 1

[don]

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

Comment 2

[Bill Law]

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).

Comment 3

[cpratt]

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)

Comment 4

[Bill Law]

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.

Comment 5

[don]

Move this to M13 because it isn't really dogfood.

Comment 6

[don]

Not required for beta 1.

Comment 7

[Richard Zach]

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

Comment 8

[Jesse Ruderman]

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

[Eugene Savitsky]

I totally agree with [e-mail]. 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

[Jesse Ruderman]

Bug 17754 says this should work on form submissions too.

Comment 11

[Matthew T (active 1999-2002)]

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

Comment 12

[Matthew T (active 1999-2002)]

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.

Comment 13

[Sean Richardson]

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

Comment 14

[don]

Move to M16 for now ...

Comment 15

[Kathleen :Brade]

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

Comment 16

[Bill Law]

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

Comment 17

[leger]

Putting on [nsbeta2+][5/16] radar.  This is a feature MUST complete work by 
05/16 or we may pull this feature for PR2.

Comment 18

[don]

Move to M19 target milestone.

Comment 19

[leger]

Putting on [nsbeta2-] radar. Removing [5/16]. Missed the Netscape 6 feature 
train.  Please set to MFuture.

Comment 20

[beryan]

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

[verah (gone)]

Nav triage team: [nsbeta3-]

Comment 22

[sairuh (rarely reading bugmail)]

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. ;-)

Comment 23

[Matthew T (active 1999-2002)]

Oh, ye gods ... helpwanted.

Comment 24

[Henri Sivonen (:hsivonen)]

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

[Sean Richardson]

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.

Comment 26

[sairuh (rarely reading bugmail)]

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].

Comment 27

[Matthew T (active 1999-2002)]

> 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:-).

Comment 28

[Daniel Veditz [:dveditz]]

Adding nsbeta3 keyword to bugs which already have nsbeta3 status markings so 
the queries don't get all screwed up.

Comment 29

[Blake Ross]

seems easy enough. so what's the decision -- shift or ctrl or win32/linux ?

Comment 30

[timeless]

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.

Comment 31

[Blake Ross]

Attachment: patch to make links clicked with control modifier or middle mouse button open in new window

Comment 32

[Blake Ross]

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.

Comment 33

[Blake Ross]

Attachment: patch - just some visual touchups

Comment 34

[Blake Ross]

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

[Jesse Ruderman]

>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

[Matthew T (active 1999-2002)]

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

[timeless]

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.

Comment 38

[Blake Ross]

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

[Bill Law]

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.

Comment 40

[Brendan Eich [:brendan]]

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

[Akkana Peck]

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.

Comment 42

[Blake Ross]

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

[Akkana Peck]

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);

Comment 44

[Blake Ross]

Oh.  Nope, I'm on windows.  So I'll try those prefs.  Thanks.

Comment 45

[Brendan Eich [:brendan]]

Adding bryner for wheel-mouse expertise.

/be

Comment 46

[sairuh (rarely reading bugmail)]

cc'ing janc who tests mousewheel stuff.

Comment 47

[Brian Ryner (not reading)]

middle button may be getting intercepted by your mouse driver on windows to go
into scroll pan mode.

Comment 48

[Bill Law]

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).

Comment 49

[James "Kovu" Russell]

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

[Matthew T (active 1999-2002)]

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?]

Comment 51

[Blake Ross]

yeah, removing `review' keyword.  This is my top priority for next milestone, 
though.

Comment 52

[Blake Ross]

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.

Comment 53

[Blake Ross]

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

[Matthew T (active 1999-2002)]

Filed bug 56690 for opening links in a new non-frontmost window.

Comment 55

[Brendan Eich [:brendan]]

>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

Comment 56

[Blake Ross]

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...

Comment 57

[Blake Ross]

Attachment: patch - js/xul implementation of alt+ and ctrl+ click

Comment 58

[Blake Ross]

Attachment: the correct patch, sorry

Comment 59

[Blake Ross]

Attachment: new patch - should ignore shift also, and return false; in linkClick()

Comment 60

[Blake Ross]

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!

Comment 61

[Brendan Eich [:brendan]]

+            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

Comment 62

[Blake Ross]

Attachment: patch with suggested cleanup

Comment 63

[James "Kovu" Russell]

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

[James "Kovu" Russell]

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

Comment 65

[Blake Ross]

Attachment: newest patch - linkClick needs to always return true; for non-modified clicks to work

Comment 66

[Blake Ross]

Attachment: grr. THIS is the real patch. don't hate me because i'm dumb.

Comment 67

[timeless]

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=]

Comment 68

[Brendan Eich [:brendan]]

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

Comment 69

[Brendan Eich [:brendan]]

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

[timeless]

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.

Comment 71

[Blake Ross]

Attachment: patch containing new file cleanup suggestions

Comment 72

[Brendan Eich [:brendan]]

r=[e-mail], thanks.

/be

Comment 73

[Johnny Stenback (:jst)]

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 [e-mail] look at the diff.

Comment 74

[Brendan Eich [:brendan]]

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

Comment 75

[Blake Ross]

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.

Comment 76

[Blake Ross]

Attachment: new patch to use nsCOMPtr's - please review

Comment 77

[Johnny Stenback (:jst)]

The new patch looks good to me. r=jst

Comment 78

[Brendan Eich [:brendan]]

sr=[e-mail].  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

[Alec Flett]

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

Comment 80

[Blake Ross]

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 )

Comment 81

[sairuh (rarely reading bugmail)]

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.