758420 - Apostrophe followed by a letter creates two apostrophes

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Reuben Morais [:reuben]]

I'm not sure whether this is an Editor or Widget: Cocoa bug.

Steps to reproduce:
1) With a US International PC keyboard layout on OS X Lion, try to write |'s|

Actual results:
Two apostrophes are displayed |''| 

Expected results:
Apostrophe followed by consonant |'s|

Comment 1

[Reuben Morais [:reuben]]

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=762e95608da3&tochange=e794cef56df6

Suspicious changesets:
69fc5803d184	Masayuki Nakano — Bug 447757 part.1 Compute DOM keycode for non-printable key from table first r=karlt
29e512736f30	Masayuki Nakano — Bug 677252 part.1 Reimplement keycode computation in cocoa widget r=smaug+smichaud

Comment 2

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

Sure, regression of bug 677252.

NSEvent's |characters| for "s" key returns "'s". insertText is called twice one is for "'" and the other is for "s". Therefore, the old code sets "s" for charCode of the keypress event but the new code sets it "'" which is the first character of |characters|.

We can fix this bug easily, but I need to decide "how" to fix this bug in code level though.

Comment 3

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

Attachment: Patch

confirming on tryserver.

Comment 4

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

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

I think, basically, it's wrong to use first character of characters of NSEvent for charCode of our DOM keypress event. But it's not matter for now since there is no other actual problem.

At handling insertText, we should honor the inserting text rather than current key event's characters. This patch makes that the caller (InsertText() in this case) can specify charCode before calling InitKeyEvent(). It's a little bit hacky, but I think that it's okay for now.

I think that we will need to change the key event handlers when we fix the relation between composition events and key events (current behavior doesn't base on D3E definition). Then, we can make them better.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

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

This looks fine to me as an interim patch.  But here are some changes to your comments that (I think) make them easier to understand:

+   *                              NOTE: When aKeyEvent is a keypress event and
+   *                                    caller expects the event causes to input
+   *                                    a character, caller should set charCode
+   *                                    before calling this.  Then, the charCode
+   *                                    value isn't modified.

"When aKeyEvent is a keypress event and the caller expects that the event will cause a character to be input (say in an editor), the caller should set aKeyEvent.charCode before calling this.  Then charCode won't be modified."

+   *                              NOTE: If caller expects this event causes
+   *                                    to input a character, caller should
+   *                                    set the character before calling this.
+   *                                    Then, the charCode is never modified.

"If the caller expects this event to cause character input (say in an editor), the caller should set aKeyEvent.charCode before calling this.  Then charCode won't be modified."

+  // If the inputting character is printable character, we're expecting that
+  // the keypress event causes to input it on editor.  For ensuring it, set
+  // charCode before calling InitKeyEvent().

"If the text to be inserted is a single printable character, we expect that the keypress event will cause it to be input in an editor.  To ensure this happens, set charCode before calling InitKeyEvent()."

+  PRUnichar inputtingChar = str.CharAt(0);

I think it makes sense to change "inputtingChar" to "insertedChar".

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/85e529ae09f9

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85e529ae09f9