Cannot search with Google when user input with IME and click one of search suggest item in Home page.

VERIFIED FIXED in Firefox 36

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: bugzilla, Assigned: arai)

Tracking

({inputmethod})

unspecified
Firefox 38
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox34 wontfix, firefox35- wontfix, firefox36+ verified, firefox37+ verified, firefox38 verified, firefox-esr31 unaffected)

Details

Attachments

(3 attachments, 4 obsolete attachments)

In the search form of Firefox Home page (about:home), when user input something and select one of suggested item with mouse (click on one of suggested item list under the form) input text will be deleted and not moved to search result.

Step to reproduce:
* Make IME on
* input something with keyboard (but not fixed yet)
  * search suggest will be shown under the search form
* click on of the suggest list
* input text will be deleted. will not moved to search result.

Expected Result:
Move to search result page with selected (clicked) search keyword.

Tested on Firefox 34 (release), 35 (Beta) on Mac OS X 10.9.
# and reproduced on Windows too by Masayuki
In mousedown event handler, InputElement.value is set even during composition. However, this doesn't work with current Gecko, see bug 549674. I guess that before setting the value, this should do input.blur(); and input.focus();. Then, the composition must be committed forcibly. IIRC, Google's web site uses same hack.

[Tracking Requested - why for this release]:
This must be easy to fix and important for UX of Japanese users.
Flags: qe-verify+
Flags: firefox-backlog+
Duplicate of this bug: 1092957
Tracking for 36. I think this is too late for 35.
Added input.blur() and input.focus(), as noted in comment #1,
and some hack for "input" event.

Is there any good way to test this in try server?
(I confirmed this patch works with STR in comment #0 locally tho)

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=addfe3a53096
Attachment #8547604 - Flags: review?(gavin.sharp)
Comment on attachment 8547604 [details] [diff] [review]
Commit composition forcibly when search suggestion list is clicked.

Can we use a workaround like attachment 515016 [details] [diff] [review] instead? That seems like it would be simpler.
Flags: needinfo?(arai_a)
Attachment #8547604 - Flags: review?(gavin.sharp)
Points: --- → 5
Thank you for reviewing :D

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #5)
> Comment on attachment 8547604 [details] [diff] [review]
> Commit composition forcibly when search suggestion list is clicked.
> 
> Can we use a workaround like attachment 515016 [details] [diff] [review]
> instead? That seems like it would be simpler.

Yeah, it should be nice!
Attachment #8547604 - Attachment is obsolete: true
Flags: needinfo?(arai_a)
Attachment #8547845 - Flags: review?(gavin.sharp)
Comment on attachment 8547845 [details] [diff] [review]
Commit composition string forcibly when search suggestion list is clicked.

Thanks for patching this.

Is there any chance you could write a test for this as well?
Attachment #8547845 - Flags: review?(gavin.sharp) → review+
Sure :)
Would you also review the test part?
Almost no changes in system part (except changing Ci to Components.interfaces, to make it work with test)

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=137065dba462
Attachment #8547845 - Attachment is obsolete: true
Attachment #8548812 - Flags: review?(gavin.sharp)
Comment on attachment 8548812 [details] [diff] [review]
Commit composition string forcibly when search suggestion list is clicked.

Awesome! Apologies for the delayed response here.

I think Drew wrote this test, so he's probably best to review the test addition.
Attachment #8548812 - Flags: review?(gavin.sharp) → review?(adw)
Comment on attachment 8548812 [details] [diff] [review]
Commit composition string forcibly when search suggestion list is clicked.

Review of attachment 8548812 [details] [diff] [review]:
-----------------------------------------------------------------

This is great, thank you.
Attachment #8548812 - Flags: review?(adw) → review+
https://hg.mozilla.org/mozilla-central/rev/086e9bb37bf7
Assignee: nobody → arai_a
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Iteration: --- → 38.1 - 26 Jan
Comment on attachment 8548812 [details] [diff] [review]
Commit composition string forcibly when search suggestion list is clicked.

Same patch is applicable to mozilla-aurora and mozilla-beta.

