If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove nsIPlaintextEditor::handleKeyPress()

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
Editor
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla1.9.3a5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

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().
Created attachment 445254 [details] [diff] [review]
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.
Attachment #445254 - Flags: review?(Olli.Pettay)

Comment 2

8 years ago
Would it be enough to add readonly attribute nsIDOMEventGroup systemEventGroup
to nsIEventListenerService?
Created attachment 445330 [details] [diff] [review]
Patch v2.0

Thanks, after I QIed nodes except document, I can use nsIDOM3EventTarget interface.
Attachment #445254 - Attachment is obsolete: true
Attachment #445330 - Flags: review?(Olli.Pettay)
Attachment #445254 - Flags: review?(Olli.Pettay)

Comment 4

8 years ago
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.
Created attachment 445367 [details] [diff] [review]
Patch v3.0

Testing on tryserver...
Attachment #445330 - Attachment is obsolete: true
Attachment #445367 - Flags: review?(Olli.Pettay)
Attachment #445330 - Flags: review?(Olli.Pettay)
(In reply to comment #5)
> Testing on tryserver...

there is no problem.

Comment 7

7 years ago
I'll try to review this tomorrow. The editor part of the patch
is a bit tricky to review.
(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

7 years ago
Sorry about the delay.
+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 on attachment 445367 [details] [diff] [review]
Patch v3.0

- for now.
Attachment #445367 - Flags: review?(Olli.Pettay) → review-
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 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
Attachment #445367 - Flags: review- → review+
(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.
I sent an email to Ms2ger, and asked why he made that change.
(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.
FYI, switch-case style has been reverted back to original in the coding style.
(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.
Created attachment 448671 [details] [diff] [review]
Patch v3.1

oops, this needs sr.
Attachment #448671 - Flags: superreview?(roc)
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?
(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.
(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!
Attachment #448671 - Flags: superreview?(roc) → superreview+
http://hg.mozilla.org/mozilla-central/rev/634e1f2cf46c
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
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...
(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);
Created attachment 448972 [details] [diff] [review]
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.
Attachment #448972 - Flags: review?(Olli.Pettay)
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
Depends on: 569988
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 570455
Created attachment 449584 [details] [diff] [review]
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.
Attachment #449584 - Flags: review?(ehsan)
Attachment #448972 - Flags: review?(Olli.Pettay)
I'll do my best to look at the patch today, but I can promise a review tomorrow.  Sorry about the delay.
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.
Attachment #449584 - Flags: review?(ehsan) → review+
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.)
I just saw that this patch landed without comment 32 addressed.  Could you please add the test later?
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.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
(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.
O.K., I'll post it (in bug 569988 is better?).
(In reply to comment #36)
> O.K., I'll post it (in bug 569988 is better?).

Sure, sounds great!  Thanks!

Updated

7 years ago
Depends on: 572969
You need to log in before you can comment on or make changes to this bug.