564669 - Remove nsIPlaintextEditor::handleKeyPress()

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

We should remove nsIPlaintextEditor::handleKeyPress() which is handler of keypress event in nsPlaintextEditor and nsHTMLEditor. It's called by only nsEditorEventListener.

Nobody doesn't call the method in our product and even if some add-ons is using this method, they should use systhesizing key events instead of this method because the editor of 1.9.3 and later is going to check whether it's focused (or activate in the DOM window) strictly, so, they shouldn't bypass the checking by this method.

And also the method doesn't handle all keys because nsEditorEventListener handles some keys before it calls nsIPlaintextEditor::handleKeyPress().

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

I compared the behavior with the new automated tests.

Basically, this patch doesn't change actual behavior except:

* ESC key events are consumed on nsPlaintextEditor too.
* Tab key event can insert \t to nsPlaintextEditor too if it's not tabbable. (This is not actually changed the <input> and <textarea> behavior.)

For the tests, I add nsIDOMWindowUtils::(Add|Remove)EventListenerInSystemGroup(). First, I tried to use nsIDOM3EventTarget::AddGroupedEventListener() but I cannot use it from js. If it's usable, nsIDOMWindowUtils::GetSystemEventGroup() is better.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Would it be enough to add readonly attribute nsIDOMEventGroup systemEventGroup
to nsIEventListenerService?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

Thanks, after I QIed nodes except document, I can use nsIDOM3EventTarget interface.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 445330 [details] [diff] [review]
Patch v2.0

>+
>+  /**
>+   * Returns system event group for the event target.
>+   */
>+  nsIDOMEventGroup getSystemEventGroup(in nsIDOMEventTarget aEventTarget);

I think this could be just a readonly attribute

>+NS_IMETHODIMP
>+nsEventListenerService::GetSystemEventGroup(nsIDOMEventTarget* aEventTarget,
>+                                            nsIDOMEventGroup** aSystemGroup)
>+{
>+  NS_ENSURE_ARG_POINTER(aEventTarget);
>+  NS_ENSURE_ARG_POINTER(aSystemGroup);
>+
>+  PRBool hasCap = PR_FALSE;
>+  nsresult rv = nsContentUtils::GetSecurityManager()->
>+                  IsCapabilityEnabled("UniversalXPConnect", &hasCap);
No need for this, since content shouldn't ever get access to this service.


>+  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ENSURE_TRUE(hasCap, NS_ERROR_DOM_SECURITY_ERR);
>+
>+  nsCOMPtr<nsPIDOMEventTarget> piTarget = do_QueryInterface(aEventTarget);
>+  NS_ENSURE_TRUE(piTarget, NS_ERROR_FAILURE);
>+  piTarget->GetSystemEventGroup(aSystemGroup);
>+  return *aSystemGroup ? NS_OK : NS_ERROR_FAILURE;
>+}
Could you perhaps make some static method to nsEventListenerManager to get
the system event group. Then there was no need for the aEventTarget parameter.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.0

Testing on tryserver...

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #5)
> Testing on tryserver...

there is no problem.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I'll try to review this tomorrow. The editor part of the patch
is a bit tricky to review.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #7)
> I'll try to review this tomorrow. The editor part of the patch
> is a bit tricky to review.

smaug?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Sorry about the delay.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

