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

()

VERIFIED FIXED
2 years ago
2 years 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

Attachments

(1 attachment)

(Reporter)

Description

2 years 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

2 years 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

2 years 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
Comment hidden (mozreview-request)

Comment 5

2 years 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

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2672f4d4b38d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years 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)
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

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

Comment 15

2 years 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

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

Updated

2 years ago
Duplicate of this bug: 1355826
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+
tracking-firefox-esr52: ? → 54+

Comment 23

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