Closed Bug 1125934 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Editor, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

(Keywords: inputmethod)

Attachments

(9 files, 2 obsolete files)

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).
Attached file win-non-e10s.txt
logged events and variables for steps in comment #0 (Windows 7, Nightly 2015-01-26, non-e10s)
Attached file mac-non-e10s.txt
logged events and variables for steps in comment #0 (Mac OS X 10.9.5, mozilla-central, non-e10s)
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 = もじら"
Blocks: 1115616
Attached file 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
Attached file 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
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.
Summary: Cannot choose suggestion list item in about:home. → Cannot choose suggestion list item in about:home when TSF is enabled and composing.
Flags: needinfo?(masayuki)
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...
Flags: needinfo?(masayuki)
Attached file tsf-on.txt
log for step 6 in comment #6, with TSF enabled
Attached file 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.
(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.
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?
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
(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.
(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
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.
Attachment #8557536 - Flags: review?(masayuki)
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.
Attachment #8557536 - Flags: review?(masayuki) → review-
Assignee: nobody → arai_a
Status: NEW → ASSIGNED
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
Attachment #8557536 - Attachment is obsolete: true
Attachment #8560468 - Flags: review?(masayuki)
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!
Attachment #8560468 - Flags: review?(masayuki) → review+
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6403265&repo=mozilla-inbound
Flags: needinfo?(arai_a)
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.
Flags: needinfo?(cbook)
Oops, sorry, I clear the unnecessary needinfo?. Sorry for the spam.
Flags: needinfo?(cbook)
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.
Flags: needinfo?(arai_a)
Yeah, sound okay. But guarantee that !!mRange != !!aCompositionEvent->mRange causes dispatching events.
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...)
Attachment #8560468 - Attachment is obsolete: true
Attachment #8562230 - Flags: review?(masayuki)
Component: Search → Editor
Product: Firefox → Core
Attachment #8562230 - Flags: review?(masayuki) → review+
https://hg.mozilla.org/mozilla-central/rev/bd14f3a87117
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
I tested 2015-02-11 Nightly build.
The problem has been fixed on both about:home and about:newtab.
Thanks so much.
Depends on: 1133802
You need to log in before you can comment on or make changes to this bug.