[e10s][TSF] When typing fast, IME composition may be committed unexpectedly and input won't cause text input after that

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks 1 bug, {inputmethod})

Trunk
mozilla47
x86
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox46 fixed, firefox47 fixed)

Details

Attachments

(5 attachments, 5 obsolete attachments)

1.50 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
1.29 KB, patch
Details | Diff | Splinter Review
9.04 KB, patch
Details | Diff | Splinter Review
8.00 KB, patch
Details | Diff | Splinter Review
7.72 KB, patch
Details | Diff | Splinter Review
I sometimes reproduce this bug when the content process is busy.

For example, I can reproduce this bug with following STR:

1. Launch debug build
2. Open TweetDeck
3. Open Twitter.com
4. Type Japanese with TSF and commit it
5. Turn off IME and type spacebar
6. Turn on IME and type some Japanese characters

Do very fast during #4 - #6. Then, the text change notification for #5 will be notified during #6. At this time, TSFTextStore tries to commit the composition forcibly but after that, TSF won't work until removing "hidden" composition string by Backspace key presses.
First, we should remove *unused* methods which makes both SelectionChangeDataBase and TextChangeDataBase have same members.
Attachment #8712119 - Flags: review?(bugs)
TextChangeDataBase may be merged multiple text changes in the editor. So, making mCausedByComposition's meaning clearer, we should rename it to mCausedOnlyByComposition.
Attachment #8712120 - Flags: review?(bugs)
mOccurredDuringComposition isn't unclear like mCausedByComposition. However, a view from widget, this should indicate if the change includes some changes which is NOT caused by composition during the *latest* composition.

Therefore, I rename this to mIncludingChangesDuringComposition. Although, mIncludingNonCompositionChangesDuringComposition is better for self-documentation, but it's too long for our coding rules :-(
Attachment #8712122 - Flags: review?(bugs)
We should think that any changes which occurred before starting current composition is changed by *unknown* sources. By this approach, widget can notify IME of text changes as not caused by *current* composition even if they are late to notify IME due to asynchronous e10s.

So, for making such delayed notifications notify IME later, we should record it to mIncludingChangesWithoutComposition. I have a plan to do that in next Q2.
Attachment #8712125 - Flags: review?(bugs)
If (merged) text change notification is received during composition, even if the notification includes some changes which occurred *before* starting composition, TSFTextStore should ignore it.

