1743346 - `TextInputListener::HandleEvent` should refer native key bindings, then, refer our shortcut key definition

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

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

This is opposite from HTMLEditor. If HTMLEditor, we refer native key bindings in keypress event listener of the Document node.
https://searchfox.org/mozilla-central/rev/70b32246fce5ca1f53af573a21c1939df58cb969/editor/libeditor/EditorEventListener.cpp#641-644

Then, the event will be handled later by the global key listeners:
https://searchfox.org/mozilla-central/rev/70b32246fce5ca1f53af573a21c1939df58cb969/dom/events/GlobalKeyListener.cpp#163-164

However, TextInputListener refers opposite order:
https://searchfox.org/mozilla-central/rev/70b32246fce5ca1f53af573a21c1939df58cb969/dom/html/TextControlState.cpp#970-972,976,989,1019

I think that native key bindings should be respected than our definition.

Comment 1

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

Ctrl-a works as "Move caret to beginning of a line" in data:text/html,<div contenteditable>abc<br>def</div>?

(copy the data URI, and paste it into the URL bar, then, test Ctrl-a)

Comment 2

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

And could you test this patched build?
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/VTalBvYdTqKdnlS6m-7osQ/runs/0/artifacts/public/build/target.tar.bz2

This includes experimental patches for this bug and bug 1743339.
https://hg.mozilla.org/try/rev/146bbd2dc42229fdb6f93678626c9e5b8876364c
https://hg.mozilla.org/try/rev/c9c3d410f8e9a449d27f33a1c068ba79a11c1f74

Comment 3

[lilydjwg]

It works with that data URI. (But that's the least important one: contenteditable editors usually have their set of key bindings. The most use case I use for Ctrl-a is in the urlbar with which I add a keyword to specify a search site.)

With the try build:

Ctrl-a works, but Ctrl-p crashes it every time (in a <textarea>; and Ctrl-n crashes twice out of three tries, the other one opens a new window). Here's a report: https://crash-stats.mozilla.org/report/index/998f0a88-1d46-4cbf-84d1-a9f570211129. I have Ctrl-p and Ctrl-n setup in GTK's stylesheet to move cursor up and down respectively.

Alt-a (which has been setup to select all for GtkTextView) in <input> and <textarea> also crashes: https://crash-stats.mozilla.org/report/index/47bbbdac-ca5e-4ce1-8e35-50add0211129

Comment 4

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

•  if (aCommands.Length() == 1u &&
  

•      aCommands[1u] == static_cast<CommandInt>(Command::SelectAll)) {
  

Oh, my silly mistake! I'll create a new test build.

Comment 5

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

Here is new build: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/OhaMj0a0T-KGIQUUH9OXxQ/runs/0/artifacts/public/build/target.tar.bz2

Could you check this again?

And if would not mind, could you attach your config file of the system settings?

Comment 6

[lilydjwg]

The new build works for Ctrl-a, Alt-a and Ctrl-p without issues.

Comment 7

[lilydjwg]

Attachment: keys.css

This is my ~/.config/gtk-3.0/keys.css, @imported by ~/.config/gtk-3.0/gtk.css

Comment 8

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

Thank you! I'll post the patches soon.

Comment 9

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

Attachment: Bug 1743346 - Make `TextInputHandler::HandleEvent` handle native key bindings first, then, our shortcut keys r=smaug!

Oddly, TextInputHandler which is keyboard event handler for
<input type="text"> and <textarea> handles our shortcut keys
first, then, refer native key bindings. So if a key combination
matches in both definitions, our shortcut key wins.

On the other hand, if HTMLEditor has focus, keypress event
listener on the Document node hanldes native key bindings:
https://searchfox.org/mozilla-central/rev/70b32246fce5ca1f53af573a21c1939df58cb969/editor/libeditor/EditorEventListener.cpp#641-644

Then, global key listener will be run later:
https://searchfox.org/mozilla-central/rev/70b32246fce5ca1f53af573a21c1939df58cb969/dom/events/GlobalKeyListener.cpp#163-164

Perhaps, it's better to refer native key bindings first because
unusual shortcut key definition must be caused by customization
by the user in the system level.

Therefore, this patch makes the order switchable with the new
pref for making it easier to back it out.

Depends on D132450

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/49043994c79b
Make `TextInputHandler::HandleEvent` handle native key bindings first, then, our shortcut keys r=smaug

Comment 11

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/49043994c79b

Comment 12

[lilydjwg]

Thank you!