dynamis (Tomoya ASAI) Reporter
	Description • 10 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 1 • 10 years ago

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.

	Sylvestre Ledru [:Sylvestre]
	Comment 3 • 10 years ago

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

	Tooru Fujisawa [:arai] Assignee
	Comment 4 • 10 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 5 • 10 years ago

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.

	Tooru Fujisawa [:arai] Assignee
	Comment 6 • 10 years ago

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!

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 7 • 10 years ago

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?

	Tooru Fujisawa [:arai] Assignee
	Comment 8 • 10 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 9 • 10 years ago

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.

	Drew Willcoxon :adw
	Comment 10 • 10 years ago

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.

	Tooru Fujisawa [:arai] Assignee
	Comment 11 • 10 years ago

Thank you for reviewing :)

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

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 10 years ago

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

	Tooru Fujisawa [:arai] Assignee
	Comment 13 • 10 years ago

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

	Hideo Oshima
	Comment 14 • 10 years ago

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.

	Tooru Fujisawa [:arai] Assignee
	Comment 15 • 10 years ago

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

	Tooru Fujisawa [:arai] Assignee
	Comment 16 • 10 years ago

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

clearing a? for now

	Tooru Fujisawa [:arai] Assignee
	Comment 17 • 10 years ago

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

	Drew Willcoxon :adw
	Comment 18 • 10 years ago

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

	Tooru Fujisawa [:arai] Assignee
	Comment 19 • 10 years ago

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

	Drew Willcoxon :adw
	Comment 20 • 10 years ago

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

	Tooru Fujisawa [:arai] Assignee
	Comment 21 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 22 • 10 years ago

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

	Sylvestre Ledru [:Sylvestre]
	Comment 23 • 10 years ago

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

	Tooru Fujisawa [:arai] Assignee
	Comment 24 • 10 years ago

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

	Tooru Fujisawa [:arai] Assignee
	Comment 25 • 10 years ago

Thank you!

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

	Hideo Oshima
	Comment 26 • 10 years ago

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

	Hideo Oshima
	Comment 27 • 10 years ago

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.

	Tooru Fujisawa [:arai] Assignee
	Comment 28 • 10 years ago

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

	Tooru Fujisawa [:arai] Assignee
	Comment 29 • 10 years ago

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

	Petruta Horea [:phorea], Desktop QA
	Comment 30 • 10 years ago

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.

	Tooru Fujisawa [:arai] Assignee
	Comment 31 • 10 years ago

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

	Tooru Fujisawa [:arai] Assignee
	Comment 32 • 10 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 33 • 10 years ago

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

	Tooru Fujisawa [:arai] Assignee
	Comment 34 • 10 years ago

Thanks, filed as bug 1125934.

	Petruta Horea [:phorea], Desktop QA
	Comment 35 • 10 years ago

Marking as verified (comment 30).