48.0b1 breaks OSX Press and hold for accents

VERIFIED FIXED in Firefox 48

Status

()

Core
Widget: Cocoa
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: Thomas Maurin, Assigned: masayuki)

Tracking

({regression})

50 Branch
mozilla50
x86
Mac OS X
regression
Points:
---

Firefox Tracking Flags

(firefox47 unaffected, firefox48+ verified, firefox49+ verified, firefox50+ verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8761497 [details]
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
Keywords: regressionwindow-wanted
OS: Unspecified → Mac OS X
Product: Firefox → Core
Hardware: Unspecified → x86
Version: 48 Branch → 50 Branch
(Reporter)

Comment 2

2 years ago
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
Keywords: regressionwindow-wanted → regression

Comment 4

a year ago
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
status-firefox47: --- → unaffected
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected
tracking-firefox48: --- → ?
tracking-firefox49: --- → ?
tracking-firefox50: --- → ?
Flags: needinfo?(masayuki)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a0fd6dea65e
Created attachment 8762993 [details]
Bug 1279170 TextInputHandler::InsertText() should set keypress event's .key value property when it replaces specified range with a character

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

Comment 9

a year ago
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?

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e16558233f9
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Tracking 50+ for this regression which is now fixed.
tracking-firefox50: ? → +
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
status-firefox50: fixed → 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')
status-firefox49: affected → fixed
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.
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.
Flags: needinfo?(masayuki) → needinfo?(cbook)

Comment 19

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/2c01d8986842
status-firefox48: affected → fixed
(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+
tracking-firefox48: ? → +
tracking-firefox49: ? → +
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.
status-firefox48: fixed → verified
status-firefox49: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.