339313 - listbox FAYT doesn't work when the Shift key is pressed

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Simon Bünzli]

... which can make it impossible to select entries containing non-alphanumeric characters (e.g. : or ") and which unnecessarily prevents searching for proper cased entries.

Steps to Reproduce:
1. In Firefox, go to Tools -> Options -> Advanced -> General -> Edit Languages...
2. Make sure that "English/United States [en-US]" is listed (or any other language/country combination)
3. Get yourself a non-en-US keyboard layout (where / is available only as a shifted key)
4. Type "English" (capital E)
5. Type "english/"

Actual Results:
Steps 4 and 5 both fail.

Note that the "Select a language to add..." selector just below correctly handles both cases.

Comment 1

[Simon Bünzli]

Attachment: don't reject keypress events when the shift key is pressed

Comment 2

[Asaf Romano (gone)]

reference: this was added in https://bugzilla.mozilla.org/show_bug.cgi?id=148010#c4.

No idea why though.

Comment 3

[Asaf Romano (gone)]

<sigh/>

Comment 4

[[e-mail]]

Comment on attachment 223422 [details] [diff] [review]
don't reject keypress events when the shift key is pressed

>-             !event.altKey && !event.ctrlKey && !event.shiftKey && !event.metaKey) {
>+             !event.altKey && !event.ctrlKey && !event.metaKey) {
We don't need to check the modifier keys because the charCode will be zero for nonprinting keys. For example, the menu code just checks the char code. See attachment 55849 [details] and try some extended keys in non-US layouts e.g. Alt+Gr+4

I think that there are other places where this is also done by mistake.

Comment 5

[Simon Bünzli]

(In reply to comment #4)
> We don't need to check the modifier keys because the charCode will be zero for
> nonprinting keys. For example, the menu code just checks the char code. See
> attachment 55849 [details] [edit] and try some extended keys in non-US layouts e.g. Alt+Gr+4

In this case we should check the modifier keys because we don't want FAYT to kick in when using accelerators and shortcuts; e.g. adding Ukrainian to the language list and then hitting Alt+U should move the current language one place up and /not/ select Ukrainian. Unfortunately we might not be able to do the right thing for AltGr key combos (because they are reported as Ctrl+Alt combos which might just as well be a shortcut).

Do you still want to drop all modifier checks under these conditions?

Comment 6

[[e-mail]]

(In reply to comment #5)
>Do you still want to drop all modifier checks under these conditions?
My mistake, I forgot that Alt+U generated a charcode, but you still ought to fix the other places that check for shift by mistake.

Interestingly menulist.xml ignores both shift and control.

Comment 7

[Simon Bünzli]

Attachment: remove shift key check for listbox and tree (Toolkit and XPFE)

After briefly searching through the sources, I only found one other instance of this issue in tree.xml. This patch fixes both occurrences for Toolkit and the suite. Should you find/know of more, feel free to either post a reference or a patch...

Comment 8

[[e-mail]]

Comment on attachment 223432 [details] [diff] [review]
remove shift key check for listbox and tree (Toolkit and XPFE)

Should I spot any other cases I'll take you up on your offer.

Comment 9

[[e-mail]]

I checked the fix in to the trunk, ask mconnor should you want branch approval.

Comment 10

[Simon Bünzli]

Comment on attachment 223432 [details] [diff] [review]
remove shift key check for listbox and tree (Toolkit and XPFE)

Thanks.

Comment 11

[[e-mail]]

Fix checked in to the branch.