1670834 - Firefox 81.0 in Windows handles some key events from Keyman incorrectly

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Marc Durdin]

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:

1. Install Keyman 13 together with the SIL IPA keyboard, e.g. https://keyman.com/keyboards/sil_ipa
2. Start Keyman and select the SIL IPA keyboard
3. 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̽

Comment 1

[Marc Durdin]

When I tested this

Comment 2

[Marc Durdin]

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

[Henri Sivonen (:hsivonen)]

Confirmed.

Masayuki, can you, please, check what's going on?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Well, I could try to take a look next week.

Comment 5

[Marc Durdin]

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.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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...

Comment 7

[Marc Durdin]

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).

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(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.)

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1670834 - Make `TSFTextStore` stop setting selection with expanding it to cluster boundaries in Nightly channel and early beta builds or active TIP is Keyman Desktop r=m_kato!

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

Comment 10

[Marc Durdin]

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.

1. I suggest using eSILKeyman, not eKeymanIPA_SIL (Keyman is not just an IPA input method -- it currently supports over 1800 different languages).
2. 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;
    }

3. 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.)

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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)

1. I suggest using eSILKeyman, not eKeymanIPA_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".

2. 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.

Comment 12

[Marc Durdin]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60be841f0025
Make `TSFTextStore` stop setting selection with expanding it to cluster boundaries in Nightly channel and early beta builds or active TIP is Keyman Desktop r=m_kato

Comment 14

[Narcis Beleuzu [:NarcisB]]

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

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(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.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

I guess that it's caused by https://phabricator.services.mozilla.com/D97954

Comment 17

[Narcis Beleuzu [:NarcisB]]

Yup. Re-landing
Sorry for the confusion

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/655eb620b6c8
Make `TSFTextStore` stop setting selection with expanding it to cluster boundaries in Nightly channel and early beta builds or active TIP is Keyman Desktop r=m_kato

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/655eb620b6c8