Closed Bug 1249184 Opened 4 years ago Closed 4 years ago

[UI Events] Implement Dead key event on Mac OS X

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

On Mac OS X, we've not implemented dead key events correctly yet.

1. keypress event shouldn't be fired when dead key doesn't commit any text (when non-editable element has focus)
2. KeyboardEvent.key value should be "Dead" when the key event doesn't cause text due to dead key.
Status: NEW → ASSIGNED
This makes Mac OS X widget's dead key behavior same as the other platforms' widget.

And not dispatching keypress event for Dead key is same behavior as the other browsers.
Comment on attachment 8722819 [details] [diff] [review]
Dead key shouldn't cause keypress event on Mac OS X

This makes Mac OS X widget's dead key behavior same as the other platforms' widget.

And not dispatching keypress event for Dead key is same behavior as the other browsers.
Attachment #8722819 - Flags: review?(m_kato)
Attachment #8722819 - Flags: review?(bugs)
Btw, not about this bug, we seem to miss keydown for some dead keys on linux, for example when pressing ¨ key on Finnish keyboard. shift+<the same key> producess ^ and AltGr+<the same key> is ~.
But keydown isn't ever dispatched.
Comment on attachment 8722819 [details] [diff] [review]
Dead key shouldn't cause keypress event on Mac OS X

mstange might want to take a look at this too, given that this is OSX specific stuff, mostly.

But could you explain the patch a bit.
I'm not too familiar with that OSX specific code.
isPrintableKey handling for example looks really odd, especially the if which sets its value explicitly to false.

And, I don't quite understand the change to the test. Is that because of the change WidgetKeyboardEvent::ShouldCauseKeypressEvents(), so the patch actually ends up doing two rather different changes.

And to limit the change, shouldn't -    if (aKeyEvent.mKeyValue.IsEmpty() ||
-        IsControlChar(aKeyEvent.mKeyValue[0])) {
+    if (aKeyEvent.mKeyNameIndex == KEY_NAME_INDEX_USE_STRING &&
+        (aKeyEvent.mKeyValue.IsEmpty() ||
+         IsControlChar(aKeyEvent.mKeyValue[0]))) {
actually have 
aKeyEvent.mKeyNameIndex !=  KEY_NAME_INDEX_Dead; ?


So, r- because I'd like to get some more explanation to this.
Attachment #8722819 - Flags: review?(bugs) → review-
Attachment #8722819 - Flags: review?(m_kato)
Comment on attachment 8722819 [details] [diff] [review]
Dead key shouldn't cause keypress event on Mac OS X

(In reply to Olli Pettay [:smaug] (vacation Feb 29 - Mar 4) from comment #5)
> mstange might want to take a look at this too, given that this is OSX
> specific stuff, mostly.

Yeah, but I asked mstange at Mozlando, whether is it okay Makoto Kato-san to review around keyboard and IME event handling of Cocoa. Then, he said, it's okay. So, I think that this is okay only with Makoto-san's review except if he thinks mstange should review this patch.

> But could you explain the patch a bit.
> I'm not too familiar with that OSX specific code.
> isPrintableKey handling for example looks really odd, especially the if
> which sets its value explicitly to false.

isPrintableKey is true if the key is not a function key (i.e., the key doesn't cause any character with any modifier state). So, even if it's true, the key may cause inputting empty string if active keyboard layout maps the key as not inputting any character. For example, IntlRo key on Japanese keyboard with the other keyboard layouts, e.g., ANSI keyboard layout. IntlRo key is Japanese keyboard layout specific key, so, no other keyboard layouts maps it to any text.

The reason why TISInputSourceWrapper::InitKeyEvent() sets isPrintableKey to false explicitly is, printable key won't (shouldn't) cause normal key events (NSKeyDown and NSKeyUp). I.e., if it causes NSFlagsChanged event, it means that the key is a modifier key. This cause could be occurred when another application sends us odd keyboard event.

When we handles a printable key event, we need to access [NSEvent characters]. However, if we call it of neither NSKeyDown not NSKeyUp, Cocoa throws an exception (bug 784783). Therefore, we clear the flag when we receive odd key event.

> And, I don't quite understand the change to the test. Is that because of the
> change WidgetKeyboardEvent::ShouldCauseKeypressEvents(), so the patch
> actually ends up doing two rather different changes.

When a key event is a dead key event (i.e., pending to input text until following key event(s)), it shouldn't cause keypress event(s) because it doesn't cause any text at the moment.  Therefore, I removed keypress events from the expected key events in the test.

> And to limit the change, shouldn't -    if (aKeyEvent.mKeyValue.IsEmpty() ||
> -        IsControlChar(aKeyEvent.mKeyValue[0])) {
> +    if (aKeyEvent.mKeyNameIndex == KEY_NAME_INDEX_USE_STRING &&
> +        (aKeyEvent.mKeyValue.IsEmpty() ||
> +         IsControlChar(aKeyEvent.mKeyValue[0]))) {
> actually have 
> aKeyEvent.mKeyNameIndex !=  KEY_NAME_INDEX_Dead; ?

I think that's okay for now but not so safe in the future. 

WidgetKeyboardEvent::mKeyValue is used (in other words, should be referred) only when mKeyNameIndex is KEY_NAME_INDEX_USE_STRING (because, otherwise, DOM KeyboardEvent returns key name as string which is mapped from mKeyNameIndex).

Therefore, I think that my change is correct because following conditions check mKeyValue.

# Hmm, you block review requests... I wait you to be back.
Attachment #8722819 - Flags: review-
Comment on attachment 8722819 [details] [diff] [review]
Dead key shouldn't cause keypress event on Mac OS X

See the previous comment.
Attachment #8722819 - Flags: review?(bugs)
Attachment #8722819 - Flags: review?(bugs) → review+
Attachment #8722819 - Flags: review?(m_kato)
Attachment #8722819 - Flags: review?(m_kato) → review+
https://hg.mozilla.org/mozilla-central/rev/1d96035b9f32
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1280053
I've added an item to the end of this section. I'd appreciate it if you could check to be sure I interpret the correction properly. Thank you!

https://developer.mozilla.org/en-US/Firefox/Releases/48#Others
Flags: needinfo?(masayuki)
(In reply to Eric Shepherd [:sheppy] from comment #11)
> I've added an item to the end of this section. I'd appreciate it if you
> could check to be sure I interpret the correction properly. Thank you!
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/48#Others

Thank you. I modified the document. The changed behavior is affected only when the focused element isn't editable. When the focused element is editable, a dead key sequence causes a set of composition events because dead key is implemented with IME on macOS.
Flags: needinfo?(masayuki)
You need to log in before you can comment on or make changes to this bug.