Frank Yan (:fryn) Reporter
	Description • 14 years ago

Step to reproduce:
Type something in the search box on about:home

Actual result:
No suggestions.

Expected result:
Suggestions appear.

This is just one step in enhancing the search experience on about:home to bring it closer to the rich interface provided by Google's home page.

=====

The experimental prototype patch uses the HTML5 datalist element to provide autocomplete suggestions via the Google Suggest API over SSL.

Unfortunately, the list of suggestions flickers when using this API, which should not happen. See our in-toolbar search box for a great non-flickery experience.

	Frank Yan (:fryn) Reporter
	Comment 1 • 14 years ago

(In reply to comment #0)
> Unfortunately, the list of suggestions flickers when using this API, which
> should not happen. See our in-toolbar search box for a great non-flickery
> experience.

Clarification: By this API, I mean using the HTML5 dropdown list seems to cause the flickering, not Google. If suggestions like these are something we want, I'll investigate how the search box does it and go from there.

	Justin Dolske [:Dolske]
	Comment 3 • 14 years ago

I'd really rather not go about this piecemeal... Seems like we're still not sure what about:home is going to contain / look like, and slowly rebuilding how Google's front page works/worked piece by piece doesn't seem like a plan.

	Alex Limi (:limi) — Firefox UX Team
	Comment 4 • 14 years ago

I'm pretty sure we want Google Suggest at the very least, since that's what we already ship with in other parts of the product. :)

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

The suggest implementation in the other parts of the product is very different and isn't easily usable from about:home. That isn't to say that we can't have suggest on about:home, but taking a patch like Frank's prototype here would require coordination with Google to make sure that remotely linking their suggest code in-product is OK, at a minimum. We'd also need a solution that isn't hardcoded to Google, ideally, since about:home supports arbitrary default engines.

	Marco Bonardo [:mak]
	Comment 6 • 14 years ago

well, the search service provides the suggestions url, isn't it? what blocks us from passing it along?

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

The suggest URL itself isn't the issue, it's the suggest *code* that's being dynamically linked in from google.com.

	Frank Yan (:fryn) Reporter
	Comment 8 • 14 years ago

Yeah, we definitely don't want to be using JSONP in production code. I just used it to prototype if HTML5's datalist element worked for this type of autocomplete, but it looks like we'll have to write something better if we want a usable, non-flickery autocomplete.

	Marco Bonardo [:mak]
	Comment 9 • 14 years ago

This uses the official url from the search engine, should have a bit less flickering, even if it's not perfect (the search field suggestion code does rarely invalidate the popup). It works fine enough though.
One thing I've not investigated is that even if I don't see any issue, sometimes I get a bunch of "WARNING: Fix the caller!: file c:/mozilla/mozilla-central/content/events/src/nsEventDispatcher.cpp, line 512"

	Marco Bonardo [:mak]
	Comment 10 • 14 years ago

(In reply to comment #9)
> One thing I've not investigated is that even if I don't see any issue,
> sometimes I get a bunch of WARNING

It seems due to the fact the listeners are added in chrome context, probably can be easily fixed by adding listeners in content, and send custom events.

	Marco Bonardo [:mak]
	Comment 12 • 14 years ago

moved some code to content, using a custom event, properly supports the case the engine doesn't have a suggestUrl with fallback to satchel.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 14 • 13 years ago

Comment on attachment 510649 [details] [diff] [review]
experiment 2.1

- getSubmission can return null if the search engine has no suggestions Url, so that needs to be handled.
- I don't understand why you use Object.defineProperty
- I'm a bit worried that adding listeners to the content documents this way will trigger leaks.
- it'd be cool if google could enable cross-site XHR for the suggest queries so that we could do this entirely in-content. I guess they might not like to open themselves up to abuse like that, though.

I wonder whether a better approach might be to re-use some of the logic in nsSearchSuggestions.js. You'd need some way to let it know which search engine URL to use (it currently just uses .currentEngine), but then you could basically ask it for the raw data and it could handle the rest (making the request, parsing, backoff, merging with form history, etc.). Maybe just define a new interface for it?

	Marco Bonardo [:mak]
	Comment 15 • 13 years ago

Well, I suppose changing the page to be chrome with a content iframe for snippets would make this much easier to do, by allowing direct use of the existing autocomplete and search stuff. So I'm not sure how much we should keep investigating the content way of doing that.

	abhijitsr
	Comment 17 • 12 years ago

Sir i would like to know whether this feature will be implemented or not in the upcoming version.
Also when i search using it.It acts like a form.So if one would click on the search box one can see my previous searches.I don't want that to happen

	Mike Connor [:mconnor]
	Comment 18 • 10 years ago

After a couple hours monkeying with this, I've come to the sad acceptance that <datalist> is not the droid we're looking for here.  Flickering is sort of unavoidable because it self-filters from existing results, then remote updates come down the pipe.  It's pretty jarring, and not at all what I think we want/expect here.  Beyond that, it also falls down when suggestions deal with misspellings (i.e. "vampire weekend lyrcs" shows no matches trying to rely on <datalist>).  So... that won't work.  Round pegs and all.

Instead, I've got a WIP that does the same thing Google does: an absolutely positioned table, and some JS magic to simulate selection.  Still to do: mouse support, better styling, and handling of repositioning/resizing.  Oh, and not hardcoding Google's suggest URL.

It's fast/smooth/flicker-free. Aiming to have a patch for review tomorrow depending on how the day goes.

	Mike Connor [:mconnor]
	Comment 19 • 10 years ago

Oh, and for bonus points: we can also do the bold-style completion effect Google does.  Or, really, anything at all.

	Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )
	Comment 20 • 10 years ago

(I see no obvious solution, but measuring this would helpful.  Maybe telemetry on "autosuggest chosen"? )

	Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )
	Comment 21 • 10 years ago

Expanding previous comments:

1.  Proposed impact on users:
  - users who rely on the old behaviour will be inconvenienced
  - more uniformity of search behaviour between search box and about:home, and google.com page

2.  suggested additional patches
  - instrumentation?  
  - pref enabled variations, deployed through funnel cake-ish mechanism or telemetry experiments?
  - hinting on about:home "Hey, autosuggest works here too".   

3.  Possible experiments (spitballing!):
  - usage of this SAP differences between "guided" and "unguided" users?
  - count / usage of autocomplete / instant for TM users?

	Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )
	Comment 22 • 10 years ago

(Also, can we run this by UX / Bryan... Bigger box / positioning, etc might increase adoption, as usual.)

Awesome work on getting this running smoothly!

	Mike Connor [:mconnor]
	Comment 23 • 10 years ago

This is only tested on Windows, so far.

Working:
* Smooth autocomplete effect
* Suggestion selection via arrows
* Mouseover/mousedown support
* Google-style bolding of deltas (i.e. the words not already typed, for easier scanning)
* Uses the current engine at all times
* Handles window resizing correctly

To-do:
* Testing on other platforms
* Cleanup (some of this feels hacky)
* Some sort of automated testing

	Mike Connor [:mconnor]
	Comment 24 • 10 years ago

Screenshot of the bolding support.  Engine-independent.

	Mike Connor [:mconnor]
	Comment 25 • 10 years ago

Depends on the SearchSuggest.jsm patch in bug 1007979

	Mike Connor [:mconnor]
	Comment 26 • 10 years ago

blue results are local, black are remote.  Bolding behaviour is based on words, rather than letters, based on feedback.

	Mike Connor [:mconnor]
	Comment 27 • 10 years ago

same as previous, but showing selection behaviour/look&feel

	Marco Bonardo [:mak]
	Comment 28 • 10 years ago

Comment on attachment 8419785 [details] [diff] [review]
aboutHomeReal

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

does this work properly on rtl? I see some left positioning related to the table

	Jennifer Morrow [:Boriss] (UX)
	Comment 29 • 10 years ago

