1523635 - [Ubuntu] “Enter” is not functional on Google account password field

Status: VERIFIED FIXED

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

Description

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Affected versions

• Nightly 67.0a1 (20190128214724)
• Firefox 66.0b3 (20190128143734)
• Firefox 65.0 build2 (20190124174741)

Affected platforms

• Ubuntu 18.04 x64

Steps to reproduce

1. Launch Firefox and go to https://accounts.google.com.
2. Insert a valid username and press “Enter” button on the keyboard.
3. Type a valid password and press “Enter”.

Expected result

• You are succesfully logged in.

Actual result

• Nothing happens.

Regression range

• First bad revision: f17b7ba6d0aa737d4f69a0fc3206da8c539225e5
• Last good revision: a38ee229fe1b7ab29d3bce205b8c8611e4a8297e
• Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a38ee229fe1b7ab29d3bce205b8c8611e4a8297e&tochange=f17b7ba6d0aa737d4f69a0fc3206da8c539225e5
• Potential regressor: bug 1505147

Additional notes

• If you type an invalid password at step 3 and press “Enter” nothing happens (the warning message is not displayed).
• The issue is not reproducible on other platforms, neither on other Ubuntu versions.

Comment 1

[Daniel Holbert [:dholbert] (PTO until 2/24)]

If it affects password fields and is specific to Ubuntu 18.04, it might be one of the symtoms of bug 1451466 (which involved a system tool interfering with password field behavior).

That bug has been fixed via an update to Ubuntu 18.04 but perhaps the fix hasn't made it to Alexandru's system....

Alexandru, could you see what version of the gnome-shell package you have running, with the following command:

dpkg -s gnome-shell | grep Version

It looks like 3.28.3-0ubuntu0.18.04.4 is the earliest version with the fix.

Comment 2

[Alexandru Trif, Desktop Test Engineering [:atrif]]

