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

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[dynamis (Tomoya ASAI)]

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

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Tracking for 36. I think this is too late for 35.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Commit composition forcibly when search suggestion list is clicked.

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

Comment 5

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Commit composition string forcibly when search suggestion list is clicked.

Thank you for reviewing :D

(In reply to :Gavin Sharp [email: [e-mail]] 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!

Comment 7

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Commit composition string forcibly when search suggestion list is clicked.

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

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 10

[Drew Willcoxon :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.

Comment 11

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/086e9bb37bf7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/086e9bb37bf7

Comment 13

[Tooru Fujisawa [:arai]]

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

Comment 14

[Hideo Oshima]

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.

Comment 15

[Tooru Fujisawa [:arai]]

Oops, sorry, I misunderstood the URI, what I tested was about:newtab.
I thought they are same.
I'll prepare a patch for it.

Comment 16

[Tooru Fujisawa [:arai]]

Comment on attachment 8548812 [details] [diff] [review]
Commit composition string forcibly when search suggestion list is clicked.

clearing a? for now

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Commit composition string forcibly when search suggestion list is clicked in about:home.

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

Comment 18

[Drew Willcoxon :adw]

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

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Commit composition string forcibly when search suggestion list is clicked in about:home.

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

Comment 20

[Drew Willcoxon :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

Comment 21

[Tooru Fujisawa [:arai]]

Thanks again!

It passes try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=819f1c6cdc71

And pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b683c06bf809

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Tooru, could you fill the uplift request to aurora & beta? thanks

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: Commit composition string forcibly when search suggestion list is clicked. r=gavin,adw

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

Comment 25

[Tooru Fujisawa [:arai]]

Thank you!

https://hg.mozilla.org/releases/mozilla-aurora/rev/00fbf594ad9f
https://hg.mozilla.org/releases/mozilla-beta/rev/2d629038c57b

Comment 26

[Hideo Oshima]

I confirmed to fix on about:home too.
Thanks a lot, Fujisawa-san.

Comment 27

[Hideo Oshima]

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.

Comment 28

[Tooru Fujisawa [:arai]]

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

Comment 29

[Tooru Fujisawa [:arai]]

sorry, I misunderstood. I didn't know that e10s was disabled on Windows.
The problem happens only with non-e10s window.

Comment 30

[Petruta Horea [:phorea], Desktop Test Engineering]

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.

Comment 31

[Tooru Fujisawa [:arai]]

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

Comment 32

[Tooru Fujisawa [:arai]]

Attachment: Ignore second input event for blur when suggestion list is clicked in about:home/about:newtab.

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.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

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

Comment 34

[Tooru Fujisawa [:arai]]

Thanks, filed as bug 1125934.

Comment 35

[Petruta Horea [:phorea], Desktop Test Engineering]

Marking as verified (comment 30).