(In reply to Mike Connor [:mconnor] from comment #26)
> Created attachment 8419799 [details]
> current look & feel
> 
> blue results are local, black are remote. 

Could anything but a server trigger blue?

This looks good, though if we're doing this on about:home I'd like to have identical behavior in about:newtab.  Any reason that should be hard?

	Mike Connor [:mconnor]
	Comment 30 • 10 years ago

Marco:

I haven't tested rtl yet.  What's the current state of the art for testing that sort of thing?

Boriss:

* The blue results are from form history, basically what you'd see above the line in the searchbar autocomplete, capped at 2 entries since I'm not implementing scrolling.  The only thing the server provides is the black entries (and the bolding is client-side).
* about:newtab is XUL, not HTML, so will require a different UI treatment.  However, the attached patch provides the right backend/event structure so that it's a pure front-end patch.  There's also an semi-related followup to unify the mechanism for engine images (i.e. about:home only has a Google logo of the default engines, about:newtab only has a Bing logo of the default engines).  I should file that.

	Marco Bonardo [:mak]
	Comment 31 • 10 years ago

(In reply to Mike Connor [:mconnor] from comment #30)
> Marco:
> 
> I haven't tested rtl yet.  What's the current state of the art for testing
> that sort of thing?

I don't know the state of the art, but I guess installing the Force RTL add-on and Tools / Force RTL Direction should be enough to test this page (sometimes the behavior is bogus and you might have to restart the browser or reload the page). I think the expected RTL behavior is that both the string in the search field and the suggestions align to the right, that is what's the current behavior with current form history and the searchbar, at least.

	Mike Connor [:mconnor]
	Comment 32 • 10 years ago

Looks like RTL _mostly_ works, but I'll have to fix the text alignment in the suggestions.  I'll get that done and request review.

	Mike Connor [:mconnor]
	Comment 33 • 10 years ago

As before, but rebased on the search suggest JSM changes, and working RTL support.  Note that this being in HTML makes this work _better_ than the current XUL, because it's automatic based on detection of the text.  (hooray dir="auto")  Screenshot pending.

	Mike Connor [:mconnor]
	Comment 35 • 10 years ago

This is currently based on top of the patch in bug 1007985 (because it refactored some of the code to make the content search message bit more generic and added a helper), but doesn't really rate a separate bug.  If for some reason we decide to do something different in that bug I can collapse the two and purge the focus stuff.

To the best of my knowledge, this now gets us to behaviour parity with the searchbar with the exception of engine selection.  That's something I'd like to tackle as a followup that includes using the searchplugin images, if available in the right dimensions.

	Matthew N. [:MattN]
	Comment 36 • 10 years ago

Comment on attachment 8422789 [details] [diff] [review]
implement deletion to match searchbar

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

::: browser/base/content/abouthome/aboutHome.js
@@ +378,5 @@
>            this._currentSearch = aEvent.target.value = this._currentSuggestions[this._currentSelection];
>          }
>          this.clearResults();
>          break;
> +      case 46: // delete

Please use the constants documented at https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.keyCode#Constants_for_keyCode_value instead of magic numbers. I'm not sure why the existing cases didn't.

	Matthew N. [:MattN]
	Comment 37 • 10 years ago

Comment on attachment 8422701 [details] [diff] [review]
aboutHomeReal

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

Quick review pass on the patch:

::: browser/base/content/abouthome/aboutHome.js
@@ +325,5 @@
> +    
> +    let tbl = document.getElementById("searchSuggestTable");
> +    tbl.style.left = left+"px";
> +    tbl.style.width = textfield.offsetWidth + "px";
> +    tbl.style.top = top+"px";

Nit: missing spaces around operators

@@ +354,5 @@
> +    if (newVal == -1) {
> +      this._currentSearch = aEvent.target.value;
> +    }
> +    switch (aEvent.keyCode) {
> +      case 38: // up

Oops, this was your new code that I mentioned in another review. Please use the constants instead of magic numbers: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.keyCode#Constants_for_keyCode_value

@@ +450,5 @@
> +    function addRow(type, str) {
> +      let row = document.createElement("tr");
> +      row.className = "resultRow";
> +      row.setAttribute("type", type);
> +      row.onmousemove = searchSuggest.handleMousemove;

use addEventListener instead of attributes/properties for events in new code

@@ +462,5 @@
> +      let terms = results.term.toLowerCase().split(" ");
> +      let innerHTML = "<span>";
> +      for (let i = 0; i < words.length; i++) {
> +        // HTML special chars
> +        let val = words[i].replace(/&/g, "&amp;")

For security, I believe using textContent is preferred over regexes.

@@ +467,5 @@
> +                          .replace(/"/g, "&quot;")
> +                          .replace(/</g, "&lt;")
> +                          .replace(/>/g, "&gt;");
> +        if (terms.indexOf(words[i]) == -1)
> +          innerHTML += "<b>" + val + "</b>";

I wonder how this affects a11y and whether you should use <strong> instead.

::: browser/base/content/abouthome/aboutHome.xhtml
@@ +41,5 @@
>            <input type="text" name="q" value="" id="searchText" maxlength="256"
> +                 autofocus="autofocus" dir="auto" autocomplete="off"
> +                 oninput="searchSuggest.requestSuggestions()"
> +                 onblur="searchSuggest.clearResults()"
> +                 onkeypress="searchSuggest.onSearchKeypress(event)"/>

Please switch to addEventListener for any new code (the onsubmit should have also used it).

@@ +42,5 @@
> +                 autofocus="autofocus" dir="auto" autocomplete="off"
> +                 oninput="searchSuggest.requestSuggestions()"
> +                 onblur="searchSuggest.clearResults()"
> +                 onkeypress="searchSuggest.onSearchKeypress(event)"/>
> +          <table id="searchSuggestTable" class="searchSuggestTable" 

Nit: trailing whitespace. You may want to set your editor to show it. The class seems redundant.

@@ +43,5 @@
> +                 oninput="searchSuggest.requestSuggestions()"
> +                 onblur="searchSuggest.clearResults()"
> +                 onkeypress="searchSuggest.onSearchKeypress(event)"/>
> +          <table id="searchSuggestTable" class="searchSuggestTable" 
> +                 style="display: none" cellspacing="0" dir="auto"/>

Did you consider getting the <datalist> flickering issues fixed before switching to <table>? That issue would affect content too and <datalist> is meant for this use case (including live suggestions). It would also handle form history and keyboard navigation for you. You're also going to have to deal with screen readers here I think e.g. apppropriate @aria* attributes.

I believe cellspacing is deprecated[1] and inline @style is frowned upon (you can just set it in the stylesheet) or use @hidden (which I think is preferred in this case for a11y).

[1] http://wiki.whatwg.org/wiki/Presentational_elements_and_attributes#Presentational_attributes

::: browser/base/content/content.js
@@ +65,5 @@
>        addEventListener("pagehide", UITour);
>    }, false, true);
>  }
>  
> +let contentSearch = {

I don't want content.js to turn into a dumping ground like browser.js became. This is border-line large enough that I think I'd prefer this new object and the getter was in content-searchSuggest.js and pre-processed here.

Also, this would make a nice WebComponent but unfortunately they're not enabled by default yet.

@@ +116,5 @@
> +    let self = this;
> +    SearchSuggest.search(searchTerm,
> +                         Services.search.currentEngine,
> +                         PrivateBrowsingUtils.isWindowPrivate(content),
> +                         function (obj) {self.addSuggestions(obj)});

The last argument looks like a good candidate for an arrow function.

	Mike Connor [:mconnor]
	Comment 38 • 10 years ago

(In reply to Matthew N. [:MattN] from comment #37)

> Oops, this was your new code that I mentioned in another review. Please use
> the constants instead of magic numbers:
> https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.
> keyCode#Constants_for_keyCode_value

For some reason I thought these had been deprecated in non-chrome contexts.  Assuming that's not true, I'll make the switch (not a big fan of magic numbers).

> @@ +450,5 @@
> > +    function addRow(type, str) {
> > +      let row = document.createElement("tr");
> > +      row.className = "resultRow";
> > +      row.setAttribute("type", type);
> > +      row.onmousemove = searchSuggest.handleMousemove;
> 
> use addEventListener instead of attributes/properties for events in new code

Is there some documentation for why this is preferred?  Doesn't this practice require explicit removeEventListener calls every time we remove a row?

> @@ +462,5 @@
> > +      let terms = results.term.toLowerCase().split(" ");
> > +      let innerHTML = "<span>";
> > +      for (let i = 0; i < words.length; i++) {
> > +        // HTML special chars
> > +        let val = words[i].replace(/&/g, "&amp;")
> 
> For security, I believe using textContent is preferred over regexes.

Can you clarify this comment?  textContent is a DOM function.

> @@ +467,5 @@
> > +                          .replace(/"/g, "&quot;")
> > +                          .replace(/</g, "&lt;")
> > +                          .replace(/>/g, "&gt;");
> > +        if (terms.indexOf(words[i]) == -1)
> > +          innerHTML += "<b>" + val + "</b>";
> 
> I wonder how this affects a11y and whether you should use <strong> instead.

In general, I _hope_ that this has the same a11y properties as what Google uses.
 
> ::: browser/base/content/abouthome/aboutHome.xhtml
> @@ +41,5 @@
> >            <input type="text" name="q" value="" id="searchText" maxlength="256"
> > +                 autofocus="autofocus" dir="auto" autocomplete="off"
> > +                 oninput="searchSuggest.requestSuggestions()"
> > +                 onblur="searchSuggest.clearResults()"
> > +                 onkeypress="searchSuggest.onSearchKeypress(event)"/>
> 
> Please switch to addEventListener for any new code (the onsubmit should have
> also used it).

Again, this seems pretty odd as a practice, and somewhat baroque, since it requires a lot of manual work.  I'm sure there's a reason for it, I'd like to understand why we're moving in that direction.

> @@ +42,5 @@
> > +                 autofocus="autofocus" dir="auto" autocomplete="off"
> > +                 oninput="searchSuggest.requestSuggestions()"
> > +                 onblur="searchSuggest.clearResults()"
> > +                 onkeypress="searchSuggest.onSearchKeypress(event)"/>
> > +          <table id="searchSuggestTable" class="searchSuggestTable" 
> 
> Nit: trailing whitespace. You may want to set your editor to show it. The
> class seems redundant.



> @@ +43,5 @@
> > +                 oninput="searchSuggest.requestSuggestions()"
> > +                 onblur="searchSuggest.clearResults()"
> > +                 onkeypress="searchSuggest.onSearchKeypress(event)"/>
> > +          <table id="searchSuggestTable" class="searchSuggestTable" 
> > +                 style="display: none" cellspacing="0" dir="auto"/>
> 
> Did you consider getting the <datalist> flickering issues fixed before
> switching to <table>? That issue would affect content too and <datalist> is
> meant for this use case (including live suggestions). It would also handle
> form history and keyboard navigation for you. You're also going to have to
> deal with screen readers here I think e.g. apppropriate @aria* attributes.

I did, right up until I realized that a core use-case for search suggestions is spelling correction, and there's no sane way to use datalist to simulate that.  There's also no good way of preventing the flickering without manually controlling updating (since the flicker was "filter current list until suggestions update"), which starts to negate the value even if you ignore the "don't actually filter" behaviour.  It also didn't allow for richer styling.  datalist is great, but it's not the droid we're looking for here.

I'll get a build to the a11y team for feedback soon.  I think the fact that I'm faking selection, instead of doing selection, will make this a lot easier.

> I believe cellspacing is deprecated[1] and inline @style is frowned upon
> (you can just set it in the stylesheet) or use @hidden (which I think is
> preferred in this case for a11y).

Yeah, I couldn't remember which CSS property correctly replaced it when I was hacking this initially, will fix.

> I don't want content.js to turn into a dumping ground like browser.js
> became. This is border-line large enough that I think I'd prefer this new
> object and the getter was in content-searchSuggest.js and pre-processed here.

pre-processing doesn't really make it less of a dumping ground, and makes scope less obvious.  I can split it out, but I don't think we're really winning much by doing it.

> Also, this would make a nice WebComponent but unfortunately they're not
> enabled by default yet.
> 
> @@ +116,5 @@
> > +    let self = this;
> > +    SearchSuggest.search(searchTerm,
> > +                         Services.search.currentEngine,
> > +                         PrivateBrowsingUtils.isWindowPrivate(content),
> > +                         function (obj) {self.addSuggestions(obj)});
> 
> The last argument looks like a good candidate for an arrow function.

You kids and your new-fangled Javascript.

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

(In reply to Mike Connor [:mconnor] from comment #38)
> > use addEventListener instead of attributes/properties for events in new code
> 
> Is there some documentation for why this is preferred?  Doesn't this
> practice require explicit removeEventListener calls every time we remove a
> row?

It does not. They do the same thing under the hood; how you add the listener doesn't impact whether you need to remove it.

> Again, this seems pretty odd as a practice, and somewhat baroque, since it
> requires a lot of manual work.  I'm sure there's a reason for it, I'd like
> to understand why we're moving in that direction.

It doesn't require any more manual work than the on* properties. I don't feel strongly anti-on* property, but consistency does help grep-ability.

> pre-processing doesn't really make it less of a dumping ground, and makes
> scope less obvious.  I can split it out, but I don't think we're really
> winning much by doing it.

Disagree! There should be no reason for shared scope to be particularly relevant here, and avoiding humongo-files is virtuous.

	Matthew N. [:MattN]
	Comment 40 • 10 years ago

(In reply to Mike Connor [:mconnor] from comment #38)
> > use addEventListener instead of attributes/properties for events in new code
> 
> Is there some documentation for why this is preferred?  Doesn't this
> practice require explicit removeEventListener calls every time we remove a
> row?

See below about content attributes. For the on* case it's to avoid clobbering existing listeners when you add a second and for consistency (always using addEventListener).

> > @@ +462,5 @@
> > > +      let terms = results.term.toLowerCase().split(" ");
> > > +      let innerHTML = "<span>";
> > > +      for (let i = 0; i < words.length; i++) {
> > > +        // HTML special chars
> > > +        let val = words[i].replace(/&/g, "&amp;")
> > 
> > For security, I believe using textContent is preferred over regexes.
> 
> Can you clarify this comment?  textContent is a DOM function.

Right, and you can create the element and set its textContent instead of using innerHTML. e.g. 
let bEl = document.createElement("b");
bEl.textContent = words[i];
span.appendChild(bEl);

Setting the textContent deals with the escaping issue for you.

> > ::: browser/base/content/abouthome/aboutHome.xhtml
> > @@ +41,5 @@
> > >            <input type="text" name="q" value="" id="searchText" maxlength="256"
> > > +                 autofocus="autofocus" dir="auto" autocomplete="off"
> > > +                 oninput="searchSuggest.requestSuggestions()"
> > > +                 onblur="searchSuggest.clearResults()"
> > > +                 onkeypress="searchSuggest.onSearchKeypress(event)"/>
> > 
> > Please switch to addEventListener for any new code (the onsubmit should have
> > also used it).
> 
> Again, this seems pretty odd as a practice, and somewhat baroque, since it
> requires a lot of manual work.  I'm sure there's a reason for it, I'd like
> to understand why we're moving in that direction.

One reason for not using the attributes is to keep a separation between the markup and the code and some other reasons at https://en.wikipedia.org/wiki/Unobtrusive_JavaScript#Separation_of_behavior_from_markup

While reviewing this and related patches I realized that some of my comments belong in the coding style guide.

> > @@ +43,5 @@
> > > +                 oninput="searchSuggest.requestSuggestions()"
> > > +                 onblur="searchSuggest.clearResults()"
> > > +                 onkeypress="searchSuggest.onSearchKeypress(event)"/>
> > > +          <table id="searchSuggestTable" class="searchSuggestTable" 
> > > +                 style="display: none" cellspacing="0" dir="auto"/>
> > 
> > Did you consider getting the <datalist> flickering issues fixed before
> > switching to <table>? That issue would affect content too and <datalist> is
> > meant for this use case (including live suggestions). It would also handle
> > form history and keyboard navigation for you. You're also going to have to
> > deal with screen readers here I think e.g. apppropriate @aria* attributes.
> 
> I did, right up until I realized that a core use-case for search suggestions
> is spelling correction, and there's no sane way to use datalist to simulate
> that.  There's also no good way of preventing the flickering without
> manually controlling updating (since the flicker was "filter current list
> until suggestions update"), which starts to negate the value even if you
> ignore the "don't actually filter" behaviour.

I see, the filtering issue does mess this up which seems like an unfortunate oversight in implementations since the spec doesn't require filtering AFAICT. It seems the HTML spec should have given a way to enable/disable UA filtering of the results to address the spelling correction use case.

> It also didn't allow for richer styling.

We can style it however we want by using a different panel like PopupAutoCompleteRichResult does but it would require a non-standard <input> extension. I have a patch from years ago to do bolding in form history dropdowns and it would be nice to share some of these styles with it.

	Mike Connor [:mconnor]
	Comment 41 • 10 years ago

* rolls in fix for bug 1007988
* rolls in fix from bug 1007985 (no concerns raised in that bug)
* rolls in deletion patch from this bug
* all feedback addressed (I think)

Only open concern is accessibility (screen readers).  This should work as well as the google.com searchbox, but I don't know how well that works.  Will flag dbolter as a followup from IRC conversation, happy to do whatever is needed to make this work on that front.

	Marco Zehe (:MarcoZ)
	Comment 42 • 10 years ago

Comment on attachment 8426507 [details] [diff] [review]
v1.0

>+    function addRow(type, str) {
>+      let row = document.createElement("tr");
>+      row.className = "resultRow";
>+      row.setAttribute("type", type);
>+      row.addEventListener("mousemove", searchSuggest.handleMousemove);
>+      row.addEventListener("mousedown", searchSuggest.handleMousedown);
>+      tbl.appendChild(row);
>+      let entry = document.createElement("td");
>+      entry.className = "resultEntry";
>+      row.appendChild(entry);
>+      let words = str.split(" ");
>+      // we're only using this for matching, and most search APIs lowercase everything.
>+      let terms = results.term.toLowerCase().split(" ");
>+      for (let i = 0; i < words.length; i++) {
>+        let word = null;
>+        if (terms.indexOf(words[i]) == -1) {
>+          word = document.createElement("strong");
>+        }
>+        else {
>+          word = document.createElement("span");
>+        }
>+        word.textContent = words[i] + " ";
>+        entry.appendChild(word);
>+      }
>+      i++;
>+    }

This creates a data table with rows which have only one table cell per row which will contain the search results, one per row.

For all intents and purposes, this is what we in accessibility land call a layout table[1]. This will not only cause problems for screen reader users, but also for the visual layout once you start throwing in High-DPI vs. non-high-DPI screens and other factors.
Can the same be achieved via nested divs?

Because: For this auto-complete to work with screen readers, we have to apply some WAI-ARIA magic.

1. the text box needs to receive an aria-autocomplete="true" to indicate that it has potentially an auto-complete functionality.
2. It should also get an aria-expanded="true" (when the auto-complete results become visible) and aria-expanded="false" when they disappear. This will automatically indicate to screen readers a change in state.
3. The main search results container, which is currently a table element, must receive the WAI-ARIA role of "listbox". This indicates to screen readers that this attached control is a list box containing multiple items.
4. Every item, which would correspond to the td inside the tr in this case, would need to get a role of "option", indicating that this is a selectable item within a listbox.
5. For screen readers to recognize the focus/highlight change, the following additional things need to happen:
a) Every item must get a unique ID.
b) An attribute called aria-activedescendant must be added to the item with role "listbox", and its value dynamically changed to the ID of the currently highlighted item.
6. If we stick to the table above, the tr elements must get a role of"presentation", because screen readers actually don't need to know that they're there.
I explain more about how auto-completes work and give an example from Twitter in my blog[2].

So the first question that we should ask is, if this really needs to be an HTML table. From reading the patch, it feels like the answer could very well be "no".

In any case, the way the patch is now, it would not mean an accessible experience for screen reader users yet.

[1] http://webaim.org/techniques/tables/#uses
[2] http://www.marcozehe.de/2014/03/11/easy-aria-tip-7-use-listbox-and-option-roles-when-constructing-autocomplete-lists/

	Mike Connor [:mconnor]
	Comment 43 • 10 years ago

Marco, I haven't figured out how to get the localization bit right yet for the polite bit, but please test that this is otherwise working correctly.

	Marco Zehe (:MarcoZ)
	Comment 44 • 10 years ago

Comment on attachment 8428810 [details] [diff] [review]
aboutHomeReal

Looks good on first glance, I'll give it an actual test tomorrow (EOD for today).

	Marco Zehe (:MarcoZ)
	Comment 45 • 10 years ago

Comment on attachment 8428810 [details] [diff] [review]
aboutHomeReal

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

::: browser/base/content/content.js
@@ +18,5 @@
>    "resource://gre/modules/PrivateBrowsingUtils.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "UITour",
>    "resource:///modules/UITour.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "SearchSuggest",
> +  "resource://gre/modules/SearchSuggest.jsm");

This file is missing from the patch, so I cannot test it properly. :( Browser Console says that it cannot load this module.

	Mike Connor [:mconnor]
	Comment 46 • 10 years ago

Sorry Marco, this is built on top of bug 1007979 (because I had to refactor suggestions to be a shareable component without autocomplete dependencies for this bug).  Please apply that patch first.

	Marco Zehe (:MarcoZ)
	Comment 47 • 10 years ago

Comment on attachment 8428810 [details] [diff] [review]
aboutHomeReal

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

Thanks Mike, that did the trick!

This works very well now! Only a few things to add, and we're really good!

::: browser/base/content/abouthome/aboutHome.js
@@ +469,5 @@
> +      let row = tbl.childNodes[i];
> +      if (i == val) {
> +        row.setAttribute("highlight", true);
> +        searchField.value = this._currentSuggestions[i];
> +        tbl.setAttribute("aria-activedescendant", this.SUGGESTION_ID_PREFIX + i);

Add another line below here along the lines of:

row.firstChild.setAttribute("aria-selected", "true");

to indicate that this is the selected option item.

@@ +472,5 @@
> +        searchField.value = this._currentSuggestions[i];
> +        tbl.setAttribute("aria-activedescendant", this.SUGGESTION_ID_PREFIX + i);
> +      }
> +      else {
> +        row.removeAttribute("highlight");

Likewise below here add:

row.firstChild.setAttribute("aria-selected", "false");

(or whichever means you feel is appropriate to get to the td element below the row.)

@@ +505,5 @@
> +      row.addEventListener("mousedown", searchSuggest.handleMousedown);
> +      tbl.appendChild(row);
> +      let entry = document.createElement("td");
> +      entry.className = "resultEntry";
> +      entry.setAttribute("role", "option");

...and preinitialize:

entry.setAttribute("aria-selected", "false"),

::: browser/base/content/abouthome/aboutHome.xhtml
@@ +39,5 @@
>          <form name="searchForm" id="searchForm" onsubmit="onSearchSubmit(event)">
>            <div id="searchLogoContainer"><img id="searchEngineLogo"/></div>
>            <input type="text" name="q" value="" id="searchText" maxlength="256"
> +                 autofocus="autofocus" dir="auto" autocomplete="off"
> +                 aria-autocomplete="true" />

Add two more attributes here:

- aria-labelledby="searchEngineLogo": Since there is no text label, but screen reader users should hear immediately which search engine is being used, label the text field using a reference to the search engine's logo to label it.

- aria-controls="searchSuggestTable": To indicate that this field controls what gets displayed in the search results. This provides an explicit link for assistive technologies so they know which item is being managed by the auto-complete functionality.

And that's it! Very nice user experience then! Thank you! :-)

	Mike Connor [:mconnor]
	Comment 48 • 10 years ago

Now with Marco's feedback addressed (woo a11y!)

	Matthew N. [:MattN]
	Comment 49 • 10 years ago

Comment on attachment 8432792 [details] [diff] [review]
aboutHomeReal

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

r- for the content-search E10S issue. Please ensure you've tested the next version in an E10S window and this hasn't regressed anything.

::: browser/base/content/abouthome/aboutHome.css
@@ +112,5 @@
> +  padding: 0;
> +  box-shadow: 0 1px 0 hsla(210,65%,9%,.02) inset,
> +              0 0 2px hsla(210,65%,9%,.1) inset,
> +              0 1px 0 hsla(0,0%,100%,.2);
> +  border-spacing: 0;

Mind sorting the properties alphabetically (where possible) to make them easier to scan and keep the border properties together?

@@ +120,5 @@
> +  padding: 0;
> +  margin: 0;
> +}
> +
> +.resultRow[type="local"] {

.resultRow.local

::: browser/base/content/abouthome/aboutHome.js
@@ +318,5 @@
> +let searchSuggest = {
> +  MAX_RESULTS: 6,
> +  SUGGESTION_ID_PREFIX: "aboutHomeSuggestion",
> +
> +  init: function (el) {

Nit: remove the spaces |function| and |(| as I believe most new code doesn't do that. Ignore if you wish.

@@ +344,5 @@
> +  },
> +
> +  receiveMessage: function (event) {
> +    switch (event.data.type) {
> +      case "SearchSuggestClear":

I personally would prefer a delimiter between the namespace and the verb but I'm not sure if there is precedence for it. e.g. SearchSuggest-Clear. Feel free to ignore.

@@ +500,5 @@
> +
> +    function addRow(type, str) {
> +      let row = document.createElement("tr");
> +      row.className = "resultRow";
> +      row.setAttribute("type", type);

I would prefer type was another class instead of using a |type| attribute which can be confused with existing HTML attributes and may be standardized in the future.

@@ +504,5 @@
> +      row.setAttribute("type", type);
> +      row.setAttribute("role", "presentation");
> +      row.addEventListener("mousemove", searchSuggest.handleMousemove);
> +      row.addEventListener("mousedown", searchSuggest.handleMousedown);
> +      tbl.appendChild(row);

For better perf you should move this down to where the contents of the row are fully ready otherwise you're mutating the visible DOM more than necessary.

@@ +505,5 @@
> +      row.setAttribute("role", "presentation");
> +      row.addEventListener("mousemove", searchSuggest.handleMousemove);
> +      row.addEventListener("mousedown", searchSuggest.handleMousedown);
> +      tbl.appendChild(row);
> +      let entry = document.createElement("td");

Nit: the function could use some whitespace. Put a new line above this line since it's for a new element.

@@ +511,5 @@
> +      entry.setAttribute("role", "option");
> +      entry.id = searchSuggest.SUGGESTION_ID_PREFIX + i;
> +      entry.setAttribute("aria-selected", "false")
> +      row.appendChild(entry);
> +      let words = str.split(" ");

Nit: newline above for readabilty

@@ +513,5 @@
> +      entry.setAttribute("aria-selected", "false")
> +      row.appendChild(entry);
> +      let words = str.split(" ");
> +      // we're only using this for matching, and most search APIs lowercase everything.
> +      let terms = results.term.toLowerCase().split(" ");

Distinguishing the |words| and |terms| variables names would be useful. They both mean basically the same thing to me in this context. How about searchTerms and resultTerms?

@@ +528,5 @@
> +      }
> +      i++;
> +    }
> +
> +    let i = 0;

This variable name could be more descriptive. Could you also hoist it above addRow as I initially wasn't sure if the |i++| above was referring to the same variable or not.

@@ +533,5 @@
> +    while (i < results.local.length && i < this.MAX_RESULTS) {
> +      this._currentSuggestions.push(results.local[i]);
> +      addRow("local", results.local[i]);
> +    }
> +    let j = 0;

ditto about the variable name

@@ +545,5 @@
> +  clearResults: function () {
> +    let input = document.getElementById("searchText");
> +    input.setAttribute("aria-expanded", false);
> +    let tbl = document.getElementById("searchSuggestTable");
> +    tbl.style.display = "none";

tbl.hidden = true

@@ +556,5 @@
> +  requestSuggestions: function () {
> +    this._currentSearch = document.getElementById("searchText").value;
> +
> +    let eventData = {
> +      type: "suggestion",

I would prefer a better type name (mostly to make suggestion plural). e.g. requestSuggestions or getSuggestions

@@ +566,5 @@
> +  setupResultsTable: function () {
> +    let textfield = document.getElementById("searchText")
> +    let textRect = textfield.getBoundingClientRect();
> +    let left = textRect.left;
> +    let top = textRect.bottom;

Nit: I think you could use object destructuring here if you want to combine these two lines but I think it's probably better separate because of the bottom => top assignment. A prefix on the |left| and |top| variables wouldn't hurt e.g. suggestionsLeft. Feel free to ignore.

::: browser/base/content/abouthome/aboutHome.xhtml
@@ +45,2 @@
>            <input id="searchSubmit" type="submit" value="&abouthome.searchEngineButton.label;"/>
> +          <table id="searchSuggestTable" class="searchSuggestTable"

In my last review I said "The class seems redundant." Please remove it or explain why it's necessary.

@@ +45,3 @@
>            <input id="searchSubmit" type="submit" value="&abouthome.searchEngineButton.label;"/>
> +          <table id="searchSuggestTable" class="searchSuggestTable"
> +                 style="display: none" role="listbox" dir="auto"/>

I suggested @hidden=true instead of the inline style in my last review but it seems like you missed some things. Was there a problem with that?

::: browser/base/content/content-search.js
@@ +12,5 @@
> +        try {
> +          let term = details.searchTerm;
> +          this.getSuggestions(term);
> +        }
> +        catch (ex) {

Nit: cuddle the catch

@@ +17,5 @@
> +          this.clearSuggestions();
> +        }
> +        break;
> +      case "focus":
> +        Services.search.currentEngine.speculativeConnect(content.window);

Um, IIUC, this is running in the content process so you shouldn't use Services.search from it. See bug 927132 and bug 930456. It seems like the contents of this file are meant for the chrome process so they need to find a new home e.g. browser-search.js. Perhaps I'm missing something.

@@ +42,5 @@
> +    let msg = {
> +      type: type,
> +      payload: payload
> +    }
> +    content.window.postMessage(msg, "*");

TODO: Does postMessage not support about URIs? This seems like a potential security issue.

@@ +54,5 @@
> +    if (Services.search.currentEngine.supportsResponseType("application/x-suggestions+json")) {
> +      this._suggest.localResults = 2;
> +      this._suggest.remoteResults = 6;
> +    }
> +    else {

Nit: cuddle the else

@@ +72,5 @@
> +
> +  _formHistoryResult: null,
> +  _removeSuggestion: function (term) {
> +    let entries = [];
> +    this._formHistoryResult.wrappedJSObject.entries

Reaching into wrappedJSObject like this makes me want a test. You can defer it to a follow-up if you're in a rush to land this as I don't expect form history result changes soon.

@@ +75,5 @@
> +    let entries = [];
> +    this._formHistoryResult.wrappedJSObject.entries
> +        .forEach(function extractEntries (el) {entries.push(el.text)});
> +    let index = entries.indexOf(term);
> +    if (index > -1)

It's not clear to me why you need to build up the temporary entries array when you could check |term == el.text| in the forEach and call removeValueAt then. If you convert it to for…of then you can |break| after the first match too. (I don't know if you can break from forEach).

	Drew Willcoxon :adw Assignee
	Comment 50 • 10 years ago

Bug 1028985 is adding search suggestions to about:newtab.  The two pages should use the same implementation where possible, and I think it's feasible to share practically all of the implementation of search suggestions between the two.  So if we fix this bug first, then fixing the newtab bug ought to be very easy, and vice versa.

The patch here has three parts: UI code in aboutHome.js that keeps the page's DOM updated, which talks with a new content-search.js content script, which calls into the SearchSuggest.jsm added by bug 1007979.

I'm thinking that the aboutHome.js changes could be moved to a new file that both about:home and newtab could include.  This code creates an implicit API by expecting a certain <table> in the page and element IDs that both pages would need to adhere to, or maybe all the necessary structure and bindings could be created programmatically.

content-search.js calls into Services.search and SearchSuggest.jsm directly, but it would be safer to do that in chrome instead of a content script.  So I think most of what content-search.js does should be moved to ContentSearch.jsm, which about:newtab's search field uses, or a helper jsm that's imported by ContentSearch.

Finally, content and chrome need a way to communicate, and that's what ContentSearchMediator in content.js already does for about:newtab, so we can reuse that without any changes at all aside from whitelisting about:home.

	Marco Zehe (:MarcoZ)
	Comment 51 • 10 years ago

If the patches here need to be re-worked somehow, please re-request a11y-review from me (now a patch flag) so we can make sure accessibility is in when this lands. Thanks!

	Mike Connor [:mconnor]
	Comment 52 • 10 years ago

Drew, this is the current version of my patch, has everything sorted from Matt's comments except for the e10s bits.

	Drew Willcoxon :adw Assignee
	Comment 54 • 10 years ago

I'm polishing the content part, but here are the content.js and ContentSearch.jsm parts.  I don't anticipate needing to change the API to accommodate the content part, but maybe I've overlooked something.

Felipe, I'm asking you for review since you've reviewed ContentSearch.jsm before, but if you want someone else to look at it due to the SearchSuggest and FormHistory uses, I understand, and I'll ask Matt.

The change to _fireEvent in content.js was necessary to avoid this:

> [86732] WARNING: Not same origin error!: file /Users/adw/fx/dom/base/nsJSEnvironment.cpp, line 491

newtab is privileged, which is why that error didn't happen there I guess.

This patch assumes a couple of changes to SearchSuggest from the patch that's posted to bug 1007979: (1) SearchSuggest's callback is passed the engine that made the suggestions instead of no engine at all, and it's also passed an empty `remote` array in failure cases instead of null.  (2) SearchSuggest.FORM_HISTORY_FIELD_NAME = "searchbar-history".

This patch also makes some changes to Mike's original suggestions patch here in this bug other than the big architectural changes sketched in comment 50:

(1) The original patch had a "clear suggestions" message, but I think we can avoid that, so this patch doesn't have it.

(2) This patch adds a AddFormHistoryEntry message that wasn't present in the original patch because the latter piggybacked on the AboutHome:Search message to update form history.  If about:home used ContentSearch, then we could just update form history on ContentSearch's Search message and we wouldn't need a separate message, but it doesn't.

(3) Instead of using formHistoryResult, an nsIAutoCompleteResult, to delete local suggestions, this patch uses FormHistory.jsm.  I think that should produce the same result?  The test passes anyway.  (Using formHistoryResult would mean that ContentSearch would need to keep around some number of formHistoryResults because the client may send a Delete message for a suggestions list previous to the suggestions list that ContentSearch last returned.  Earlier versions of my patch worked that way.)

	Drew Willcoxon :adw Assignee
	Comment 55 • 10 years ago

Another change from Mike's patch is that (4) messages that need an engine take an engineName argument instead of assuming that clients want to use the current engine because all the asynchronicity makes that assumption unsafe.

	Drew Willcoxon :adw Assignee
	Comment 56 • 10 years ago

Actually, giving an unprivileged page access to the user's form history is a bad idea, no?  If about:home were compromised, malicious script could try a bunch of searches to see what the user's been typing into the search bar.

	Drew Willcoxon :adw Assignee
	Comment 57 • 10 years ago

These are the content changes.  They're more or less done, maybe some tweaking left, but I'm still working on tests.

Try builds will be here: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/dwillcoxon@mozilla.com-587cd4c3b6e9

I don't think I like how untyped words are bold.  Logically it makes sense -- calling attention to possible completions -- but visually, once you start typing, everything is bold, and it's all just too heavy.  I tried inverting it so typed words are bold and I think I like that better.  The contrast is still there, and that's the key thing.

I really wish text-overflow could ellipsize only the beginning of text.

	Drew Willcoxon :adw Assignee
	Comment 58 • 10 years ago

Matt, would you mind reviewing since you looked at Mike's patch?  And what do you think about comment 56?

	Drew Willcoxon :adw Assignee
	Comment 59 • 10 years ago

Based on the patch in bug 1007979.

	Drew Willcoxon :adw Assignee
	Comment 60 • 10 years ago

Comment on attachment 8453661 [details] [diff] [review]
content.js and ContentSearch.jsm changes

Actually this needs one more change to content.js, which I'll batch with review comments:

> @@ -242,33 +243,35 @@ let ContentSearchMediator = {
>        return;
>      }
>      if (this._contentWhitelisted) {
>        this._fireEvent(msg.data.type, msg.data.data);
>      }
>    },
> 
>    get _contentWhitelisted() {
> -    return this.whitelist.has(content.document.documentURI.toLowerCase());
> +    return this.whitelist.has(content.document.documentURI);

	:Felipe Gomes (needinfo for replies!)
	Comment 61 • 10 years ago

Comment on attachment 8453661 [details] [diff] [review]
content.js and ContentSearch.jsm changes

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

::: browser/base/content/content.js
@@ +230,5 @@
>      });
>    },
>  
>    _fireEvent: function (type, data=null) {
> +    let event = Cu.cloneInto({

oh, nice

::: browser/modules/ContentSearch.jsm
@@ +188,5 @@
>    },
>  
> +  _onMessageGetSuggestions: function (msg, data) {
> +    let win = msg.target.contentWindow;
> +    let winData = this._suggestionsMap.get(win);

why use the contentWindow CPOW as the map key, instead of just msg.target which is the browser element?  Does it need to be different if the content window changes, or is it enough to be different per tab?

	Matthew N. [:MattN]
	Comment 62 • 10 years ago

Comment on attachment 8455951 [details] [diff] [review]
about:home changes + searchSuggest.js + tests

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

(In reply to Drew Willcoxon :adw from comment #56)
> Actually, giving an unprivileged page access to the user's form history is a
> bad idea, no?  If about:home were compromised, malicious script could try a
> bunch of searches to see what the user's been typing into the search bar.

I agree it's not a good idea. Neither is allowing additions or deletions. Additions may not be too hard to solve (see below) but it seems like appending the history entries on the chrome side may be somewhat more work. I don't think the security issue is that bad so perhaps we should get another opinion. If it's deemed not acceptable then I think landing this without the history APIs initially is fine as I think the server suggestions are more valuable.

::: browser/base/content/abouthome/aboutHome.js
@@ +318,4 @@
>  }
>  
>  
> +var gSearchSuggestController;

Nit: s/var/let/

::: browser/base/content/searchSuggest.js
@@ +84,5 @@
> +        row.firstChild.setAttribute("aria-selected", "true");
> +        this._table.setAttribute("aria-activedescendant", row.firstChild.id);
> +        this.input.value = this.suggestionAtIndex(i);
> +      }
> +      else {

Nit: cuddle the else

@@ +121,5 @@
> +    return row && row.getAttribute("type") == "formHistory";
> +  },
> +
> +  addInputValueToFormHistory: function () {
> +    this._sendMsg("AddFormHistoryEntry", this.input.value);

The ability to stuff spam into form history for the search box is also not ideal for an unprivileged page. We may be able to save the form history entry the normal way based on form submission. See https://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/formSubmitListener.js?rev=67e9e1fd1b6d#157 for why this is currently being done manually here.

::: browser/base/content/test/general/browser.ini
@@ +363,5 @@
>  [browser_save_video.js]
>  skip-if = e10s # Bug ?????? - test directly manipulates content (event.target)
>  [browser_scope.js]
> +[browser_searchSuggest.js]
> +skip-if = e10s # Bug ?????? - test directly manipulates content

I think this test should be made to work with e10s as it doesn't seem too hard and we intentionally changed the implementation to make it work with e10s so we should test it so it doesn't regress.

It may be easiest to switch this to mochitest-chrome.

::: browser/base/content/test/general/browser_searchSuggest.js
@@ +337,5 @@
> +  let url = getRootDirectory(gTestPath) + TEST_PAGE_BASENAME;
> +  promiseTabLoadEvent(tab, url).then(() => {
> +    let msgMan = tab.linkedBrowser.messageManager;
> +    msgMan.sendAsyncMessage("ContentSearch", {
> +      type: "AddToWhitelist",

Could you explain how the whitelist works or what it's for? At first glance, it seems like content can whitelist itself but perhaps I'm reading that wrong.

::: browser/base/content/test/general/searchSuggest.html
@@ +1,1 @@
> +<html>

Nit:missing doctype and public domain header

@@ +5,5 @@
> +        src="chrome://browser/content/searchSuggest.js">
> +</script>
> +<script type="application/javascript;version=1.8">
> +
> +var gSearchSuggestController;

Nit: s/var/let/

::: browser/base/content/test/general/searchSuggest.xml
@@ +1,2 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<SearchPlugin xmlns="http://www.mozilla.org/2006/browser/search/">

Missing public domain header

	Drew Willcoxon :adw Assignee
	Comment 63 • 10 years ago

There was an intermittent failure on try: https://tbpl.mozilla.org/?tree=Try&rev=dd9366e8cb5b

It took me a while to figure out, but the problem was due to the fact that SearchSuggest assumes that form history results will always be returned before remote provider results, but in the intermittent failures, form history results were actually returned after remote results.  This patch fixes that by not assuming one will finish before the other.  Green try run: https://tbpl.mozilla.org/?tree=Try&rev=f30073d1e3c4

	Drew Willcoxon :adw Assignee
	Comment 64 • 10 years ago

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #62)
> Additions may not be too hard to solve (see below)...
> 
> The ability to stuff spam into form history for the search box is also not
> ideal for an unprivileged page. We may be able to save the form history
> entry the normal way based on form submission. See
> https://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/
> formSubmitListener.js?rev=67e9e1fd1b6d#157 for why this is currently being
> done manually here.

Are you saying that a nicer fix to bug 461820 than what landed would help here?  Would we have to whitelist about:home and newtab, or is there some other way?  Do the security concerns -- for adding and fetching form history results -- go away in that case?

> I think this test should be made to work with e10s as it doesn't seem too
> hard and we intentionally changed the implementation to make it work with
> e10s so we should test it so it doesn't regress.

OK.

> ::: browser/base/content/test/general/browser_searchSuggest.js
> @@ +337,5 @@
> > +  let url = getRootDirectory(gTestPath) + TEST_PAGE_BASENAME;
> > +  promiseTabLoadEvent(tab, url).then(() => {
> > +    let msgMan = tab.linkedBrowser.messageManager;
> > +    msgMan.sendAsyncMessage("ContentSearch", {
> > +      type: "AddToWhitelist",
> 
> Could you explain how the whitelist works or what it's for? At first glance,
> it seems like content can whitelist itself but perhaps I'm reading that
> wrong.

Chrome can whitelist content, but content can't whitelist content.  Practically, the AddToWhitelist message is intended for tests so they can whitelist their test pages.  browser_ContentSearch.js and now browser_searchSuggest.js both do that.

This mediator thing receives events from content and forwards them as messages to chrome, and vice versa.  It only recognizes events from whitelisted pages, and it only forwards messages from chrome to whitelisted pages.

	Drew Willcoxon :adw Assignee
	Comment 65 • 10 years ago

This has an e10s-safe test.  It passes when I run it with --e10s, but it leaks several about:blanks and a couple of docshells.  I'm hoping it's some weird, local thing and tryserver will be OK.  Although are e10s browser chrome tests even running?  It doesn't look like it according to tbpl.

https://tbpl.mozilla.org/?tree=Try&rev=154f181973d4

This also has a fix for the problem on about:newtab where the popup would appear in the wrong place, like we talked about.  I tried inserting the table into document.documentElement, but it was still in the wrong place.  I think the reason has to do with the table's "containing block," since absolutely positioned elements are positioned relative to their containing blocks, and choosing the wrong containing block meant that the table would not be positioned where we want it.  You can't just choose any old containing block.  So I added a tableParent param to SearchSuggestController(), and that takes care of it.

This also fixes a problem where if the mouse happened to be over the popup when it opens, then the row that the mouse is over gets injected into the input, and it's impossible to edit the text in the input because the row keeps getting injected.  The problem was using mouseover instead of mousemove, like Mike's patch did, so I switched back to mousemove.

Finally this leaves in the form history stuff like we talked about.

	Matthew N. [:MattN]
	Comment 66 • 10 years ago

Comment on attachment 8453661 [details] [diff] [review]
content.js and ContentSearch.jsm changes

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

::: browser/modules/ContentSearch.jsm
@@ +40,5 @@
> + *     Adds an entry to the search form history.
> + *     data: the entry, a string
> + *   GetSuggestions
> + *     Retrieves an array of search suggestions given a search string.
> + *     data: { engineName, searchStr }

Nit: Why not searchString like "Search" uses?

	Matthew N. [:MattN]
	Comment 67 • 10 years ago

Comment on attachment 8457828 [details] [diff] [review]
SearchSuggest.jsm tweaks v2

Hey Drew, can you rebase this now that some of this was addressed in bug 1007979 and that patch significantly changed?

	Drew Willcoxon :adw Assignee
	Comment 68 • 10 years ago

Matt, Felipe reviewed this before, but after our discussions about form history, etc., it'd be good to get your review.  Next I'll post the interdiff between this and the patch that Felipe r+'ed.

	Drew Willcoxon :adw Assignee
	Comment 70 • 10 years ago

Next I'll post the interdiff between this and the v2 patch.

	Drew Willcoxon :adw Assignee
	Comment 71 • 10 years ago

I renamed many files, so in this interdiff I'm using the old file names to make the substantive changes much easier to see.

	Drew Willcoxon :adw Assignee
	Comment 72 • 10 years ago

Based on the latest patch in bug 1007979.

	Drew Willcoxon :adw Assignee
	Comment 73 • 10 years ago

With the latest patches: https://tbpl.mozilla.org/?tree=Try&rev=9962643f42a3

	Drew Willcoxon :adw Assignee
	Comment 74 • 10 years ago

This try looks better so far: https://tbpl.mozilla.org/?tree=Try&rev=4f25cd5bbee6

Due to a change in the test:

> diff --git a/browser/base/content/test/general/searchSuggestionUI.js b/browser/base/content/test/general/searchSuggestionUI.js
> --- a/browser/base/content/test/general/searchSuggestionUI.js
> +++ b/browser/base/content/test/general/searchSuggestionUI.js
> @@ -36,41 +36,35 @@ let messageHandlers = {
>    },
> 
>    mousemove: function (suggestionIdx) {
>      // Copied from widget/tests/test_panel_mouse_coords.xul and
>      // browser/base/content/test/newtab/head.js
>      let row = gController._table.children[suggestionIdx];
>      let rect = row.getBoundingClientRect();
>      let left = content.mozInnerScreenX + rect.left;
> -    let x1 = left + rect.width * 1 / 3;
> -    let x2 = left + rect.width * 2 / 3;
> +    let x = left + rect.width / 2;
>      let y = content.mozInnerScreenY + rect.top + rect.height / 2;
> 
>      let utils = content.SpecialPowers.getDOMWindowUtils(content);
>      let scale = utils.screenPixelsPerCSSPixel;
> 
>      let widgetToolkit = content.SpecialPowers.
>                          Cc["@mozilla.org/xre/app-info;1"].
>                          getService(Ci.nsIXULRuntime).
>                          widgetToolkit;
>      let nativeMsg = widgetToolkit == "cocoa" ? 5 : // NSMouseMoved
>                      widgetToolkit == "windows" ? 1 : // MOUSEEVENTF_MOVE
>                      3; // GDK_MOTION_NOTIFY
> 
> -    // On Windows and Linux, the mouse doesn't actually move and the test fails
> -    // unless sendNativeMouseEvent is called a few times like this...
> -    utils.sendNativeMouseEvent(x1 * scale, y * scale, nativeMsg, 0, null);
> -    setTimeout(() => {
> -      utils.sendNativeMouseEvent(x2 * scale, y * scale, nativeMsg, 0, null);
> -      setTimeout(() => {
> -        utils.sendNativeMouseEvent(x2 * scale, y * scale, nativeMsg, 0, null);
> -        ack();
> -      });
> +    row.addEventListener("mousemove", function onMove() {
> +      row.removeEventListener("mousemove", onMove);
> +      ack();
>      });
> +    utils.sendNativeMouseEvent(x * scale, y * scale, nativeMsg, 0, null);
>    },
> 
>    mousedown: function (suggestionIdx) {
>      gController.onSubmit = () => {
>        gController.onSubmit = null;
>        ack();
>      };
>      let row = gController._table.children[suggestionIdx];

	Drew Willcoxon :adw Assignee
	Comment 75 • 10 years ago

Uh, in my haste yesterday I hooked up the remoteTimeout property to SearchSuggestionUIController but not to SearchSuggestionController, so it didn't actually do anything.  Here's a new patch that makes SearchSuggestionUIController pass its remoteTimeout value to ContentSearch, which then sets the remoteTimeout property on its SearchSuggestionController to the value.

There was one very intermittent failure in the last try results that made me realize this:

17:37:02     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/modules/test/browser_ContentSearch.js | Console message: [JavaScript Error: "SearchSuggestionController: HTTP timeout" {file: "resource://gre/modules/SearchSuggestionController.jsm" line: 269}]

I'll post an interdiff next.

	Drew Willcoxon :adw Assignee
	Comment 76 • 10 years ago

This is the interdiff between the patch I just posted and the previous version, v3 -> v4.  It would be a lot easier to follow changes if I had been making commits to a repo somewhere...

	Drew Willcoxon :adw Assignee
	Comment 77 • 10 years ago

Removes my change to REMOTE_TIMEOUT and makes the timeout be REMOTE_TIMEOUT if the remoteTimeout property is falsey.

Try with most recent patches: https://tbpl.mozilla.org/?tree=Try&rev=d39ec6aa79b9

	Drew Willcoxon :adw Assignee
	Comment 78 • 10 years ago

browser_aboutHome.js should increase the remoteTimeout of the about:home controller to avoid the same intermittent failures.  I'll make that change locally.

	Matthew N. [:MattN]
	Comment 79 • 10 years ago

Comment on attachment 8462683 [details] [diff] [review]
SearchSuggestionController tweaks v4

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

::: toolkit/components/search/SearchSuggestionController.jsm
@@ +47,5 @@
>  
> +  /**
> +   * The maximum time (ms) to wait before giving up on a remote suggestions.
> +   */
> +  remoteTimeout: REMOTE_TIMEOUT,

Why don't you move the timeout value here and get rid of the const?

@@ +195,5 @@
>      if (this._request.channel instanceof Ci.nsIPrivateBrowsingChannel) {
>        this._request.channel.setPrivate(privateMode);
>      }
>      this._request.mozBackgroundRequest = true; // suppress dialogs and fail silently
> +    this._request.timeout = this.remoteTimeout || REMOTE_TIMEOUT;

This line doesn't exist in the new patch but I don't think we should worry about invalid values for the timeout.

@@ +313,5 @@
> +this.SearchSuggestionController.engineOffersSuggestions = function(engine) {
> + return engine.supportsResponseType(SEARCH_RESPONSE_SUGGESTION_JSON);
> +};
> +
> +this.SearchSuggestionController.DEFAULT_FORM_HISTORY_PARAM = DEFAULT_FORM_HISTORY_PARAM;

Move this as a public property on the SearchSuggestionController prototype and remove "default" from the name. That would allow consumers to change the field.

	Matthew N. [:MattN]
	Comment 80 • 10 years ago

Comment on attachment 8461762 [details] [diff] [review]
content.js and ContentSearch.jsm changes v2

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

::: browser/modules/ContentSearch.jsm
@@ +254,5 @@
> +  },
> +
> +  _onMessageRemoveFormHistoryEntry: function (msg, entry) {
> +    let browserData = this._suggestionsMap.get(msg.target);
> +    if (browserData && browserData.previousFormHistoryResult) {

When wouldn't we have previousFormHistoryResult?

@@ +268,5 @@
> +  },
> +
> +  _onMessageSpeculativeConnect: function (msg, engineName) {
> +    let engine = Services.search.getEngineByName(engineName);
> +    if (engine && msg.target.contentWindow) {

We shouldn't be so silent about the engine no being found. It will make it harder to detect regressions that break speculative connect.

	Matthew N. [:MattN]
	Comment 81 • 10 years ago

Comment on attachment 8462680 [details] [diff] [review]
about:home changes + searchSuggest.js + tests v4

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

I still have to review the test searchSuggestionUI.js, real searchSuggestionUI.css, browser_searchSuggestionUI.js and browser_ContentSearch.js.

::: browser/base/content/abouthome/aboutHome.xhtml
@@ +23,5 @@
>  
>      <link rel="icon" type="image/png" id="favicon"
>            href="chrome://branding/content/icon32.png"/>
>      <link rel="stylesheet" type="text/css" media="all"
> +          href="chrome://browser/content/searchSuggestionUI.css"/>

Why not <style scoped> (with @import) like I suggested on IRC? _makeTable would append the <style> dynamically so you would just drop-in the JS for a page.

@@ +30,5 @@
>  
>      <script type="text/javascript;version=1.8"
>              src="chrome://browser/content/abouthome/aboutHome.js"/>
> +    <script type="text/javascript;version=1.8"
> +            src="chrome://browser/content/searchSuggestionUI.js"/>

It looks like we could add @defer for a possible perf win since I don't think any JS outside searchSuggestionUI.js is reaching into it.

::: browser/base/content/searchSuggestionUI.js
@@ +30,5 @@
> + *        top left of the page.
> + * @param onSubmit
> + *        A function that's called when a search suggestion is clicked.  Ideally
> + *        we could call submit() on inputElement's ancestor form, but that
> + *        doesn't trigger submit listeners.

It seems like it would be more accurate to call it like it is: a function that gets called when results are clicked rather than assuming it's for form submission.

@@ +36,5 @@
> + *        The IDs of elements created by the object will be prefixed with this
> + *        string.
> + */
> +function SearchSuggestionUIController(inputElement, tableParent, onSubmit=null,
> +                                      idPrefix="") {

Nit: I've been seeing spaces around the default argument "=" most commonly.

@@ +42,5 @@
> +  this.onSubmit = onSubmit;
> +  this._idPrefix = idPrefix;
> +
> +  let tableID = idPrefix + "searchSuggestionTable";
> +  this.input.setAttribute("autocomplete", "off");

My understanding is that properties should be preferred over setAttribute for perf reasons (and conciseness) when possible so use this.input.autocomplete = "off";

@@ +86,5 @@
> +    this._table.removeAttribute("aria-activedescendant");
> +    for (let i = 0; i < this._table.children.length; i++) {
> +      let row = this._table.children[i];
> +      if (i == idx) {
> +        row.setAttribute("selected", "true");

.selected = …?

@@ +91,5 @@
> +        row.firstChild.setAttribute("aria-selected", "true");
> +        this._table.setAttribute("aria-activedescendant", row.firstChild.id);
> +        this.input.value = this.suggestionAtIndex(i);
> +      }
> +      else {

Cuddle else throughout

@@ +110,5 @@
> +
> +  // The timeout (ms) of the remote suggestions.  Corresponds to
> +  // SearchSuggestionController.remoteTimeout.  Uses
> +  // SearchSuggestionController's default timeout if falsey.
> +  remoteTimeout: undefined,

Don't we normally put the properties at the beginning of the prototype?

@@ +154,5 @@
> +
> +  _onKeypress: function (event) {
> +    let selectedIndexDelta = 0;
> +    switch (event.keyCode) {
> +    case event.DOM_VK_UP:

Nit: Indent another level

@@ +206,5 @@
> +      event.preventDefault();
> +    }
> +  },
> +
> +  _onFocus: function () {

Maybe rename to _speculativeConnect so it makes more sense seeing it called from the engineName setter? I forgot the function name is built in handleEvent.

@@ +213,5 @@
> +    }
> +  },
> +
> +  _onBlur: function () {
> +    this._hideSuggestions();

Delete this function and call _hideSuggestions directly? NVM, I forgot the function name is built in handleEvent

@@ +290,5 @@
> +
> +  _makeTableRow: function (type, suggestionStr, currentRow, searchWords) {
> +    let row = document.createElementNS(HTML_NS, "tr");
> +    row.classList.add("searchSuggestionRow");
> +    row.setAttribute("type", type);

It may help understanding to prefix our custom attributes with "data-" (using dataset) or ideally use a class if possible.

@@ +299,5 @@
> +    let entry = document.createElementNS(HTML_NS, "td");
> +    entry.classList.add("searchSuggestionEntry");
> +    entry.setAttribute("role", "option");
> +    entry.id = this._idPrefix + SUGGESTION_ID_PREFIX + currentRow;
> +    entry.setAttribute("aria-selected", "false")

Nit: missing ;

@@ +306,5 @@
> +    for (let i = 0; i < suggestionWords.length; i++) {
> +      let word = suggestionWords[i];
> +      let wordSpan = document.createElementNS(HTML_NS, "span");
> +      if (searchWords.has(word)) {
> +        wordSpan.setAttribute("typed", "true");

Use a class (.typed) instead?

@@ +341,5 @@
> +  },
> +
> +  _indexOfTableRow: function (row) {
> +    while (row && row.localName != "tr") {
> +      row = row.parentNode;

I guess this function doesn't just take rows and can take children of rows? Are we using this ability? If we keep it, change the argument name and/or function name to document this. Otherwise, this function is mostly the same as HTMLTableRowElement.rowIndex

@@ +347,5 @@
> +    if (!row) {
> +      throw new Error("Element is not a row");
> +    }
> +    for (let i = 0; i < this._table.children.length; i++) {
> +      if (this._table.children[i] == row) {

Can't you use HTMLTableRowElement.rowIndex for this second half if we decide to keep the above part?

@@ +358,5 @@
> +  },
> +
> +  _makeTable: function (id) {
> +    this._table = document.createElementNS(HTML_NS, "table");
> +    this._table.setAttribute("id", id);

this._table.id = …

@@ +362,5 @@
> +    this._table.setAttribute("id", id);
> +    this._table.classList.add("searchSuggestionTable");
> +    this._table.setAttribute("hidden", "true");
> +    this._table.setAttribute("role", "listbox");
> +    this._table.setAttribute("dir", "auto");

Use .hidden for consistency with other places where you set it. Same for @role and @dir, if applicable.

@@ +366,5 @@
> +    this._table.setAttribute("dir", "auto");
> +    return this._table;
> +  },
> +
> +  _sendMsg: function (type, data=null) {

Nit: spaces around =

::: browser/base/content/test/general/browser_aboutHome.js
@@ +394,5 @@
> +      EventUtils.synthesizeKey("x", {});
> +
> +      // Wait for the search suggestions to become visible.
> +      let table =
> +        gBrowser.contentDocument.getElementById("searchSuggestionTable")

Nit: missing semicolon

::: browser/base/content/test/general/searchSuggestionUI.html
@@ +1,4 @@
> +<!-- Any copyright is dedicated to the Public Domain.
> +   - http://creativecommons.org/publicdomain/zero/1.0/ -->
> +
> +<html>

add <!DOCTYPE html> to avoid quirks mode

::: browser/modules/test/contentSearchSuggestions.sjs
@@ +1,1 @@
> +/* Any copyright is dedicated to the Public Domain.

I would be fine with not duplicating this file and the engine by moving the tests to one directory (with a search subdirectory if you prefer).

::: toolkit/components/search/SearchSuggestionController.jsm
@@ +47,5 @@
>  
> +  /**
> +   * The maximum time (ms) to wait before giving up on a remote suggestions.
> +   */
> +  remoteTimeout: REMOTE_TIMEOUT,

I already reviewed this file in your other attachment.

	Matthew N. [:MattN]
	Comment 82 • 10 years ago

Comment on attachment 8462680 [details] [diff] [review]
about:home changes + searchSuggest.js + tests v4

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

Can you fix the splitting of the patches so I can test them together?

::: browser/base/content/searchSuggestionUI.css
@@ +5,5 @@
> +.searchSuggestionTable {
> +  position: absolute;
> +  z-index: 1001;
> +  text-align: start;
> +  background-color: hsla(0,0%,100%,.99);

Nit: sort alphabetically unless there is a good reason to keep some properties together. It makes it easier to scan to find a property.

@@ +25,5 @@
> +  cursor: default;
> +}
> +
> +.searchSuggestionRow[type="formHistory"] {
> +  color: #06C;

Consistency on hex RGB vs. hsl would be nice

::: browser/base/content/test/general/browser_searchSuggestionUI.js
@@ +198,5 @@
> +    yield msg("focus");
> +  });
> +}
> +
> +function msg(type, data=null) {

Nit: spaces around =

@@ +206,5 @@
> +  });
> +  let deferred = Promise.defer();
> +  gMsgMan.addMessageListener(TEST_MSG, function onMsg(msg) {
> +    gMsgMan.removeMessageListener(TEST_MSG, onMsg);
> +    deferred.resolve(msg.data)

Nit: missing semicolon

::: browser/base/content/test/general/searchSuggestionUI.js
@@ +1,1 @@
> +/* Any copyright is dedicated to the Public Domain.

Why do we need to mock this script?

@@ +7,5 @@
> +const ENGINE_NAME = "browser_searchSuggestionEngine searchSuggestionEngine.xml";
> +
> +let input = content.document.querySelector("input");
> +let gController =
> +  new content.SearchSuggestionUIController(input, input.parentNode);

Won't this fit on 100? The wrapping like this makes it harder to read IMO.

::: browser/modules/test/browser_ContentSearch.js
@@ +239,5 @@
> +    },
> +  });
> +
> +  // The formHistory suggestions in the Suggestions response should be empty.
> +  let msg = yield waitForTestMsg("Suggestions");

you're redeclaring msg here

	Drew Willcoxon :adw Assignee
	Comment 83 • 10 years ago

Carrying over r+.

(In reply to Matthew N. [:MattN] from comment #79)
> ::: toolkit/components/search/SearchSuggestionController.jsm
> @@ +47,5 @@
> >  
> > +  /**
> > +   * The maximum time (ms) to wait before giving up on a remote suggestions.
> > +   */
> > +  remoteTimeout: REMOTE_TIMEOUT,
> 
> Why don't you move the timeout value here and get rid of the const?
> 
> @@ +195,5 @@
> >      if (this._request.channel instanceof Ci.nsIPrivateBrowsingChannel) {
> >        this._request.channel.setPrivate(privateMode);
> >      }
> >      this._request.mozBackgroundRequest = true; // suppress dialogs and fail silently
> > +    this._request.timeout = this.remoteTimeout || REMOTE_TIMEOUT;
> 
> This line doesn't exist in the new patch but I don't think we should worry
> about invalid values for the timeout.

The point of this is to allow SearchSuggestionController's client to reset the timeout to the default value by setting remoteTimeout to undefined (or another falsey value).

ContentSearch's GetSuggestions message takes a remoteTimeout option.  That's the only way for its client to specify a timeout.  So what should ContentSearch do if one of its clients passes a remoteTimeout option in one message but then doesn't pass it the next message?  Because one SearchSuggestionController instance is meant to be used per content window and therefore per ContentSearch client, ContentSearch would have to set SearchSuggestionController.remoteTimeout to some default value.  But this way, it just sets remoteTimeout to undefined and SearchSuggestionController takes care of it.

> @@ +313,5 @@
> > +this.SearchSuggestionController.engineOffersSuggestions = function(engine) {
> > + return engine.supportsResponseType(SEARCH_RESPONSE_SUGGESTION_JSON);
> > +};
> > +
> > +this.SearchSuggestionController.DEFAULT_FORM_HISTORY_PARAM = DEFAULT_FORM_HISTORY_PARAM;
> 
> Move this as a public property on the SearchSuggestionController prototype

Done.  I had to change ContentSearch._onMessageAddFormHistoryEntry to create a SearchSuggestionController for the client if one didn't already exist.

	Drew Willcoxon :adw Assignee
	Comment 84 • 10 years ago

Carrying over the r+.

(In reply to Matthew N. [:MattN] from comment #80)
> ::: browser/modules/ContentSearch.jsm
> @@ +254,5 @@
> > +  },
> > +
> > +  _onMessageRemoveFormHistoryEntry: function (msg, entry) {
> > +    let browserData = this._suggestionsMap.get(msg.target);
> > +    if (browserData && browserData.previousFormHistoryResult) {
> 
> When wouldn't we have previousFormHistoryResult?

When maxRemoteResults is 0.  ContentSearch hardcodes maxRemoteResults now to a non-0 value, but it seemed safer to have this check.  But, with the ContentSearch._onMessageAddFormHistoryEntry change I just described in the previous comment, it's now possible for this to happen when the client hasn't sent a GetSuggestions but has sent an AddFormHistoryEntry.

> 
> @@ +268,5 @@
> > +  },
> > +
> > +  _onMessageSpeculativeConnect: function (msg, engineName) {
> > +    let engine = Services.search.getEngineByName(engineName);
> > +    if (engine && msg.target.contentWindow) {
> 
> We shouldn't be so silent about the engine no being found. It will make it
> harder to detect regressions that break speculative connect.

Changed to throw an error when the engine is null, like _onMessageGetSuggestions does.  (The error is caught higher up the stack by ContentSearch and reported.)

	Drew Willcoxon :adw Assignee
	Comment 85 • 10 years ago

(In reply to Matthew N. [:MattN] from comment #81)
> Why not <style scoped> (with @import) like I suggested on IRC? _makeTable
> would append the <style> dynamically so you would just drop-in the JS for a
> page.

I hit a failing nsCSSRuleProcessor.cpp assertion in about:newtab when I tried that, as we talked about, so I stuck with <link> and <?xml-stylesheet> for now.  Bug 1017798 and bug 1021572 may be related.

> @@ +30,5 @@
> >  
> >      <script type="text/javascript;version=1.8"
> >              src="chrome://browser/content/abouthome/aboutHome.js"/>
> > +    <script type="text/javascript;version=1.8"
> > +            src="chrome://browser/content/searchSuggestionUI.js"/>
> 
> It looks like we could add @defer for a possible perf win since I don't
> think any JS outside searchSuggestionUI.js is reaching into it.

Done.

> ::: browser/base/content/searchSuggestionUI.js
> @@ +30,5 @@
> > + *        top left of the page.
> > + * @param onSubmit
> > + *        A function that's called when a search suggestion is clicked.  Ideally
> > + *        we could call submit() on inputElement's ancestor form, but that
> > + *        doesn't trigger submit listeners.
> 
> It seems like it would be more accurate to call it like it is: a function
> that gets called when results are clicked rather than assuming it's for form
> submission.

Changed to onClick.

> @@ +42,5 @@
> > +  this.onSubmit = onSubmit;
> > +  this._idPrefix = idPrefix;
> > +
> > +  let tableID = idPrefix + "searchSuggestionTable";
> > +  this.input.setAttribute("autocomplete", "off");
> 
> My understanding is that properties should be preferred over setAttribute
> for perf reasons (and conciseness) when possible so use
> this.input.autocomplete = "off";

Changed this one and all the others that are standard attributes.

> @@ +91,5 @@
> > +        row.firstChild.setAttribute("aria-selected", "true");
> > +        this._table.setAttribute("aria-activedescendant", row.firstChild.id);
> > +        this.input.value = this.suggestionAtIndex(i);
> > +      }
> > +      else {
> 
> Cuddle else throughout

No way Jose.

> @@ +110,5 @@
> > +
> > +  // The timeout (ms) of the remote suggestions.  Corresponds to
> > +  // SearchSuggestionController.remoteTimeout.  Uses
> > +  // SearchSuggestionController's default timeout if falsey.
> > +  remoteTimeout: undefined,
> 
> Don't we normally put the properties at the beginning of the prototype?

Moved.

> @@ +206,5 @@
> > +      event.preventDefault();
> > +    }
> > +  },
> > +
> > +  _onFocus: function () {
> 
> Maybe rename to _speculativeConnect so it makes more sense seeing it called
> from the engineName setter? I forgot the function name is built in
> handleEvent.

Moved the body to a new _speculativeConnect method and called it from _onFocus and the engineName setter.

> @@ +290,5 @@
> > +
> > +  _makeTableRow: function (type, suggestionStr, currentRow, searchWords) {
> > +    let row = document.createElementNS(HTML_NS, "tr");
> > +    row.classList.add("searchSuggestionRow");
> > +    row.setAttribute("type", type);
> 
> It may help understanding to prefix our custom attributes with "data-"
> (using dataset) or ideally use a class if possible.

Switched to using classes for everything and updated the test.

> 
> @@ +341,5 @@
> > +  },
> > +
> > +  _indexOfTableRow: function (row) {
> > +    while (row && row.localName != "tr") {
> > +      row = row.parentNode;
> 
> I guess this function doesn't just take rows and can take children of rows?
> Are we using this ability? If we keep it, change the argument name and/or
> function name to document this. Otherwise, this function is mostly the same
> as HTMLTableRowElement.rowIndex

Yes, the target of mouse events is either the td or a span depending on whether the mouse is over text.  Changed the name to _indexOfTableRowOrDescendent and changed the implementation to use row.rowIndex, which I didn't know about, thanks.

> ::: browser/modules/test/contentSearchSuggestions.sjs
> @@ +1,1 @@
> > +/* Any copyright is dedicated to the Public Domain.
> 
> I would be fine with not duplicating this file and the engine by moving the
> tests to one directory (with a search subdirectory if you prefer).

Duplicating those two files isn't great, but I think that's much better than moving tests away from the code they're testing, so I left them.

(In reply to Matthew N. [:MattN] from comment #82)
> @@ +25,5 @@
> > +  cursor: default;
> > +}
> > +
> > +.searchSuggestionRow[type="formHistory"] {
> > +  color: #06C;
> 
> Consistency on hex RGB vs. hsl would be nice

Changed the three hex RGB's to hsl's, even the white.

> ::: browser/base/content/test/general/searchSuggestionUI.js
> @@ +1,1 @@
> > +/* Any copyright is dedicated to the Public Domain.
> 
> Why do we need to mock this script?

I don't know what you mean by mock.  I'm testing searchSuggestionUI in isolation in an e10s-safe way, so that's why this script is here.

We talked in person about a content script used by the tests, but at the time I just barely glanced at your comments and thought you were talking about browser_ContentSearch.js's content script, so I don't think anything I said then actually applies here.

> @@ +7,5 @@
> > +const ENGINE_NAME = "browser_searchSuggestionEngine searchSuggestionEngine.xml";
> > +
> > +let input = content.document.querySelector("input");
> > +let gController =
> > +  new content.SearchSuggestionUIController(input, input.parentNode);
> 
> Won't this fit on 100? The wrapping like this makes it harder to read IMO.

I thought 80 was the target.

	Drew Willcoxon :adw Assignee
	Comment 86 • 10 years ago

Madhava, Matt has raised some concerns about the UX created by these patches. Could you please comment on them or ask someone on your team to comment?

Try builds are here: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/dwillcoxon@mozilla.com-d0c4b5f248d3/  Suggestions only appear on about:home, not newtab.

In summary, Matt's concerned that this about:home autocomplete popup doesn't look or act like our standard autocomplete popups, like the one attached to the main search bar in chrome.

In specific, the about:home popup has the following behavior and appearance that are different from our standard popups:

* When you mouse over a suggestion, it's placed in the input.  Note that using the arrow keys to choose a suggestion does place it in the input, both in about:home's popup and the main search bar's.

* The visual difference between suggestions from your local form history and suggestions from the remote search provider are merely indicated by a difference in text color.  Local suggestions are blue, remote suggestions are black.  It's not clear what the colors mean.  In the main search bar's popup, there's a line separating the two, and "Suggestions" appears at the top right of the bottom section (supposedly to indicate remote suggestions).

* Complete words that you've typed are bolded in the suggestions.  That's flipped from mconnor's original patches -- he bolded words you didn't type, but I thought that made the suggestions look too heavy, so I flipped it.  The awesomebar bolds substrings that you've typed instead of waiting for whole words.  The main search bar doesn't bold anything.

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

Madhava's on PTO, redirecting to Philipp - but I would suggest that these are all followup-worthy issues (perhaps deserving their own UX bug), and not something that should hold up any of this landing.

	Matthew N. [:MattN]
	Comment 88 • 10 years ago

Normally the UX would have been decided before implementation and so without that I think we should stick with what we do in the search box as the default. The final UX isn't a blocker for landing but it probably should be for uplift and doing it here makes uplift easier.

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

I'm fine with going with a more conservative styling here, I don't feel strongly. But this bug has been carried over twice, and so I'd like it not to be carried over again for separable concerns. We can land+iterate, even if that makes the uplift a bit trickier.

	Matthew N. [:MattN]
	Comment 90 • 10 years ago

Comment on attachment 8465084 [details] [diff] [review]
about:home changes + searchSuggest.js + tests v5

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

r=me but the UX inconsistencies we discussed and you noted should be addressed before uplift or shipping.

::: browser/base/content/abouthome/aboutHome.xhtml
@@ +24,5 @@
>      <link rel="icon" type="image/png" id="favicon"
>            href="chrome://branding/content/icon32.png"/>
>      <link rel="stylesheet" type="text/css" media="all"
> +          href="chrome://browser/content/searchSuggestionUI.css"/>
> +    <link rel="stylesheet" type="text/css" media="all" defer="true"

I believe this should be defer="defer". HTML doesn't use true/false for boolean attributes. Their existence alone is enough but the pattern is to use the attribute name as the value in XHTML.

::: browser/base/content/test/general/searchSuggestionUI.html
@@ +1,2 @@
> +<!-- Any copyright is dedicated to the Public Domain.
> +   - http://creativecommons.org/publicdomain/zero/1.0/ -->

I kinda thought the doctype needed to be first but I could be wrong.

::: browser/base/content/test/general/searchSuggestionUI.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +(function () {

Add "use strict";

::: browser/base/jar.mn
@@ +140,5 @@
>  #endif
>          content/browser/socialmarks.xml               (content/socialmarks.xml)
>          content/browser/socialchat.xml                (content/socialchat.xml)
> +        content/browser/searchSuggestionUI.js         (content/searchSuggestionUI.js)
> +        content/browser/searchSuggestionUI.css        (content/searchSuggestionUI.css)

Move these up with the other alphabetical entries between sanitizeDialog.css and tabbrowser.css. The social lines are misplaced and should have been sorted.

	(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo
	Comment 91 • 10 years ago

I'm fine with landing this and then following up on the UX inconsistencies (assuming that they would be easy to uplift since no new strings are needed).

I'll file a UX bug

	Drew Willcoxon :adw Assignee
	Comment 92 • 10 years ago

Addresses comment 90.  Carrying over the r+.

	Drew Willcoxon :adw Assignee
	Comment 93 • 10 years ago

Thanks for all the reviews, Matt.

https://hg.mozilla.org/integration/fx-team/rev/54d218a39e14
https://hg.mozilla.org/integration/fx-team/rev/e755641f95b0
https://hg.mozilla.org/integration/fx-team/rev/fa1f7ba7fdf0

Latest try push, which is slightly different from the landed patches but should be functionally equivalent: https://tbpl.mozilla.org/?tree=Try&rev=d0c4b5f248d3

	Tim Taubert [:ttaubert] (inactive)
	Comment 94 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/54d218a39e14
https://hg.mozilla.org/mozilla-central/rev/e755641f95b0
https://hg.mozilla.org/mozilla-central/rev/fa1f7ba7fdf0

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

Verified using Nightly 34.0a1 20140804030205 under Win 7 64-bit, Mac OSX 10.9.4 and Ubuntu 12.10 32-bit: search suggestions are displayed on about:home page for all default search engines except Twitter. Logged separate issues for the bugs found.

	Drew Willcoxon :adw Assignee
	Comment 96 • 10 years ago

Bug 1054516 is the uplift bug for this feature.

mozilla-aurora try push with this and patches for other bugs mentioned in bug 1054516: https://tbpl.mozilla.org/?tree=Try&rev=fba51f37ff40

Approval Request Comment
[Feature/regressing bug #]:
this bug, search suggestions in about:home

[User impact if declined]:
no search suggestions in about:home

[Describe test coverage new/current, TBPL]:
currently covered by automated tests on 34; this patch adds tests to 33; see also try link above

[Risks and why]:
moderate risk mediated by aforementioned testing; modifies the about:home page; we want about:home search suggestions on 33 as stated in bug 1054516

[String/UUID change made/needed]:
none

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

Release Note Request (optional, but appreciated)
[Why is this notable]: New feature
[Suggested wording]: Search suggestions in the homepage
[Links (documentation, blog post, etc)]: Is there anything?

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

Comment on attachment 8478728 [details] [diff] [review]
folded Aurora/33 patch with appropriate changes from 34 patches

unable to find 'toolkit/components/search/SearchSuggestionController.jsm' for patching

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea4892a75c90

Sorry, not enough caffeination yet today.

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

Comment on attachment 8478728 [details] [diff] [review]
folded Aurora/33 patch with appropriate changes from 34 patches

Just getting it re-added for posterity.

	[SV Manager] Florin Mezei, QA (:FlorinMezei)
	Comment 101 • 10 years ago

Marking as Verified on 34 based on comment 95.

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

Added in the 33 beta release notes with "Search suggestions on the Firefox Start Page (about:home)" as wording.

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

Verified that the search suggestions are displayed on about:home page using latest Aurora 33.0a2 (20140828004002) under Win 7 64-bit, Ubuntu 13.04 32-bit and Mac OSX 10.8.5.

	Dennis Schieferdecker
	Comment 104 • 10 years ago

Is there a possibilty to disable this change? Some people -like myself- use the Firefox about:home start page precisely because there are no Google Suggests. Otherwise, one could directly use the Google website as start page. I prefer to see a list of recently entered search queries just as before - as they reflect my search behaviour and not what Google thinks I am searching for (which is wrong about 100% of the time).

	Mike Connor [:mconnor]
	Comment 105 • 10 years ago

This feature respects the global preference for search suggestions, which is accessible if you right click on the search bar.

I'll note that we don't quite get this right, since we still cap at 2 local entries if this is disabled.  I'll file a followup.

	Matthew N. [:MattN]
	Comment 106 • 10 years ago

(In reply to Mike Connor [:mconnor] from comment #105)
> I'll note that we don't quite get this right, since we still cap at 2 local
> entries if this is disabled.  I'll file a followup.

That's bug 1060846.

	Dennis Schieferdecker
	Comment 107 • 10 years ago

(In reply to Mike Connor [:mconnor] from comment #105)
> This feature respects the global preference for search suggestions, which is
> accessible if you right click on the search bar.
> 
> I'll note that we don't quite get this right, since we still cap at 2 local
> entries if this is disabled.  I'll file a followup.

Thank you for the update that the feature is not yet complete. Personally, though, I would prefer a local preference for more fine-grained adjustments where suggestions are shown.

But there is also a another problem with the new search box:
When the input box has focus and is empty, you can press the down key to get a dropdown menu with the 2 local entries you mentioned previously. However, with the old input box, you could also click into the box with the mouse, and click again and the dropdown menu would open. This mouse interaction is missing from the new implementation. And in my opinion, this is an important feature as it allows access to previous search phrases without requiring keyboard input.

	Matthew N. [:MattN]
	Comment 108 • 10 years ago

(In reply to Dennis Schieferdecker from comment #107)
> However, with the old input box, you could also click into the box with the mouse,
> and click again and the dropdown menu would open. This mouse interaction is
> missing from the new implementation. And in my opinion, this is an important
> feature as it allows access to previous search phrases without requiring
> keyboard input.

Bug 1066787 would fix this.