Closed Bug 1346499 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Widget: Win32, defect)

52 Branch
All
Windows
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- wontfix
firefox-esr52 54+ fixed
firefox53 --- verified
firefox54 --- verified
firefox55 --- verified

People

(Reporter: tustamido, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
Blocks: 1303273
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Extension Compatibility
Ever confirmed: true
Keywords: regression
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
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/2672f4d4b38d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Should we consider uplifting this?
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+
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
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.
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
I'd like to have this uplifted to ESR52 since this affects editing in TB: 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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: