1125934 - Cannot choose suggestion list item in about:home when TSF is enabled and composing.

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Tooru Fujisawa [:arai]]

Derived from bug 1115616.

(In reply to Hideo Oshima from comment #27)
> I tested again on Windows 8.1 and found mysterious result.
> 
> 1. Open about:home.
> 2. Turn IME on.
> 3. Input もじら
> 4. Select モジラ in suggestions list.
> 
> Expected result:
> Search モジラ.
> 
> Actual result:
> Search もじら
> 
> This problem doesn't occur at about:newtab and Linux build neither.
> If this is not related to this bug, I will submit another bug.

Confirmed similar problem happens with Nightly 2015-01-26, non-e10s window on Windows 7 and Microsoft IME.
(e10s is disabled with "The keyboard being used has activated IME" message, and cannot test with it)

Doesn't happen on beta/aurora on Windows7/OSX 10.9.5,
and nightly 2015-01-26 on OSX 10.9.5 (e10s/non-e10s).

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: patch for logging

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: win-non-e10s.txt

logged events and variables for steps in comment #0 (Windows 7, Nightly 2015-01-26, non-e10s)

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: mac-non-e10s.txt

logged events and variables for steps in comment #0 (Mac OS X 10.9.5, mozilla-central, non-e10s)

Comment 4

[Tooru Fujisawa [:arai]]

no difference between e10s and non-e10s on Mac OS X.

Then, on Windows, `_onInput` is called twice after calling `blur()`, and second one calls `_getSuggestions`, and it overwrites `_stickyInputValue`.

1422298557586 "_onMousedown" "blur()"

1422298557591 "_onInput" "current _ignoreInputEvent:" true
1422298557594 "_onInput" "current input.value:" "もじら"
1422298557599 "_onInput" "_ignoreInputEvent = false"

1422298557603 "_onInput" "current _ignoreInputEvent:" false
1422298557606 "_onInput" "current input.value:" "もじら"

1422298557608 "_getSuggestions" "_stickyInputValue = もじら"

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: win-aurora.txt

logged events and variables for steps in comment #0 (Windows 7, Aurora 2015-01-26)

some more events are fired while typing, but `_onInput` is fired only once after calling `blur()`, which is same as OS X's case

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: test.html

input event is fired twices even with attached testcase, on Windows 7.
  1. open attached HTML file
  2. open javascript console
  3. focus to the input field in the HTML file
  4. turn IME on
  5. type もじら
  6. click other places than the input field

expected result:
  "on input" is shown once after step 6

actual result:
  "on input" is shown twice after step 6

here is regression range:
  Last good revision: e6614d8d85f9
  First bad revision: 7f81be7db528
  https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e6614d8d85f9&tochange=7f81be7db528

I guess this is caused by some Nightly-only feature

Comment 7

[Tooru Fujisawa [:arai]]

It's bug 1037328
https://hg.mozilla.org/mozilla-central/rev/ad532252ff9e
When I set intl.tsf.enable to false on Nightly, input event is fired only once,
and when I set intl.tsf.enable to true on Aurora, input event is fired twice.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

I guess that NSPR_LOG_MODULES=nsTextStoreWidgets:5 might help you to research TSF mode behavior with e10s.
# I'm still working on TSF but I'm not familiar with e10s mode.

See also (written in Japanese):
https://dev.mozilla.jp/2012/09/ime_logging_of_tsf/

Anyway, I don't have much time since I have an urgent work by Feb 5th. So, I cannot do anything for this at least for now...

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: testing patch for more logging

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: tsf-on.txt

log for step 6 in comment #6, with TSF enabled

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: tsf-off.txt

log for step 6 in comment #6, with TSF disabled

with TSF disabled, only EndIMEComposition is called.
with TSF enabled, nsPlaintextEditor::UpdateIMEComposition is also called before EndIMEComposition.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

(In reply to Tooru Fujisawa [:arai] from comment #11)
> with TSF disabled, only EndIMEComposition is called.
> with TSF enabled, nsPlaintextEditor::UpdateIMEComposition is also called
> before EndIMEComposition.

Although, I don't confirm the logs yet, this is a big hint.

Comment 13

[Tooru Fujisawa [:arai]]

So, the problem is that nsTextStore::OnUpdateComposition is called for step 6, right?
(not sure it's controllable from our side)

Should we somehow ignore it to fire "input" event only once,
or fix this bug in searchSuggestionUI.js side?

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Um, in TSF mode, looks like there is a redundant NS_COMPOSITION_UPDATE event immediately before NS_COMPOSITION_COMMIT. In TextComposition, we should check if we really need to dispatch DOM text event by if checking composition string or clauses are changed.
http://mxr.mozilla.org/mozilla-central/source/dom/events/TextComposition.cpp#215

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

(In reply to Tooru Fujisawa [:arai] from comment #13)
> So, the problem is that nsTextStore::OnUpdateComposition is called for step
> 6, right?
> (not sure it's controllable from our side)

Not controllable. It's called by IME via TSF.

> Should we somehow ignore it to fire "input" event only once,
> or fix this bug in searchSuggestionUI.js side?

I think that this should be fixed in TextComposition. It's difficult to prevent such difference between native IME behavior in native IME handler level. Although, we waste IPC cost in e10s mode, it's reasonable to discard such redundant event immediately before dispatching a DOM event from TextComposition.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #14)
> Um, in TSF mode, looks like there is a redundant NS_COMPOSITION_UPDATE event
> immediately before NS_COMPOSITION_COMMIT.

Oops,

s/NS_COMPOSITION_UPDATE/NS_COMPOSITION_CHANGE

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Discard redundant NS_COMPOSITION_CHANGE event which is send just before NS_COMPOSITION_END on TSF.

Made TextComposition::DispatchCompositionEvent to discard NS_COMPOSITION_CHANGE event if string and ranges are same as previous one.

Green on try runs:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ceeb6664fbb
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef62476c062c (added test)

Also, "on input" is shown once with attachment 8555192 [details], and I can search by モジラ on the STR in comment #0.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Comment on attachment 8557536 [details] [diff] [review]
Discard redundant NS_COMPOSITION_CHANGE event which is send just before NS_COMPOSITION_END on TSF.

I'm very sorry for the long delay due to my urgent work. And thank you very much for your work.

>+static bool
>+isSameRanges(nsRefPtr<TextRangeArray> aCurrent, nsRefPtr<TextRangeArray> aLast)
>+{
>+  NS_ASSERTION(aCurrent && aLast, "both range shouldn't be null");
>+  size_t len = aCurrent->Length();
>+  if (len != aLast->Length()) {
>+    return false;
>+  }
>+  for (size_t i = 0; i < len; i++) {
>+    TextRange& currElem = aCurrent->ElementAt(i);
>+    TextRange& lastElem = aLast->ElementAt(i);
>+    if (currElem.mStartOffset != lastElem.mStartOffset ||
>+        currElem.mEndOffset != lastElem.mEndOffset ||
>+        currElem.mRangeType != lastElem.mRangeType) {
>+      return false;
>+    }
>+  }
>+  return true;
>+}

I think that you should add bool Equals(const TextRangeArray& aOther) into TextRangeArray class.

>@@ -219,16 +239,26 @@ TextComposition::DispatchCompositionEven
>   // composition string empty or didn't have clause information), we don't
>   // need to dispatch redundant DOM text event.
>   if (dispatchDOMTextEvent &&
>       aCompositionEvent->message != NS_COMPOSITION_CHANGE &&
>       !mIsComposing && mLastData == aCompositionEvent->mData) {
>     dispatchEvent = dispatchDOMTextEvent = false;
>   }
> 
>+  // TSF calls OnUpdateComposition just before OnEndComposition with same data
>+  // as previous one.

// widget may dispatch redundant NS_COMPOSITION_CHANGE event
// which modifies neither composition string, clauses nor caret
// position.  In such case, we shouldn't dispatch DOM events.

I believe that this could be possible on other platforms. And also with JS-IME which uses nsITextInputProcessor.

>+  if (dispatchDOMTextEvent &&
>+      aCompositionEvent->message == NS_COMPOSITION_CHANGE &&
>+      mIsComposing && aCompositionEvent->IsComposing() &&
>+      mLastData == aCompositionEvent->mData &&
>+      mRanges && isSameRanges(aCompositionEvent->mRanges, mRanges)) {

How about both mRanges and aCompositionEvent->mRanges are nullptr?

>+function runRedundantChangeTest()

>+  // synthesize same change event again
>+  // TSF calls OnUpdateComposition just before OnEndComposition with same data
>+  // as previous one, and it should be discarded.

Please remove these two lines.

Otherwise, looks good to me.

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Discard redundant NS_COMPOSITION_CHANGE event which is send just before NS_COMPOSITION_END on TSF.

Thank you for reviewing!

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #18)
> I think that you should add bool Equals(const TextRangeArray& aOther) into
> TextRangeArray class.

Okay, added TextRangeArray::Equals and TextRange::Equals, also made TextRangeStyle::Equals, operator==, and operator!= to be const.
(I missed mRangeStyle property on last patch)

> >+  if (dispatchDOMTextEvent &&
> >+      aCompositionEvent->message == NS_COMPOSITION_CHANGE &&
> >+      mIsComposing && aCompositionEvent->IsComposing() &&
> >+      mLastData == aCompositionEvent->mData &&
> >+      mRanges && isSameRanges(aCompositionEvent->mRanges, mRanges)) {
> 
> How about both mRanges and aCompositionEvent->mRanges are nullptr?

Added the case.
Also, if the case is possible, we don't need to check IsComposing.

Almost green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d074ab61d0b3

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Comment on attachment 8560468 [details] [diff] [review]
Discard redundant NS_COMPOSITION_CHANGE event which is send just before NS_COMPOSITION_END on TSF.

Looks great! Thanks!

Comment 21

[Tooru Fujisawa [:arai]]

Thank you! :D

https://hg.mozilla.org/integration/mozilla-inbound/rev/88c746188a40

Comment 22

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6403265&repo=mozilla-inbound

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Oh, it must detect a bug of the patch.

I guess that the test tries to select the range to remove and just send NS_COMPOSITION_CHANGE with empty string and null range.

If coming NS_COMPOSITION_CHANGE is the first event of a composition, mRanges is nullptr. Additionaly, coming WidgetCompositionEvent::mRanges is also nullptr in this case. Additionally, both mLastData and WidgetCompositionEvent::mData are both empty string. So, I guess that TextComposition should have mCompositionChanged as bool, it should become true when first NS_COMPOSITION_CHANGE is received (at this time, selected string is removed), and false when NS_COMPOSITION_END is being dispatched.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Oops, sorry, I clear the unnecessary needinfo?. Sorry for the spam.

Comment 25

[Tooru Fujisawa [:arai]]

Is it reasonable to drop "both are nullptr" condition from this patch?  Of course it might be nice to also discard such redundant events, but I guess it's not important part for this bug, because ranges are always non-null for this bug's case.  If it's okay, I'd like to leave that part to another bug.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Yeah, sound okay. But guarantee that !!mRange != !!aCompositionEvent->mRange causes dispatching events.

Comment 27

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/projects/cypress/rev/1bd47ee5fde9

Comment 28

[Tooru Fujisawa [:arai]]

Attachment: Discard redundant NS_COMPOSITION_CHANGE event which is send just before NS_COMPOSITION_END on TSF.

Okay, removed `!mRanges && !aCompositionEvent->mRanges` case, and added testcase which synthesizes non-null, null, and non-null ranges events sequencially, to check `!!mRange != !!aCompositionEvent->mRange` conditions (runNotRedundantChangeTest).

Would you review that part?

In the new testcase, null ranges event doesn't trigger "input" event, even without this patch, is it correct? Of course other events are dispatched on that case.

Almost green on try runs:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7326875f426
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=8173def0a23c (test 
added)
(linux64 seems to have so much pendings jobs...)

Comment 29

[Tooru Fujisawa [:arai]]

Thank you again :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd14f3a87117

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd14f3a87117

Comment 31

[Hideo Oshima]

I tested 2015-02-11 Nightly build.
The problem has been fixed on both about:home and about:newtab.
Thanks so much.