Closed Bug 1015269 Opened 10 years ago Closed 10 years ago

about:newtab search behaviour should match searchbar


(Firefox :: New Tab Page, defect)

Not set





(Reporter: mconnor, Assigned: mconnor)




(1 file)

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 (I thought I was going crazy for a while)

This should be separate from the other patches working their way through reviews.
Attachment #8427829 - Flags: review?(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:

They don't talk directly, though.  This ContentSearchMediator thing passes their messages back and forth using events in the page:

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=""/>

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._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)?
Attachment #8427829 - Flags: review?(adw)
Component: Search → New Tab Page
OS: Windows 8.1 → All
Hardware: x86_64 → All
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.
Blocks: 962490
No longer blocks: 962490
Depends on: 962490
Closed: 10 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.