Closed Bug 1279170 Opened 3 years ago Closed 3 years ago

48.0b1 breaks OSX Press and hold for accents

Categories

(Core :: Widget: Cocoa, defect)

50 Branch
x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox47 --- unaffected
firefox48 + verified
firefox49 + verified
firefox50 + verified

People

(Reporter: thomas, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached image OSX Accent menu example
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160606200529

Steps to reproduce:

- Long press a key which has an accent menu (like "e") in a text field
- Select an accentuated character using a number key (no issue if using arrow keys or mouse to select the accentuated character)


Actual results:

- The accent menu disappears (expected)
- The character doesn't get accentuated (not expected)
- The character gets selected (not expected)


Expected results:

- The accent menu disappears
- The character is replaced by the selected accentuated character (like "e" > "é")

Un e
Hi,
I tested on Mac OS X 10.11 with Firefox Nightly 50.0a1 (2016-06-12) and Firefox 47 release. I can reproduce the issue on Nightly but I can't reproduce it on release 47. I think this is a regression. I will come back with a regression range. 

 Please try Firefox 47 release and see if you can reproduce this issue.
Status: UNCONFIRMED → NEW
Component: Untriaged → Widget: Cocoa
Ever confirmed: true
OS: Unspecified → Mac OS X
Product: Firefox → Core
Hardware: Unspecified → x86
Version: 48 Branch → 50 Branch
I can't reproduce it either on the release version of 47.0.
I can reproduce it though on 48.0b1 (as reported above) as well as 49.0a2 2016-06-12 (Dev Edition)
I did a regression and here are the results:

Last good revision: 29022393615224517283b16adae938128f75e27b
First bad revision: eb7c36e2ef5d48262bc8566da9ea37623e7d0883
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=29022393615224517283b16adae938128f75e27b&tochange=eb7c36e2ef5d48262bc8566da9ea37623e7d0883
The push contained fixes for 5 bugs: bug 1137572, 1137563, 1137565, 1137561. I'm guessing that one of the patches in bug 1137563 caused this.
Blocks: 1137563
Flags: needinfo?(masayuki)
It must be so. Taking.

[Tracking Requested - why for this release]:

Serious regression of bug 1137563 according to comment 3. And it's impossible to back them out since a lot of bug fixes are based on the landing.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
TextEventDispatcher::MaybeDispatchKeypressEvents() dispatches keypress events with passed event's mKeyNameIndex and mKeyValue values. I.e., setting mCharCode doesn't make sense in this case. Similarly, mKeyCode value is also ignored (overwritten by 0) if it's printable key's key event (mKeyNameIndex == KEY_NAME_INDEX_USE_STRING).

Review commit: https://reviewboard.mozilla.org/r/59380/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59380/
Attachment #8762993 - Flags: review?(m_kato)
Attachment #8762993 - Flags: review?(m_kato) → review+
Comment on attachment 8762993 [details]
Bug 1279170 TextInputHandler::InsertText() should set keypress event's .key value property when it replaces specified range with a character

https://reviewboard.mozilla.org/r/59380/#review56458
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e16558233f9
TextInputHandler::InsertText() should set keypress event's .key value property when it replaces specified range with a character r=m_kato
Comment on attachment 8762993 [details]
Bug 1279170 TextInputHandler::InsertText() should set keypress event's .key value property when it replaces specified range with a character

Approval Request Comment
[Feature/regressing bug #]: Important regression of bug 1137563.
[User impact if declined]: Cannot input accented characters with long press of a key on OS X. And some IME might use same path for inputting a character without key events.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Low because it's really special path. Cocoa calls InputText() with replacing range when user does NOT press a key. In such case, the inserting string is only one character, user meet this bug.  In other words, most work of IMEs won't meet this bug.
[String/UUID change made/needed]: Nothing.
Attachment #8762993 - Flags: approval-mozilla-beta?
Attachment #8762993 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/4e16558233f9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Tracking 50+ for this regression which is now fixed.
I verified this on Mac OS X 10.10 FF Nightly 50.0a1(2016-06-20) and everything works as expected. I can't reproduce this issue.
Status: RESOLVED → VERIFIED
Comment on attachment 8762993 [details]
Bug 1279170 TextInputHandler::InsertText() should set keypress event's .key value property when it replaces specified range with a character

Fix a major user facing regression, taking it in aurora & beta
Should be in 48 beta 3
Attachment #8762993 - Flags: approval-mozilla-beta?
Attachment #8762993 - Flags: approval-mozilla-beta+
Attachment #8762993 - Flags: approval-mozilla-aurora?
Attachment #8762993 - Flags: approval-mozilla-aurora+
Could we have some automated tests for this so that we don't regress?
 https://hg.mozilla.org/releases/mozilla-aurora/rev/5337f17b3c1f

but has problems on uplift to beta:

grafting 350775:5337f17b3c1f "Bug 1279170 - TextInputHandler::InsertText() should set keypress event's .key value property when it replaces specified range with a character. r=m_kato, a=sylvestre" (tip)
merging widget/cocoa/TextInputHandler.mm
warning: conflicts while merging widget/cocoa/TextInputHandler.mm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(masayuki)
(In reply to Sylvestre Ledru [:sylvestre] from comment #15)
> Could we have some automated tests for this so that we don't regress?

Unfortunately, no. We don't have a way to emulate the behavior with our testing API.

(In reply to Carsten Book [:Tomcat] from comment #16)
>  https://hg.mozilla.org/releases/mozilla-aurora/rev/5337f17b3c1f
> 
> but has problems on uplift to beta:
> 
> grafting 350775:5337f17b3c1f "Bug 1279170 - TextInputHandler::InsertText()
> should set keypress event's .key value property when it replaces specified
> range with a character. r=m_kato, a=sylvestre" (tip)
> merging widget/cocoa/TextInputHandler.mm
> warning: conflicts while merging widget/cocoa/TextInputHandler.mm! (edit,
> then use 'hg resolve --mark')
> abort: unresolved conflicts, can't continue
> (use 'hg resolve' and 'hg graft --continue')

Oh, okay, I'll check it and post a patch for beta tomorrow.
Here is a patch for beta. Bug 1254755 which renamed members of WidgetKeyboardEvent (appending "m" prefix) is the cause of the conflict. Therefore, this patch doesn't change any logic.
Flags: needinfo?(masayuki) → needinfo?(cbook)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (not in London) from comment #18)
> Created attachment 8764085 [details] [diff] [review]
> Patch for beta (coflict reason is bug 1254755, so, not changed anything
> except member names of WidgetKeyboardEvent)
> 
> Here is a patch for beta. Bug 1254755 which renamed members of
> WidgetKeyboardEvent (appending "m" prefix) is the cause of the conflict.
> Therefore, this patch doesn't change any logic.


thanks landed!
Flags: needinfo?(cbook)
Flags: qe-verify+
I verified this on Mac OS X 10.10 on Firefox Aurora 49.0a2 (2016-06-29) and Firefox Beta 48.0b4, I couldn't reproduce the issue.
You need to log in before you can comment on or make changes to this bug.