1015269 - about:newtab search behaviour should match searchbar

Status: RESOLVED DUPLICATE of bug 1028985

(Firefox :: New Tab Page, defect)

Description

[Mike Connor [:mconnor]]

Attachment: v1: suggestions + saving to correct form history

It's chrome-privileged XUL/XHTML, this should be straightforward.

* search suggestions should be supported, including local content
* searching through this UI should add to searchbar-history so we have a shared set of local completions

As a note on the patch, ontextentered has to be an initial attribute, per https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Textbox_%28Toolkit_autocomplete%29#Attributes (I thought I was going crazy for a while)

This should be separate from the other patches working their way through reviews.

Comment 1

[Drew Willcoxon :adw]

Comment on attachment 8427829 [details] [diff] [review]
v1: suggestions + saving to correct form history

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

Nice!  But there's a big architecture concern below.

::: browser/base/content/newtab/newTab.css
@@ +302,5 @@
>  #newtab-search-text:-moz-dir(rtl) {
>    border-radius: 0 2.5px 2.5px 0;
>  }
>  
> +#newtab-search-text[focused],

Why these focus CSS changes?

::: browser/base/content/newtab/newTab.js
@@ +21,5 @@
>    "resource://gre/modules/PrivateBrowsingUtils.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "UpdateChannel",
>    "resource://gre/modules/UpdateChannel.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "FormHistory",
> +  "resource://gre/modules/FormHistory.jsm");

Eventually we want about:newtab to be OOP and unprivileged, so we don't really want to add any new dependencies that block that.  The search bar goes out of its way to be OOP-safe and unprivileged for example, and suggestions should work the same way.

newtab communicates with this module in chrome to get a list of engines, get the current engine, and search: http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm

They don't talk directly, though.  This ContentSearchMediator thing passes their messages back and forth using events in the page: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.js?rev=d2214e4edf1b#193

So suggestions should be added to ContentSearch.jsm.  You shouldn't need to change ContentSearchMediator.

::: browser/base/content/newtab/newTab.xul
@@ +33,5 @@
>        <xul:label>&cmd_engineManager.label;</xul:label>
>      </xul:hbox>
>    </xul:panel>
>  
> +  <xul:panel type="autocomplete" id="PopupAutoComplete" noautofocus="true" hidden="true"/>

This should probably be ID'ed something like newtab-search-autocomplete to match the ID style here.

@@ +69,5 @@
> +                       maxrows="10"
> +                       completeselectedindex="true"
> +                       showcommentcolumn="true"
> +                       tabscrolling="true"
> +                       ontextentered="gSearch.search(param)"/>

Bug 993329 says we shouldn't use inline event handlers like this.

::: browser/base/content/newtab/search.js
@@ +78,5 @@
>    onCurrentEngine: function (engineName) {
>      this._setCurrentEngine(engineName);
>    },
>  
> +  onKeypress: function (evt) {

on* methods are kind of special in this file because they're called by handleEvent to dispatch ContentSearchService events, so to avoid confusion this should be named something else, like handleKeypress.

@@ +96,5 @@
>    _nodes: {},
>  
>    _initWhenInitalStateReceived: function () {
>      this._nodes.form.addEventListener("submit", e => this.search(e));
> +    this._nodes.text.addEventListener("keypress", e => this.onKeypress(e));

Is this to handle when you press return while a suggestion is focused?  Does this conflict with the submit listener when you press return while the input is focused (e.g., are both search and onKeypress called in that case)?

Comment 2

[Mike Connor [:mconnor]]

re: ontextentered attr: The bug can say that, but ontextentered has to be an attribute according to MDN (and, indeed, doesn't work otherwise).

re: keypress: enter in a XUL textbox doesn't send a submit event.

I can go with a mediated approach (it's basically what I'm doing in about:home for bug 612453), it'd be good to comment super explicitly somewhere in this code the conditions we're working under.  If I was a first-time contributor I'd feel burned (I'm fine, this was Friday morning monkeying around).

I'll refactor to use an approach similar to about:home.