Ideally, TSFTextStore should store the text changes and notify TSF of them after ending current composition and adjust offset of queries from TIP with the changes. However, it requires a lot of work. I have a plan to redesign TSFTextStore for that, but I don't have much time. I'll perhaps do it in Q2. So, for now, we should just ignore the delayed notifications. (I don't find any regressions with Japanese TIPs.)
Attachment #8712129 - Flags: review?(m_kato)
Attachment #8712119 - Flags: review?(bugs) → review+
Comment on attachment 8712120 [details] [diff] [review]
part.2 Rename TextChangeDataBase::mCausedByComposition to mCausedOnlyByComposition

+    // Note that TextChangeDataBase may be merged two or more changes
+    // especially in e10s mode.
sounds a bit odd.

Perhaps 
Note that TextChangeDataBase may be the result of merging two or more changes
especially in e10s mode.
Attachment #8712120 - Flags: review?(bugs) → review+
Comment on attachment 8712122 [details] [diff] [review]
part.3 Rename TextChangeDataBase::mOccurredDuringComposition to mIncludingChangesDuringComposition

mIncludingNonCompositionChangesDuringComposition would be indeed better but starting to get rather long, so mIncludingChangesDuringComposition is fine.

"there is some changes" s/is/are/

-      mOccurredDuringComposition = aOccurredDuringComposition;
+      mIncludingChangesDuringComposition =
+        !aCausedByComposition && aOccurredDuringComposition;
Aha, this isn't just rename

+  // mIncludingChangesDuringComposition should be true when at least one of
+  // merged changes occurred during the latest composition.
'.. one of the merged...'
But should it be
"mIncludingChangesDuringComposition should be true when at least one of
 the merged non-composition changes occurred during the latest composition."



"IME doesn't want need outdated text changes as text change during"
IME doesn't want outdated...


"So, we should treat the outdated text changes occurred not during composition."
that is a bit oddly said, but don't have good suggestion to fix that. Perhaps drop that sentence.
Attachment #8712122 - Flags: review?(bugs) → review+
Comment on attachment 8712125 [details] [diff] [review]
part.4 Add TextChangeDataBase::mIncludingChangesWithoutComposition

>+    // mIncludingChangesWithoutComposition is true if at least one change which
>+    // is not caused when there is no composition.
>+    bool mIncludingChangesWithoutComposition;
s/if at least one/if there is at least one/


s/which is not caused when there is no composition/which did occur when there wasn't composition ongoing/
Something like that perhaps?


>   if (!newData.mCausedOnlyByComposition &&
>       !newData.mIncludingChangesDuringComposition) {
>     // If new change is neither caused by composition nor occurred during
>     // composition, set mIncludingChangesDuringComposition to false because
>     // IME doesn't want need outdated text changes as text change during
>     // current composition.  So, we should treat the outdated text changes
>     // occurred not during composition.
>+    if (oldData.mIncludingChangesDuringComposition) {
>+      mIncludingChangesWithoutComposition = true;
>+    }
I don't understand this 'if'. Why don't we set mIncludingChangesWithoutComposition always true here?
Please clarify and add some comment or fix. I'd like to see an update patch for this.
Attachment #8712125 - Flags: review?(bugs) → review-
Indeed, you're right. In that case, newData.mIncludingChangesWithoutComposition should be always true. Therefore, mIncludingChangesWithoutComposition of the instance should be set to true above.

I just added MOZ_ASSERT()s into the if block.
Attachment #8712125 - Attachment is obsolete: true
Attachment #8712524 - Flags: review?(bugs)
Attachment #8712129 - Flags: review?(m_kato) → review+
Comment on attachment 8712524 [details] [diff] [review]
part.4 Add TextChangeDataBase::mIncludingChangesWithoutComposition

"...did occur where there wasn't composition ongoing..."
s/where/when/

(I think it should be also 'a composition', not just 'composition')
Attachment #8712524 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b21fb00a6138ea698c6fd54053b2aa73db012d1e
Bug 1242331 part.1 Remove unused methods of IMENotification r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/66b71325dfbf5cf32f2eb20a724d0dfcd805ccd0
Bug 1242331 part.2 Rename TextChangeDataBase::mCausedByComposition to mCausedOnlyByComposition r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/3442a539cc8b8f2a7ab504f1e3fad11541579652
Bug 1242331 part.3 Rename TextChangeDataBase::mOccurredDuringComposition to mIncludingChangesDuringComposition r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/04d8a95c6a5e19ca4debf62a052e8011cf6af478
Bug 1242331 part.4 Add TextChangeDataBase::mIncludingChangesWithoutComposition r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/2432259685a715b5ed9376bd2709a44246f90c8c
Bug 1242331 part.5 TSFTextStore should ignore text changes not caused by composition but not occurred during current composition r=m_kato
Comment on attachment 8712520 [details] [diff] [review]
part.1 Remove unused methods of IMENotification (r=smaug)

Approval Request Comment
[Feature/regressing bug #]: regression of e10s
[User impact if declined]: when user composing with IME, the composition string may be committed suddenly. (of course, unexpectedly.)
[Describe test coverage new/current, TreeHerder]: landed m-c, and tested manually with try build of aurora.
[Risks and why]: The cause is, typed text is handled by content asynchronously. Therefore, when user composes text with IME, previous text change before current composing may be notified from content process. Then, our TSF module commits composition forcibly for preventing TSF or TIP (IME for TSF) to be confused. This patch ignores text changes before starting current composition. Therefore, this prevents delayed notification from content process blocks user's composition.  However, there is a demerit.  The new behavior may cause showing candidate window or suggest window of TIP at wrong position because content process may still not handle whole input events.  However, that's smaller problem than this bug.  So, I strongly recommend land this patch.
[String/UUID change made/needed]: Nothing.
Attachment #8712520 - Flags: approval-mozilla-aurora?
Comment on attachment 8712522 [details] [diff] [review]
part.2 Rename TextChangeDataBase::mCausedByComposition to mCausedOnlyByComposition (r=smaug)

Approval Request Comment: See comment 20.
Attachment #8712522 - Flags: approval-mozilla-aurora?
Comment on attachment 8712523 [details] [diff] [review]
part.3 Rename TextChangeDataBase::mOccurredDuringComposition to mIncludingChangesDuringComposition (r=smaug)

Approval Request Comment: See comment 20.
Attachment #8712523 - Flags: approval-mozilla-aurora?
Comment on attachment 8718861 [details] [diff] [review]
part.4 Add TextChangeDataBase::mIncludingChangesWithoutComposition (r=smaug)

Approval Request Comment: See comment 20.
Attachment #8718861 - Flags: approval-mozilla-aurora?
Comment on attachment 8712129 [details] [diff] [review]
part.5 TSFTextStore should ignore text changes not caused by composition but not occurred during current composition

Approval Request Comment: See comment 20.
Attachment #8712129 - Flags: approval-mozilla-aurora?
Comment on attachment 8712520 [details] [diff] [review]
part.1 Remove unused methods of IMENotification (r=smaug)

Has had manual testing, should improve text editing for some users with e10s.
OK to uplift these patches to aurora.
Attachment #8712520 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8712522 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8712523 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8718861 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8712129 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.