Last Comment Bug 758420 - Apostrophe followed by a letter creates two apostrophes
: Apostrophe followed by a letter creates two apostrophes
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: x86 Mac OS X
: -- major (vote)
: mozilla15
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on: 759524 775414
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-24 15:42 PDT by Reuben Morais [:reuben]
Modified: 2012-07-18 23:08 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (11.23 KB, patch)
2012-05-25 03:29 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
smichaud: review+
Details | Diff | Review

Description Reuben Morais [:reuben] 2012-05-24 15:42:26 PDT
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] 2012-05-24 19:21:39 PDT
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] (Mozilla Japan) 2012-05-24 21:34:04 PDT
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] (Mozilla Japan) 2012-05-25 03:29:26 PDT
Created attachment 627159 [details] [diff] [review]
Patch

confirming on tryserver.
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-05-25 11:41:57 PDT
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) 2012-05-25 13:31:18 PDT
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] (Mozilla Japan) 2012-05-25 19:36:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/85e529ae09f9
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-05-26 05:34:51 PDT
https://hg.mozilla.org/mozilla-central/rev/85e529ae09f9

Note You need to log in before you can comment on or make changes to this bug.