Firefox 81.0 in Windows handles some key events from Keyman incorrectly
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox85 | --- | fixed |
People
(Reporter: marc, Assigned: masayuki)
Details
(Keywords: inputmethod)
Attachments
(1 file)
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.102 Safari/537.36
Steps to reproduce:
Steps to reproduce the problem in Firefox 81.0.1 on Windows 10:
- Install Keyman 13 together with the SIL IPA keyboard, e.g. https://keyman.com/keyboards/sil_ipa
- Start Keyman and select the SIL IPA keyboard
- Type xa** into any text field (or address bar) in Firefox.
In Firefox 80.0, the output is correct; you can also observe correct behaviour as a baseline test in other applications, such as Notepad or MS Word.
See also Keyman issue report at https://github.com/keymanapp/keyman/issues/3704.
Actual results:
The resultant output is x̽
When you press * the first time, Keyman inserts a diaresis combining diacritic (U+0308).
When you press * the second time, Keyman issues an event to delete the diaresis diacritic character to the left of the insertion point, and replace it with a U+033D combining x above.
However, it appears that Firefox is deleting both the diacritic and the base characters when it receives a backspace event from Keyman, in this situation. If a backspace is received when a non-Keyman keyboard is active, only the diacritic is removed.
Expected results:
The output should be: xa̽
Updated•4 years ago
|
Reporter | ||
Comment 1•4 years ago
|
||
When I tested this
Reporter | ||
Comment 2•4 years ago
|
||
When I tested this earlier, I tested in Firefox 80 and believed the output was correct. I re-tested just now and found the same, incorrect behaviour in Firefox 80 as well as in earlier versions. Hence I have removed the "Regression" label. Apologies for the confusion.
Comment 3•4 years ago
|
||
Confirmed.
Masayuki, can you, please, check what's going on?
Assignee | ||
Comment 4•4 years ago
|
||
Well, I could try to take a look next week.
Reporter | ||
Comment 5•4 years ago
|
||
What Keyman is doing here is deleting n
characters prior to insertion point, and then inserting new characters. It was going wrong because instead of just 1 character being deleted, when it was a combining mark, it would extend the selection to the base, to cover the whole cluster.
I believe this may be able to be fixed in TSFTextStore.cpp:TSFTextStore::FlushPendingActions()
.
if (action.mAdjustSelection) {
// Select composition range so the new composition replaces the range
WidgetSelectionEvent selectionSet(true, eSetSelection, widget);
widget->InitEvent(selectionSet);
selectionSet.mOffset = static_cast<uint32_t>(action.mSelectionStart);
selectionSet.mLength = static_cast<uint32_t>(action.mSelectionLength);
selectionSet.mReversed = false;
selectionSet.mExpandToClusterBoundary = false; /// <<--- add flag to avoid expanding the selection
DispatchEvent(selectionSet);
if (!selectionSet.mSucceeded) {
MOZ_LOG(sTextStoreLog, LogLevel::Error,
("0x%p TSFTextStore::FlushPendingActions() "
"FAILED due to eSetSelection failure",
this));
break;
}
}
I don't think Firefox should be modifying the selection that the input method requests when the input method is sending precise range modifications as part of its text manipulation.
I have tested this patch locally and it appears to resolve the issue for Keyman. I don't know if other input methods will negatively impacted though.
Assignee | ||
Comment 6•4 years ago
|
||
Thank you for your investigation, and sorry for the delay due to some my private reasons.
Yes, the fix makes sense even though it's risky because in my experience, some IMEs do hack for some bugs of Gecko with checking the window class, but they usually don't report to us. And I still don't get what's going on. Keyman is indeed listed up in the lang bar, but I cannot catch switching keyboard layout in TSF level. If we could catch it, we could enable new behavior only in the Nightly channel and beta channel for any IMEs, and ride it on the train only for Keyman first...
Assignee | ||
Updated•4 years ago
|
Reporter | ||
Comment 7•4 years ago
|
||
Thanks for the reply :)
And I still don't get what's going on. Keyman is indeed listed up in the lang bar, but I cannot catch switching keyboard layout in TSF level.
I am not quite sure I understand what you mean here?
ride it on the train only for Keyman first...
Certainly I think it would be a good idea to restrict this change for Keyman initially. I note that you store guidProfile in TSFStaticSink::OnActivated. Because Keyman installs keyboard layouts for a vast array of languages, it generates a guidProfile on install of the keyboard, and so you can't compare against that. However, Keyman's CLSID is constant: {FE0420F1-38D1-4B4C-96BF-E7E20A74CFB7}, so you could compare against that (means adding mActiveTIPCLSID to TSFStaticSink I guess).
(I note that there are comments about GUID comparison being slow -- I don't think this is really a big issue to worry about given it's computed just once on input method activation and not every keystroke, and even then it's not going to have a measurable impact).
Assignee | ||
Comment 8•4 years ago
|
||
(In reply to Marc Durdin from comment #7)
Thanks for the reply :)
And I still don't get what's going on. Keyman is indeed listed up in the lang bar, but I cannot catch switching keyboard layout in TSF level.
I am not quite sure I understand what you mean here?
Sorry, ignore this. I forgot registering ITfInputProcessorProfileActivationSink
when active TIP is checked first time. So, I tried to debug it without running a path to register the sink. Now, I confirmed that its OnActivated()
is called as expected.
ride it on the train only for Keyman first...
I note that you store guidProfile in TSFStaticSink::OnActivated. Because Keyman installs keyboard layouts for a vast array of languages, it generates a guidProfile on install of the keyboard, and so you can't compare against that. However, Keyman's CLSID is constant: {FE0420F1-38D1-4B4C-96BF-E7E20A74CFB7}, so you could compare against that (means adding mActiveTIPCLSID to TSFStaticSink I guess).
Thank you for the information! I've never realized that if you mentioned it!
(I note that there are comments about GUID comparison being slow -- I don't think this is really a big issue to worry about given it's computed just once on input method activation and not every keystroke, and even then it's not going to have a measurable impact).
We mainly check active TIP for ITextStoreACP::GetTextExt()
because most TIPs are not async layout aware and previously, TSF had a bug which converts returned TS_E_NOLAYOUT
to E_FAIL
from the point of view of TIP. CJKT TIPs call it a lot unfortunately and it's required if TIPs want to move their window at scrolling. Therefore, we'd love to avoid the expensive comparison as far as possible. (We experienced the slowness with QueryInterface
of XPCOM objects, and it was a long standing issue of Gecko's performance until shipping Firefox Quantum.)
Assignee | ||
Comment 9•4 years ago
|
||
Keyman's IPA (SIL) tries to delete only a combined diacritical mark when user
toggles it. However, TSFTextStore
uses eSetSelection
event without setting
mExpandToClusterBoundary
. Its default value is true
so that always the
previous base character of the deleting diacritical mark is also selected
and deleted.
I think that it should be set to false
when the selection range is specified
by IME framework and IME itself. However, some TIPs may have already done
some hack if focused window is ours. For avoiding the regression risk, we
should change our behavior only when
- Keyman's IPA (SIL) is active
- In the Nightly channel or early beta builds
Reporter | ||
Comment 10•4 years ago
|
||
The changes look good, thank you; I took a look at the attachment and have a few comments; I am unable to submit a review on phabricator so sticking them here for now. Mostly cosmetic, but one important thing.
- I suggest using
eSILKeyman
, noteKeymanIPA_SIL
(Keyman is not just an IPA input method -- it currently supports over 1800 different languages). - The check against Keyman's CLSID must be done for any language code, not just the set of transient language codes; this means it should be tested immediately after the GUID_NULL check, something like:
if (mActiveTIPGUID == GUID_NULL) {
mActiveTIP = TextInputProcessorID::eNone;
return;
}
mActiveTIP = ComputeActiveTIPAsOthers();
if(mActiveTIP != TextInputProcessorID::eNotComputed) {
return;
}
- I tweaked the ComputeActiveTIPAsOthers function:
TextInputProcessorID ComputeActiveTIPAsOthers() {
// Note that SIL Keyman assigns its GUID on install randomly, but
// CLSID is constant in any environments.
// https://bugzilla.mozilla.org/show_bug.cgi?id=1670834#c7
// https://github.com/keymanapp/keyman/blob/318c73a9e1d571d942837ff9964590626e5bd5aa/windows/src/engine/kmtip/globals.cpp#L37
// {FE0420F1-38D1-4B4C-96BF-E7E20A74CFB7}
static constexpr CLSID kSILKeyman_CLSID = {
0xFE0420F1,
0x38D1,
0x4B4C,
{0x96, 0xBF, 0xE7, 0xE2, 0x0A, 0x74, 0xCF, 0xB7}};
if (mActiveTIPCLSID == kSILKeyman_CLSID) {
return TextInputProcessorID::eSILKeyman;
}
return TextInputProcessorID::eNotComputed;
}
(You may want to rename it as it is not really correct to call it Others
now.)
Assignee | ||
Comment 11•4 years ago
|
||
Really thank you for your reviewǃ I don't know why you cannot use Phablicator. It might be permission issue of bugzilla account if you tried to connect the bugzilla's account...
(In reply to Marc Durdin from comment #10)
- I suggest using
eSILKeyman
, noteKeymanIPA_SIL
(Keyman is not just an IPA input method -- it currently supports over 1800 different languages).
Well, I installed another language, but the CLSID are same. So, the CLSID indicates "Keyman Deskop"? Then, I think that eKeymanDesktop
must be better than containing "SIL".
- The check against Keyman's CLSID must be done for any language code, not just the set of transient language codes; this means it should be tested immediately after the GUID_NULL check, something like:
Hmm, I still see non-other lang ID in "Keyman Desktop", but checking it without the lang code is fine for checking just one TIP.
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Reporter | ||
Comment 12•4 years ago
|
||
Updated code changes look great! Note that we are renaming 'Keyman Desktop for Windows' to 'Keyman for Windows' with upcoming version 14, but that won't impact your code.
Comment 13•4 years ago
|
||
Comment 14•4 years ago
|
||
Backed out for MinGW bustages on ToString.h
Backout link: https://hg.mozilla.org/integration/autoland/rev/d20bc2412405792bc88466f736d4c50d12261ccf
Log link: https://treeherder.mozilla.org/logviewer?job_id=322876844&repo=autoland&lineNumber=76235
Assignee | ||
Comment 15•4 years ago
|
||
(In reply to Narcis Beleuzu [:NarcisB] from comment #14)
Backed out for MinGW bustages on ToString.h
Backout link: https://hg.mozilla.org/integration/autoland/rev/d20bc2412405792bc88466f736d4c50d12261ccf
Log link: https://treeherder.mozilla.org/logviewer?job_id=322876844&repo=autoland&lineNumber=76235
I think that you backed out wrong patch. I guess that the bustage is caused by a patch for bug 1678553.
Assignee | ||
Comment 16•4 years ago
|
||
I guess that it's caused by https://phabricator.services.mozilla.com/D97954
Comment 17•4 years ago
|
||
Yup. Re-landing
Sorry for the confusion
Comment 18•4 years ago
|
||
Comment 19•4 years ago
|
||
bugherder |
Assignee | ||
Updated•4 years ago
|
Description
•