+nsIDOMEventGroup*
+nsEventListenerManager::GetSystemEventGroup()
+{
+  if (gSystemEventGroup) {
+    return gSystemEventGroup;
+  }
+
+  nsCOMPtr<nsIDOMEventGroup> group(do_CreateInstance(kDOMEventGroupCID));
+  NS_ENSURE_TRUE(group, nsnull);
+  NS_ADDREF(gSystemEventGroup = group.get());

nsCOMPtr<nsIDOMEventGroup> group(do_CreateInstance(kDOMEventGroupCID));
gSystemEventGroup = group.forget.get();
return gSystemEventGroup;

diff --git a/content/events/src/nsEventListenerManager.h b/content/events/src/nsEventListenerManager.h
--- a/content/events/src/nsEventListenerManager.h
+++ b/content/events/src/nsEventListenerManager.h
@@ -42,23 +42,26 @@
 #include "jsapi.h"
 #include "nsCOMPtr.h"
 #include "nsIDOMEventTarget.h"
 #include "nsIDOM3EventTarget.h"
 #include "nsHashtable.h"
 #include "nsIScriptContext.h"
 #include "nsCycleCollectionParticipant.h"
 #include "nsTObserverArray.h"
+#include "nsGUIEvent.h"
 
 class nsIDOMEvent;
 class nsIAtom;
 class nsIWidget;
 struct nsPoint;
 struct EventTypeData;
 class nsEventTargetChainItem;
+class nsPIDOMWindow;
+

Why you need to include "nsGUIEvent.h"?
And why you need "class nsPIDOMWindow;"


@@ -5025,16 +5026,71 @@ nsresult
 nsEditor::RemoveAttributeOrEquivalent(nsIDOMElement * aElement,
                                       const nsAString & aAttribute,
                                       PRBool aSuppressTransaction)
 {
   return RemoveAttribute(aElement, aAttribute);
 }
 
 nsresult
+nsEditor::HandleKeyPressEvent(nsIDOMKeyEvent* aKeyEvent)
+{
+  // NOTE: When you change this method, you should also change:
+  //   * editor/libeditor/text/tests/test_texteditor_keyevent_handling.html
+  //   * editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html
+  //
+  // And also when you add new key handling, you need to change the subclass's
+  // HandleKeyPressEvent()'s switch statement.
+
+  nsKeyEvent* nativeKeyEvent = GetNativeKeyEvent(aKeyEvent);
+  NS_ENSURE_TRUE(nativeKeyEvent, NS_ERROR_UNEXPECTED);
+  NS_ASSERTION(nativeKeyEvent->message == NS_KEY_PRESS,
+               "HandleKeyPressEvent gets non-keypress event");
+
+  // if we are readonly or disabled, then do nothing.
+  if (IsReadonly() || IsDisabled()) {
+    // consume backspace for disabled and readonly textfields, to prevent
+    // back in history, which could be confusing to users
+    if (nativeKeyEvent->keyCode == nsIDOMKeyEvent::DOM_VK_BACK_SPACE) {
+      aKeyEvent->PreventDefault();
+    }
+    return NS_OK;
+  }
+
+  switch (nativeKeyEvent->keyCode) {
+  case nsIDOMKeyEvent::DOM_VK_META:
+  case nsIDOMKeyEvent::DOM_VK_SHIFT:
+  case nsIDOMKeyEvent::DOM_VK_CONTROL:
+  case nsIDOMKeyEvent::DOM_VK_ALT:
+    aKeyEvent->PreventDefault(); // consumed
+    return NS_OK;
+  case nsIDOMKeyEvent::DOM_VK_BACK_SPACE:
+    if (nativeKeyEvent->isControl || nativeKeyEvent->isAlt ||
+        nativeKeyEvent->isMeta) {
+      return NS_OK;
+    }
+    DeleteSelection(nsIEditor::ePrevious);
+    aKeyEvent->PreventDefault(); // consumed
+    return NS_OK;
+  case nsIDOMKeyEvent::DOM_VK_DELETE:
+    // on certain platforms (such as windows) the shift key
+    // modifies what delete does (cmd_cut in this case).
+    // bailing here to allow the keybindings to do the cut.
+    if (nativeKeyEvent->isShift || nativeKeyEvent->isControl ||
+        nativeKeyEvent->isAlt || nativeKeyEvent->isMeta) {
+      return NS_OK;
+    }
+    DeleteSelection(nsIEditor::eNext);
+    aKeyEvent->PreventDefault(); // consumed
+    return NS_OK; 
+  }
Nit, switch-case indentation is wrong.
switch () : {
  case ...:
  case ...:
}

diff --git a/editor/libeditor/html/nsHTMLEditor.cpp b/editor/libeditor/html/nsHTMLEditor.cpp
...
+nsresult
+nsHTMLEditor::HandleKeyPressEvent(nsIDOMKeyEvent* aKeyEvent)
+{
+  // NOTE: When you change this method, you should also change:
+  //   * editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html
+
+  if (IsReadonly() || IsDisabled()) {
+    // When we're not editable, the events handled on nsEditor, so, we can
+    // bypass nsPlaintextEditor.
..., the events are handled in nsEditor ...

+    return nsEditor::HandleKeyPressEvent(aKeyEvent);
+  }
+
+  nsKeyEvent* nativeKeyEvent = GetNativeKeyEvent(aKeyEvent);
+  NS_ENSURE_TRUE(nativeKeyEvent, NS_ERROR_UNEXPECTED);
+  NS_ASSERTION(nativeKeyEvent->message == NS_KEY_PRESS,
+               "HandleKeyPressEvent gets non-keypress event");
+
+  switch (nativeKeyEvent->keyCode) {
+  case nsIDOMKeyEvent::DOM_VK_META:
+  case nsIDOMKeyEvent::DOM_VK_SHIFT:
+  case nsIDOMKeyEvent::DOM_VK_CONTROL:
+  case nsIDOMKeyEvent::DOM_VK_ALT:
+  case nsIDOMKeyEvent::DOM_VK_BACK_SPACE:
+  case nsIDOMKeyEvent::DOM_VK_DELETE:
+    // These keys are handled on nsEditor, so, we can bypass nsPlaintextEditor.
+    return nsEditor::HandleKeyPressEvent(aKeyEvent);
+  case nsIDOMKeyEvent::DOM_VK_ESCAPE:
+    // This key is handled on nsPlaintextEditor.
+    return nsPlaintextEditor::HandleKeyPressEvent(aKeyEvent);
+  case nsIDOMKeyEvent::DOM_VK_TAB: {
+    if (IsPlaintextEditor()) {
+      // If this works as plain text editor, e.g., mail editor for plain
+      // text, should be handled on nsPlaintextEditor.
+      return nsPlaintextEditor::HandleKeyPressEvent(aKeyEvent);
+    }
+
+    if (IsTabbable()) {
+      return NS_OK; // let it be used for focus switching
+    }
+
+    if (nativeKeyEvent->isControl || nativeKeyEvent->isAlt ||
+        nativeKeyEvent->isMeta) {
+      return NS_OK;
+    }
+
+    nsCOMPtr<nsISelection> selection;
+    nsresult rv = GetSelection(getter_AddRefs(selection));
+    NS_ENSURE_SUCCESS(rv, rv);
+    PRInt32 offset;
+    nsCOMPtr<nsIDOMNode> node, blockParent;
+    rv = GetStartNodeAndOffset(selection, address_of(node), &offset);
+    NS_ENSURE_SUCCESS(rv, rv);
+    NS_ENSURE_TRUE(node, NS_ERROR_FAILURE);
+
+    PRBool isBlock = PR_FALSE;
+    NodeIsBlock(node, &isBlock);
+    if (isBlock) {
+      blockParent = node;
+    } else {
+      blockParent = GetBlockNodeParent(node);
+    }
+
+    if (!blockParent) {
+      break;
+    }
+
+    PRBool handled = PR_FALSE;
+    if (nsHTMLEditUtils::IsTableElement(blockParent)) {
+      rv = TabInTable(nativeKeyEvent->isShift, &handled);
+      if (handled) {
+        ScrollSelectionIntoView(PR_FALSE);
+      }
+    } else if (nsHTMLEditUtils::IsListItem(blockParent)) {
+      nsAutoString indentstr;
+      if (nativeKeyEvent->isShift) {
+        indentstr.AssignLiteral("outdent");
+      } else {
+        indentstr.AssignLiteral("indent");
+      }
+      rv = Indent(indentstr);
+      handled = PR_TRUE;
+    }
+    NS_ENSURE_SUCCESS(rv, rv);
+    if (handled) {
+      return aKeyEvent->PreventDefault(); // consumed
+    }
+    if (nativeKeyEvent->isShift) {
+      return NS_OK; // don't type text for shift tabs
+    }
+    aKeyEvent->PreventDefault();
+    nsAutoString str(PRUnichar('\t'));
+    return TypedText(str, eTypedText);
Could you just pass NS_LITERAL_STRING("\t") as parameter?


+  }
+  case nsIDOMKeyEvent::DOM_VK_RETURN:
+  case nsIDOMKeyEvent::DOM_VK_ENTER:
+    if (nativeKeyEvent->isControl || nativeKeyEvent->isAlt ||
+        nativeKeyEvent->isMeta) {
+      return NS_OK;
+    }
Here you actually change the behavior (well, depends on TypedText implementation). Why?
Pending review until this is answered.

diff --git a/editor/libeditor/text/nsPlaintextEditor.cpp b/editor/libeditor/text/nsPlaintextEditor.cpp
--- a/editor/libeditor/text/nsPlaintextEditor.cpp
+++ b/editor/libeditor/text/nsPlaintextEditor.cpp
@@ -344,59 +344,93 @@ nsPlaintextEditor::GetIsDocumentEditable
   return NS_OK;
 }
 
 PRBool nsPlaintextEditor::IsModifiable()
 {
   return !IsReadonly();
 }
 
+nsresult
+nsPlaintextEditor::HandleKeyPressEvent(nsIDOMKeyEvent* aKeyEvent)
+{
+  // NOTE: When you change this method, you should also change:
+  //   * editor/libeditor/text/tests/test_texteditor_keyevent_handling.html
+  //   * editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html
+  //
+  // And also when you add new key handling, you need to change the subclass's
+  // HandleKeyPressEvent()'s switch statement.
+
+  if (IsReadonly() || IsDisabled()) {
+    // When we're not editable, the events handled on nsEditor.
+    return nsEditor::HandleKeyPressEvent(aKeyEvent);
+  }
+
+  nsKeyEvent* nativeKeyEvent = GetNativeKeyEvent(aKeyEvent);
+  NS_ENSURE_TRUE(nativeKeyEvent, NS_ERROR_UNEXPECTED);
+  NS_ASSERTION(nativeKeyEvent->message == NS_KEY_PRESS,
+               "HandleKeyPressEvent gets non-keypress event");
+
+  switch (nativeKeyEvent->keyCode) {
+  case nsIDOMKeyEvent::DOM_VK_META:
+  case nsIDOMKeyEvent::DOM_VK_SHIFT:
+  case nsIDOMKeyEvent::DOM_VK_CONTROL:
+  case nsIDOMKeyEvent::DOM_VK_ALT:
+  case nsIDOMKeyEvent::DOM_VK_BACK_SPACE:
+  case nsIDOMKeyEvent::DOM_VK_DELETE:
+    // These keys are handled on nsEditor
+    return nsEditor::HandleKeyPressEvent(aKeyEvent);
+  case nsIDOMKeyEvent::DOM_VK_TAB: {
+    if (IsTabbable()) {
+      return NS_OK; // let it be used for focus switching
+    }
+
+    if (nativeKeyEvent->isShift || nativeKeyEvent->isControl ||
+        nativeKeyEvent->isAlt || nativeKeyEvent->isMeta) {
+      return NS_OK;
+    }
+
+    // else we insert the tab straight through
+    aKeyEvent->PreventDefault();
+    nsAutoString str(PRUnichar('\t'));
+    return TypedText(str, eTypedText);
Use NS_LITERAL_STRING

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 445367 [details] [diff] [review]
Patch v3.0

- for now.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, smaug.

(In reply to comment #10)
> Nit, switch-case indentation is wrong.
> switch () : {
>   case ...:
>   case ...:
> }

No, the current coding style document defines so...
https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Control_Structures

I agree it's strange style, I feel so too...

> diff --git a/editor/libeditor/html/nsHTMLEditor.cpp
> b/editor/libeditor/html/nsHTMLEditor.cpp
> +  }
> +  case nsIDOMKeyEvent::DOM_VK_RETURN:
> +  case nsIDOMKeyEvent::DOM_VK_ENTER:
> +    if (nativeKeyEvent->isControl || nativeKeyEvent->isAlt ||
> +        nativeKeyEvent->isMeta) {
> +      return NS_OK;
> +    }
> Here you actually change the behavior (well, depends on TypedText
> implementation). Why?

No, I didn't check the behavior. nsEditorEventListener::KeyPress() is checking the modifier key pressed state on current trunk.
# I want to clean up such layered key event handlers by this patch.

> -      case nsIDOMKeyEvent::DOM_VK_RETURN:
> -      case nsIDOMKeyEvent::DOM_VK_ENTER:
> -        if (isAnyModifierKeyButShift)
> -          return NS_OK;
> -
> -        if (!mEditor->IsSingleLineEditor())
> -        {
> -          textEditor->HandleKeyPress(keyEvent);

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 445367 [details] [diff] [review]
Patch v3.0

Ok.

I wonder if MDC's coding style is actually wrong - the old style is here http://www-archive.mozilla.org/hacking/mozilla-style-guide.html

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #13)
> (From update of attachment 445367 [details] [diff] [review])
> Ok.

Thank you, very much.

> I wonder if MDC's coding style is actually wrong - the old style is here
> http://www-archive.mozilla.org/hacking/mozilla-style-guide.html

Um... The case indent is modified at latest change by Ms2ger.
https://developer.mozilla.org/index.php?title=En/Developer_Guide/Coding_Style&action=diff&revision=43&diff=44
It seems that it was changed intentionally, not accidentally.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

I sent an email to Ms2ger, and asked why he made that change.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #10)
> diff --git a/content/events/src/nsEventListenerManager.h
> b/content/events/src/nsEventListenerManager.h
> --- a/content/events/src/nsEventListenerManager.h
> +++ b/content/events/src/nsEventListenerManager.h
> @@ -42,23 +42,26 @@
>  #include "jsapi.h"
>  #include "nsCOMPtr.h"
>  #include "nsIDOMEventTarget.h"
>  #include "nsIDOM3EventTarget.h"
>  #include "nsHashtable.h"
>  #include "nsIScriptContext.h"
>  #include "nsCycleCollectionParticipant.h"
>  #include "nsTObserverArray.h"
> +#include "nsGUIEvent.h"
> 
>  class nsIDOMEvent;
>  class nsIAtom;
>  class nsIWidget;
>  struct nsPoint;
>  struct EventTypeData;
>  class nsEventTargetChainItem;
> +class nsPIDOMWindow;
> +
> 
> Why you need to include "nsGUIEvent.h"?
> And why you need "class nsPIDOMWindow;"

The nsEventListenerManager.h depends on the nsGUIEvent.h and needs nsPIDOMWindow forward declaration. The other including files include nsGUIEvent.h and nsPIDOMWindow.h before including nsEventListenerManager.h.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

FYI, switch-case style has been reverted back to original in the coding style.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #17)
> FYI, switch-case style has been reverted back to original in the coding style.

O.K. I'll update the indent before I'll land it. Thank you.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.1

oops, this needs sr.

Comment 20

[(no longer active)]

This patch looks great, thanks Masayuki!

One question.  This is not supposed to change the key handling behavior for plaintext or HTML editors, right?  Have you tried running the unit tests that this patch adds with the current tip to make sure that we're actually preserving the behavior?

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #20)
> One question.  This is not supposed to change the key handling behavior for
> plaintext or HTML editors, right?  Have you tried running the unit tests that
> this patch adds with the current tip to make sure that we're actually
> preserving the behavior?

Basically, Yes. But I changed ESC key behavior on nsPlaintextEditor. nsPlaintextEditor doesn't eat ESC key events. This is wrong behavior, on nsHTMLEditor, of course, it's eating the events. I think that this is a bug of nsPlaintextEditor. If a XUL application has ESC related shortcut key, it also works on input/textarea elements even if the editor was edited by the input.

Comment 22

[(no longer active)]

(In reply to comment #21)
> (In reply to comment #20)
> > One question.  This is not supposed to change the key handling behavior for
> > plaintext or HTML editors, right?  Have you tried running the unit tests that
> > this patch adds with the current tip to make sure that we're actually
> > preserving the behavior?
> 
> Basically, Yes. But I changed ESC key behavior on nsPlaintextEditor.
> nsPlaintextEditor doesn't eat ESC key events. This is wrong behavior, on
> nsHTMLEditor, of course, it's eating the events. I think that this is a bug of
> nsPlaintextEditor. If a XUL application has ESC related shortcut key, it also
> works on input/textarea elements even if the editor was edited by the input.

Makes sense.  Thanks!

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/634e1f2cf46c

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/ae1773250d39

I disabled the new test for nsPlaintextEditor because it was failed on some tinderbox machines. On Gtk2, we honor the native key combination, so, the combination depend on the system. But they are different between tryserver's and m-c's. So, we cannot pass all tests on either machines...

Comment 25

[[e-mail]]

(In reply to comment #10)
> nsCOMPtr<nsIDOMEventGroup> group(do_CreateInstance(kDOMEventGroupCID));
> gSystemEventGroup = group.forget.get();
> return gSystemEventGroup;

Can use group.forget(&gSystemEventGroup);

Or CallCreateInstance(kDOMEventGroupCID, &gSystemEventGroup);

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Followup patch v1.0

* updates the code which was pointed by comment 25.
* adds a pref which can disable the native key event handler for the tests, the performance shouldn't be matter.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 448972 [details] [diff] [review]
Followup patch v1.0


>diff -r c74f10481e3a modules/libpref/src/init/all.js
>--- a/modules/libpref/src/init/all.js	Wed Jun 02 17:05:49 2010 -0700
>+++ b/modules/libpref/src/init/all.js	Thu Jun 03 18:42:37 2010 +0900
>@@ -842,16 +842,19 @@ pref("network.negotiate-auth.delegation-
> pref("network.negotiate-auth.allow-proxies", true);
> 
> // Path to a specific gssapi library
> pref("network.negotiate-auth.gsslib", "");
> 
> // Specify if the gss lib comes standard with the OS
> pref("network.negotiate-auth.using-native-gsslib", true);
> 
>+// Enable/Disable native key bindings
>+pref("ui.nativeKeyBindings.enabled", true);
So you really need to set this value to all.js?
Couldn't 'true' be the default for the pref and
tests just set it to false.

> PRBool
> nsNativeKeyBindings::KeyPress(const nsNativeKeyEvent& aEvent,
>                               DoCommandCallback aCallback, void *aCallbackData)
> {
>+  nsCOMPtr<nsIPrefService> prefService =
>+    do_GetService(NS_PREFSERVICE_CONTRACTID);
>+  nsCOMPtr<nsIPrefBranch> prefBranch = do_QueryInterface(prefService);
>+  if (prefBranch) {
>+    PRBool enabled;
>+    nsresult rv =
>+      prefBranch->GetBoolPref("ui.nativeKeyBindings.enabled", &enabled);
>+    if (NS_SUCCEEDED(rv) && !enabled) {
>+      return PR_FALSE;
>+    }
>+  }
>+
Any chance to cache prefService in nsNativeKeyBindings?
Or even better, could something in widget/ provide
prefService. Something similar to nsContentUtils::GetBoolPref

Comment 28

[(no longer active)]

I backed out the patch for this bug, as it breaks the Esc key in XUL dialogs.  For more details, please see bug 569988.

http://hg.mozilla.org/mozilla-central/rev/e5f1739ad7f1
http://hg.mozilla.org/mozilla-central/rev/b477fae76acc

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.2

I removed the PreventDefault() from ESC key handler of nsPlaintextEditor (nsHTMLEditor still does it, it's current behavior).

And also this has the change of comment 2.

Comment 30

[(no longer active)]

I'll do my best to look at the patch today, but I can promise a review tomorrow.  Sorry about the delay.

Comment 31

[(no longer active)]

Comment on attachment 449584 [details] [diff] [review]
Patch v3.2

The interdiff <https://bugzilla.mozilla.org/attachment.cgi?oldid=448671&action=interdiff&newid=449584&headers=1> looks good to me, but I'm still not sure why we need to consume the Esc key event for the HTML editor.  But we can leave it to another bug I think.

Comment 32

[(no longer active)]

Also, could you please add a test to make sure that the Esc key event is not consumed by the plaintext editor?  A simple way to test that is calling prompt, catch the domwindowopened notification, synthesizing Esc, and making sure that the dialog is closed (and we really should have that test anyways.)

Comment 33

[(no longer active)]

I just saw that this patch landed without comment 32 addressed.  Could you please add the test later?

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/145c54158531

(In reply to comment #32)
> Also, could you please add a test to make sure that the Esc key event is not
> consumed by the plaintext editor?  A simple way to test that is calling prompt,
> catch the domwindowopened notification, synthesizing Esc, and making sure that
> the dialog is closed (and we really should have that test anyways.)

Aren't the tests in the patch enough?

# I'll create the followup patch.

Comment 35

[(no longer active)]

(In reply to comment #34)
> (In reply to comment #32)
> > Also, could you please add a test to make sure that the Esc key event is not
> > consumed by the plaintext editor?  A simple way to test that is calling prompt,
> > catch the domwindowopened notification, synthesizing Esc, and making sure that
> > the dialog is closed (and we really should have that test anyways.)
> 
> Aren't the tests in the patch enough?

No.  They didn't catch the original regression, and that's a pretty important thing to test.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

O.K., I'll post it (in bug 569988 is better?).

Comment 37

[(no longer active)]

(In reply to comment #36)
> O.K., I'll post it (in bug 569988 is better?).

Sure, sounds great!  Thanks!