396832 - Editor accepts Esc as a character (looks like a space)

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Jesse Ruderman]

Steps to reproduce:
1. javascript:void prompt("title","default")
2. Press Esc.

Result: the text "default" is replaced by what looks like a space as the prompt sheet rolls up into the titlebar.

Expected: the text should not change.


Or:
1. Open the popup exception window.
2. Press Esc.

Result: it looks like you just pressed space.

Expected: nothing should happen, or the window should close (arguable).

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

FWIW, I can't reproduce this on Windows.

Comment 2

[Jesse Ruderman]

I think this appeared around the same time as bug 385292, so maybe it needs a similar fix.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Chris, can you take this on Mac?

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Taking bug.

When you press ESC on Windows, nsEditorEventListeners::KeyPress() which calls nsPlaintextEditor::HandleKeyPress() with the key code DOM_VK_ESCAPE. However on the Mac, we get the first call with ESC key code, but we also have a second call to nsPlaintextEditor::HandleKeyPress(), with key code 0. This results in a string of length 1 with contents "\0" being set as the value of the text edit in the popup alert/dialog.

So for some reason we're getting a second event on the Mac, possibly with the event zeroed out?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Stephen, Josh, see comment #4.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

PresShell::HandleEvent() is being called twice with aEvent={NS_KEY_EVENT,NS_KEY_PRESS}. The first time it's called, it returns with (*aEventStatus)==0, the second time with (*aEventStatus)==1. So presumably the first time its failing, and then some default handler gets called which calls it again somehow slightly differently, resulting it not failing.

Comment 7

[Josh Aas]

I'm guessing this has the same cause as bug 401425.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

The problem here is that nsChildView::keyDown() is calling DispatchWindowEvent() with a key press event here: http://mxr.mozilla.org/seamonkey/source/widget/src/cocoa/nsChildView.mm#3985 and it's also calling [super interpretKeyEvents:[NSArray arrayWithObject:theEvent]] here: http://mxr.mozilla.org/seamonkey/source/widget/src/cocoa/nsChildView.mm#3997 which is also calling DispatchWindowEvent() with a key press event.

There are two ways to fix this.

1. Do not call interpretKeyEvents() when dispatchedKeyPress is true. Note that it can only be true when !nsTSMManager::IsComposing(). Still, I'm not sure if this is a safe approach, but it does fix the bug.

2. Apply Masayuki's patch for bug 401425 and modify it so that kEscapeKeyCode is HandledByCommand(), which prevents the first DispatchWindowEvent() call. Unfortunately MacOs seems to be doing something stupid. interpretKeyEvents() is calling convertCocoaKeyEvent:toGeckoEvent() with  aKeyEvent which has keyCode 0, but which has [aKeyEvent charactersIgnoringModifiers:0]==27 (the ASCII escape char). The usual value for [aKeyEvent keyCode] when escape is pressed is 53, and for keyCode==53, [aKeyEvent charactersIgnoringModifiers:0] == 27, so it's not entirely wrong. It is a mystery as to how the keyCode gets zeroed out though. So we need to take that into account as well.

I'll post my patches so that makes sense...

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: [Fix1] Don't call interpretKeyEvents()

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: [Fix2] (Masayuki's patch)++

Comment 11

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

The first patch looks nicer. But I want to test it with IME. However, I don't have much time today.

Comment 12

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

attachment 287943 [details] [diff] [review] works fine with IME, we should use this approach.

Comment 13

[Josh Aas]

Chris - do you have commit privs? If not I'll check it in for you, pretty important to get this in quickly. I have added it to the checkin queue for me to do already, but I can change that if you get back to me before my turn comes up.

Comment 14

[Josh Aas]

landed on trunk