User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0 Build Identifier: latest nightly 11-Feb-2005 OS X allows you to remap keys via a file called DefaultKeyBinding.dict. For instance, one can change Home from the Apple-default meaning of "move to beginning of buffer" to "move to beginning of line". My remap works in most Apple apps, but not in Firefox or Moz (or in T-bird, per bug 242474) Reproducible: Always Steps to Reproduce: 1. Working from your home directory, create the file /Library/KeyBindings/DefaultKeyBinding.dict and enter this as the contents (I'll also upload this as an attachment): { "\UF729" = "moveToBeginningOfLine:"; /* Home */ "\UF72B" = "moveToEndOfLine:"; /* End */ "$\UF729" = "moveToBeginningOfLineAndModifySelection:"; /* Shift-Home */ "$\UF72B" = "moveToEndOfLineAndModifySelection:"; /* Shift-End */ "^\UF729" = "moveToBeginningOfDocument:"; /* Ctrl-Home */ "^\UF72B" = "moveToEndOfDocument:"; /* Ctrl-End */ "^$\UF729" = "moveToBeginningOfDocumentAndModifySelection:"; /* Ctrl-Shift-Home */ "^$\UF72B" = "moveToEndOfDocumentAndModifySelection:"; /* Ctrl-Shift-End */ "^\UF702" = "moveWordBackward:"; /* Control-Left arrow */ "^\UF703" = "moveWordForward:"; /* Control-Right arrow */ "^$\UF702" = "moveWordBackwardAndModifySelection:"; /* Ctrl-Shift-Left arrow */ "^$\UF703" = "moveWordForwardAndModifySelection:"; /* Ctrl-Shift-Right arrow */ } 2. Log out 3. Log in; your keys are now remapped. 4. To unmap, remove the file and log out/in again. Actual Results: Mail.App, TextEdit and Safari respect my new key bindings (e.g. Home moves to beginning of line, Ctrl+right arrow moves one word right, etc.). But Firefox and Mozilla ignore these keystrokes. The cursor doesn't react at all in the address bar, the search bar, or single-line input boxes, and in this multiline input box, home/end use the Apple-default beginning/end of buffer. Expected Results: FF/Moz should respect my KeyBindings. This has also been reported (by someone else) as a bug in Thunderbird. https://bugzilla.mozilla.org/show_bug.cgi?id=242474

Mac widget issue. Bug 257405 fixed a similar problem for GTK2 and added a generic nsINativeKeyBindings.h interface that widget impls that need this can implement to override what keys do by default.

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

AFAIK no non-Cocoa app respects DefaultKeyBinding.dict.

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

Kicking this to Widget:Cocoa, since the work needed to fix it would be trunk-only (and thus Widget:Cocoa) at this point. Note that many bindings that map to the standard Cocoa/emacs bindings do work, because they're currently hard-coded in platformHTMLbindings.xml

Would this also make up-arrow work in XUL textfields, or is that a separate bug? (up-arrow should go to the beginning of the line, if you're currently at the end.)

I'm sure this is stating the obvious, but the user-specific keybindings located in ~/Library/KeyBindings/DefaultKeyBinding.dict will override system-specified bindings located in /System/​Library/​Frameworks/​AppKit.framework/​Resources/​StandardKeyBinding.dict And just because it isn't a Cocoa app doesn't mean it can't read from those files, just that it doesn't have an API to make it easier.

Not sure if I should write this here or file a new bug for this but so far I've noticed the following with the trunk in Mac OS X Leopard. I can't use Ctrl+f/b/n/p/d/a/e/o/k/y/ and Command+Delete in text input area of the web page. Alt+Delete is working. In Firefox's text input are (Location bar, search box), I can use Ctrl+f/b/d/a/e/k/ but can't yank with Ctrl+y. Since there's no lines in these text area so I don't know about Ctrl+n/p though I think you should be able to move through the autocompletion list of Location bar with Ctrl+p/n like Safari does. In the Library, when I try to move through lines in "Description", Ctrl+p seems to work but Ctrl+n moves the focus to the "Name".

(In reply to comment #7) > Would this also make up-arrow work in XUL textfields, or is that a separate > bug? (up-arrow should go to the beginning of the line, if you're currently at > the end.) In case anyone's curious, comment 7 is bug 356883 (which is the fault of toolkit's autocomplete, not the lack of a nsINativeKeyBindings implementation).

Has there been any momentum on this? If not, I am at the point where I am willing to dive in and resolve this as this is my number one obstruction to using Firefox as my primary browser. I am not sure how Firefox is interacting with OS X's APIs, but the Emacs-style keybindings as well as the custom keybindings defined in DefaultKeyBinding.dict should work in any truly native Cocoa text field.

David, no one is working on this bug. If you want to, feel free to! Note that our text fields are not native Cocoa text fields, which is why bugs like this can happen. ;)

This patch handles the bindings that have a direct translation to commands that Gecko editors already support. Most users of the key binding system (such as the reporter) appear to map only various movement keys, most of which are covered. Along with the straight-forward single key to single command mappings, multi-key and multi-command bindings are also supported. Assuming this goes will, I plan to file further bugs to address the more complex bindings which will likely require deeper integration with the editor. This is my first patch for Gecko, and also my first attempt at Cocoa / Objective-C, so there may be various obvious things I have missed. Please let me know! :)

Comment on attachment 732667 [details] [diff] [review] Patch v1 This looks very promising, but I'm not really the best one to review it. I do have one question: Will this patch allow changes made in the System : Keyboard : Keyboard shortcuts panel to have effect in Firefox? (I assume the answer is yes, but I just want to make sure. People have been complaining for years about that not working.)

(In reply to Steven Michaud from comment #17) > Comment on attachment 732667 [details] [diff] [review] > Patch v1 > > This looks very promising, but I'm not really the best one to review it. Thanks for the redirect, I wasn't sure who was best. > I do have one question: Will this patch allow changes made in the System : > Keyboard : Keyboard shortcuts panel to have effect in Firefox? I gave this a shot, but it actually does not appear to work. I am guessing this method of setting shortcuts fakes menu item invocation instead of mapping key events, so the current patch won't handle that.

Hmm, although I've not tested the patch actually, I worry about that it might break IME behavior because it calls [mResponder interpretKeyEvents]. The method call causes Cocoa may do something internally. For example, it might kill scroll with spacebar because it may be eaten by IME. Did you test such cases? I've planned a way of fixing this bug. My idea is, we should read the command list dictionary directly. Can you do that (I'm not sure if it's possible)? I think that it's the best approach if it's possible because it never delegates handling the key event to Cocoa. And also, I think that you should remove following bindings: http://mxr.mozilla.org/mozilla-central/source/content/xbl/builtin/mac/platformHTMLBindings.xml#34 http://mxr.mozilla.org/mozilla-central/source/content/xbl/builtin/mac/platformHTMLBindings.xml#82 http://mxr.mozilla.org/mozilla-central/source/content/xbl/builtin/mac/platformHTMLBindings.xml#153 (or all of them in the file??) And it *might* be great if we do record the command to nsKeyEvent before dispatching keydown/keypress event. I'm not sure this makes TextInputHandler.mm simpler though. For example, we already handles some command in TextInputHandler.mm. So, I worry about the double handling with your patch.

There's one thing I really like about jryans patch, which I think it would be a shame to lose: It catches NSResponder methods that are bound to keys (and key combinations), like cancelOperation:. Doing that would be the best way to fix this bug, if we can manage it without breaking IME (and other things). The same strategy can eventually be used to allow Firefox to obey changes made in the System : Keyboard : Keyboard Shortcuts panel. I'm pretty sure other Apple apps (like Safari) use this strategy, and do so without breaking IME. If we can find out how Safari (or Webkit browsers in general) do this, we can do it ourselves. I currently don't have time to spend on this bug, and won't for at least another month. So, jyans, are you willing to try to figure out how this works in Safari, or in Webkit apps? If Webkit supports this, you can look for clues in Webkit source code. It's also possible to get the information by reverse engineering, using things like class-dump and "nm -pam" and interpose libraries. I use interpose libraries a lot, and have posted examples in some bugs -- for example bug 738335.

This includes some additional mappings for paragraph commands (uses line movements for now, like GTK2 does) and left/right movements. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #19) > Hmm, although I've not tested the patch actually, I worry about that it > might break IME behavior because it calls [mResponder interpretKeyEvents]. > The method call causes Cocoa may do something internally. For example, it > might kill scroll with spacebar because it may be eaten by IME. Did you test > such cases? I understand your concern, as it is a bit of a black box. I do not regularly use an IME myself, but I performed a few tests on OS X 10.8.3 with a US keyboard and the Japanese IME in Hiragana mode: * If you are not in some kind of editable field, pressing spacebar flows through to the page and does not go to the IME (same as without patch) * If you are in an editable field, you can use the IME as you would normally, like using spacebar to choose the word meaning (same as without patch) * If you define a key binding that conflicts with an IME shortcut (such as ^K for "Transliterate to Katakana"), then: * When are not composing with the IME, the user's defined key binding is executed * When you are composing, the IME shortcut is triggered In summary, editing with an IME continues to work as expected. I am happy to perform more tests if you have some to suggest! > I've planned a way of fixing this bug. My idea is, we should read the > command list dictionary directly. Can you do that (I'm not sure if it's > possible)? I think that it's the best approach if it's possible because it > never delegates handling the key event to Cocoa. I believe we could, but if we aren't seeing conflicts from Cocoa, is it really valuable to reimplement the same logic Cocoa seems to handle for us? I am not yet convinced that we need to take this approach. Hopefully I can convince you too! :) > And also, I think that you should remove following bindings: > http://mxr.mozilla.org/mozilla-central/source/content/xbl/builtin/mac/ > platformHTMLBindings.xml#34 > http://mxr.mozilla.org/mozilla-central/source/content/xbl/builtin/mac/ > platformHTMLBindings.xml#82 > http://mxr.mozilla.org/mozilla-central/source/content/xbl/builtin/mac/ > platformHTMLBindings.xml#153 > > (or all of them in the file??) Good catch! In this version of the patch, I removed many of the XUL bindings which aren't needed anymore. In a few cases, the XUL binding was slightly different than the OS X meaning, but I removed them anyway, since it brings us closer to OS defaults. I was not sure how to test the bindings in the "browser" block, so I left those alone for now. What's an example of those bindings being used? > And it *might* be great if we do record the command to nsKeyEvent before > dispatching keydown/keypress event. I'm not sure this makes > TextInputHandler.mm simpler though. For example, we already handles some > command in TextInputHandler.mm. So, I worry about the double handling with > your patch. I don't think I understand... Add an extra field to nsKeyEvent to store the command? I couldn't find anywhere in TextInputHandler.mm where these sorts of the commands are already handled.

With some help from the #developers channel, I've been able to test the "browser" bindings, and those are now gone as well.

(In reply to Steven Michaud from comment #20) > I currently don't have time to spend on this bug, and won't for at least > another month. So, jyans, are you willing to try to figure out how this > works in Safari, or in Webkit apps? If Webkit supports this, you can look > for clues in Webkit source code. Yep, I am happy to dig into this issue, but it feels like a separate bug to me, most likely bug 429824 (so many duplicates!), so I'd propose tackling this over there.

Comment on attachment 733403 [details] [diff] [review] Patch v3 I tested the patch too. I guess that non-focused responder doesn't send key events to IME, perhaps. Basically, I agree with your approach for now. >diff --git a/widget/cocoa/nsNativeKeyBindings.h b/widget/cocoa/nsNativeKeyBindings.h Now, we don't use ns prefix for new class. So, please rename this file CocoaNativeKeyBindings.h >new file mode 100644 >--- /dev/null >+++ b/widget/cocoa/nsNativeKeyBindings.h >+#define NS_NATIVEKEYBINDINGS_CID \ >+{0x8477f934, 0xfebf, 0x4c79, {0xb7, 0xfe, 0xbb, 0x7f, 0x9e, 0xbb, 0x9b, 0x4f}} nit: #define NS_NATIVEKEYBINDINGS_CID \ {0x8477f934, 0xfebf, 0x4c79, {0xb7, 0xfe, 0xbb, 0x7f, 0x9e, 0xbb, 0x9b, 0x4f}} this style is typically used. >+class nsNativeKeyBindings MOZ_FINAL : public nsINativeKeyBindings Please rename this mozilla::widget::NativeKeyBindings >+@interface NativeKeyBindingsResponder : NSResponder >+{ >+ @private Don't need the indentation. >+ nsTArray<nsCString> mCommands; >+} >+ >+- (void)startRecording; >+ >+- (void)doCommandBySelector:(SEL)aSelector; >+ >+- (void)insertText:(id)aString; >+ >+- (const nsTArray<nsCString>&)recordedCommands; >+ >+@end // NativeKeyBindingsResponder I think that this may be useful in TextInputHanandler.mm in the future. So, I think that it's better to move this to nsCocoaUtils.mm or TextInputHandler.mm. Then, only a static method should be exported, e.g., void GetCommandSelectorsFor(NSEvent* aEvent, nsTArray<nsCString>& aSelectors) which returns raw selectors. And convert the raw selectors to Gecko commands in NativeKeyBindings. >diff --git a/widget/cocoa/nsNativeKeyBindings.mm b/widget/cocoa/nsNativeKeyBindings.mm Please rename this NativeKeyBindings.mm >new file mode 100644 >--- /dev/null >+++ b/widget/cocoa/nsNativeKeyBindings.mm >+#ifdef MOZ_LOGGING >+#define FORCE_PR_LOG >+#endif Why do you think that we need to get the log on release builds? >+nsNativeKeyBindings::nsNativeKeyBindings() >+{ >+ mResponder = [NativeKeyBindingsResponder new]; >+} So, mResponder should be managed by nsCocoaUtils or TextInputHandler. >+NS_IMETHODIMP_(bool) >+nsNativeKeyBindings::KeyDown(const nsNativeKeyEvent& aEvent, >+ DoCommandCallback aCallback, void *aCallbackData) >+{ >+ NS_OBJC_BEGIN_TRY_ABORT_BLOCK; >+ >+ PR_LOG(gNativeKeyBindingsLog, PR_LOG_DEBUG, >+ ("%p nsNativeKeyBindings::KeyDown", this)); >+ >+ // Recover the current event, which should always be the key down we are >+ // responding to. >+ NSEvent* cocoaEvent = [NSApp currentEvent]; This is a little bit hacky. I think that nsKeyEvent should have mNativeKeyEvent as void*. And it should be set at dispatching key events. It's useful for GTK too. However, I don't mind even if you don't want to work on this change if this works with nsIWidget::SynthesizeNativeKeyEvent(). >+ if (aSelector == @selector(cancelOperation:)) { >+ } else if (aSelector == @selector(capitalizeWord:)) { >+ } else if (aSelector == @selector(centerSelectionInVisibleArea:)) { >+ } else if (aSelector == @selector(changeCaseOfLetter:)) { >+ } else if (aSelector == @selector(complete:)) { >+ } else if (aSelector == @selector(deleteBackward:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_deleteCharBackward")); >+ } else if (aSelector == @selector(deleteBackwardByDecomposingPreviousCharacter:)) { >+ } else if (aSelector == @selector(deleteForward:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_deleteCharForward")); >+ >+ // TODO: deleteTo* selectors are also supposed to add text to a kill buffer >+ } else if (aSelector == @selector(deleteToBeginningOfLine:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_deleteToBeginningOfLine")); >+ } else if (aSelector == @selector(deleteToBeginningOfParagraph:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_deleteToBeginningOfLine")); >+ } else if (aSelector == @selector(deleteToEndOfLine:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_deleteToEndOfLine")); >+ } else if (aSelector == @selector(deleteToEndOfParagraph:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_deleteToEndOfLine")); >+ } else if (aSelector == @selector(deleteToMark:)) { >+ >+ } else if (aSelector == @selector(deleteWordBackward:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_deleteWordBackward")); >+ } else if (aSelector == @selector(deleteWordForward:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_deleteWordForward")); >+ } else if (aSelector == @selector(indent:)) { >+ } else if (aSelector == @selector(insertBacktab:)) { >+ } else if (aSelector == @selector(insertContainerBreak:)) { >+ } else if (aSelector == @selector(insertLineBreak:)) { >+ } else if (aSelector == @selector(insertNewline:)) { >+ } else if (aSelector == @selector(insertNewlineIgnoringFieldEditor:)) { >+ } else if (aSelector == @selector(insertParagraphSeparator:)) { >+ } else if (aSelector == @selector(insertTab:)) { >+ } else if (aSelector == @selector(insertTabIgnoringFieldEditor:)) { >+ } else if (aSelector == @selector(insertDoubleQuoteIgnoringSubstitution:)) { >+ } else if (aSelector == @selector(insertSingleQuoteIgnoringSubstitution:)) { >+ } else if (aSelector == @selector(insertText:)) { >+ } else if (aSelector == @selector(lowercaseWord:)) { >+ } else if (aSelector == @selector(moveBackward:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_charPrevious")); >+ } else if (aSelector == @selector(moveBackwardAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectCharPrevious")); >+ } else if (aSelector == @selector(moveParagraphForwardAndModifySelection:)) { >+ } else if (aSelector == @selector(moveParagraphBackwardAndModifySelection:)) { >+ } else if (aSelector == @selector(moveToBeginningOfDocumentAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectTop")); >+ } else if (aSelector == @selector(moveToEndOfDocumentAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectBottom")); >+ } else if (aSelector == @selector(moveToBeginningOfLineAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectBeginLine")); >+ } else if (aSelector == @selector(moveToEndOfLineAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectEndLine")); >+ } else if (aSelector == @selector(moveToBeginningOfParagraphAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectBeginLine")); >+ } else if (aSelector == @selector(moveToEndOfParagraphAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectEndLine")); >+ } else if (aSelector == @selector(moveToLeftEndOfLine:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_beginLine")); >+ } else if (aSelector == @selector(moveToLeftEndOfLineAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectBeginLine")); >+ } else if (aSelector == @selector(moveToRightEndOfLine:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_endLine")); >+ } else if (aSelector == @selector(moveToRightEndOfLineAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectEndLine")); >+ } else if (aSelector == @selector(moveDown:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_lineNext")); >+ } else if (aSelector == @selector(moveDownAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectLineNext")); >+ } else if (aSelector == @selector(moveForward:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_charNext")); >+ } else if (aSelector == @selector(moveForwardAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectCharNext")); >+ } else if (aSelector == @selector(moveLeft:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_charPrevious")); >+ } else if (aSelector == @selector(moveLeftAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectCharPrevious")); >+ } else if (aSelector == @selector(moveRight:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_charNext")); >+ } else if (aSelector == @selector(moveRightAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectCharNext")); >+ } else if (aSelector == @selector(moveToBeginningOfDocument:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_moveTop")); >+ } else if (aSelector == @selector(moveToBeginningOfLine:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_beginLine")); >+ } else if (aSelector == @selector(moveToBeginningOfParagraph:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_beginLine")); >+ } else if (aSelector == @selector(moveToEndOfDocument:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_moveBottom")); >+ } else if (aSelector == @selector(moveToEndOfLine:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_endLine")); >+ } else if (aSelector == @selector(moveToEndOfParagraph:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_endLine")); >+ } else if (aSelector == @selector(moveUp:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_linePrevious")); >+ } else if (aSelector == @selector(moveUpAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectLinePrevious")); >+ } else if (aSelector == @selector(moveWordBackward:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_wordPrevious")); >+ } else if (aSelector == @selector(moveWordBackwardAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectWordPrevious")); >+ } else if (aSelector == @selector(moveWordForward:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_wordNext")); >+ } else if (aSelector == @selector(moveWordForwardAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectWordNext")); >+ } else if (aSelector == @selector(moveWordLeft:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_wordPrevious")); >+ } else if (aSelector == @selector(moveWordRight:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_wordNext")); >+ } else if (aSelector == @selector(moveWordRightAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectWordNext")); >+ } else if (aSelector == @selector(moveWordLeftAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectWordPrevious")); >+ } else if (aSelector == @selector(pageDown:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_movePageDown")); >+ } else if (aSelector == @selector(pageDownAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectPageDown")); >+ } else if (aSelector == @selector(pageUp:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_movePageUp")); >+ } else if (aSelector == @selector(pageUpAndModifySelection:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectPageUp")); >+ } else if (aSelector == @selector(scrollToBeginningOfDocument:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_scrollTop")); >+ } else if (aSelector == @selector(scrollToEndOfDocument:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_scrollBottom")); >+ } else if (aSelector == @selector(scrollLineUp:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_scrollLineUp")); >+ } else if (aSelector == @selector(scrollLineDown:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_scrollLineDown")); >+ } else if (aSelector == @selector(scrollPageUp:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_scrollPageUp")); >+ } else if (aSelector == @selector(scrollPageDown:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_scrollPageDown")); >+ } else if (aSelector == @selector(selectAll:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectAll")); >+ } else if (aSelector == @selector(selectLine:)) { >+ // Note: This is functional, but Cocoa's version is direction-less in >+ // that selection direction is not determined until some future directed >+ // action is taken. >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_beginLine")); >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectEndLine")); >+ } else if (aSelector == @selector(selectParagraph:)) { >+ } else if (aSelector == @selector(selectToMark:)) { >+ } else if (aSelector == @selector(selectWord:)) { >+ // Note: This is functional, but Cocoa's version is direction-less in >+ // that selection direction is not determined until some future directed >+ // action is taken. >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_wordPrevious")); >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectWordNext")); >+ } else if (aSelector == @selector(setMark:)) { >+ } else if (aSelector == @selector(showContextHelp:)) { >+ } else if (aSelector == @selector(swapWithMark:)) { >+ } else if (aSelector == @selector(transpose:)) { >+ } else if (aSelector == @selector(transposeWords:)) { >+ } else if (aSelector == @selector(uppercaseWord:)) { >+ } else if (aSelector == @selector(yank:)) { >+ } else if (aSelector == @selector(supplementalTargetForAction:sender:)) { >+ >+ // Selectors from NSText's "Action Methods for Editing" section >+ // Ordering taken from Apple's docs >+ >+ } else if (aSelector == @selector(copy:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_copy")); >+ } else if (aSelector == @selector(cut:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_cut")); >+ } else if (aSelector == @selector(paste:)) { >+ mCommands.AppendElement(NS_LITERAL_CSTRING("cmd_paste")); >+ } else if (aSelector == @selector(copyFont:)) { >+ } else if (aSelector == @selector(pasteFont:)) { >+ } else if (aSelector == @selector(copyRuler:)) { >+ } else if (aSelector == @selector(pasteRuler:)) { >+ } else if (aSelector == @selector(delete:)) { >+ } This looks very expensive. Can we use switch-case for @selector(something)? If it's possible, please use it instead of a lot of comparison. >+- (void)insertText:(id)aString >+{ >+ if (PR_LOG_TEST(gNativeKeyBindingsLog, PR_LOG_DEBUG)) { >+ nsAutoString nsString; >+ nsCocoaUtils::GetStringForNSString(aString, nsString); >+ >+ PR_LOG(gNativeKeyBindingsLog, PR_LOG_DEBUG, >+ ("%p NativeKeyBindingsResponder::insertText string=%s", >+ self, ToNewCString(nsString))); >+ } >+ >+ // interpretKeyEvents will call this on actual text entry, not yet supported >+} You needs #ifdef PR_LOGGING. Additionally, you should make some automated tests for some commands on editors into widget/tests. You can synthesize native key event with nsIDOMWindowUtils::sendNativeKeyEvent(). See test_keyCode.xul for the example. And also test_texteditor_keyevent_handling.html and test_htmleditor_keyevent_handling.html might help you to understand testing on editor. If you have some questions about making tests, feel free to ask me.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #24) > Comment on attachment 733403 [details] [diff] [review] > Patch v3 > > >diff --git a/widget/cocoa/nsNativeKeyBindings.h b/widget/cocoa/nsNativeKeyBindings.h > > Now, we don't use ns prefix for new class. So, please rename this file > CocoaNativeKeyBindings.h Okay, I've renamed it NativeKeyBindings.h. It didn't seem necessary to include the "Cocoa" prefix to me since it lives in widget/cocoa, but let me know if that goes against convention. > >new file mode 100644 > >--- /dev/null > >+++ b/widget/cocoa/nsNativeKeyBindings.h > > >+#define NS_NATIVEKEYBINDINGS_CID \ > >+{0x8477f934, 0xfebf, 0x4c79, {0xb7, 0xfe, 0xbb, 0x7f, 0x9e, 0xbb, 0x9b, 0x4f}} > > nit: > #define NS_NATIVEKEYBINDINGS_CID \ > {0x8477f934, 0xfebf, 0x4c79, > {0xb7, 0xfe, 0xbb, 0x7f, 0x9e, 0xbb, 0x9b, 0x4f}} > > this style is typically used. The code base appears to contain just about every possible way of formatting these CIDs... ;) But that one sounds fine to me, changed. > >+ nsTArray<nsCString> mCommands; > >+} > >+ > >+- (void)startRecording; > >+ > >+- (void)doCommandBySelector:(SEL)aSelector; > >+ > >+- (void)insertText:(id)aString; > >+ > >+- (const nsTArray<nsCString>&)recordedCommands; > >+ > >+@end // NativeKeyBindingsResponder > > I think that this may be useful in TextInputHanandler.mm in the future. > > So, I think that it's better to move this to nsCocoaUtils.mm or > TextInputHandler.mm. Then, only a static method should be exported, e.g., > void GetCommandSelectorsFor(NSEvent* aEvent, nsTArray<nsCString>& > aSelectors) which returns raw selectors. And convert the raw selectors to > Gecko commands in NativeKeyBindings. Moved to nsCocoaUtils. > >diff --git a/widget/cocoa/nsNativeKeyBindings.mm b/widget/cocoa/nsNativeKeyBindings.mm > > >+#ifdef MOZ_LOGGING > >+#define FORCE_PR_LOG > >+#endif > > Why do you think that we need to get the log on release builds? It doesn't need to be. That was just the pattern I saw from looking at TextInputHandler, and it was convenient during my testing. I've removed it. > >+NS_IMETHODIMP_(bool) > >+nsNativeKeyBindings::KeyDown(const nsNativeKeyEvent& aEvent, > >+ DoCommandCallback aCallback, void *aCallbackData) > >+{ > >+ NS_OBJC_BEGIN_TRY_ABORT_BLOCK; > >+ > >+ PR_LOG(gNativeKeyBindingsLog, PR_LOG_DEBUG, > >+ ("%p nsNativeKeyBindings::KeyDown", this)); > >+ > >+ // Recover the current event, which should always be the key down we are > >+ // responding to. > >+ NSEvent* cocoaEvent = [NSApp currentEvent]; > > This is a little bit hacky. I think that nsKeyEvent should have > mNativeKeyEvent as void*. And it should be set at dispatching key events. > It's useful for GTK too. However, I don't mind even if you don't want to > work on this change if this works with nsIWidget::SynthesizeNativeKeyEvent(). Yeah, I wasn't sure if adding a field to nsKeyEvent would be recommended or not. But it turns the above doesn't work with SynthesizeNativeKeyEvent, so I've gone ahead and added the native event to nsKeyEvent as you suggested. > >+ if (aSelector == @selector(cancelOperation:)) { > [Omitted many more lines...] > >+ } > > This looks very expensive. Can we use switch-case for @selector(something)? > If it's possible, please use it instead of a lot of comparison. I agree, it wasn't a great way to do all those checks. I couldn't get a switch statement to work, but I put all the one to one mappings into a hashtable, so those should be much faster to look up. > Additionally, you should make some automated tests for some commands on > editors into widget/tests. You can synthesize native key event with > nsIDOMWindowUtils::sendNativeKeyEvent(). See test_keyCode.xul for the > example. And also test_texteditor_keyevent_handling.html and > test_htmleditor_keyevent_handling.html might help you to understand testing > on editor. If you have some questions about making tests, feel free to ask > me. I've added some tests for the system-default key bindings. However, I've noticed there are now a few key event tests that are failing. The main issue is that these tests call synthesizeKey and then rely on the bindings from platformHTMLBindings.xml to trigger the appropriate action. Since my patch removes those lines from platformHTMLBindings.xml and relies on the OS instead, those bindings don't work anymore unless you also have a native key event as well. I can think of several ways to resolve this: * Change the failing tests to use synthesizeNativeKey instead * Change the NativeKeyBindings code to "guess" the Cocoa event if one is not present (due to testing). The first option seems best to me, since that is closer to what happens in real usage outside of testing. There may be other options too. What would you recommend?

Sorry for the delay. I'll check it as far as possible. (Perhaps, late this week or early next week)

Comment on attachment 737081 [details] [diff] [review] Patch v4 Excellent! >+++ b/widget/cocoa/NativeKeyBindings.mm >+NativeKeyBindings::NativeKeyBindings() >+{ >+} >+ >+#define SEL_TO_COMMAND(sel, command) \ >+ mSelectorToCommand.Put( \ >+ reinterpret_cast<struct objc_selector *>(@selector(sel)), \ >+ NS_LITERAL_CSTRING(command).get()) Why don't use #command instead of NS_LITERAL_CSTRING(command).get()? And please use aSel and aCommand for the names of the arguments. >+ // selectLine: is complex, see KeyDown >+ // selectWord: is complex, see KeyDown Could you file new commands for them after this bug is fixed? >+NS_IMETHODIMP_(bool) >+NativeKeyBindings::KeyDown(const nsNativeKeyEvent& aEvent, >+ DoCommandCallback aCallback, void *aCallbackData) >+{ >+ PR_LOG(gNativeKeyBindingsLog, PR_LOG_DEBUG, >+ ("%p NativeKeyBindings::KeyDown", this)); >+ >+ // Recover the current event, which should always be the key down we are >+ // responding to. >+ nsKeyEvent* geckoEvent = static_cast<nsKeyEvent*>(aEvent.geckoEvent); Hmm, I'm not sure why we're not using nsKeyEvent* directly instead of nsNativeKeyEvent. Perhaps, it should be changed later. >+ if (!geckoEvent) { >+ PR_LOG(gNativeKeyBindingsLog, PR_LOG_DEBUG, >+ ("%p NativeKeyBindings::KeyDown, no Gecko event", this)); >+ >+ return false; >+ } Do you actually assume that this case occurs? If not so, just use MOZ_ASSERT(geckoEvent). >+ for (uint16_t i = 0; i < bindingCommands.Length(); i++) { Why do you use uint16_t instead of uint32_t? Isn't this cause warning? >+ const char* commandStr; >+ SEL selector = bindingCommands[i].selector; >+ >+ // Try to find a simple mapping in the hashtable >+ commandStr = mSelectorToCommand.Get( >+ reinterpret_cast<struct objc_selector*>(selector)); const char* commandStr should be defined here. >+ if (commandStr) { >+ geckoCommands.AppendElement(commandStr); >+ } else if (selector == @selector(selectLine:)) { >+ // Note: This is functional, but Cocoa's version is direction-less in >+ // that selection direction is not determined until some future directed >+ // action is taken. >+ geckoCommands.AppendElement(NS_LITERAL_CSTRING("cmd_beginLine").get()); >+ geckoCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectEndLine").get()); >+ } else if (selector == @selector(selectWord:)) { >+ // Note: This is functional, but Cocoa's version is direction-less in >+ // that selection direction is not determined until some future directed >+ // action is taken. >+ geckoCommands.AppendElement(NS_LITERAL_CSTRING("cmd_wordPrevious").get()); >+ geckoCommands.AppendElement(NS_LITERAL_CSTRING("cmd_selectWordNext").get()); >+ } Why don't you use just "cmd_*" instead of NS_LITERAL_CSTRING().get()? If somebody would implement new commands for them, we could avoid this ugly code. >+ for (uint16_t i = 0; i < geckoCommands.Length(); i++) { Same, why do you use uint16_t instead of uint32_t? >diff --git a/widget/cocoa/nsCocoaUtils.mm b/widget/cocoa/nsCocoaUtils.mm >--- a/widget/cocoa/nsCocoaUtils.mm >+++ b/widget/cocoa/nsCocoaUtils.mm >@@ -566,8 +566,62 @@ nsCocoaUtils::HiDPIEnabled() > // 3 if both lo- and hi-DPI screens > // We'll enable HiDPI support if there's only a single screen type, > // OR if the pref setting is explicitly greater than 1. > sHiDPIEnabled = (scaleFactors <= 2) || (prefSetting > 1); > } > > return sHiDPIEnabled; > } >+ >+void >+nsCocoaUtils::GetCommandsFromKeyEvent(NSEvent* aEvent, >+ nsTArray<KeyBindingsCommand>& aCommands) >+{ >+ NS_OBJC_BEGIN_TRY_ABORT_BLOCK; >+ >+ if (!aEvent) { >+ return; >+ } I think that this has to always non-null. Please use MOZ_ASSERT(aEvent). >diff --git a/widget/nsGUIEvent.h b/widget/nsGUIEvent.h >--- a/widget/nsGUIEvent.h >+++ b/widget/nsGUIEvent.h >@@ -1057,31 +1057,34 @@ private: > public: > nsKeyEvent() > { > } > > nsKeyEvent(bool isTrusted, uint32_t msg, nsIWidget *w) > : nsInputEvent(isTrusted, msg, w, NS_KEY_EVENT), > keyCode(0), charCode(0), >- location(nsIDOMKeyEvent::DOM_KEY_LOCATION_STANDARD), isChar(0) >+ location(nsIDOMKeyEvent::DOM_KEY_LOCATION_STANDARD), isChar(0), >+ nativeKeyEvent(nullptr) > { > } > > /// see NS_VK codes >- uint32_t keyCode; >+ uint32_t keyCode; > /// OS translated Unicode char > uint32_t charCode; > // One of nsIDOMKeyEvent::DOM_KEY_LOCATION_* > uint32_t location; > // OS translated Unicode chars which are used for accesskey and accelkey > // handling. The handlers will try from first character to last character. > nsTArray<nsAlternativeCharCode> alternativeCharCodes; > // indicates whether the event signifies a printable character > bool isChar; >+ // OS-specific native event can optionally be preserved >+ void* nativeKeyEvent; > }; Use mNativeKeyEvent instead of nativeKeyEvent. nsGUIEvent.h's members aren't named with our coding rules, however, new members should be named with the rules. And this cannot be copied across the process boundary. So, it might be better to add some comment in nsGUIEventIPC.h. > struct nsNativeKeyEvent > { >- nsEvent *nativeEvent; // see bug 406407 to see how this is used >+ nsEvent* geckoEvent; // see bug 406407 to see how this is used Can you use nsKeyEvent* instead of nsEvent*? And name it mGeckoEvent? > uint32_t keyCode; > uint32_t charCode; > bool altKey; > bool ctrlKey; > bool shiftKey; > bool metaKey; I don't understand why these members are still here. But it's not in the scope of this bug. Thank you for your great job! And sorry for the long delay.

Thanks for another review and encouragement! :) I'll take a look at your comments soon. Do you have any thoughts on my testing questions at the end of comment 25? I am not quite sure how to proceed with fixing the failing tests which will likely now require native events on OS X.

(In reply to J. Ryan Stinnett [:jryans] from comment #25) > Created attachment 737081 [details] [diff] [review] > Patch v4 > > > >diff --git a/widget/cocoa/nsNativeKeyBindings.mm b/widget/cocoa/nsNativeKeyBindings.mm > > > > >+#ifdef MOZ_LOGGING > > >+#define FORCE_PR_LOG > > >+#endif > > > > Why do you think that we need to get the log on release builds? > > It doesn't need to be. That was just the pattern I saw from looking at > TextInputHandler, and it was convenient during my testing. I've removed it. The reason why TextInputHandler logs the behavior on release build is, the behavior depends on IME. If user would report a bug reproduced only with third party vendor's IME, we couldn't test it actually. For such case, we need a way to log the behavior by such user. > > Additionally, you should make some automated tests for some commands on > > editors into widget/tests. You can synthesize native key event with > > nsIDOMWindowUtils::sendNativeKeyEvent(). See test_keyCode.xul for the > > example. And also test_texteditor_keyevent_handling.html and > > test_htmleditor_keyevent_handling.html might help you to understand testing > > on editor. If you have some questions about making tests, feel free to ask > > me. > > I've added some tests for the system-default key bindings. > > However, I've noticed there are now a few key event tests that are failing. > The > main issue is that these tests call synthesizeKey and then rely on the > bindings > from platformHTMLBindings.xml to trigger the appropriate action. Since my > patch > removes those lines from platformHTMLBindings.xml and relies on the OS > instead, > those bindings don't work anymore unless you also have a native key event as > well. > > I can think of several ways to resolve this: > > * Change the failing tests to use synthesizeNativeKey instead > * Change the NativeKeyBindings code to "guess" the Cocoa event if one is not > present (due to testing). > > The first option seems best to me, since that is closer to what happens in > real > usage outside of testing. There may be other options too. What would you > recommend? synthesizeNativeKey depends on each platform's widget. So, it's not good idea to use it on XP level behavior tests. Could you post a full list of new test failures? Then, I'll check how should we do for each test. Perhaps, in most cases, we can just skip the tests only on Mac.

This version includes an important change to improve correctness: Native commands are sent in response to KeyPress, instead of KeyDown like they were in previous patches. This ensures KeyPress handlers from the web content get a chance to work as expected. I have also noticed that currently user bindings with multiple keys pressed independently in sequence don't work. To be clear, multiple keys all pressed together (like Option-V at the same time) are fine. But, if the user for example had a binding for "Press and release d, followed by press and release d again" then that would not currently work. Since there are no system default bindings of this format, and this capability is even lesser known than the user key bindings feature itself, I'd purpose delaying this until a separate bug (which I'll file assuming you agree). Also, after looking over test failures, I've realized that the "browser" bindings are still required at this time, since NativeKeyBindings is only called for "editor" documents, text inputs, and text areas, but not for "browser" documents. It seems more web pages trigger editor mode than I thought originally, so I wasn't properly testing browser mode before. In any case, the browser bindings have now returned. All review comments have been addressed. I've replied below to specific points. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #27) > Comment on attachment 737081 [details] [diff] [review] > Patch v4 > > >+ // selectLine: is complex, see KeyDown > >+ // selectWord: is complex, see KeyDown > > Could you file new commands for them after this bug is fixed? Yes, I'll do so after this bug is fixed. There are a number of other additional commands that could be added as well that I've noticed from doing this work. I'll file for those too. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #29) > (In reply to J. Ryan Stinnett [:jryans] from comment #25) > > I've added some tests for the system-default key bindings. > > > > However, I've noticed there are now a few key event tests that are failing. > > The > > main issue is that these tests call synthesizeKey and then rely on the > > bindings > > from platformHTMLBindings.xml to trigger the appropriate action. Since my > > patch > > removes those lines from platformHTMLBindings.xml and relies on the OS > > instead, > > those bindings don't work anymore unless you also have a native key event as > > well. > > > > I can think of several ways to resolve this: > > > > * Change the failing tests to use synthesizeNativeKey instead > > * Change the NativeKeyBindings code to "guess" the Cocoa event if one is not > > present (due to testing). > > > > The first option seems best to me, since that is closer to what happens in > > real > > usage outside of testing. There may be other options too. What would you > > recommend? > > synthesizeNativeKey depends on each platform's widget. So, it's not good > idea to use it on XP level behavior tests. Could you post a full list of new > test failures? Then, I'll check how should we do for each test. Perhaps, in > most cases, we can just skip the tests only on Mac. Here's a try run that contains the test failures: https://tbpl.mozilla.org/?tree=Try&rev=e1e5615be4f5 And here's a list of the test files with one or more failures: editor/libeditor/text/tests/test_bug596506.html editor/libeditor/text/tests/test_texteditor_keyevent_handling.html layout/generic/test/test_movement_by_words.html browser/devtools/webconsole/test/browser_webconsole_bug_804845_ctrl_key_nav.js toolkit/content/tests/chrome/test_autocomplete_mac_caret.xul toolkit/content/tests/chrome/test_bug451540.xul accessible/tests/mochitest/text/test_atcaretoffset.html I don't think we'd want to lose coverage for all of these on Mac by excluding the tests, but let me know what you think.

Comment on attachment 744234 [details] [diff] [review] Patch v5 >+NS_IMETHODIMP_(bool) >+NativeKeyBindings::KeyPress(const nsNativeKeyEvent& aEvent, >+ DoCommandCallback aCallback, void *aCallbackData) >+{ >+ PR_LOG(gNativeKeyBindingsLog, PR_LOG_DEBUG, >+ ("%p NativeKeyBindings::KeyPress", this)); >+ >+ // Recover the current event, which should always be the key down we are >+ // responding to. >+ nsKeyEvent* geckoEvent = aEvent.mGeckoEvent; >+ >+ MOZ_ASSERT(geckoEvent); >+ >+ NSEvent* cocoaEvent = reinterpret_cast<NSEvent*>(geckoEvent->mNativeKeyEvent); >+ >+ if (!cocoaEvent || [cocoaEvent type] != NSKeyDown) { >+ PR_LOG(gNativeKeyBindingsLog, PR_LOG_DEBUG, >+ ("%p NativeKeyBindings::KeyPress, no Cocoa key down event", this)); >+ >+ return false; >+ } >+ >+ PR_LOG(gNativeKeyBindingsLog, PR_LOG_DEBUG, >+ ("%p NativeKeyBindings::KeyPress, interpreting", this)); >+ >+ nsTArray<KeyBindingsCommand> bindingCommands; nsTAutoArray is better, it doesn't use heap if the result is smaller than you expected. So, it can prevent memory fragmentation. >+ nsTArray<const char*> geckoCommands; Same. >+ for (uint32_t i = 0; i < geckoCommands.Length(); i++) { >+ >+ const char* geckoCommand = geckoCommands[i]; I don't think the blank line is necessary. >+ >+ PR_LOG(gNativeKeyBindingsLog, PR_LOG_DEBUG, >+ ("%p NativeKeyBindings::KeyPress, command=%s", >+ this, geckoCommand)); >+ >+ // Execute the Gecko command >+ aCallback(geckoCommand, aCallbackData); >+ >+ } This blank line too. >+struct KeyBindingsCommand { >+ SEL selector; >+ id data; >+}; Please put |{| to the next line. i.e., struct KeyBindingsCommand { ... }; I'll look the all tests you listed up.

> editor/libeditor/text/tests/test_bug596506.html > editor/libeditor/text/tests/test_texteditor_keyevent_handling.html > layout/generic/test/test_movement_by_words.html > browser/devtools/webconsole/test/browser_webconsole_bug_804845_ctrl_key_nav.js > toolkit/content/tests/chrome/test_autocomplete_mac_caret.xul > toolkit/content/tests/chrome/test_bug451540.xul > accessible/tests/mochitest/text/test_atcaretoffset.html I see what happens. These tests try to kick shortcut key which you removed by the patch with nsDOMWindowUtils::SendKeyEvent(). However, mGeckoEvent is always null if the event is synthesized with this method. So, I think that you need to emulate/make a native key event with #ifdef in the method. Then, your patch will work with the tests if shortcut key isn't changed by your patch.

Fixed all review notes from comment #31. For the tests, I added nsIWidget::AttachNativeKeyEvent to emulate the native key event data during testing. This, along with some test adjustments, takes care of most of the failures from last time. I tried to get toolkit/content/tests/chrome/test_bug451540.xul to work, but was unable to do so. Currently it does not use the typical synthesizeKey method from EventUtils.js, so it does not go through nsDOMWindowUtils::SendKeyEvent. I attempted to convert the test to this style, but then I wasn't able to figure out how to get the key events to hit the element under test. For now, I have disabled this test on Cocoa. Try run: https://tbpl.mozilla.org/?tree=Try&rev=a756035eba5c

Hmm, would you separate the patch for the additional part for new orange fix? It's really not clear for me where are changed for the oranges.

I believe the interdiff between patch v5 and patch v6 captures the work done to address the oranges pretty well: https://bugzilla.mozilla.org/attachment.cgi?oldid=744234&action=interdiff&newid=755401&headers=1 If you would still like a separate patch, I am happy to prepare one, though it would look basically like the interdiff above. Here's a summary of the changes that were needed: * As discussed in comment #32, when synthesizing keys during tests, the mNativeKeyEvent field on the Gecko key event is null, since it didn't come from the OS. I added nsIWidget::AttachNativeKeyEvent to generate a simulated native event when synthesizing keys (called in nsDOMWindowUtils::SendKeyEvent). This only does any work on Cocoa. ** Part of achieving this required getting the Cocoa characters for the key event. I added nsCocoaUtils::ConvertGeckoKeyCodeToMacCharCode to do this, which borrows and extends a mapping of key codes originally used in nsMenuItemX to do this. * For editor/libeditor/text/tests/test_texteditor_keyevent_handling.html, I updated the test to match the fact that more keys are captured on OS X due to system keybindings that we now pick up on via this patch. * I also found that there were still a few key bindings left over in the includes that get used in platformHTMLBindings.xml that are no longer needed on OS X, since they are now handled by NativeKeyBindings. ** I removed the unnecessary key bindings from the common include files, and pushed them down into the platformHTMLBindings.xml file for all non-Cocoa platforms. * I needed to take different actions with up and down arrow keys depending on whether the target was a text input or a text area ** To achieve that, I used the same "type" concept used to init the GTK version of NativeKeyBindings. This results in 3 different instances of NativeKeyBindings, one for each field type. * As mentioned in comment #33, I disabled toolkit/content/tests/chrome/test_bug451540.xul on Cocoa because I could not get the target element to respond to synthesized key events. I tested the element manually, and it appears to work fine.

(In reply to J. Ryan Stinnett [:jryans] from comment #35) > If you would still like a separate patch, I am happy to prepare one, though > it would look basically like the interdiff above. Yes, please do it. It's worthwhile for the change history of mercurial. Basically, all fixes should be separated to small patches as far as possible for safer/clearer review and history. For example, see bug 813445. In it, I separated patches for each flag change.

Okay, I've broken this up into separate patches. Parts 6 - 8 are the ones related to testing and your question from comment #34.

Comment on attachment 759445 [details] [diff] [review] Part 3: Add key bindings recorder to nsCocoaUtils >+ static NativeKeyBindingsRecorder* sNativeKeyBindingsRecorder; >+ if (!sNativeKeyBindingsRecorder) { >+ sNativeKeyBindingsRecorder = [NativeKeyBindingsRecorder new]; >+ } Is this safe for our memory leak detector? I.e., it doesn't become orange at testing after debug build? If it becomes orange, please move the static method as a member of nsCocoaUtils and release it at shutting down.

Comment on attachment 759446 [details] [diff] [review] Part 4: Create NativeKeyBindings for Cocoa Please use PR_LOG_ALWAYS instead of PR_LOG_DEBUG because you don't use other values than PR_LOG_DEBUG. So, people who want to log the behavior wants all behavior. >+namespace mozilla { >+namespace widget { >+ >+enum NativeKeyBindingsType { >+ eNativeKeyBindingsType_Input, >+ eNativeKeyBindingsType_TextArea, >+ eNativeKeyBindingsType_Editor >+}; Please use this style: enum NativeKeyBindingsType { ... }; >+ NS_IMETHOD_(bool) KeyDown(const nsNativeKeyEvent& aEvent, >+ DoCommandCallback aCallback, >+ void *aCallbackData); void* aCallbackData >+ >+ NS_IMETHOD_(bool) KeyPress(const nsNativeKeyEvent& aEvent, >+ DoCommandCallback aCallback, >+ void *aCallbackData); Same. >+ >+ NS_IMETHOD_(bool) KeyUp(const nsNativeKeyEvent& aEvent, >+ DoCommandCallback aCallback, >+ void *aCallbackData); Same. >+#define SEL_TO_COMMAND(aSel, aCommand) \ >+ mSelectorToCommand.Put( \ >+ reinterpret_cast<struct objc_selector *>(@selector(aSel)), aCommand) Thank you for your idea which we should use hash table for the performance! >+ // SEL_TO_COMMAND(delete:, ); What's this? Deletes the content only when there is selection? If so, "cmd_delete"? >+ SEL_TO_COMMAND(deleteToBeginningOfParagraph:, "cmd_deleteToBeginningOfLine"); >+ SEL_TO_COMMAND(deleteToEndOfParagraph:, "cmd_deleteToEndOfLine"); Should we map them? I guess that they work as expected only when it's in single line editor. Otherwise, i.e., focused editor is <div contenteditable> or <textarea> with wrapped text, they shouldn't work as expected. But I'm not sure what we should do for now. >+ SEL_TO_COMMAND(paste:, "cmd_paste"); I'm not sure if "cmd_pasteTransferable" is better. >+ // selectLine: is complex, see KeyDown >+ // SEL_TO_COMMAND(selectParagraph:, ); If the focused editor is <input>, "select all"? >+NS_IMETHODIMP_(bool) >+NativeKeyBindings::KeyDown(const nsNativeKeyEvent& aEvent, >+ DoCommandCallback aCallback, void *aCallbackData) void* aCallbackData >+NS_IMETHODIMP_(bool) >+NativeKeyBindings::KeyPress(const nsNativeKeyEvent& aEvent, >+ DoCommandCallback aCallback, void *aCallbackData) Same. >diff --git a/widget/cocoa/nsWidgetFactory.mm b/widget/cocoa/nsWidgetFactory.mm >--- a/widget/cocoa/nsWidgetFactory.mm >+++ b/widget/cocoa/nsWidgetFactory.mm >@@ -71,16 +71,75 @@ NS_GENERIC_FACTORY_CONSTRUCTOR(nsStandal > #include "GfxInfo.h" > namespace mozilla { > namespace widget { > // This constructor should really be shared with all platforms. > NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(GfxInfo, Init) > } > } > >+#include "NativeKeyBindings.h" >+namespace mozilla { >+namespace widget { >+ >+static nsresult >+NativeKeyBindingsConstructor(nsISupports *aOuter, REFNSIID aIID, >+ void **aResult, NativeKeyBindingsType aType) nsISupports* aOuter void** aResult >+{ >+ nsresult rv; You don't need this line. >+ >+ NativeKeyBindings* inst; >+ >+ *aResult = NULL; >+ if (NULL != aOuter) { >+ rv = NS_ERROR_NO_AGGREGATION; >+ return rv; return NS_ERROR_NO_AGGREGATION; >+ } >+ >+ inst = new NativeKeyBindings(); >+ if (NULL == inst) { >+ rv = NS_ERROR_OUT_OF_MEMORY; >+ return rv; >+ } If OOM occurs, our process is forcibly crashed by our library. So, you don't need to check this. >+ NS_ADDREF(inst); >+ rv = inst->Init(aType); nsresult rv = ... >+ if (NS_SUCCEEDED(rv)) { >+ rv = inst->QueryInterface(aIID, aResult); >+ } >+ NS_RELEASE(inst); >+ >+ return rv; >+} >+ >+static nsresult >+NativeKeyBindingsInputConstructor(nsISupports *aOuter, REFNSIID aIID, >+ void **aResult) nsISupports* aOuter void** aResult >+{ >+ return NativeKeyBindingsConstructor(aOuter, aIID, aResult, >+ eNativeKeyBindingsType_Input); >+} >+ >+static nsresult >+NativeKeyBindingsTextAreaConstructor(nsISupports *aOuter, REFNSIID aIID, >+ void **aResult) nsISupports* aOuter void** aResult >+{ >+ return NativeKeyBindingsConstructor(aOuter, aIID, aResult, >+ eNativeKeyBindingsType_TextArea); >+} >+ >+static nsresult >+NativeKeyBindingsEditorConstructor(nsISupports *aOuter, REFNSIID aIID, >+ void **aResult) nsISupports* aOuter void** aResult

Comment on attachment 759449 [details] [diff] [review] Part 5: Remove platform bindings for Cocoa I'm not sure this approach is the best. Forward to ehsan.

Comment on attachment 759450 [details] [diff] [review] Part 6: Add Gecko key to Cocoa char conversion >diff --git a/widget/cocoa/nsCocoaUtils.mm b/widget/cocoa/nsCocoaUtils.mm >--- a/widget/cocoa/nsCocoaUtils.mm >+++ b/widget/cocoa/nsCocoaUtils.mm >@@ -618,8 +618,186 @@ nsCocoaUtils::GetCommandsFromKeyEvent(NS > @selector(insertText:), > aString > }; > > mCommands->AppendElement(command); > } > > @end // NativeKeyBindingsRecorder >+ >+struct KeyConversionData { >+ const char* str; >+ size_t strLength; >+ uint32_t geckoKeyCode; >+ uint32_t charCode; >+}; Use this style: struct KeyConversionData { .. }; Following comment is just "comments". You don't need to change your patch. >+ KEYCODE_ENTRY(VK_ENTER, NSEnterCharacter), NS_VK_ENTER is never used, though. >+ KEYCODE_ENTRY(VK_SEPARATOR, 0), Mac JIS keyboard has the separator key... And there are a lot of non-processed keycodes... >+uint32_t >+nsCocoaUtils::ConvertGeckoNameToMacCharCode(const nsAString& aKeyCodeName) >+{ >+ if (aKeyCodeName.IsEmpty()) { >+ return 0; >+ } >+ >+ nsAutoCString keyCodeName; >+ keyCodeName.AssignWithConversion(aKeyCodeName); >+ // We want case-insensitive comparison with data stored as uppercase. >+ ToUpperCase(keyCodeName); >+ >+ uint32_t keyCodeNameLength = keyCodeName.Length(); >+ const char* keyCodeNameStr = keyCodeName.get(); >+ for (uint16_t i = 0; >+ i < (sizeof(gKeyConversions) / sizeof(KeyConversionData)); ++i) { Cannot you use ArrayLength()? >+uint32_t >+nsCocoaUtils::ConvertGeckoKeyCodeToMacCharCode(uint32_t aKeyCode) >+{ >+ if (!aKeyCode) { >+ return 0; >+ } >+ >+ for (uint16_t i = 0; >+ i < (sizeof(gKeyConversions) / sizeof(KeyConversionData)); ++i) { Same.

Comment on attachment 759452 [details] [diff] [review] Part 7: Simulate native events for testing I completely don't like to change nsIWidget interface with this patch. You just call the new method with the arguments which are set to nsKeyEvent. That means you can implement this only under widget/cocoa. First, in nsDOMWindowUtils::SendKeyEvent(), set event.mFlags.mIsSynthesizedForTests true. Then, nsChildView::DispatchEvent() can distinguish if the dispatching event is synthesized key event. Next, if the dispatching event is synthesized key event, attach the native key event information to the dispatching event.

Comment on attachment 759453 [details] [diff] [review] Part 8: Clean up test expectations > MOCHITEST_CHROME_FILES += test_panel_focus.xul \ > window_panel_focus.xul \ > test_chromemargin.xul \ >- window_chromemargin.xul >+ window_chromemargin.xul \ >+ bug451540_window.xul \ >+ test_bug451540.xul Please use same indent as above lines.

Comment on attachment 759446 [details] [diff] [review] Part 4: Create NativeKeyBindings for Cocoa NativeKeyBindings.mm should be reviewed by Steven too.

Comment on attachment 759449 [details] [diff] [review] Part 5: Remove platform bindings for Cocoa Review of attachment 759449 [details] [diff] [review]: ----------------------------------------------------------------- I think Neil is probably a better reviewer for this.

Comment on attachment 759449 [details] [diff] [review] Part 5: Remove platform bindings for Cocoa So, it turns out that it was ehsan who added the whole of input-fields-base.inc, textareas-base.inc and editor-base.inc to unix/platformHTMLBindings.xml back in attachment 477998 [details] [diff] [review] to bug 410986 just to get paste without formatting to work. This means that it is actually wrong to copy most of the commands into the unix bindings file because the gtk native key bindings already handle the cursor and editing keys. I suggest the correct approach is to remove the includes of the base key bindings from the mac bindings file, only copying those bindings that you still need. Since there are three files that do still need all of those bindings, that's more efficient than copying the bindings that you don't need into each of the those files. (Ideally you would fix the unix bindings file at the same time, assuming ehsan will help you figure out which bindings to copy.)

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (Offline 6/11 - 6/17) from comment #46) > Comment on attachment 759445 [details] [diff] [review] > Part 3: Add key bindings recorder to nsCocoaUtils > > >+ static NativeKeyBindingsRecorder* sNativeKeyBindingsRecorder; > >+ if (!sNativeKeyBindingsRecorder) { > >+ sNativeKeyBindingsRecorder = [NativeKeyBindingsRecorder new]; > >+ } > > Is this safe for our memory leak detector? I.e., it doesn't become orange at > testing after debug build? > > If it becomes orange, please move the static method as a member of > nsCocoaUtils and release it at shutting down. It appears to be safe, yes. I did not see any oranges on the OS X builds for this try push, which includes debug: https://tbpl.mozilla.org/?tree=Try&rev=6a437bc61c90

Rebased, carrying over masayuki's r+ from attachment #759442 [details] [diff] [review].

Rebased, review comments addressed. Carrying over flags from attachment #759446 [details] [diff] [review]. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (Offline 6/11 - 6/17) from comment #47) > Comment on attachment 759446 [details] [diff] [review] > Part 4: Create NativeKeyBindings for Cocoa > > >+namespace mozilla { > >+namespace widget { > >+ > >+enum NativeKeyBindingsType { > >+ eNativeKeyBindingsType_Input, > >+ eNativeKeyBindingsType_TextArea, > >+ eNativeKeyBindingsType_Editor > >+}; > > Please use this style: > > enum NativeKeyBindingsType > { > ... > }; I have updated the enum style, but a search of the codebase with DXR suggests that a large number of enums have the opening brace on the same line as the enum keyword. If your style is the correct one for use throughout the codebase, should I update the Coding Style page (https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style)? > >+ // SEL_TO_COMMAND(delete:, ); > > What's this? Deletes the content only when there is selection? If so, > "cmd_delete"? I was hoping that would work too, but "cmd_delete" currently doesn't verify that there is a selection before deleting. Look at the editor code, it seems like that is not intended so I will file a bug. Added a TODO on this line. > >+ SEL_TO_COMMAND(deleteToBeginningOfParagraph:, "cmd_deleteToBeginningOfLine"); > >+ SEL_TO_COMMAND(deleteToEndOfParagraph:, "cmd_deleteToEndOfLine"); > > Should we map them? I guess that they work as expected only when it's in > single line editor. Otherwise, i.e., focused editor is <div contenteditable> > or <textarea> with wrapped text, they shouldn't work as expected. But I'm > not sure what we should do for now. That's a good point. I've changed it to only map the paragraph selectors when you are in a single line input for now. > >+ SEL_TO_COMMAND(paste:, "cmd_paste"); > > I'm not sure if "cmd_pasteTransferable" is better. "cmd_pasteTransferable" doesn't seem to do anything, so leaving as "cmd_paste". It seems like "cmd_paste" does carry over rich formatting to editors, so this is probably fine. > >+ // SEL_TO_COMMAND(selectParagraph:, ); > > If the focused editor is <input>, "select all"? Sounds reasonable, I've added it.

(In reply to neil@parkwaycc.co.uk from comment #54) > Comment on attachment 759449 [details] [diff] [review] > Part 5: Remove platform bindings for Cocoa > > I suggest the correct approach is to remove the includes of the base key > bindings from the mac bindings file, only copying those bindings that you > still need. Since there are three files that do still need all of those > bindings, that's more efficient than copying the bindings that you don't > need into each of the those files. > > (Ideally you would fix the unix bindings file at the same time, assuming > ehsan will help you figure out which bindings to copy.) I would like to avoid changing other platforms in the scope of this bug. I've updated the patch to take the approach you recommended.

Carrying over masayuki's r+ from attachment #759540 [details]. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #49) > Comment on attachment 759450 [details] [diff] [review] > Part 6: Add Gecko key to Cocoa char conversion > > >+struct KeyConversionData { > >+ const char* str; > >+ size_t strLength; > >+ uint32_t geckoKeyCode; > >+ uint32_t charCode; > >+}; > > Use this style: > > struct KeyConversionData > { > .. > }; I have updated the struct style, but a search of the codebase with DXR suggests that a large number of structs have the opening brace on the same line as the structs keyword. If your style is the correct one for use throughout the codebase, should I update the Coding Style page (https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style)? > >+ for (uint16_t i = 0; > >+ i < (sizeof(gKeyConversions) / sizeof(KeyConversionData)); ++i) { > > Cannot you use ArrayLength()? Makes sense, I've updated both instances.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #50) > Comment on attachment 759452 [details] [diff] [review] > Part 7: Simulate native events for testing > > I completely don't like to change nsIWidget interface with this patch. > > You just call the new method with the arguments which are set to nsKeyEvent. > That means you can implement this only under widget/cocoa. > > First, in nsDOMWindowUtils::SendKeyEvent(), set > event.mFlags.mIsSynthesizedForTests true. Then, nsChildView::DispatchEvent() > can distinguish if the dispatching event is synthesized key event. > > Next, if the dispatching event is synthesized key event, attach the native > key event information to the dispatching event. I like your approach much better. :) At first it seemed like I needed the additional arguments, but you're right that they are not necessary. I've updated the patch as you've suggested.

(In reply to J. Ryan Stinnett from comment #57) > (In reply to Masayuki Nakano from comment #47) > > (From update of attachment 759446 [details] [diff] [review]) > > >+ // SEL_TO_COMMAND(delete:, ); > > > > What's this? Deletes the content only when there is selection? If so, > > "cmd_delete"? > > I was hoping that would work too, but "cmd_delete" currently doesn't verify > that there is a selection before deleting. Look at the editor code, it seems > like that is not intended so I will file a bug. Added a TODO on this line. cmd_delete should not be enabled if there is no selection (you are checking that the command is enabled before trying to invoke it aren't you?)

Comment on attachment 765499 [details] [diff] [review] Part 5: Remove platform bindings for Cocoa (v2) >+ <handler event="keypress" key="c" modifiers="accel" command="cmd_copy"/> >+ <handler event="keypress" key="x" modifiers="accel" command="cmd_cut"/> >+ <handler event="keypress" key="v" modifiers="accel" command="cmd_paste"/> I see copy/cut/paste mentioned in your previous patch, do those not work?

(In reply to neil@parkwaycc.co.uk from comment #61) > (In reply to J. Ryan Stinnett from comment #57) > > (In reply to Masayuki Nakano from comment #47) > > > (From update of attachment 759446 [details] [diff] [review]) > > > >+ // SEL_TO_COMMAND(delete:, ); > > > > > > What's this? Deletes the content only when there is selection? If so, > > > "cmd_delete"? > > > > I was hoping that would work too, but "cmd_delete" currently doesn't verify > > that there is a selection before deleting. Look at the editor code, it seems > > like that is not intended so I will file a bug. Added a TODO on this line. > > cmd_delete should not be enabled if there is no selection (you are checking > that the command is enabled before trying to invoke it aren't you?) The code path that actually invokes the commands from nsINativeKeyBindings was added in bug #257405 (the GTK2 version), and I hadn't looked at that closely. But, as you are guessing, it currently does not check if the command is enabled for some reason. I'll look into changing that with an additional patch for this bug, and then "cmd_delete" can likely be added.

(In reply to neil@parkwaycc.co.uk from comment #62) > Comment on attachment 765499 [details] [diff] [review] > Part 5: Remove platform bindings for Cocoa (v2) > > >+ <handler event="keypress" key="c" modifiers="accel" command="cmd_copy"/> > >+ <handler event="keypress" key="x" modifiers="accel" command="cmd_cut"/> > >+ <handler event="keypress" key="v" modifiers="accel" command="cmd_paste"/> > I see copy/cut/paste mentioned in your previous patch, do those not work? I assume you are referring to the Part 4 patch that maps Cocoa selectors to Gecko commands? If a key event is mapped to the Cocoa selectors for cut / copy / paste, we will invoke the commands via the NativeKeyBindings code in Part 4. However, these handlers are still needed in platformHTMLBindings.xml because the system default keystrokes (which are the ones the above handler lines use) for cut / copy / paste don't trigger these Cocoa selectors. So for these actions, the NativeKeyBindings code is only effective if someone defines a custom key binding for the cut / copy / paste selectors.

(In reply to J. Ryan Stinnett [:jryans] from comment #59) > Created attachment 765505 [details] [diff] [review] > Part 6: Add Gecko key to Cocoa char conversion (v2) > > Carrying over masayuki's r+ from attachment #759540 [details]. > > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #49) > > Comment on attachment 759450 [details] [diff] [review] > > Part 6: Add Gecko key to Cocoa char conversion > > > > >+struct KeyConversionData { > > >+ const char* str; > > >+ size_t strLength; > > >+ uint32_t geckoKeyCode; > > >+ uint32_t charCode; > > >+}; > > > > Use this style: > > > > struct KeyConversionData > > { > > .. > > }; > > I have updated the struct style, but a search of the codebase with DXR > suggests > that a large number of structs have the opening brace on the same line as the > structs keyword. If your style is the correct one for use throughout the > codebase, should I update the Coding Style page > (https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style)? See here: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Control_Structures and here: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Classes Especially, the former's note defines it clearly: > Class and function definitions are not control structures; left brace goes by itself on the second line and without extra indentation, in general.

Comment on attachment 765507 [details] [diff] [review] Part 7: Simulate native events for testing (v2) Could you move the new method AttachNativeKeyEvent() to widget::TextInputHandlerBase which also implements SynthesizeNativeKeyEvent()? Otherwise, looks okay.

Carrying over masayuki's r+ from attachment #765507 [details] [diff] [review]. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #66) > Comment on attachment 765507 [details] [diff] [review] > Part 7: Simulate native events for testing (v2) > > Could you move the new method AttachNativeKeyEvent() to > widget::TextInputHandlerBase which also implements > SynthesizeNativeKeyEvent()? Done.

I forgot to update the tests after changing the paragraph mappings as part of attachment #765493 [details] [diff] [review], so this corrects that.

Comment on attachment 765755 [details] [diff] [review] Part 7: Simulate native events for testing (v3) >@@ -1467,16 +1467,22 @@ NS_IMETHODIMP nsChildView::DispatchEvent > #ifdef DEBUG > debug_DumpEvent(stdout, event->widget, event, nsAutoCString("something"), 0); > #endif > > NS_ASSERTION(!(mTextInputHandler && mTextInputHandler->IsIMEComposing() && > NS_IS_KEY_EVENT(event)), > "Any key events should not be fired during IME composing"); > >+ if (NS_IS_KEY_EVENT(event) && event->mFlags.mIsSynthesizedForTests) { >+ nsKeyEvent keyEvent = *reinterpret_cast<nsKeyEvent*>(event); >+ nsresult rv = mTextInputHandler->AttachNativeKeyEvent(keyEvent); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } Oops, nsChildView::DispatchEvent() is used for all events. So, this might sensitive for the performance. Therefore, NS_IS_KEY_EVENT() becomes false in most cases. However, NS_IS_KEY_EVENT() checks if the event message is one of key event message. This means that the cost of NS_IS_KEY_EVENT() is more expensive than checking mIsSynthesizedForTests. So, could you rewrite the if condition as following order?: if (event->mFlags.mIsSynthesizedForTests && NS_IS_KEY_EVENT(event)) { By this, non-synthesized events are only checked the flag, this cost is cheaper. I'm sorry for that I didn't realize this at writing the previous comment.

Carrying over masayuki's r+ from attachment #765507 [details] [diff] [review]. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #69) > Comment on attachment 765755 [details] [diff] [review] > Part 7: Simulate native events for testing (v3) > > >@@ -1467,16 +1467,22 @@ NS_IMETHODIMP nsChildView::DispatchEvent > > #ifdef DEBUG > > debug_DumpEvent(stdout, event->widget, event, nsAutoCString("something"), 0); > > #endif > > > > NS_ASSERTION(!(mTextInputHandler && mTextInputHandler->IsIMEComposing() && > > NS_IS_KEY_EVENT(event)), > > "Any key events should not be fired during IME composing"); > > > >+ if (NS_IS_KEY_EVENT(event) && event->mFlags.mIsSynthesizedForTests) { > >+ nsKeyEvent keyEvent = *reinterpret_cast<nsKeyEvent*>(event); > >+ nsresult rv = mTextInputHandler->AttachNativeKeyEvent(keyEvent); > >+ NS_ENSURE_SUCCESS(rv, rv); > >+ } > > > Oops, nsChildView::DispatchEvent() is used for all events. So, this might > sensitive for the performance. Therefore, NS_IS_KEY_EVENT() becomes false in > most cases. However, NS_IS_KEY_EVENT() checks if the event message is one of > key event message. This means that the cost of NS_IS_KEY_EVENT() is more > expensive than checking mIsSynthesizedForTests. > > So, could you rewrite the if condition as following order?: > > if (event->mFlags.mIsSynthesizedForTests && NS_IS_KEY_EVENT(event)) { > > By this, non-synthesized events are only checked the flag, this cost is > cheaper. > > I'm sorry for that I didn't realize this at writing the previous comment. It's okay, I had to update the patch for a pointer copy error anyway. :) I've made this change now.

This patch ensures the editor commands that nsINativeKeyBindings attempts to invoke are enabled.

This patch ensures the input / textarea commands that nsINativeKeyBindings attempts to invoke are enabled.

Now that parts 9 and 10 have been added to ensure the commands are enabled, we can safely add the mapping to cmd_delete.

Comment on attachment 765830 [details] [diff] [review] Part 9: Ensure editor commands are enabled Review of attachment 765830 [details] [diff] [review]: ----------------------------------------------------------------- I don't know this code well enough to review it. Maybe Neil does.

Comment on attachment 765834 [details] [diff] [review] Part 10: Ensure input commands are enabled Review of attachment 765834 [details] [diff] [review]: ----------------------------------------------------------------- Over to Neil

Comment on attachment 765499 [details] [diff] [review] Part 5: Remove platform bindings for Cocoa (v2) P.S. Thanks for fixing the delete issue.

Comment on attachment 759445 [details] [diff] [review] Part 3: Add key bindings recorder to nsCocoaUtils Looks fine to me. (Sorry for the long delay.)

Comment on attachment 765839 [details] [diff] [review] Part 4: Create NativeKeyBindings for Cocoa (v4) This basically looks fine to me. Though, since Masayuki knows a lot more about key handling than I do, I'm glad he also reviewed it. But I do have a couple of questions: + // This is functional, but Cocoa's version is direction-less in that + // selection direction is not determined until some future directed + // action is taken. What does this mean? Apple's doc on -[NSResponder selectLine:] says that it selects "all elements in the line or lines containing the selection or insertion point": Which seems to imply that the "selection direction" has nothing to do with it, and that the results don't depend on any future action. + aKeyEvent.mNativeKeyEvent = aNativeKeyEvent; nsNativeEvent.mNativeKeyEvent (which was actually added in Part 1) isn't retained or released -- which implies that it might get accessed when it's no longer valid. I *think* TextInputHandlerBase::mCurrentKeyEvents (in TextInputHandler.h) guarantees that it will be valid as long as we need it, but I'm not entirely sure. Masayuki, you wrote that code. What do you think? Even if we don't have to retain and release nsNativeEvent.mNativeKeyEvent, I think we should add a comment explaining why.

(In reply to Steven Michaud from comment #78) > Comment on attachment 765839 [details] [diff] [review] > Part 4: Create NativeKeyBindings for Cocoa (v4) > > But I do have a couple of questions: > > + // This is functional, but Cocoa's version is direction-less in that > + // selection direction is not determined until some future directed > + // action is taken. > > What does this mean? Apple's doc on -[NSResponder selectLine:] says that it > selects "all elements in the line or lines containing the selection or > insertion point": Which seems to imply that the "selection direction" has > nothing to do with it, and that the results don't depend on any future > action. You're correct that there doesn't seem to be anything documented. I used TextEdit as my reference for a typical Cocoa editor application. If I define a custom binding for selectLine, I can use that inside TextEdit, and in there I am able to expand my selection either up or down after invoking selectLine (perhaps using line motions like moveDownAndModifySelection / moveUpAndModifySelection, also known as Shift + Up and Shift + Down), and this second motion determines the "direction" of future selection modifications. Safari also displays this behavior in an editable field. Gecko didn't seem to have a way to select a line without also setting the "direction" at the same time, so that's what this comment is denoting.

(In reply to comment #79) You should probably write a fuller comment, along the lines of what you just said in comment #79. Or just write the gist of it and refer people to "bug 282097" for more information.

(In reply to Steven Michaud from comment #78) > Comment on attachment 765839 [details] [diff] [review] > Part 4: Create NativeKeyBindings for Cocoa (v4) > > This basically looks fine to me. Though, since Masayuki knows a lot more > about key handling than I do, I'm glad he also reviewed it. I've already done ;-) > + aKeyEvent.mNativeKeyEvent = aNativeKeyEvent; > > nsNativeEvent.mNativeKeyEvent (which was actually added in Part 1) isn't > retained or released -- which implies that it might get accessed when it's > no longer valid. I *think* TextInputHandlerBase::mCurrentKeyEvents (in > TextInputHandler.h) guarantees that it will be valid as long as we need it, > but I'm not entirely sure. > > Masayuki, you wrote that code. What do you think? > > Even if we don't have to retain and release nsNativeEvent.mNativeKeyEvent, I > think we should add a comment explaining why. It should be safe since the native key is used only while the event is being dispatched. This is performed synchronously. So, the native key must not be released.

Carrying over masayuki's r+ from attachment #759441 [details] [diff] [review], adding reviewer in commit message.

Carrying over masayuki's r+ from attachment #759442 [details] [diff] [review], adding reviewer in commit message.

Carrying over masayuki and smichaud's r+ from attachment #759445 [details] [diff] [review], adding reviewer in commit message.

Carrying over masayuki and smichaud's r+ from attachment #765839 [details] [diff] [review], adding reviewer in commit message. Updated and added comments in the patch to address smichaud's review in comment #78.

Carrying over neil's r+ from attachment #765499 [details] [diff] [review], adding reviewer in commit message.

Carrying over masayuki's r+ from attachment #759450 [details] [diff] [review], adding reviewer in commit message.

Carrying over masayuki's r+ from attachment #765507 [details] [diff] [review], adding reviewer in commit message.

Carrying over masayuki's r+ from attachment #759453 [details] [diff] [review], adding reviewer in commit message.

Carrying over enndeakin's r+ from attachment #765830 [details] [diff] [review], adding reviewer in commit message.

Carrying over enndeakin's r+ from attachment #765834 [details] [diff] [review], adding reviewer in commit message.

Try push looks green: https://tbpl.mozilla.org/?tree=Try&rev=2c57bd6c4023

https://hg.mozilla.org/integration/mozilla-inbound/rev/96749d774571 https://hg.mozilla.org/integration/mozilla-inbound/rev/431ec2a50165 https://hg.mozilla.org/integration/mozilla-inbound/rev/d4404aa64300 https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb128a0bcef https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb8320acc0c https://hg.mozilla.org/integration/mozilla-inbound/rev/65e885345668 https://hg.mozilla.org/integration/mozilla-inbound/rev/a00e83067235 https://hg.mozilla.org/integration/mozilla-inbound/rev/993613b44914 https://hg.mozilla.org/integration/mozilla-inbound/rev/116295f5b522 https://hg.mozilla.org/integration/mozilla-inbound/rev/0daef1c8ef64

https://hg.mozilla.org/mozilla-central/rev/96749d774571 https://hg.mozilla.org/mozilla-central/rev/431ec2a50165 https://hg.mozilla.org/mozilla-central/rev/d4404aa64300 https://hg.mozilla.org/mozilla-central/rev/4bb128a0bcef https://hg.mozilla.org/mozilla-central/rev/4bb8320acc0c https://hg.mozilla.org/mozilla-central/rev/65e885345668 https://hg.mozilla.org/mozilla-central/rev/a00e83067235 https://hg.mozilla.org/mozilla-central/rev/993613b44914 https://hg.mozilla.org/mozilla-central/rev/116295f5b522 https://hg.mozilla.org/mozilla-central/rev/0daef1c8ef64

I did some manual verification around this using the attached DefaultKeyBinding.dict. These commands don't seem to be working: "^\UF702" = "moveWordBackward:"; /* Control-Left arrow */ "^\UF703" = "moveWordForward:"; /* Control-Right arrow */ Control+arrow keys change Desktops instead. Any thoughts?

The system-wide shortcuts like those (that are configured in System Preferences -> Keyboard -> Keyboard Shortcuts) get the highest priority, so if you use something there, it's not available to regular applications and can't be used in DefaultKeyBinding.dict. You'll notice that TextEdit and other Cocoa application display the same behavior. If you disable the corresponding settings in the system Keyboard Shortcuts, then they will work in your applications to move between words as you've defined. So, this is expected behavior.

Verified fixed FF 25 beta based on comment 96.

Using Firefox 26 on OSX Mavericks I am seeing broken functionality when trying to map function keys (F7,F9,F10,F11,F12) to non-standard actions (Home,End,Page Down, Page Up, Delete Forward). The key mapping is made in ~/Library/KeyBindings/DefaultKeyBinding.dict and is confirmed working in TextEdit.

I'm sorry, I should have been more specific. The F7,F9, and F12 key remappings do seem to work when in the title bar, search bar, or text box area. However, F10 & F11 (page down/page up) do not work. F11 continues to make Firefox full screen and I'm not sure what F10 is doing, maybe nothing?

(In reply to Randy Syring from comment #100) > I'm sorry, I should have been more specific. The F7,F9, and F12 key > remappings do seem to work when in the title bar, search bar, or text box > area. However, F10 & F11 (page down/page up) do not work. F11 continues to > make Firefox full screen and I'm not sure what F10 is doing, maybe nothing? Hmm, it's possible specific changes are needed for those keys. Let's move this to a new bug. Can you file a new bug in Widget: Cocoa for this issue and attach your keybindings file? Please CC me as well.

Should this have fixed the issue in Thunderbird too? I'm on version 24.6.0 and it's still jumping to beginning/end of email instead of beginning/end of line. Firefox works as expected though.