Closed Bug 946044 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox27 --- unaffected
firefox28 --- affected

People

(Reporter: cpeterson, Assigned: masayuki)

Details

(Whiteboard: [good first verify])

Attachments

(1 file, 1 obsolete file)

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.
U+0010 'DATA LINK ESCAPE' hexbox: http://www.fileformat.info/info/unicode/char/0010/index.htm
Summary: Pressing the Windows "Application" key inserts a " " character into text editor → Pressing the Windows "Application" key inserts a U+0010 'DATA LINK ESCAPE' hexbox into text editor
I think bug 941940 just made the bug more visible. Maybe a key event handling issue?
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.
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.
No longer blocks: 941940
I guess that this is a bug of widget/cocoa. I'll check it.
Assignee: nobody → masayuki
Component: Editor → Widget: Cocoa
Version: unspecified → Trunk
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8342823 - Flags: review?(smichaud)
Comment on attachment 8342823 [details] [diff] [review]
Patch

Oops, this patch works, but IsControlChar() should return true for null character too.
Attachment #8342823 - Flags: review?(smichaud) → review-
Attached patch PatchSplinter Review
See above comment for the detail of this patch.
Attachment #8342823 - Attachment is obsolete: true
Attachment #8342936 - Flags: review?(smichaud)
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)
(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?
(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.
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 on attachment 8342936 [details] [diff] [review]
Patch

This looks fine to me.

I agree with Masayuki's minimalist approach.  Here be dragons :-)
Attachment #8342936 - Flags: review?(smichaud) → review+
https://hg.mozilla.org/mozilla-central/rev/ac41fb323652
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.