946044 - Pressing the Windows "Application" key inserts a U+0010 'DATA LINK ESCAPE' hexbox into text editor

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Chris Peterson [:cpeterson]]

STR:
1. In the Bugzilla comment editor on OS X, press the Windows "Application" key, the key between the right Alt and Ctrl keys: http://www.seoconsultants.com/windows/keyboard/#Application

RESULT:
A "" character is inserted. This is a regression in Nightly 28. It might be a regression from bug 941940.

Comment 1

[Chris Peterson [:cpeterson]]

U+0010 'DATA LINK ESCAPE' hexbox: http://www.fileformat.info/info/unicode/char/0010/index.htm

Comment 2

[Masatoshi Kimura [:emk]]

I think bug 941940 just made the bug more visible. Maybe a key event handling issue?

Comment 3

[Jonathan Kew [:jfkthame]]

Right - bug 941940 certainly didn't -cause- the unwanted control character to be inserted. In an earlier release, you should be able to detect the presence of the "" character by moving the insertion point back and forth through the typed text using the arrow keys (there'll be a "pause" where the insertion point stays in the same place). Or press the Application key in the middle of typing a word, then try to to use Find-in-page to search for that word (without the Application key this time), and it won't be found.

Comment 4

[Jonathan Kew [:jfkthame]]

Removing regression keyword; it's probably always been like this (just invisible, so we didn't notice).

Probably needs to be fixed in the editor's event handling, I guess.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I guess that this is a bug of widget/cocoa. I'll check it.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

This patch registers the keycode (0x6E) as a non-printable key. And ignoring insertText argument if its first character is a control character.

I maps the key as NS_VK_CONTEXT_MENU. However, I don't change it as opening context menu because other applications don't do so.

However, I guess, most Mac users don't use PC keyboard. And if PC keyboard is used, working context menu key might make the users happy. Additionally, in default settings (I confirmed on Mavericks), our context menu shortcut key, control + space, is reserved by system wide shortcut key. So, anyway, we need to improve a11y for context menu for users who prefer keyboard navigation on Mac. I'll file a bug for it later.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8342823 [details] [diff] [review]
Patch

Oops, this patch works, but IsControlChar() should return true for null character too.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

See above comment for the detail of this patch.

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8342936 [details] [diff] [review]
Patch

Review of attachment 8342936 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/TextInputHandler.mm
@@ +342,5 @@
>  
> +static bool
> +IsControlChar(uint32_t aCharCode)
> +{
> +  return aCharCode < ' ' || aCharCode == 0x7F;

Should we also block the C1 Controls characters? (0x80..0x9f)

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jonathan Kew (:jfkthame) from comment #9)
> Comment on attachment 8342936 [details] [diff] [review]
> Patch
> 
> Review of attachment 8342936 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/cocoa/TextInputHandler.mm
> @@ +342,5 @@
> >  
> > +static bool
> > +IsControlChar(uint32_t aCharCode)
> > +{
> > +  return aCharCode < ' ' || aCharCode == 0x7F;
> 
> Should we also block the C1 Controls characters? (0x80..0x9f)

Hmm, is it possible to input them on Mac?

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #10)
> (In reply to Jonathan Kew (:jfkthame) from comment #9)
> > Comment on attachment 8342936 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 8342936 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: widget/cocoa/TextInputHandler.mm
> > @@ +342,5 @@
> > >  
> > > +static bool
> > > +IsControlChar(uint32_t aCharCode)
> > > +{
> > > +  return aCharCode < ' ' || aCharCode == 0x7F;
> > 
> > Should we also block the C1 Controls characters? (0x80..0x9f)
> 
> Hmm, is it possible to input them on Mac?

Not with a single key on a standard keyboard, AFAIK, but it's certainly possible for a keyboard layout to generate them.

For testing, you can use the Unicode Hex Input layout (enable it via Language & Text Preferences / Input Sources), and key a sequence like <Opt-0, Opt-0, Opt-8, Opt-0>; this will enter the "" character displayed as a hexbox (with current Nightly) here.

So this is a very rare case, I'm sure, but if we're going to prevent people typing arbitrary control characters (i.e. any controls except the specific ones we support, like <tab> and <newline>), then IMO we should block these as well.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Then, we need to block all control characters defined in Unicode. So, I think that we don't need to care such edge case...

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8342936 [details] [diff] [review]
Patch

This looks fine to me.

I agree with Masayuki's minimalist approach.  Here be dragons :-)

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac41fb323652

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac41fb323652