1430628 - Command + LeftArrow during composition causes going back the page

Status: VERIFIED FIXED

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

Description

[Kyrodes]

Attachment: firefox.mp4

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

Steps to reproduce:

Command + ← is a MacOS shortcut to move the text carrier to the beginning of line.

In Firefox this shortcut also goes to previous page.

In my language, quotes are also used to input an accent when using the keyboard US International and some other keyboard layouts.

On Mac, to type ["]:
- press the key ["]
- An underline will appear under the " waiting for the next command
- press [space] to confirm the insertion of: "

On Mac, to type [ü]
- press the key ["]
- An underline will appear under the " waiting for the next command
- press the key [u] and the " will change to: ü

The problem happens in Firefox Nightly 59 when the user press Command + ← while the ["] still is underlined.

Steps to reproduce:
- press the key ["] 
- While it still is underlined, press Command + ← 


Actual results:

In Firefox Nightly 59:
- Firefox goes to previous page. 

Looks like the text box lost focus temporarily and Firefox is executing the back shortcut instead of the text shortcut.


Expected results:

In Firefox Release 57 and Firefox Dev 58:
- The underline disappears as expected.
- Then the user can press Command + ← again to the carrier moves to the beginning of line normally.

This is the normal behavior of the OS and was the normal behavior in Firefox until FF59.

Comment 1

[Kyrodes]

I forgot to say that attached to the previous message is a video showing the behavior first in Firefox Release 57 and later in Firefox Nightly 59.0a1.

Comment 2

[Flore Allemandou [:flore]]

Confirmed on latest Nightly on MacOS 10.13.
French language is using the same way to create accentuated letters as described by the reporter (we use ¨ and not ", but the mechanism is the same).
I would say, it probably can be reproduced for german locale (¨) and maybe spanish (~). Probably every language that uses that kind of signs over letters

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Sounds it's a regression. 
Is it possible to get the regression commit/window by mozregression tool http://mozilla.github.io/mozregression/ ?

Comment 4

[Flore Allemandou [:flore]]

I can do that (mozregression is already installed and working on my mac) in the next few days.

Comment 5

[Flore Allemandou [:flore]]

Mozregression done

INFO: Last good revision: 525f2f1623850d9de7f0b1995f0c68fe1a14e0cc
INFO: First bad revision: afc17c7465c2371b7ba0f50c92ec065c099e9fd3
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=525f2f1623850d9de7f0b1995f0c68fe1a14e0cc&tochange=afc17c7465c2371b7ba0f50c92ec065c099e9fd3

Masayuki : one of your patches may have broken this behaviour on MacOS, can you check, please?

Comment 6

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

> Expected results:
> 
> In Firefox Release 57 and Firefox Dev 58:
> - The underline disappears as expected.
> - Then the user can press Command + ← again to the carrier moves to the beginning of line normally.
> 
> This is the normal behavior of the OS and was the normal behavior in Firefox until FF59.

That sounds really odd to me. How do you think that only committing composing string when you press Command + LeftArrow is "normal" behavior of macOS?

I made Firefox respect commands sent by macOS (or active IME/keyboard layout) even while there is composition. So, if the change caused this new behavior, it must be expected by macOS. And such behavior has been changed a lot by OS X and macOS when they're upgraded...

Comment 7

[Flore Allemandou [:flore]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> That sounds really odd to me. How do you think that only committing
> composing string when you press Command + LeftArrow is "normal" behavior of
> macOS?

Because it is the behavior in native macOS apps (Safari, Textedit...), so it is how people expect application to behave in Mac OS.

Safari behaves exactly like the expected result in comment 0

Comment 8

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

Thanks. I don't believe Safari's behavior because it reimplement form widget, but Textedit is enough to consider. If I have much time to be back this bug next week, I'd try to investigate.

Comment 9

[Flore Allemandou [:flore]]

Attachment: safari behavior.mov

Safari behavior

Comment 10

[Flore Allemandou [:flore]]

Attachment: firefox behavior.mov

Firefox behavior

Comment 11

[Flore Allemandou [:flore]]

I just created 2 videos : one with safari and one with Nightly
What I did :
open mozilla.org
open google.fr
type "azerty ¨" (1 word and the character ¨(which is underlined waiting for a letter to be altered))
Then press Command + ← 

Safari : caret goes back to the start of the line. When not typing text in a form, Command + ← makes Safari go back to previous page.
Firefox : goes back to previous page (to mozilla.org)

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 59 in hopes we can fix this while 59 is in beta.

Comment 13

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

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

Comment 14

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

According to Cocoa's behavior and testing with other applications, I think that the correct behavior is, Command + LeftArrow during composition should commit composition *and* move caret to the beginning of the line (but shouldn't go back from current page).

Comment 15

[Kyrodes]

Exactly. This is the behavior in Safari.

Comment 16

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

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

Comment 17

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

Attachment: Bug 1430628 - TextInputHandler::HandleCommand() should use native key event when it dispatches a keypress event either initializing with native event or creating fake event

TextInputHandler::HandleCommand() has two bugs.  One is, checking whether
the key event has caused composition events.  Even if it caused composition
events, we decided to dispatch keypress event for emulating native behavior.
Therefore, this patch removes the check of
|currentKeyEvent->CanDispatchKeyPress()|.

The other is, for making content handle dispatching keypress event as given
command, it needs to dispatch a keypress event whose key combination will
cause the command.  However, HandleCommand() needs to set native key event
since content may not refer key combination for some edit actions, they just
refer command which is computed with native key event with NativeKeyBindings.
Therefore, even if current native key event has already caused dispatching
some events, HandleCommand() needs to set
WidgetKeyboardEvent::mNativeKeyEvent to current native key event for
NativeKeyBindings.  Although it must be rare case, given key could be
not related to the command or not key could cause the command.  In this
case, and perhaps in all cases, we should set all commands of dispatching
keypress event before dispatching it.  Howevever, this needs more work,
so, we shouldn't do it in this bug to making it possible to uplift.
Therefore, this patch makes always set mNativeKeyEvent to current native
key event.  So, just warning it when command is caused without native
key event.

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

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8945342 [details]
Bug 1430628 - TextInputHandler::HandleCommand() should use native key event when it dispatches a keypress event either initializing with native event or creating fake event

https://reviewboard.mozilla.org/r/215544/#review221526

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/897cb1885507
TextInputHandler::HandleCommand() should use native key event when it dispatches a keypress event either initializing with native event or creating fake event r=m_kato

Comment 20

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/897cb1885507

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.

Comment 22

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

Comment on attachment 8945342 [details]
Bug 1430628 - TextInputHandler::HandleCommand() should use native key event when it dispatches a keypress event either initializing with native event or creating fake event

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1403759.

[User impact if declined]:
macOS users may execute non-edit commands even when they are composing text with IME or dead key. E.g., go forward or go back the history with Command + RightArrow or LeftArrow.  Such thing must be annoying and may cause losing inputting data.

[Is this code covered by automated tests?]:
No because we cannot test this in automated tests.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]:
Yes.

1. Load this page.
2. Click this link https://bugzilla.mozilla.org/query.cgi?format=advanced
3. Ensure input field for "Summary" has focus.
4. Choose "U.S. International" keyboard layout (if you haven't installed it, install it from system settings -> keyboard).
5. Type option + u, then, you'll see "¨" with underline.
6. Type Command + LeftArrow.

Then, the underline should disappear and caret should be moved to begin of the line in the input field. (Previously, it caused going back the history.)

[List of other uplifts needed for the feature/fix]:
No.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
In bug 1403759, I forgot to set native key event of dispatching keypress event for NativeKeyBindings immediately after committing composition. Therefore, content don't treat the keypress event is an editing command, then, global key bindings treat it as non-edit command.  This patch just sets native key event of dispatching keypress event to current (handling) native key event.

[String changes made/needed]:
No.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8945342 [details]
Bug 1430628 - TextInputHandler::HandleCommand() should use native key event when it dispatches a keypress event either initializing with native event or creating fake event

Sounds like an annoying regression for 59 users, let's uplift this for beta 7

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/95291d42399e

Comment 25

[Petruta Horea [:phorea], Desktop Test Engineering]

Reproduced using the steps from comment 22 with Firefox 59 beta 5 under OS X 10.13.
Verified as fixed on Firefox 59 beta 7 and latest Nightly 60.0a1 2018-02-06.