1280053 - content pasted twice after combining grave accent in mac os

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Kaiido]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160606200529

Steps to reproduce:

- Set the focus to any text input, on any page, even the location-bar.
- Insert a combining grave accent ([alt]+[`] on qwerty keyboards, default [`] in azerty).
- Paste some content from the clipboard.


Actual results:

The content is pasted twice.


Expected results:

On other platforms and any other applications, this results in a closing of the combining character to it's accent grave form (`). 
Nothing should be pasted on first action.

Comment 1

[ovidiu boca[:Ovidiu]]

Tested on Mac OS X 10.10 with FF Nightly 50.0a1(2016-06-16) and I can reproduce it.

Comment 2

[Stefan [:stefanh]]

I can reproduce this with my trunk build, but not with 47. So this is a regression in 48.

Comment 3

[Stefan [:stefanh]]

I have the regression range now:
Last good build: Revision 288893 which is https://hg.mozilla.org/mozilla-central/rev/290223936152.
First bad build: Revision 288921, which is https://hg.mozilla.org/mozilla-central/rev/1d96035b9f32 (the last of the changesets in bug 1279170, comment 3)

So this is actually the same regression as the one in bug 1279170, comment #3.

Comment 4

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

(In reply to Stefan [:stefanh] from comment #3)
> I have the regression range now:
> Last good build: Revision 288893 which is
> https://hg.mozilla.org/mozilla-central/rev/290223936152.
> First bad build: Revision 288921, which is
> https://hg.mozilla.org/mozilla-central/rev/1d96035b9f32 (the last of the
> changesets in bug 1279170, comment 3)
> 
> So this is actually the same regression as the one in bug 1279170, comment
> #3.

Are you sure? bug 1249184 could cause this bug too.

Comment 5

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

Okay, this is a regression of bug 1249184. However, this is caused *not* only by bug 1249184.

Even in current release build, the Cmd+V's .key value is wrong. It includes the dead key character (i.e., committed character). For example, Opt+e -> Cmd+v causes, the .key value of "V" key is "´v". TextEventDispatcher tries to insert all of them with multiple keypress events.  However, our shortcut key handler checks .charCode value for matching with the shortcut key list.  Therefore, Cmd+v is handled twice. We need to remove the committed string from following keypress event.

Comment 6

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

Hmm, if there is composition, Cmd+v causes committing the text but not pasting the text in other applications of OS X. So, our shortcut key handling is really wrong in OS X.  On the other hand, Cmd+a works fine even if there is composition. I'm not sure what's the difference of them. However, we should keep current behavior (doing paste) in this bug because we should uplift the patch if it's enough safe.

Comment 7

[Stefan [:stefanh]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (not in London) from comment #4)
> > First bad build: Revision 288921, which is
> > https://hg.mozilla.org/mozilla-central/rev/1d96035b9f32 (the last of the
> > changesets in bug 1279170, comment 3)
> > 
> > So this is actually the same regression as the one in bug 1279170, comment
> > #3.
> 
> Are you sure? bug 1249184 could cause this bug too.

Sorry, I mixed it up - revision 288921 is actually bug 1249184.

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56f58bf4f938

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6cce5898723

Comment 10

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

Attachment: Bug 1280053 TextInputHandler should initialize WidgetKeyboardEvent without already handled characters

TextInputHandler may dispatch keypress events after InsertText() is called if there was composition and it's committed by "current" keydown event. In that case, [NSEvent characters] may have the committing string.  For example, when Opt+e of US keyboard layout started composition, Cmd+v causes committing the "`" character and pasting the clipboard text. Then, the "v" key's keydown event's |characters| is "`v". So, after InsertText() is called with "`", TextInputHandler shouldn't dispatch keypress event for "`" again. I.e., the KeyboardEvent.key value should be "v" rather than "`v".

For solving this issue, TextInputHandlerBase::AutoInsertStringClearer which is created at every InsertText() call should store the inserted string to TextInputHandlerBase::KeyEventState. However, for making the implemntation simpler, it should recode only when the inserting string is actually a part of [mKeyEvent characters]. Then, TextInputHandlerBase::KeyEventState can compute unhandled insert string at initializing WidgetKeyboardEvent.

So, finally, TextInputHandlerBase::InitKeyEvent() should be called via TextInputHandlerBase::KeyEventState::InitKeyEvent(). This ensures that all key events which may cause InsertText() calls are always initialized with unhandled string.

Review commit: https://reviewboard.mozilla.org/r/59636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59636/

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8763439 [details]
Bug 1280053 TextInputHandler should initialize WidgetKeyboardEvent without already handled characters

https://reviewboard.mozilla.org/r/59636/#review57012

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08e186081db2
TextInputHandler should initialize WidgetKeyboardEvent without already handled characters r=m_kato

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/08e186081db2

Comment 14

[Andreas Wagner [:TheOne] [use NI]]

Without knowing much about the process and being naive here (sorry), could we possibly uplift this to aurora?

Comment 15

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

(In reply to Andreas Wagner [:TheOne] from comment #14)
> Without knowing much about the process and being naive here (sorry), could
> we possibly uplift this to aurora?

Yeah, I think so, but not so really low risk due to touching a lot of place where handling keyboard event. Therefore, this is a new regression, though, we shouldn't uplift it to Beta.

Anyway, we should wait a couple of days for watching regression reports.

Comment 16

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

Comment on attachment 8763439 [details]
Bug 1280053 TextInputHandler should initialize WidgetKeyboardEvent without already handled characters

Approval Request Comment
[Feature/regressing bug #]: Regression of bug 1249184 or bug 1137563 (Anyway, new regression of 48)
[User impact if declined]: Pasting text during dead key sequence causes duplicate the copied text into the editor.
[Describe test coverage new/current, TreeHerder]: Landed on m-c and there are not regression reports.
[Risks and why]: Mid. This patch touches a lot of places of handling keyboard events since this needs to interrupt calls of InitKeyEvent(). On the other hand, I *think* that the scenario is not so major. According to the release schedule, I recommend that this should be fixed on Aurora (49). For avoiding another risk, I think that it's okay to keep this bug on Beta (48).
[String/UUID change made/needed]: Nothing.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8763439 [details]
Bug 1280053 TextInputHandler should initialize WidgetKeyboardEvent without already handled characters

OK, let's take it in aurora and make a discussion for beta later.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fe554517e032

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Masayuki, please let me know when/if we can land this in beta?

Comment 20

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #19)
> Masayuki, please let me know when/if we can land this in beta?

It's possible anytime (I succeeded to do |hg graft|). As I said in comment 16, there is only a problem of risk management.

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8763439 [details]
Bug 1280053 TextInputHandler should initialize WidgetKeyboardEvent without already handled characters

Based on comment #16, the risk is medium and it's okay to keep this bug on Beta (48). Let's won't fix in 48 beta.