Closed Bug 1280053 Opened 8 years ago Closed 8 years ago

content pasted twice after combining grave accent in mac os

Categories

(Core :: Widget: Cocoa, defect)

48 Branch
All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- unaffected
firefox48 --- wontfix
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: tristan.fraipont, Assigned: masayuki)

References

Details

(Keywords: inputmethod, regression)

Attachments

(1 file)

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.
Tested on Mac OS X 10.10 with FF Nightly 50.0a1(2016-06-16) and I can reproduce it.
Status: UNCONFIRMED → NEW
Component: Untriaged → Widget: Cocoa
Ever confirmed: true
Product: Firefox → Core
I can reproduce this with my trunk build, but not with 47. So this is a regression in 48.
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.
Blocks: 1137563
Flags: needinfo?(masayuki)
(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.
Assignee: nobody → masayuki
Flags: needinfo?(masayuki)
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.
Status: NEW → ASSIGNED
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.
(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.
Blocks: 1249184
No longer blocks: 1137563
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/
Attachment #8763439 - Flags: review?(m_kato)
Attachment #8763439 - Flags: review?(m_kato) → review+
Comment on attachment 8763439 [details]
Bug 1280053 TextInputHandler should initialize WidgetKeyboardEvent without already handled characters

https://reviewboard.mozilla.org/r/59636/#review57012
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08e186081db2
TextInputHandler should initialize WidgetKeyboardEvent without already handled characters r=m_kato
https://hg.mozilla.org/mozilla-central/rev/08e186081db2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Without knowing much about the process and being naive here (sorry), could we possibly uplift this to aurora?
(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 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.
Attachment #8763439 - Flags: approval-mozilla-beta?
Attachment #8763439 - Flags: approval-mozilla-aurora?
OS: Unspecified → Mac OS X
Hardware: Unspecified → All
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.
Attachment #8763439 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Masayuki, please let me know when/if we can land this in beta?
Flags: needinfo?(masayuki)
(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.
Flags: needinfo?(masayuki)
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.
Attachment #8763439 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Depends on: 1309515
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: