Closed
Bug 12056
Opened 25 years ago
Closed 24 years ago
[FEATURE] Click link with key modifier and open URL in new chromed window
Categories
(Core :: DOM: UI Events & Focus Handling, defect, P1)
Core
DOM: UI Events & Focus Handling
Tracking
()
VERIFIED
FIXED
mozilla0.9
People
(Reporter: don, Assigned: bugzilla)
References
Details
(Keywords: helpwanted, Whiteboard: trunk-only fix)
Attachments
(10 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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
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?
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
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•25 years ago
|
QA Contact: beppe → 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)
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 8•25 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•25 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•25 years ago
|
||
Bug 17754 says this should work on form submissions too.
Comment 11•25 years ago
|
||
*** Bug 22761 has been marked as a duplicate of this bug. ***
Comment 12•25 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•25 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
Comment 15•25 years ago
|
||
*** Bug 36199 has been marked as a duplicate of this bug. ***
Comment 16•25 years ago
|
||
*** Bug 37807 has been marked as a duplicate of this bug. ***
Comment 17•25 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]
Comment 19•25 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•24 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 22•24 years ago
|
||
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
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•24 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.
Comment 26•24 years ago
|
||
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•24 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:-).
Comment 28•24 years ago
|
||
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•24 years ago
|
||
seems easy enough. so what's the decision -- shift or ctrl or win32/linux ?
Assignee: law → BlakeR1234
Status: ASSIGNED → NEW
Comment 30•24 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•24 years ago
|
||
Assignee | ||
Comment 32•24 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•24 years ago
|
||
Assignee | ||
Comment 34•24 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•24 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•24 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•24 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•24 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•24 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.
Comment 40•24 years ago
|
||
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•24 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•24 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•24 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•24 years ago
|
||
Oh. Nope, I'm on windows. So I'll try those prefs. Thanks.
Comment 45•24 years ago
|
||
Adding bryner for wheel-mouse expertise.
/be
Comment 46•24 years ago
|
||
cc'ing janc who tests mousewheel stuff.
Comment 47•24 years ago
|
||
middle button may be getting intercepted by your mouse driver on windows to go
into scroll pan mode.
Comment 48•24 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).
Comment 49•24 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•24 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•24 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•24 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•24 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•24 years ago
|
||
Filed bug 56690 for opening links in a new non-frontmost window.
Comment 55•24 years ago
|
||
>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•24 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•24 years ago
|
||
Assignee | ||
Comment 58•24 years ago
|
||
Assignee | ||
Comment 59•24 years ago
|
||
Assignee | ||
Comment 60•24 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!
Comment 61•24 years ago
|
||
+ 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•24 years ago
|
||
Comment 63•24 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•24 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•24 years ago
|
||
Assignee | ||
Comment 66•24 years ago
|
||
Comment 67•24 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=]
Comment 68•24 years ago
|
||
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•24 years ago
|
||
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•24 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•24 years ago
|
||
Comment 72•24 years ago
|
||
r=brendan@mozilla.org, thanks.
/be
Comment 73•24 years ago
|
||
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.
Comment 74•24 years ago
|
||
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•24 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•24 years ago
|
||
Comment 77•24 years ago
|
||
The new patch looks good to me. r=jst
Comment 78•24 years ago
|
||
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•24 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•24 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
Closed: 24 years ago
Resolution: --- → FIXED
Updated•24 years ago
|
Whiteboard: [nsbeta2-][nsbeta3-] → trunk-only fix
Comment 81•24 years ago
|
||
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
Updated•6 years ago
|
Component: Keyboard: Navigation → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•