Closed Bug 1523635 Opened 5 years ago Closed 5 years ago

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

Categories

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

Product:
Core
Component:
DOM: Events
Platform:
All
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
relnote-firefox --- 65+
firefox-esr60 --- unaffected
firefox65 + verified
firefox66 + verified
firefox67 + verified

People

(Reporter: atrif, Assigned: masayuki)

References

(
URL
)

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1523635 - part 1: Rename `maybeHandledAsynchronously` to `probablyHandledAsynchronously`
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
RyanVM
: approval-mozilla-release+
Details | Review
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
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
RyanVM
: approval-mozilla-release+
Details | Review

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

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.
Component: Layout: Form Controls → DOM: Events
Flags: needinfo?(masayuki)

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.

Flags: needinfo?(alexandru.trif)

(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.

Flags: needinfo?(alexandru.trif)

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.

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?)

Flags: needinfo?(masayuki) → needinfo?(alexandru.trif)

(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.

Flags: needinfo?(alexandru.trif)

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

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

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.

Flags: needinfo?(alexandru.trif)

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.

Flags: needinfo?(alexandru.trif)
Priority: -- → P2

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...

Assignee: nobody → masayuki
Status: NEW → ASSIGNED

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

(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...

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

Flags: needinfo?(alexandru.trif)

(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.

Flags: needinfo?(alexandru.trif)

(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!

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.

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.

Pushed by masayuki@d-toybox.com:
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 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

Attachment #9041352 - Flags: approval-mozilla-release?
Attachment #9041352 - Flags: approval-mozilla-beta?

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.

Attachment #9041353 - Flags: approval-mozilla-release?
Attachment #9041353 - Flags: approval-mozilla-beta?

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.

Attachment #9041352 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9041353 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Has Regression Range: --- → yes
Has STR: --- → yes
Whiteboard: [qa-triaged]

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.

Status: RESOLVED → VERIFIED
status-firefox66: fixed → verified
status-firefox67: fixed → verified
Flags: qe-verify+
Flags: qe-verify+

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

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

Attachment #9041352 - Flags: approval-mozilla-release? → approval-mozilla-release+
Attachment #9041353 - Flags: approval-mozilla-release? → approval-mozilla-release+

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

status-firefox65: fixed → verified
Flags: qe-verify+
QA Whiteboard: [qa-triaged]
Whiteboard: [qa-triaged]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: