Closed Bug 613710 Opened 14 years ago Closed 14 years ago

Cmd+. on OSX should cancel the dialog

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: Gavin, Assigned: jaas)

References

Details

(Keywords: regression, Whiteboard: [skip comments 1-15][softblocker])

Attachments

(2 files)

fix v1.0
(deleted), patch
smichaud
: review+
Details | Diff | Splinter Review
Alternative patch
(deleted), patch
jaas
: review-
Details | Diff | Splinter Review 
A couple of separate but related issues with the tab modal prompts from bug 59314:

- Enter should accept the prompt (bz says it didn't work for him)
- Enter should always trigger the default action, even if another button is focused (see bug 59314 comment 159)
- Cmd+. on Mac should Cancel the dialog. This works in 3.6, but seems to be broken even for normal modal prompts on trunk.

Maybe we should split some of these out...
Component: DOM → General
Product: Core → Toolkit
QA Contact: general → general
blocking2.0: --- → ?
Also, pressing Tab will continue tabbing around the focusable elements in the page behind the prompt.
I think Mardak fixed the first issue in bug 613767 / 613760. Might have been OS X only, WFM on Windows.
Not WFM on current OSX trunk
Seems like there is only one issue left? If not please file separate bugs for the others.
Summary: tab modal prompt keyboard shortcut issues → Cmd+. on OSX should cancel the dialog
No longer blocks: 614379
(In reply to comment #1)
> Also, pressing Tab will continue tabbing around the focusable elements in the
> page behind the prompt.

This is now bug 614379 (thanks), and may be fixed by the patch in bug 613763.

(In reply to comment #5)
> Not WFM on current OSX trunk

Boriss: check again? Should be fixed, let's look tomorrow if it's still broken for you.
So, 2 issues left from comment 0:

- Sort of some remaining default button / key action oddness (move to followup)
- Cmd-. on OS X

I think this bug (as titled) should just be WONTFIX... As far as I can tell Command-. does nothing special elsewhere in OS X (or Safari), and I can't find any online references that talk about it. Closest I found was Control-Alt-Command-. [and -,] to adjust screen contrast!

This seems to date back to the original xpfe/global/resources/content/mac/platformDialog.xml in CVS, which just appeared in the initial landing from hewitt in 2001.
(In reply to comment #8)
> - Cmd-. on OS X
> 
> I think this bug (as titled) should just be WONTFIX... As far as I can tell
> Command-. does nothing special elsewhere in OS X (or Safari), and I can't find
> any online references that talk about it. 

Cmd+. selects cancels prompt() and confirm() (but not alert(), which has no Cancel button) in Safari, and in all three modal dialogues in Firefox. AFAIR, esc or Cmd+. selecting any existing Cancel (or, lacking one, Stop or No) button has been a UI convention in every version of Mac OS, at least back to Macintosh OS 7.5. Cmd+. has been a ubiquitous and universal "cancel or stop foo" shortcut on Macs as long as -- well, come to think of it, I can't remember when it hasn't been so, but I'm not as old as the Macintosh brand, and don't remember any Mac OS version older than ... 6(?). Did you try only alert() in Safari, or, were trying on Safari for Windows?
(In reply to comment #9)

> Cmd+. selects cancels prompt() and confirm() (but not alert(), which has no
> Cancel button) in Safari

What version of OS X / Safari are you using? I can't get it to do anything for any of these prompts, tried both from a javascript: URL and as script in a test page.

> Cmd+. has been a ubiquitous and universal "cancel or stop foo" shortcut
> on Macs as long as -- well, come to think of it, I can't remember when it
> hasn't been so

Can you find any online reference for this? I couldn't!

> trying on Safari for Windows?

Goodness no. :-)
Ok, bizarre, it does nothing for me but works on Fryn's system. O_o

So, uhh, guess I'll make it work again in Minefield!
Mystery solved: my IRC client was using Command-. as a custom keyboard shortcut, and apparently that breaks the default Command-. stuff everwhere else!
(In reply to comment #8)
 
> - Sort of some remaining default button / key action oddness (move to followup)

Let's use bug 613760 for this.
Though the mystery was solved, per comment 12, I thought it might be rude not to answer anyway:

(In reply to comment #10)
> What version of OS X / Safari are you using?
Mac OS X v. 10.6 / Safari 5.0.3

> Can you find any online reference for this? I couldn't!
See, for examples:
  - http://docs.info.apple.com/article.html?path=Mac/10.5/en/cdb_dlgs.html
  - http://docs.info.apple.com/article.html?path=Mac/10.5/en/cdb_frzs.html

Both of those can also be found in Mac Help (i.e., through the Help menu on a Mac) by searching for "command period", "shortcuts for dialogs", or "shortcuts for freezes".

For other examples, look at Safari, try it in Terminal (where it is equivalent to ^C), or try: http://www.google.ca/search?q="command+period"+site:apple.com

Hope that clears some fog. Thanks for keeping things consistent.
Hmm. That link to the Google search is broken. Should I file a Bugzilla bug?
blocking2.0: ? → final+
Keywords: regression
Hmm. I think this might be a widget level bug, as I can't get Command-. to work with window-modal dialogs on 4.0, but I can get them to work with FF 3.6. Command-. for window-modal prompts is still handled by dialog.xml, and shouldn't have been affected by the tab-modal changes. You can reenable the window-modal prompts by flipping prompts.tab_modal.enabled in about:config.

I did some debugging with breaking in nsViewManager::HandleEvent() [which seems to be about the earliest I can break after the ObjC bits, which I don't know how to deal with in gdb]. I don't seem to get keypress events for Command-. at all. (Hmm, I should probably spin a 3.6 build to confirm I can catch them in gdb there?)

[gdb note to self: "break nsViewManager::HandleEvent", then "cond 1 (aEvent->eventStructType ==9 && aEvent->message == 131)]

A regression window might help narrow down exactly when this stopped working with window-modal prompts on trunk.

[The patch to add Cmd-. support to tabmodal prompts should be a trivial 2-liner, but I can't test it until the events are working for window-modal prompts.]
Component: General → Widget: Cocoa
Product: Toolkit → Core
QA Contact: general → cocoa
Whiteboard: [skip comments 1-15]
Version: unspecified → Trunk
Whiteboard: [skip comments 1-15] → [skip comments 1-15][hardblocker]
I doubt this is a widget bug but I haven't confirmed. There is nothing special about "Cmd+." on Mac OS X and if that was broken at the widget level we'd probably have a lot of other problems.

Firefox does put "Cmd+." in the native menu bar as the keyboard command for "Stop" (under the View menu). I haven't looked at the native menu bar code in a while but I think we give native menu items priority by design for various reasons. We might be cutting off the event handling after it is rejected by a disabled "Stop" menu item with the key combo you're looking for.
The performKeyEquivalent part of receiving the Cmd+. keypress happens (the code in nsMenuBarX.mm runs), but never makes it to the keyDown, despite returning NO.

Cmd+. is the only combination that seems to have this issue.

For tab-modal dialogs, a second call to performKeyEquivalent is made with a 0 keyCode. This doesn't happen for the sheet-type alerts.
Testing shows this to be regression from bug 584965.
Blocks: 584965
I added an nsChildView::performKeyEquivalent implementation and it was called. The calls are as follows:

nsCocoaWindow::performKeyEquivalent with keycode 47
nsMenuBarX::performKeyEquivalent with keycode 47
The nsChildView::performKeyEquivalent I added with keycode 0 and charcode 27 (escape key)

It seems that Cmd+. is translated into an Escape key at some point. Testing shows that Cmd+. works wherever and only where Escape cancels a dialog.

Josh, would a handler for this in nsChildView::performKeyEquivalent be suitable?
(In reply to comment #18)
> Cmd+. is the only combination that seems to have this issue.
> 
> For tab-modal dialogs, a second call to performKeyEquivalent is made with a 0
> keyCode. This doesn't happen for the sheet-type alerts.
(In reply to comment #20)
> It seems that Cmd+. is translated into an Escape key at some point. Testing
> shows that Cmd+. works wherever and only where Escape cancels a dialog.

I'm not sure if this is relevant for non-Cocoa/Obj-C applications, but someone with a Mac available, try this:
1. In Xcode, make a new application of any kind which has a GUI (such as a Cocoa Application).
2. In Interface Builder, add any kind of button onto which you can place a text label (such as a NSButton).
3. Label that button "Cancel".
4. Build and launch your new application (without adding a line of code).
5. Type esc and Cmd+., and watch the Cancel button be clicked for you.

The esc/Cmd+. -> Cancel convention has been "standard" behaviour in the Mac OSs for so long that it seems Apple has built the behaviour into the
(In reply to comment #18)
> Cmd+. is the only combination that seems to have this issue.
> 
> For tab-modal dialogs, a second call to performKeyEquivalent is made with a 0
> keyCode. This doesn't happen for the sheet-type alerts.
(In reply to comment #20)
> It seems that Cmd+. is translated into an Escape key at some point. Testing
> shows that Cmd+. works wherever and only where Escape cancels a dialog.

I'm not sure if this is relevant for non-Cocoa/Obj-C applications, but someone with a Mac available, try this:
1. In Xcode, make a new application of any kind which has a GUI (such as a Cocoa Application).
2. In Interface Builder, add any kind of button onto which you can place a text label (such as a NSButton).
3. Label that button "Cancel".
4. Build and launch your new application (without adding a line of code).
5. Type esc and Cmd+., and watch the Cancel button be clicked for you.

The esc/Cmd+. -> Cancel convention has been "standard" behaviour in the Mac OSs for so long that it seems Apple has built in at some point. It's been a (In reply to comment #18)
> Cmd+. is the only combination that seems to have this issue.
> 
> For tab-modal dialogs, a second call to performKeyEquivalent is made with a 0
> keyCode. This doesn't happen for the sheet-type alerts.
(In reply to comment #20)
> It seems that Cmd+. is translated into an Escape key at some point. Testing
> shows that Cmd+. works wherever and only where Escape cancels a dialog.

I'm not sure if this is relevant for non-Cocoa/Obj-C applications, but someone with a Mac available, try this:
1. In Xcode, make a new application of any kind which has a GUI (such as a Cocoa Application).
2. In Interface Builder, add any kind of button onto which you can place a text label (such as a NSButton).
3. Label that button "Cancel".
4. Build and run your new application (without adding a line of code).
5. Type esc and Cmd+., and watch the Cancel button be clicked for you. Quit.
6. Add a menu item (with any name) to any menu. Make it's key equivalent Cmd+. (Don't connect it to any code methods yet.)
7. Build and run again. The new menu item will be disabled, as it lacks a receiver for any actions. Hence, Cmd+. will still click Cancel. Quit.
8. Connect the new menu item to some method; a nop will work.
9. Build and run one last time. The new menu item should be enabled, and so Cmd+. will activate it instead of the Cancel button, but esc will still click the Cancel button.

The esc/Cmd+. -> Cancel convention has been "standard" behaviour in the Mac OSs for so long (circa System 5 [1987] or earlier?) that it seems Apple has "built it in" at some point. To my currently limited understanding, it seems that, by default, any Cmd+. which is not already captured by an enabled menu item will be directed at any enabled Cancel button present. It's been a long time since I've built a an application specifically for a Mac, but perhaps someone with more (and more recent) experience can provide some indication of whether I'm making any sense here or completely off the wall.
Sorry for the spam, seems I mis-clicked earlier. I thought I'd stopped it on time. So there's no way to revise or discard one's comment, even if there are no comments following it?
I'm not sure how my (completed) comment got preceded with a partial earlier edition of itself. I'd edit that out too if I could. Sorry again for the spam. I think I'll retreat into a dark corner and hide myself in shame now....
Looks like this is waiting on feedback from Josh.

Is this really a hardblocker?
Assignee: nobody → joshmoz
I think this is indeed a softblocker. It's annoying to someone's muscle-memory, but the dialogs are fully functional, and can be dismissed by clicking a button (or hitting ESC, if keyboard access is a must).
Whiteboard: [skip comments 1-15][hardblocker] → [skip comments 1-15][softblocker]
The problem is this method in NSResponder, which is NSWindow's superclass:

- (void)cancelOperation:(id)sender

I confirmed in a debugger that this is getting called. From the docs:

This method is bound to the Escape and Command-. (period) keys. The key window first searches the view hierarchy for a view whose key equivalent is Escape or Command-., whichever was entered. If none of these views handles the key equivalent, the window sends a default action message of cancelOperation: to the first responder and from there the message travels up the responder chain.

If no responder in the responder chain implements cancelOperation:, the key window searches the view hierarchy for a view whose key equivalent is Escape (note that this may be redundant if the original key equivalent was Escape). If no such responder is found, then a cancel: action message is sent to the first responder in the responder chain that implements it.
Attached patch fix v1.0 (deleted) — — Splinter Review 
Looks like the default behavior for NSWindow is to special-case "Cmd+.". It is trapped and converted into a "cancelOperation:" command. The stack for that looks like this:

-[BaseWindow cancelOperation:]
-[NSWindow _processKeyboardUIKey:] ()
-[NSWindow sendEvent:] ()
-[ToolbarWindow sendEvent:]
-[NSApplication sendEvent:]
-[NSApplication run]

This prevents it from propagating normally to keyDown: and as a result we don't send events to Gecko for it. If we claim that our native window (NSWindow) doesn't respond to "cancelOperation:" we can get the event to propagate in the expected way.

This makes window-modal dialogs close with "Cmd+." but the tab-modal dialogs don't close, I think because they don't have a "Cmd+." handler yet.
Attachment #505974 - Flags: review?(smichaud)
Blocks: 627896
Attached patch Alternative patch (deleted) — — Splinter Review 
Josh, you patch is good as far as it goes.

But here's an alternative patch, and I think a better one.  It makes
Cmd+. work in both window-modal dialogs (sheets) and tab-modal ones.
(Cmd+. already works fine in app-modal dialogs, like the file picker.)

I've tested this patch on OS X 10.5.8 and 10.6.6 (the latter in both
32-bit and 64-bit modes).  I didn't see any problems.

I believe we should eventually be sending events directly to Gecko
from [ChildView cancel:] and similar message handlers.  This is how
WebKit apps behave, and is in general more "standard" (from Apple's
point of view).  (There are many similar NSResponder message handlers,
including ones like selectAll:, selectLine:, selectWord: and
selectParagraph:.  I suspect using these will make it easier to
support global keyboard-shortcut customizations.)

But for now I've made [ChildView cancel:] synthesize Escape keyboard
events and send them to the relevant ChildView object (using keyDown:
and keyUp:).
Comment on attachment 507285 [details] [diff] [review]
Alternative patch

Why aren't you implementing this on BaseWindow? That seems like a more appropriate place than in ChildView - no point in letting the action propagate to a view when you're going to look up the first responder anyway.

I think there are strong arguments for both patches. My patch gives the exact events, empowering XUL developers to choose what they think is best. Your patch theoretically allows for customizing keyboard commands for this action.

I don't think the keyboard commands for the cancel: action are commonly changed, and when they are changed I'm not sure that synthesizing an escape key event is always the appropriate response. Furthermore, XUL developer will only be able to figure this out by trial-and-error - it's not expected that this event transformation will happen. Thanks for the alternative, but for these reasons I'd like to go with my patch.
Attachment #507285 - Flags: review-
(In reply to comment #29)

> But for now I've made [ChildView cancel:] synthesize Escape keyboard
> events and send them to the relevant ChildView object (using keyDown:
> and keyUp:).

Wouldn't this cause Cmd-. to be equivalent to pressing Escape all of the time (instead of just being a special "cancel the dialog" shortcut)? I'd be a bit wary of that, unless that expected as a platform convention.
> Why aren't you implementing this on BaseWindow? That seems like a
> more appropriate place than in ChildView - no point in letting the
> action propagate to a view when you're going to look up the first
> responder anyway.

I implemented cancel: in ChildView because the OS (NSWindow's
cancelOperation: handler) propagates it to the correct destination --
the key view, which is also the correct (and actual) destination for
keyboard events.

I still think mine is the better patch.

Insisting that all keyboard events should be sent to Gecko exactly as
they were entered at the keyboard is too narrow an approach to the
problem of how to convey the user's intentions to the browser.  The
whole idea of keyboard shortcuts is that a given key combination can
be made to stand for something else.  It doesn't make sense that the
only place these can be modified is in the browser itself --
especially if the OS has comprehensive support for making these
customizations outside the browser (in individual apps or in all apps
together).

But, even though I think my general argument (stated in the previous
paragraph) is valid, we'd need a lot more keyboard-shortcut-message
handlers to really put it into practice.

There's another, more practical and more compelling argument for my
patch:  It works for both window-modal and tab-modal dialogs, while
yours only works for window-modal dialogs.
(In reply to comment #32)

> There's another, more practical and more compelling argument for my
> patch:  It works for both window-modal and tab-modal dialogs, while
> yours only works for window-modal dialogs.

That's because there's already a <handler> for Cmd-. in dialog.xml, but when I wrote tabprompts.xml I didn't include it because I thought it didn't work anyway (see my confusion earlier in this bug ;). Bug 627896 will add that code. So, I'm not concerned about this difference, unless your point was that Cmd-. should work anywhere Escape does something. [For example, Escape exits Panorama. Should Cmd-. also exit Panorama?]
>> But for now I've made [ChildView cancel:] synthesize Escape
>> keyboard events and send them to the relevant ChildView object
>> (using keyDown: and keyUp:).
>
> Wouldn't this cause Cmd-. to be equivalent to pressing Escape all of
> the time (instead of just being a special "cancel the dialog"
> shortcut)? I'd be a bit wary of that, unless that expected as a
> platform convention.

The cancel: message is (presumably) sent whenever the OS receives a
key combination that it interprets as meaning "cancel".  So what
triggers this message might be Cmd+., or it might be some other key
combination.

Yes, my current cancel: handler always synthesizes pressing the Escape
key.  But (like I said) in the future it should probably send some
kind of general "cancel" command to Gecko -- which Gecko might
interpret in different ways, depending on the context.

And it's not as if your approach has any greater flexibility.
Josh, you haven't convinced me your approach is better.  But the kind
of changes I envision (using a lot more key-combination-handler
messages, and having them send higher level events to Gecko) isn't
something that can be done overnight.

I'd be happy to r+ any patch of yours that works for both window-modal
dialogs (sheets) and tab-modal dialogs.
Tomorrow I'll test your original v1.0 patch together with whatever patch you currently have for bug 627896.
Steven - I see your point but I think we need to report the actual key combination in this case for the reasons I listed before. Your patch makes it impossible for XUL authors to detect cmd-. if they want to and it might not generate an appropriate response for all "cancel:" actions (escape is probably reasonable in most cases, but it might not be). I don't think that's an acceptable cost for what we gain from your patch. It's also not what we did before and I don't want to change things like that at this point in the release cycle.
We have philosophical differences on this issue that we're not going to be able to resolve now.  I think my position is most clearly stated in the paragraph in comment #32 starting "Insisting that all keyboard events ...".

I do concede, though, that now's not the time to start using key-combination-handler messages in a major way.  Not just before a major release :-)

I'll review a combination of your v1.0 patch and whatever you have at bug 627896.
Comment on attachment 505974 [details] [diff] [review]
fix v1.0

I've just tested Josh's v1.0 patch together with the v.1 patch from
bug 627896, and found that it doesn't really make Cmd+. work in
tab-modal dialogs:  It works *exactly once*, and then never again
during the current app session.

This is true both on OS X 10.5.8 and 10.6.6, and (on 10.6.6) in both
32-bit mode and 64-bit mode.

I'll try to come up with a patch that fixes the problem without using
a cancel: message handler.

This *isn't* a problem with the code that implements tab-modal
dialogs.  If you add an NSLog() statement to [ChildView keyDown:], you
find that Cmd+. only causes keyDown: to be called the first time you
use it in an app session -- whether or not you're in a tab-modal
dialog.
Attachment #505974 - Flags: review?(smichaud) → review-
(In reply to comment #39)

> I've just tested Josh's v1.0 patch together with the v.1 patch from
> bug 627896, and found that it doesn't really make Cmd+. work in
> tab-modal dialogs:  It works *exactly once*, and then never again
> during the current app session.

I don't see this. I have both patches applied and everything works fine, repeatedly, with tab and window modal dialogs. I'm using very recent (an hour old) mozilla-central source on Mac OS X 10.6.6.

I'm reproducing like this:

1. Start Minefield to Minefield Start Page
2. Type "javascript: alert(1)" into the URL bar, hit enter
3. Hit cmd-. to dismiss tab or window modal dialog.
I built from source that's a couple of days old.

I'll try again with current trunk code.
> I'll try again with current trunk code.

I have the same problem with current code, using a 32-bit build made
on OS X 10.5.8.  I'll try a 64-bit build made on OS X 10.6.6, and a
full-scale universal build (also made on 10.6.6).

Have you tested your build on OS X 10.5.8?
> I have the same problem with current code, using a 32-bit build made
> on OS X 10.5.8.

Oops, I made a stupid mistake in the debugging code I added to your
patch.  After fixing the problem the 32-bit build works fine on OS X
10.5.8.  I'll also try it on OS X 10.6.6, and do another 64-bit build
and try that.
I just tested with both patches on Mac OS X 10.5.8 and didn't see any problems.
Comment on attachment 505974 [details] [diff] [review]
fix v1.0

I just tested on OS X 10.6.6 (32-bit and 64-bit) and had no problems.

So your patch is fine, after all (in combination with the current
patch for bug 627896).

Sorry for the confusion.
Attachment #505974 - Flags: review- → review+
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/915e30959444

I should be able to write a test for this.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: