1309515 - Double composite Arabic characters

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Abd Allah Diab]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160922113459

Steps to reproduce:

Running Mac OS X 10.11.6 El Capitan, Firefox 49.0.1, I have the Arabic PC layout in my keyboard layout settings, switch to the Arabic PC layout, and then press each one of the following key combinations on a qwerty keyboard:

1. Shift+G
2. Shift+T
3. Shift+B
4. B


Actual results:

You would get the same character twice:

1. لألأ
2. لإلإ
3. لآلآ
4. لالا



Expected results:

Each character must be show once:


1. لأ
2. لإ
3. لآ
4. لا

---

Extra info:

These are composed of the letter lam (ل) and one of the letters (إ, أ, آ, ا), and they have their own Unicode characters and have their own key combination to make it faster to write.

Typing the letter lam and then one of the other letters doesn't produce the problem, but pressing on the combinations above produces it.

I didn't have this problem before, so I guess it's new in the latest update.

Comment 1

[Abd Allah Diab]

Just a note also, I disabled all extensions and add-ons and made sure I don't have anything enabled, and the problem persists.

Comment 2

[Tooru Fujisawa [:arai]]

Confirmed on Firefox 49.0.1 and Nightly 52.0a1 (2016-10-11) (64-bit).

Comment 3

[Tooru Fujisawa [:arai]]

regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=da005aa1d83c4a32c2ff1d211d359581013352ed&tochange=08e186081db2275a88437b51e925b92f13f572dc

seems like a regression from Bug 1280053

Comment 4

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

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

Comment 5

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

Attachment: Bug 1309515 part.1 Add automated tests for Arabic - PC keyboard layout which can input 2 characters with a key press

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

Comment 6

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

Attachment: Bug 1309515 part.2 TextInputHandler::InsertText() should consume current key event when it dispatches composition events

The cause of bug 1309515 is, HandleKeyDownEvent() dispatches eKeyPress events even after InsertText() dispatches composition events via InsertTextAsCommittingComposition().

Therefore, this patch consumes the current key event after dispatching composition events.

Note that for consistency with Windows, InsertText() should use eKeyPress events rather than composition events at least in this case.  However, changing the behavior has some risk.  So, we should fix this bug with the safest hack for uplift.

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

Comment 7

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

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

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8800548 [details]
Bug 1309515 part.1 Add automated tests for Arabic - PC keyboard layout which can input 2 characters with a key press

https://reviewboard.mozilla.org/r/85468/#review84376

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8800549 [details]
Bug 1309515 part.2 TextInputHandler::InsertText() should consume current key event when it dispatches composition events

https://reviewboard.mozilla.org/r/85470/#review84394

Comment 10

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

Comment on attachment 8800548 [details]
Bug 1309515 part.1 Add automated tests for Arabic - PC keyboard layout which can input 2 characters with a key press

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85468/diff/1-2/

Comment 11

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

Comment on attachment 8800549 [details]
Bug 1309515 part.2 TextInputHandler::InsertText() should consume current key event when it dispatches composition events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85470/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8eeb7978e143
part.1 Add automated tests for Arabic - PC keyboard layout which can input 2 characters with a key press r=m_kato
https://hg.mozilla.org/integration/autoland/rev/093adc51a4db
part.2 TextInputHandler::InsertText() should consume current key event when it dispatches composition events r=m_kato

Comment 13

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

Attachment: Bug 1309515 part.1 Add automated tests for Arabic - PC keyboard layout which can input 2 characters with a key press (for aurora)

Only part.1 cannot be merged with Aurora nor Beta because bug 1303273 added some keyboard layouts for Windows in EventUtils.js.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8eeb7978e143
https://hg.mozilla.org/mozilla-central/rev/093adc51a4db

Comment 15

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

Comment on attachment 8800994 [details] [diff] [review]
Bug 1309515 part.1 Add automated tests for Arabic - PC keyboard layout which can input 2 characters with a key press (for aurora)

Approval Request Comment
[Feature/regressing bug #]: regression of bug 1230053
[User impact if declined]: Some Arabic users who use "Arabic - PC" keyboard layout cannot type text as they expected.
[Describe test coverage new/current, TreeHerder]: Landed on m-c at last week.
[Risks and why]: Low because this just stops dispatching eKeyPress events if a set of composition events are fired during handling a native NSKeyDown event.
[String/UUID change made/needed]: Nothing.

Comment 16

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

Comment on attachment 8800549 [details]
Bug 1309515 part.2 TextInputHandler::InsertText() should consume current key event when it dispatches composition events

Approval Request Comment: See the previous comment.

Comment 17

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

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #15)
> [Feature/regressing bug #]: regression of bug 1230053

Oops, the bug# is wrong. It's bug 1280053.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Abd Allah Diab, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8800994 [details] [diff] [review]
Bug 1309515 part.1 Add automated tests for Arabic - PC keyboard layout which can input 2 characters with a key press (for aurora)

Let's uplift to Aurora51 to stabilize. I'll be inclined to take this in Beta50 only if there is a verification, otherwise we will let it ride the 51 train.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f19b4ba41c4f
https://hg.mozilla.org/releases/mozilla-aurora/rev/1814128f8755

Comment 21

[Abd Allah Diab]

Tested firefox-52.0a1 and the problem is fixed.

Thank you guys!

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Abd Allah Diab from comment #21)
> Tested firefox-52.0a1 and the problem is fixed.
> 
> Thank you guys!

Thanks a lot for a prompt verification.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8800994 [details] [diff] [review]
Bug 1309515 part.1 Add automated tests for Arabic - PC keyboard layout which can input 2 characters with a key press (for aurora)

Fix was verified on Nightly52, has been on Aurora51 for a few days, Beta50+

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5eb505aa4773
https://hg.mozilla.org/releases/mozilla-beta/rev/60d80f10cbad