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

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

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

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)

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.

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

Not required for beta 1.

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

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)

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.

Bug 17754 says this should work on form submissions too.

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

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.

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

Move to M16 for now ...

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

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

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

Move to M19 target milestone.

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

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

Nav triage team: [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. ;-)

Oh, ye gods ... 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?

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

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

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

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.

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.

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

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

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

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.

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

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

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.

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?

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

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.

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

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.

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

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

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.

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)

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

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

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

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.

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

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

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.

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

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.

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

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

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 )

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.