Approval Request Comment
[Feature/regressing bug #]: This feature was added in bug 612453
[User impact if declined]: Cannot search with Google by clicking suggestion list while composing in about:home
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-central&revision=540077a30866
[Risks and why]: Low. Change is limited to search field in about:home, clicking suggestion while composing, which didn't work before
[String/UUID change made/needed]: None
Attachment #8548812 - Flags: approval-mozilla-beta?
Attachment #8548812 - Flags: approval-mozilla-aurora?
I confirmed the problem has been fixed on about:newtab
but I can still reproduce on on about:home.
I reported this as bug 1092957 that has been marked duplicate.
Oops, sorry, I misunderstood the URI, what I tested was about:newtab.
I thought they are same.
I'll prepare a patch for it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8548812 [details] [diff] [review]
Commit composition string forcibly when search suggestion list is clicked.

clearing a? for now
Attachment #8548812 - Flags: approval-mozilla-beta?
Attachment #8548812 - Flags: approval-mozilla-aurora?
Iteration: 38.1 - 26 Jan → ---
In "about:home", searchSuggestionUI.js runs under content priv and it cannot access to `this.input.editor`, so I added the fallback code (`input.blur()` and `input.focus()`), which was in initial patch, for that case.

Then, in the test case (browser_aboutHome.js), I need to set `gSearchSuggestionController.onClick` to `null` to suppress page transition when suggestion is clicked. Is there any other better and simpler way than `eval`?

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ee0e9a57555
Attachment #8552547 - Flags: review?(gavin.sharp)
Sorry Tooru, I should have caught that.

Can we just use the fallback code always so there aren't two paths?  That would be simpler, and also we eventually want to unprivilege about:newtab, too.  I know Gavin earlier asked you to use the XPCOM component, but we should reconsider that I think.

For browser_aboutHome.js, gBrowser.contentWindow.eval() works now because that test doesn't run under e10s, but we shouldn't add new code that's not e10s-safe.  I think you have a couple of options.  The test happens to load a frame script, aboutHome_content_script.js, so you could simply move the eval there: content.eval("gSearchSuggestionController.onClick = null").  I think that should work.  Or you could use waitForDocLoadAndStopIt like this test does after it clicks the Search button: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_aboutHome.js#83
Thank you Drew for quick feedback!

(In reply to Drew Willcoxon :adw from comment #18)
> Can we just use the fallback code always so there aren't two paths?  That
> would be simpler, and also we eventually want to unprivilege about:newtab,
> too.

Yeah, in that case, removing XPCOM code will be better.
blur/focus also works well with about:newtab.

> For browser_aboutHome.js, gBrowser.contentWindow.eval() works now because
> that test doesn't run under e10s, but we shouldn't add new code that's not
> e10s-safe.  I think you have a couple of options.  The test happens to load
> a frame script, aboutHome_content_script.js, so you could simply move the
> eval there: content.eval("gSearchSuggestionController.onClick = null").  I
> think that should work.  Or you could use waitForDocLoadAndStopIt like this
> test does after it clicks the Search button:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> general/browser_aboutHome.js#83

Thanks, the second one should be simpler, and... it's in the same file (!), I should've notice that :O
Attachment #8552547 - Attachment is obsolete: true
Attachment #8552547 - Flags: review?(gavin.sharp)
Attachment #8552611 - Flags: review?(adw)
Comment on attachment 8552611 [details] [diff] [review]
Part 2: Commit composition string forcibly when search suggestion list is clicked in about:home.

Review of attachment 8552611 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Tooru.

::: browser/base/content/searchSuggestionUI.js
@@ +239,5 @@
>      this._stickyInputValue = suggestion;
>  
>      // Commit composition string forcibly, because setting input value does not
>      // work if input has composition string (see bug 1115616 and bug 632744).
> +    // Ignore input event for compisition end to avoid getting suggestion again.

Typo nit: compisition -> composition
Attachment #8552611 - Flags: review?(adw) → review+
https://hg.mozilla.org/mozilla-central/rev/b683c06bf809
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Iteration: --- → 38.1 - 26 Jan
Tooru, could you fill the uplift request to aurora & beta? thanks
Flags: needinfo?(arai_a)
Sorry for being late.
Merged Part 1 and 2 since Part 2 overwrites almost code added in Part 1.

Approval Request Comment
[Feature/regressing bug #]: This feature was added in bug 612453
[User impact if declined]: Cannot search with Google by clicking suggestion list while composing in about:home and about:newtab
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-central&revision=86f9d0128ccf
[Risks and why]: Low. Change is limited to search suggestion list in about:home and about:newtab. It calls blur/focus on the search field, but it could also happen in normal operation.
[String/UUID change made/needed]: None
Flags: needinfo?(arai_a)
Attachment #8553200 - Flags: review+
Attachment #8553200 - Flags: approval-mozilla-beta?
Attachment #8553200 - Flags: approval-mozilla-aurora?
Attachment #8553200 - Flags: approval-mozilla-beta?
Attachment #8553200 - Flags: approval-mozilla-beta+
Attachment #8553200 - Flags: approval-mozilla-aurora?
Attachment #8553200 - Flags: approval-mozilla-aurora+
I confirmed to fix on about:home too.
Thanks a lot, Fujisawa-san.
QA Contact: petruta.rasa
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.
Tested on Windows 7 and OS X 10.9.5, with beta, aurora (2015-01-26), and nightly (2015-01-25).
The problem happens only with nightly (2015-01-25) e10s window on Windows 7 (non-e10s window does not show content area, I'll test another build soon).
sorry, I misunderstood. I didn't know that e10s was disabled on Windows.
The problem happens only with non-e10s window.
Verified that the issue is fixed in Firefox 36 beta 3, latest Developer Edition 37.0a2 and latest Nightly 38.0a1 under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.

I reproduced the issue from comment 27 on Windows 7 using Nightly 38.0a1 - non e10s window (opened by default when opening Nightly with a new profile).

I'll mark the bug accordingly either after the new issue is filled separately or a patch for it lands here.
Adding setTimeout to `this.input.value = suggestion;` and following block solves the problem, locally.
I guess there might be better and reliable solution though.

    setTimeout(() => {
      this.input.value = suggestion;
      this.input.setAttribute("selection-index", idx);
      this.input.setAttribute("selection-kind", "mouse");
      this._hideSuggestions();
      if (this.onClick) {
        this.onClick.call(null);
      }
    }, 0);
Sorry, that was my mistake.
I added `this._ignoreInputEvent = false;` while making testcase. But it shouldn't be there, `input` event is fired twice when calling input.blur() (at least on Windows nightly), and second one breaks the fix.

   _onInput: function () {
     if (this._ignoreInputEvent) {
       this._ignoreInputEvent = false;   <--- here
       return;
     }

So, just removing the line will fix the problem.

Try is running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bb78078a818

It would be better to uplift this too.
Attachment #8554562 - Flags: review?(gavin.sharp)
(In reply to Tooru Fujisawa [:arai] from comment #32)
> `input` event is fired twice when calling input.blur()
> (at least on Windows nightly), and second one breaks the fix.

And only for non-e10s windows? That sounds odd.

Can you file another bug about this? Better to fix one issue in one bug, and it sounds like this might need some other fix.
Attachment #8554562 - Attachment is obsolete: true
Attachment #8554562 - Flags: review?(gavin.sharp)
Thanks, filed as bug 1125934.
You need to log in before you can comment on or make changes to this bug.