KeyboardEvent.ctrlKey of "keypress" event at Ctrl + Space on Windows should be true unless it doesn't cause different character without Ctrl key

VERIFIED FIXED in Firefox -esr52

Status

()

Core
Widget: Win32
VERIFIED FIXED
9 months ago
7 months ago

People

(Reporter: tustamido, Assigned: masayuki)

Tracking

({regression})

52 Branch
mozilla55
All
Windows
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox52 wontfix, firefox-esr5254+ fixed, firefox53 verified, firefox54 verified, firefox55 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170302120751

Steps to reproduce:

- Install Fast Tab Switcher (https://addons.mozilla.org/pt-br/firefox/addon/fast-tab-switcher/)
- The default shortcut to activate the addon popup is Ctrl + Spacebar, so press it


Actual results:

- Nothing, the page scrolls down as if Ctrl is not pressed


Expected results:

- Should open the popup, as happened until Firefox 51.0.1



mozregression:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6173757aa33a0adb8ce3239e97eb3281eb6a4c9a&tochange=927bfc865b55d9d7f94df43f13cc725f555d580e

"Bug 1303273 part.9 NativeKey::mFollowingCharMsgs should be AutoTArray r=m_kato

Similarly, NativeKey is a stack class and mFollowingCharMsgs which stores following char messages of WM_(SYS)KEYDOWN should be AutoTArray for preventing to use heap at handling inputs with usual keyboard layout.

5 is enough size for handling usual keyboard layout because we support only 5 or less characters per dead key sequence.

MozReview-Commit-ID: IphcIOmPW0C"


The Fast Tab Switcher is just an example to verify that Ctrl + Spacebar isn't working. Another could be install Dorando Keyconfig (https://addons.mozilla.org/en-US/firefox/addon/dorando-keyconfig/) and try to set one shortcut as Ctrl + Spacebar. Until Fx 51.0.1 this worked, but in Fx 52 it doesn't recognize Ctrl in this combination, just Spacebar.

Updated

9 months ago
Blocks: 1303273
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox52: --- → affected
Component: Untriaged → Extension Compatibility
Ever confirmed: true
Keywords: regression
(Reporter)

Comment 1

9 months ago
Fast Tab Switcher created a workaround in version 1.0.10, so consider Dorando Keyconfig as the way to reproduce. Since Firefox 52 it can't set Ctrl + Space as a shortcut, Ctrl is ignored in this case.
Thank you for the report. Oddly, ctrlKey is false of keypress event at Ctrl+Space. The reason is, Ctrl+Space causes WM_CHAR for ' '. I don't know the reason, but it causes inputting ' ' in Notepad too.

Although, this is native behavior of editor from a point of view of editor, we should restore ctrlKey state for the compatibility with the other browsers.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → affected
Component: Extension Compatibility → Widget: Win32
OS: Unspecified → Windows
Product: Firefox → Core
Hardware: Unspecified → All
Summary: Custom keyboard shortcut Ctrl + Spacebar not working → KeyboardEvent.ctrlKey of "keypress" event at Ctrl + Space on Windows should be true unless it doesn't cause different character without Ctrl key
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2104c0170de9
(Assignee)

Updated

9 months ago
See Also: → bug 1346832
Comment hidden (mozreview-request)

Comment 5

9 months ago
mozreview-review
Comment on attachment 8846941 [details]
Bug 1346499 Don't remove Ctrl nor Alt modifier state at dispatching eKeyPress event when the modifier doesn't change inputting character

https://reviewboard.mozilla.org/r/119692/#review122356
Attachment #8846941 - Flags: review?(m_kato) → review+

Comment 6

9 months ago
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/2672f4d4b38d
Don't remove Ctrl nor Alt modifier state at dispatching eKeyPress event when the modifier doesn't change inputting character r=m_kato

Comment 7

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2672f4d4b38d
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Should we consider uplifting this?
status-firefox52: affected → wontfix
Flags: needinfo?(masayuki)
Yeah, I think that this is serious regression. However, we usually get regression reports around keyboard after release. So, I think that we should fix it in 53 for now. And then, we should try to uplift to 52ESR.
Flags: needinfo?(masayuki)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eaaa3165bb6
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10fbe2e756ee
Comment on attachment 8846941 [details]
Bug 1346499 Don't remove Ctrl nor Alt modifier state at dispatching eKeyPress event when the modifier doesn't change inputting character

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

[User impact if declined]:
Cannot use Ctrl+Space on Windows even with extensions nor web apps.

[Is this code covered by automated tests?]:
Yes, for the specific case. However, I don't know similar cases (pressing a printable key either with or without Ctrl or Alt key causes same character).

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

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

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

[Is the change risky?]:
Could be, but perhaps, enough safe.

[Why is the change risky/not risky?]:
At bug 1303273, I changed to respect following WM_CHAR message at keydown (WM_CHAR indicates a character which should be inputted with the key and modifier state).

However, Ctrl+Space causes WM_CHAR with ASCII whitespace. Therefore, we remove ctrlKey state from keypress event for causing text input.  However, before bug 1303273, modifier state wouldn't be ignored when Ctrl state or Alt state doesn't change inputting character.

Therefore, this patch adds the check to the new path.  However, the changing path is major path, which is used everybody. So, adding code must be enough safe, but the touching path is not so safe.

But anyway, in my experience, regression reports will come after release. So, I believe that it's worthwhile to uplift the patch to Aurora and Beta. However, after 53 is released, we shouldn't take this into ESR52.

[String changes made/needed]:
No.
Attachment #8846941 - Flags: approval-mozilla-beta?
Attachment #8846941 - Flags: approval-mozilla-aurora?
Comment on attachment 8846941 [details]
Bug 1346499 Don't remove Ctrl nor Alt modifier state at dispatching eKeyPress event when the modifier doesn't change inputting character

Fix a broken keypress event (Ctrl+Space on Windows) regression issue. Aurora54+ & Beta53+.

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
Attachment #8846941 - Flags: approval-mozilla-beta?
Attachment #8846941 - Flags: approval-mozilla-beta+
Attachment #8846941 - Flags: approval-mozilla-aurora?
Attachment #8846941 - Flags: approval-mozilla-aurora+

Comment 14

9 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/31a1c091957e
status-firefox54: affected → fixed

Comment 15

9 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/96801dce9f75
status-firefox53: affected → fixed
Flags: qe-verify+
Reproduced this on Firefox 52.0.1, with Fast Tab Switcher, version 1.0.8 on Windows 10 x64.

I can confirm the fix on latest Nightly 55.0a1, Build ID 20170324030205, on Windows 10 x64.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: needinfo?(brindusa.tot)
Reproduced the initial issue with Fast Tab Switcher version 1.0.8 and Dorando Keyconfig on Firefox 52.0.1 under Win 10 64-bit. 
Verified as fixed using Firefox 53 beta 6 and latest Aurora 54.0a2 2017-03-27.
status-firefox53: fixed → verified
status-firefox54: fixed → verified
Flags: qe-verify+
Comment on attachment 8846941 [details]
Bug 1346499 Don't remove Ctrl nor Alt modifier state at dispatching eKeyPress event when the modifier doesn't change inputting character

Verified on 53/54 and no known regressions.
Attachment #8846941 - Flags: approval-mozilla-esr52?
[Tracking Requested - why for this release]: per comment 9 this might be a good fix for 52.2 once it proves itself on release53
tracking-firefox-esr52: --- → ?

Comment 20

8 months ago
I'd like to have this uplifted to ESR52 since this affects editing in TB: Bug 1355826

Updated

8 months ago
Duplicate of this bug: 1355826

Comment 22

7 months ago
Comment on attachment 8846941 [details]
Bug 1346499 Don't remove Ctrl nor Alt modifier state at dispatching eKeyPress event when the modifier doesn't change inputting character

This fixes a core scenario in TB, no known regressions on 53, 54, let's uplift to ESR52.2+
Attachment #8846941 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Updated

7 months ago
tracking-firefox-esr52: ? → 54+

Comment 23

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/747fd6c81983
status-firefox-esr52: affected → fixed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.