1403759 - [macOS] Backspace key should remove composing Hangul character rather than committing it

Status: RESOLVED FIXED

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

Description

[chanil1218]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170925150345

Steps to reproduce:

* Cmd+L to locate search field
* Enter Korean character(ㄱ)
* Click delete


Actual results:

Instead of delete chacter quickly, just remove focus. 


Expected results:

Delete focused character.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Masayuki, is this a dupe of bug 1400189?

Comment 2

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

Perhaps, yes. But I'm not sure about "just remove focus" since I don't see any reaction when I press delete key in URL bar.

Comment 3

[chanil1218]

I mean "just remove focus" is that underline shown below "ㄱ" disappeared instead of removing "ㄱ". 
I'm using Mac FYI.

Comment 4

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

Thanks. Do you still reproduce this bug with Beta 57 b5 or Nightly?

Comment 5

[chanil1218]

Attachment: Firefox search bar bug.mov

I pressed delete button firstly and later pressed delete button again. 
I tested this on 57.0b6
In Chrome deletion happens in single delete press.

Comment 6

[Marco Bonardo [:mak]]

masayuki-san, what's the next step here?

Comment 7

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

Please keep waiting. I'm working on some regression bugs related to nsINode::GetChildAt(). I'll be back here when I get a chance.

Comment 9

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

This is another case of bug 1358958. Korean IME started to commit composition and send deleteBackward command when user presses Backspace key during composition.

Comment 10

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f73f4a28c08cb3fcc2f74e257d6564d7efd5553

Comment 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fece34586dec7ac4f24202572f890c7b029161ef

Comment 12

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

Attachment: Bug 1403759 - part 1: Make TextInputHandler::InsertNewline() treat other commands

Currently, we handle insertNewline: of NSResponder with
TextInputHandler::InsertNewline().  However, its
implementation is useful for handling some other
commands.  So, let's rename it to HandleCommand() and
make it take Command as its argument for handling
specific behavior of each command.

Review commit: https://reviewboard.mozilla.org/r/204796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204796/

Comment 13

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

Attachment: Bug 1403759 - part 2: Handle edit/selection commands like insertNewline: in TextInputHandler::HandleCommand()

Let's make TextInputHandler::HandleCommand() handle other
commands which are caused by Backspace, Delete, Tab, ArrowUp,
ArrowDown, ArrowRight, ArrowLeft, PageUp, PageDown, Home, End
and Escape keys with various modifiers.

This patch makes Korean users can do most key operation in
editor even with composing Hangul character.

Note that this patch has a hack for cancelOperation: command.
The command is typically fired for Escape key press.  However,
it's also fired for Command + Period.  Unfortunately, this
behavior is really odd if subclass of NSResponder implements
|void cancelOperation:(id)sender|.  If it's implemented,
Cocoa doesn't call its |void keyDown:(NSEvent)theEvent|.
Instead, it calls only |void doCommandBySelector:(SEL)aSelector|
and |void cancelOperation:(id)sender| when Command + Period is
pressed.  Therefore, we cannot dispatch keydown nor keypress
event for this key combination if we implement it.  Therefore,
this patch doesn't implement the method but handle it in
doCommandBySelector even though the super class of ChildView
cannot handle the command with this path.

Review commit: https://reviewboard.mozilla.org/r/204798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204798/

Comment 14

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

While composing Korean IME, other control keys like arrow keys causes committing composing Hangul character and should work as usual.  However, on Gecko, they cause only committing composing Hangul character because Gecko ignores following commands came from Korean IME.

So, the patches make ChildView and TextInputHandler handle such commands as usual.  Note that, there are some other commands which are not included in the patches, but our editor doesn't support the commands even with different key combination.  So, the commands included in the patches are enough for current implementation.

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 8934056 [details]
Bug 1403759 - part 1: Make TextInputHandler::InsertNewline() treat other commands

https://reviewboard.mozilla.org/r/204796/#review210832

Comment 16

[Makoto Kato [:m_kato]]

Comment on attachment 8934057 [details]
Bug 1403759 - part 2: Handle edit/selection commands like insertNewline: in TextInputHandler::HandleCommand()

https://reviewboard.mozilla.org/r/204798/#review210844

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5039cf4ead7
part 1: Make TextInputHandler::InsertNewline() treat other commands r=m_kato
https://hg.mozilla.org/integration/autoland/rev/afc17c7465c2
part 2: Handle edit/selection commands like insertNewline: in TextInputHandler::HandleCommand() r=m_kato

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/f5039cf4ead7
https://hg.mozilla.org/mozilla-central/rev/afc17c7465c2