(In reply to Daniel Holbert [:dholbert] from comment #1)

Alexandru, could you see what version of the gnome-shell package you have running, with the following command:

dpkg -s gnome-shell | grep Version

It looks like 3.28.3-0ubuntu0.18.04.4 is the earliest version with the fix.

I used the command you mentioned above and here is the result:
Version: 3.28.3-0ubuntu0.18.04.4
I tested this on two different stations on which I managed to reproduce the issue.

Comment 3

[Daniel Holbert [:dholbert] (PTO until 2/24)]

OK, thanks for checking that. I guess this is independent from bug 1451466 then (though there might end up being a similar ibus/gnome-shell issue partly involved with causing this, which that gnome-shell update didn't address).

Given that we've got a likely regressing bug 1505147, hopefully masayuki can take a look.

Comment 4

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

Sigh, I cannot reproduce this bug on either Nightly nor Firefox 65.0 (bundled by Ubuntu) on Ubuntu 18.04.1 LTS.

Was your Ubuntu environment installed only with English locale? And do you use ibus? (I.e., not using Fcitx as default of Ubuntu?)

Comment 5

[Alexandru Trif, Desktop Test Engineering [:atrif]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #4)

Sigh, I cannot reproduce this bug on either Nightly nor Firefox 65.0 (bundled by Ubuntu) on Ubuntu 18.04.1 LTS.

Was your Ubuntu environment installed only with English locale? And do you use ibus? (I.e., not using Fcitx as default of Ubuntu?)

The keyboard input method system is "IBus". The current used input source is "English (US, euro on 5)". Using "locale -a" command I get the following installed locales: https://drive.google.com/open?id=16NPj7v8ztwth47AxuEF_1T6xgHPcLIul.
If you need more information please let me know.

Comment 6

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

Thanks for the information. I'll try to create pure environment with en-US locale later.

(Why ibus behaves so differently in some environments...)

Comment 7

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

I installed Ubuntu 18.04 cleanly with English UI and English (US, euro on 5) keyboard layout. However, I cannot reproduce this either Firefox 65.0 or today's mozilla-central build.

Alexandru:

Could you attach a log file to this bug with the following steps?

1. Open Firefox, make it restore last open tabs from Options, open Google account auth page, then close Firefox.
2. Open terminal.
3. Run MOZ_LOG=nsGtkIMModuleWidgets:5,KeymapWrapperWidgets:4,sync MOZ_LOG_FILE=~/fx.log firefox
4. Click the input field to input email address
5. If choose a valid address from autocomplete (if you don't save account information, type it on other app like gedit, the copy it, and paste it into the input field. I.e., don't type your address directly in the input field).
6. Type "a" and press "Enter"
7. Close Firefox with mouse.
8. Attach fx.log in your home directory to this bug.

Please note that the log file includes any typed characters during the session. So, DO NOT type anything your privacy in the logging session.

Comment 8

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Using the command “MOZ_LOG=nsGtkIMModuleWidgets:5,KeymapWrapperWidgets:4,sync MOZ_LOG_FILE=~/fx.log firefox” and Firefox 65.0 (20190128144255) I get this log: https://drive.google.com/open?id=1Kia9Ihv8akcBGGznjmBodrAcqVFxTCb6.

I also used the following commands on a Nightly 67.0a1 (20190131214909):
“export MOZ_LOG=nsGtkIMModuleWidgets:5,KeymapWrapperWidgets:4,sync
export MOZ_LOG_FILE=/home/alexandru.trif/Desktop/asd2/fx.log
cd /home/alexandru.trif/Desktop/firefox
./firefox” and here is the log: https://drive.google.com/open?id=1Zjh5_v_chlhrOqaJiJQSX1A0l3AOR7Mw.

I hope these are useful.

Comment 9

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

Sigh, the change of bug 1505147 probably fixed a hidden bug. That is, when I started to work on these series of bugs struggling with hacky behavior of ibus and Fcitx, I confirmed that they don't use async event handling on password fields. However, at least ibus on Ubuntu does it. So, we need to investigate more around here...

Comment 10

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

I mean that currently, the code works as expected completely, but ibus's current behavior is not expected by the code.

Comment 11

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

(Well, I thought that the patch for bug 1505147 should be backed out from release channel, but unfortunately, the patches for bug 1511752, bug 1516323, bug 1498823 and bug 1516326 depend on it. So, that it's too risky to do it...

Comment 12

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

Could you verify this patched build fixes this bug on your environment?
https://queue.taskcluster.net/v1/task/HZJwgM6IQvS1lhXOU2TGuw/runs/0/artifacts/public/build/target.tar.bz2

Comment 13

[Alexandru Trif, Desktop Test Engineering [:atrif]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #12)

Could you verify this patched build fixes this bug on your environment?
https://queue.taskcluster.net/v1/task/HZJwgM6IQvS1lhXOU2TGuw/runs/0/artifacts/public/build/target.tar.bz2

I have verified the build you provided, and the issue is no longer reproducible.

Comment 14

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

(In reply to Alexandru Trif from comment #13)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #12)

Could you verify this patched build fixes this bug on your environment?
https://queue.taskcluster.net/v1/task/HZJwgM6IQvS1lhXOU2TGuw/runs/0/artifacts/public/build/target.tar.bz2

I have verified the build you provided, and the issue is no longer reproducible.

Thank you!

Comment 15

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

Attachment: Bug 1523635 - part 1: Rename `maybeHandledAsynchronously` to `probablyHandledAsynchronously`

Now, we believe that when maybeHandledAsynchronously is set to true,
ibus handles the event asynchronously in usual cases. However, the behavior
of ibus on password field is unclear. Currently, on Ubuntu 18.04,
Ubuntu 18.10 and Debian Cinnamon (9.6 / 3.2.7), ibus handles key events
asynchronously even in password fields even though I confirmed it was not
so at initial fix. So, it could be just my mistake, but we need to prepare
for both cases here for safer fix.

So, in the following patch, I need to add another variable for weaker
decision, and we treat maybeHandledAsynchronously stronger than its
nuance. Therefore, this patch renames it to probablyHandledAsynchronously.

Comment 16

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

Attachment: Bug 1523635 - part 2: Make IMContextWrapper::OnKeyEvent() trust the result of gtk_im_context_filter_keypress() to decide whether handling event causes async event later

Unfortunately, we're not sure whether ibus always handles non-dead key events
asynchronously or synchoronously. Therefore, for safer fix, this patch makes
IMContextWrapper::OnKeyEvent() decide that with the result of
gtk_im_context_filter_keypress(). If active IME is ibus and it consumes non-
synthesized events on password fields, it adjusts probablyHandledAsynchronously
value.

So, this patch changes the behavior of only when:

• active IME is ibus.
• only when a password field or ime-mode:disabled field has focus.
• not in dead key sequence.
• and the key event is consumed by ibus but nothing happend.

This must be enough safe to uplift.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f88aad071714
part 1: Rename `maybeHandledAsynchronously` to `probablyHandledAsynchronously` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/2c8433c4b3f1
part 2: Make IMContextWrapper::OnKeyEvent() trust the result of gtk_im_context_filter_keypress() to decide whether handling event causes async event later r=m_kato

Comment 19

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

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

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f88aad071714
https://hg.mozilla.org/mozilla-central/rev/2c8433c4b3f1

Comment 21

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

Comment on attachment 9041352 [details]
Bug 1523635 - part 1: Rename maybeHandledAsynchronously to probablyHandledAsynchronously

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1524525

User impact if declined

Linux users who use ibus cannot do something if web apps handles keydown/keyup events in password field like Google's auth form.

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

Yes

If yes, steps to reproduce

1. Open https://d-toybox.com/studio/lib/input_event_viewer.html on Ubuntu or other Linux systems.
2. Choose "<input type="password">"
3. Check "keypress" in preventDefault()
4. Type something.

You should see one keydown event and one keyup event in the bottom of the page for every key press and key release, and "data/key" value shouldn't be "Process" unless you type something with IME. Especially, dead key sequence may have been broken with this kind of patches. Note that these patches change only when you use ibus. I.e., if you use Fcitx (default of Debian), any behavior shouldn't be changed.

List of other uplifts needed

None

Risk to taking this patch

Medium

Why is the change risky/not risky? (and alternatives if risky)

We don't have any automated tests around them due to API limitation. Additionally, ibus's behavior depends on environment. E.g., ibus on Ubuntu and ibus on Debian behave differently in some points. (Perhaps, taking only to Beta is reasonable from point of the view of risk vs. importance.)

String changes made/needed

None

Comment 22

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

Comment on attachment 9041353 [details]
Bug 1523635 - part 2: Make IMContextWrapper::OnKeyEvent() trust the result of gtk_im_context_filter_keypress() to decide whether handling event causes async event later

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1524525

User impact if declined

See above.

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

Yes

If yes, steps to reproduce

See above.

List of other uplifts needed

None

Risk to taking this patch

Medium

Why is the change risky/not risky? (and alternatives if risky)

See above.

String changes made/needed

None.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9041352 [details]
Bug 1523635 - part 1: Rename maybeHandledAsynchronously to probablyHandledAsynchronously

[Triage Comment]
Let's get this into 66.0b6 with an eye on taking it in next week's dot release.

Comment 24

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f950041314ee
https://hg.mozilla.org/releases/mozilla-beta/rev/380761f22d52

Comment 25

[Oana Botisan, Desktop QA]

I managed to reproduce the issue on an older version of Nightly 2019-01-29 on Ubuntu 18.04 x64.
I retested everything using latest Nightly 67.0a1 and beta 66.0b6 (https://tools.taskcluster.net/index/gecko.v2.mozilla-beta.latest.firefox/linux64-opt) on the same platform and the bug is not reproducing anymore. I think this issue is fixed.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9041352 [details]
Bug 1523635 - part 1: Rename maybeHandledAsynchronously to probablyHandledAsynchronously

[Triage Comment]
Verified on Beta, approved for 65.0.1 also.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/e58952a2b64a
https://hg.mozilla.org/releases/mozilla-release/rev/c48e0ec8db4f

Comment 28

[Oana Botisan, Desktop QA]

I retested everything using Firefox 65.0.1 on Ubuntu 18.0.1. The bug is not reproducing anymore.