Closed Bug 1171344 Opened 9 years ago Closed 9 years ago

[implement] One-off searches on about:home and about:newtab

Categories

(Firefox :: Search, defect, P2)

Product:
Firefox
Component:
Search
Platform:
All
Unspecified
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: nhnt11, Assigned: nhnt11)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [ui][fxsearch])

Attachments

(3 files, 11 obsolete files)

Patch v7
106.82 KB, patch
nhnt11
: review+
Details | Diff | Splinter Review
Tests v3
55.81 KB, patch
adw
: review+
Details | Diff | Splinter Review
Delay setting up one off buttons till suggestions table is unhidden
2.19 KB, patch
adw
: review+
Details | Diff | Splinter Review 
Implement one off search UI for about:home and about:newtab.
UX: bug 1106057
Attached patch WIP (obsolete) — — Splinter Review 
What's done:
- UI implementation
- Keyboard/mouse interactions

What's left:
- l10n of strings used in searchSuggestionUI.js
- Search engine icons should be passed in a more efficient way (ArrayBuffers?)
- Search suggestions on about:newtab are broken (but easily fixable, and we get the new suggestions UI for free)
- Make about:home also use ContentSearch.jsm
- Fix up about:newtab
- Waiting on icons from shorlander

What I want feedback on:
- Keyboard/mouse interactions! I think I covered everything but I may have missed something or the other. There are a couple of slightly hacky workarounds (to keep the searchbox in focus after a mousedown on the suggestions table for example) - they need comments, which will come in the next patch.
- UI/CSS (adw recommended you for this, Florian :))

I know there's some code duplication and unnecessary CSS and stuff, so I suggest you avoid doing a code review for now. I'll request r? once I think it's ready for it. I wanted to upload a patch and start getting some feedback on the interactions without too much further ado.
Attachment #8615122 - Flags: feedback?(florian)
Attachment #8615122 - Flags: feedback?(adw)
Comment on attachment 8615122 [details] [diff] [review]
WIP

(In reply to Nihanth Subramanya [:nhnt11] from comment #1)

Great progress!

> there's some code duplication and unnecessary CSS and stuff, so I
> suggest you avoid doing a code review for now. I'll request r? once I think
> it's ready for it. I wanted to upload a patch and start getting some
> feedback on the interactions without too much further ado.

I assume you want me to try the patch and see if I notice details that need fixing. Here are some issues I've found. I'm not claiming that all of them need fixing before the patch gets r+.

About the behavior of the panel in general:
- the provider icons are pixelated on my retina screen.
- you are missing the vertical gray bar after the last provider icon.
- Hovering a one-off button shows a tooltip saying "Search with [object Object]"
- the "Search for WORD with:" line needs fixing. (it's obvious, but you didn't mention in your todo list, so I'm listing it here just in case you forgot about it)
- clicking the first gray bar at the top should start a search with the default engine.
- I haven't been able to figure out the steps to reproduce, but in some cases I ended up with the "Search" placeholder of the text field replaced by a string containing several search provider names. I'm sure I saw "Search Yahoo Yahoo" once. I saw a longer string that seemed to include all the provider names once. Maybe check if you have any code touching that placeholder?
- your .getEngines() call should be replaced with getVisibleEngines() otherwise hiding some one-offs from the pref UI doesn't have any visible effect.
- due to the previous point, I haven't been able to verify how your patch behaves if there are more one-off engines than can fit on a single line, nor if all the engines have been unchecked in the preference panel.

About keyboard interaction (I'm assuming this is expected to match the searchbox UI) :
- I think the Escape key should close the panel, but that can be a follow-up as the previous suggestion panel didn't handle that key.
- I'm always surprised that the <tab> key closes the panel and focuses the -> button instead of focusing the first one-off item.
- alt+up/down should cycle through the one-off buttons. It currently behaves like up/down without 'alt'.
- command+up/down should change the default engine.
- when a one-off button is selected from the keyboard (blue background on it), if the textfield is empty, pressing <enter> should load the search form of that search engine. Same for clicking these buttons in that case.

About CSS/theming:
- the color of the borders you use within the panel not matching the color of the panel's border doesn't feel good.
- I kinda think the panel would look better if it had rounder corners matching the rounded corners of the search box it's attached to.
- the hover background color of the "Change Search Settings" button doesn't feel good. I wonder if we would get something better by imitating the behaviors of the Downloads/Bookmarks/... buttons at the bottom of about:home.
- the focus ring of the "->" button (you see it when pressing <tab> while the input field is focused) would look much better if it matched the shape of the button. This kind of detail can usually get fixed by removing the padding or margin on one of the elements.
- the height of the 2 gray bars in the panel being smaller than the height of one suggestion item doesn't look great.
Attachment #8615122 - Flags: feedback?(florian) → feedback+
Comment on attachment 8615122 [details] [diff] [review]
WIP

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

I don't have anything to add to what Florian said regarding the feedback you requested except that all SearchSuggestionUI DOM elements should probably have their IDs and classes prefixed with something like "searchSuggestion", as is the case now, to avoid name conflicts since search suggestions are designed to be dropped into any page.
Attachment #8615122 - Flags: feedback?(adw) → feedback+
Attached patch WIP 2 (obsolete) — — Splinter Review 
Stuff that's fixed in this new version:

> - the provider icons are pixelated on my retina screen.
Search engine data is now provided by ContentSearch.jsm, and icons are passed as ArrayBuffer references.
-moz-resolution is correctly applied for retina displays.
> - you are missing the vertical gray bar after the last provider icon.
> - Hovering a one-off button shows a tooltip saying "Search with [object
> Object]"
> - the "Search for WORD with:" line needs fixing. (it's obvious, but you
> didn't mention in your todo list, so I'm listing it here just in case you
> forgot about it)
l10n stuff is fixed.
> - I haven't been able to figure out the steps to reproduce, but in some
> cases I ended up with the "Search" placeholder of the text field replaced by
> a string containing several search provider names. I'm sure I saw "Search
> Yahoo Yahoo" once. I saw a longer string that seemed to include all the
> provider names once. Maybe check if you have any code touching that
> placeholder?
> - your .getEngines() call should be replaced with getVisibleEngines()
> otherwise hiding some one-offs from the pref UI doesn't have any visible
> effect.
This is weird: the code is now using ContentSearch.jsm's state object's engines array, which is populated using getVisibleEngines(). However, getVisibleEngines() is returning hidden engines as well! I haven't figured out what's going on yet.
> - due to the previous point, I haven't been able to verify how your patch
> behaves if there are more one-off engines than can fit on a single line, nor
> if all the engines have been unchecked in the preference panel.
Multi-line one-off buttons is working now.

> About keyboard interaction (I'm assuming this is expected to match the
> searchbox UI) :
> - I think the Escape key should close the panel, but that can be a follow-up
> as the previous suggestion panel didn't handle that key.
> - when a one-off button is selected from the keyboard (blue background on
> it), if the textfield is empty, pressing <enter> should load the search form
> of that search engine. Same for clicking these buttons in that case.
> 
> About CSS/theming:
> - the hover background color of the "Change Search Settings" button doesn't
> feel good. I wonder if we would get something better by imitating the
> behaviors of the Downloads/Bookmarks/... buttons at the bottom of about:home.
I've changed this to be closer to the buttons at the bottom like you suggested, see if you like it better now?
> - the focus ring of the "->" button (you see it when pressing <tab> while
> the input field is focused) would look much better if it matched the shape
> of the button. This kind of detail can usually get fixed by removing the
> padding or margin on one of the elements.
> - the height of the 2 gray bars in the panel being smaller than the height
> of one suggestion item doesn't look great.
I reduced the font size and padding on the suggestions and increased the padding to match on the headers. They are equally sized now, and the dropdown occupies roughly the same amount of space.

I've started work to deduplicate stuff in both about:home and about:newtab and do everything in searchSuggestionUI.js, tab-content.js, and ContentSearch.jsm. It's getting there - search now works through searchSuggestionUI.js in about:newtab (and could theoretically work in any page, as long as support is added in tab-content.js!). However, it's still using AboutHome:* events to do searches, which are picked up and executed by AboutHome.jsm. This needs to be moved to ContentSearch.jsm, which is what I want to do next.

searchSuggestionUI.js now requests and receives search engine data directly from ContentSearch.jsm.

After this work is complete, I want to focus on fixing up the tests for searchSuggestionUI.

adw and I think we can leave the remaining keyboard navigation stuff for a follow-up bug, and focus on fixing tests as priority to land this.

Again, I don't think you should spend much time looking at all the code in this patch. I'm requesting feedback mostly to confirm that my approach for communicating with ContentSearch.jsm is fine (if it's hard to tell from glancing through the patch, I can discuss it in person @adw).

@flo, I'd also like quick feedback on how it looks now.

I'd also appreciate it if you could try out the search UI from about:newtab and confirm that it's working correctly (though it's still going through AboutHome.jsm).
Attachment #8615122 - Attachment is obsolete: true
Attachment #8616319 - Flags: feedback?
Attachment #8616319 - Flags: feedback?(florian)
Attachment #8616319 - Flags: feedback?(adw)
Attachment #8616319 - Flags: feedback?
Comment on attachment 8616319 [details] [diff] [review]
WIP 2

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

You forgot to hg add the svg files ;) Also, can you combine the 2 magnifying glass files in one ? (See content-contextmenu.svg in our code)
(In reply to Tim Nguyen [:ntim] from comment #5)
> Comment on attachment 8616319 [details] [diff] [review]
> WIP 2
> 
> Review of attachment 8616319 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
>  Also, can you combine the 2 magnifying
> glass files in one ? (See content-contextmenu.svg in our code)

Meant go icons instead of magnifying glass
Comment on attachment 8616319 [details] [diff] [review]
WIP 2

(In reply to Nihanth Subramanya [:nhnt11] from comment #4)
> Created attachment 8616319 [details] [diff] [review]

> @flo, I'd also like quick feedback on how it looks now.
> 
> I'd also appreciate it if you could try out the search UI from about:newtab
> and confirm that it's working correctly (though it's still going through
> AboutHome.jsm).

I'm not sure what you specifically need feedback about. I saw lots of the things I pointed out in my previous round of feedback have been fixed, and a few remain.

About the appearance, this is the colors I see: http://i.imgur.com/uzTp7mW.png It seems there are still several difference nuances of gray there. Note that the hover background-color of the "change search settings" item doesn't disappear when the mouse is moved out of the panel.

On about:newtab, things seem to work. The panel is aligned with the textbox rather than with the glass icon, which means the text of the suggestions isn't aligned with the text the user typed.

Ping me on IRC if you need more detailed feedback.
Attachment #8616319 - Flags: feedback?(florian)
Comment on attachment 8616319 [details] [diff] [review]
WIP 2

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

My comments focus on the searchSuggestionUI and ContentSearch changes.

This really expands the UI beyond suggestions.  It's now a full-fledged in-content search UI to match chrome's, so we should think about taking "suggestions" out of the names of the files and identifiers.  Like, just contentSearchUI.  That's fine for a follow-up though (but fine for this bug too IMO if you'd like!).

I expect a future patch, when you move everything to ContentSearch, to replace the new CustomEvents with the existing ContentSearchClient event, like what newtab's search.js does, right?  http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/search.js#136

It looks like you removed the parts that fixed bug 1115616 related to the input and IME.  We'll need to make sure we don't ultimately regress that.

::: browser/base/content/abouthome/aboutHome.js
@@ +163,1 @@
>        if (!gInitialized) {

Setting up search when snippetsVersion is changed seems weird.  Should it be in the !gInitialized block?

::: browser/base/content/searchSuggestionUI.js
@@ +74,5 @@
> +  get engineIcon() {
> +    return this._engineIcon;
> +  },
> +
> +  get searchEventName() {

Does this need to be "public" (non-underscored)?  I'm asking from the POV of someone looking at this prototype to understand how to use it.  Although that should be a moot point once you switch over to ContentSearch/ContentSearchMediator, right?

@@ +78,5 @@
> +  get searchEventName() {
> +    return "ContentSearchEvent";
> +  },
> +
> +  set defaultEngine(val) {

This makes setting and getting the current/default engine asymmetrical: the setter takes an object with name and icon properties, but there are two getters, one for name and one for icon.  I only mention it because all three are public.  If it were _setDefaultEngine instead, an internal implementation detail, that would be OK with me.  So I think you should either make the setter "private," or you should replace the two getters with a single getter symmetric to the setter.

I guess there's no reason to keep the setter private, and indeed the engineName setter wasn't private, so I vote for the latter.

@@ +192,5 @@
>    handleEvent: function (event) {
>      this["_on" + event.type[0].toUpperCase() + event.type.substr(1)](event);
>    },
>  
> +  onSearch: function(aEvent) {

I'd prefer this to be called onCommand or something since it's called on Return or click, and it can do something other than searching depending on what's selected.  An onCommand that looks at the event and what's selected and then calls a search() or showSettings() makes sense to me.

@@ +428,5 @@
> +      header.firstChild.setAttribute("src", this._engineIcon);
> +    if (!this._strings)
> +      return;
> +    while (header.firstChild.nextSibling)
> +      header.firstChild.nextSibling.remove();

Nit: Please use braces around at least loops.  (I think braces around everything is our usual style, but seems like most of us aren't strict about it.)  More of a review comment than a feedback comment.

@@ +430,5 @@
> +      return;
> +    while (header.firstChild.nextSibling)
> +      header.firstChild.nextSibling.remove();
> +    header.appendChild(document.createTextNode(
> +      this._strings.searchHeader.replace("%S", this._engineName)));

This works but it's not great.  See next comment too.

@@ +445,5 @@
> +      let span = document.createElementNS(HTML_NS, "span");
> +      span.setAttribute("class", "searchWithHeaderSearchText");
> +      span.appendChild(document.createTextNode(" " + this.input.value + " "));
> +      searchWithHeader.appendChild(span);
> +      searchWithHeader.appendChild(document.createTextNode(this._strings.searchWith));

This is bad for l10n because it assumes how the larger string is constructed from its substrings, but that can be different in different languages.  Normally we'd just use nsIStringBundle.formatStringFromName, but that's not available here.  I think you're going to have to send a message to ContentSearch asking for it to call formatStringFromName for you and send back the result.  I'm not sure what the state of the art in front-end e10s is for this.

tab-content.js can access strings, so we could avoid a round trip between chrome and content by having ContentSearchMediator owning all the strings instead of ContentSearch.jsm.  How's that sound?

@@ +469,5 @@
>  
>      let entry = document.createElementNS(HTML_NS, "td");
> +    let img = document.createElementNS(HTML_NS, "div");
> +    img.setAttribute("class", "historyIcon");
> +    entry.appendChild(img);

Hmm, instead of a whole div just for the result icon, can't applying the image to the result be done entirely in CSS?  Like with a background-image?

@@ +532,5 @@
>    },
>  
> +  _indexOfTableItem: function (elt) {
> +    if (elt.classList.contains("searchbar-engine-one-off-item")) {
> +      return this.numSuggestions + this._oneOffButtons.indexOf(elt);

Would be great to localize all this arithmetic to a small number of orthogonal methods whose job is nothing but that.  I don't have a great idea off the top of my head, but maybe you can think of something.

@@ +556,5 @@
> +
> +    this._table.addEventListener("mousedown", () => { this._mousedown = true; });
> +    document.addEventListener("mouseup", () => { delete this._mousedown; });
> +
> +    window.addEventListener("beforeunload", () => { this._hideSuggestions(); });

Is this really necessary?  beforeunload handlers prevent pages from being stored in the back-forward cache ("bfcache").  Maybe not a big deal for about:home/newtab, but would be good to avoid if possible.  What's the problem you're solving?
Attachment #8616319 - Flags: feedback?(adw) → feedback+
Attached patch Patch v1 (obsolete) — — Splinter Review 
OK, I think this is mostly ready for review! Note that I haven't modified the newtab page's searchbar UI. I think that can be done as a follow up once bug 1167601 lands.
Please note that there may be stuff left to do in newtab's search.js. I will request review on that once I have looked at it.

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> About the appearance, this is the colors I see:
> http://i.imgur.com/uzTp7mW.png It seems there are still several difference
> nuances of gray there. Note that the hover background-color of the "change
> search settings" item doesn't disappear when the mouse is moved out of the
> panel.

I've modified this slightly, but I think I'll just ask the UX people to specify the exact colours.

(In reply to Drew Willcoxon :adw from comment #8)
> I expect a future patch, when you move everything to ContentSearch, to
> replace the new CustomEvents with the existing ContentSearchClient event,
> like what newtab's search.js does, right? 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/
> search.js#136
Done.

> It looks like you removed the parts that fixed bug 1115616 related to the
> input and IME.  We'll need to make sure we don't ultimately regress that.
The search is no longer happening using a form's onsubmit event, so we no longer forcibly update the inputbox's value before the submit event fires. I think we're fine, let me know if I'm missing something.

> ::: browser/base/content/searchSuggestionUI.js
> @@ +74,5 @@
> > +  get engineIcon() {
> > +    return this._engineIcon;
> > +  },
> > +
> > +  get searchEventName() {
> 
> Does this need to be "public" (non-underscored)?  I'm asking from the POV of
> someone looking at this prototype to understand how to use it.  Although
> that should be a moot point once you switch over to
> ContentSearch/ContentSearchMediator, right?

I've removed it, it's no longer used.

> @@ +78,5 @@
> > +  get searchEventName() {
> > +    return "ContentSearchEvent";
> > +  },
> > +
> > +  set defaultEngine(val) {
> 
> This makes setting and getting the current/default engine asymmetrical: the
> setter takes an object with name and icon properties, but there are two
> getters, one for name and one for icon.  I only mention it because all three
> are public.  If it were _setDefaultEngine instead, an internal
> implementation detail, that would be OK with me.  So I think you should
> either make the setter "private," or you should replace the two getters with
> a single getter symmetric to the setter.
> 
> I guess there's no reason to keep the setter private, and indeed the
> engineName setter wasn't private, so I vote for the latter.

Did the latter.

> @@ +192,5 @@
> >    handleEvent: function (event) {
> >      this["_on" + event.type[0].toUpperCase() + event.type.substr(1)](event);
> >    },
> >  
> > +  onSearch: function(aEvent) {
> 
> I'd prefer this to be called onCommand or something since it's called on
> Return or click, and it can do something other than searching depending on
> what's selected.  An onCommand that looks at the event and what's selected
> and then calls a search() or showSettings() makes sense to me.

Done.

> @@ +428,5 @@
> > +      header.firstChild.setAttribute("src", this._engineIcon);
> > +    if (!this._strings)
> > +      return;
> > +    while (header.firstChild.nextSibling)
> > +      header.firstChild.nextSibling.remove();
> 
> Nit: Please use braces around at least loops.  (I think braces around
> everything is our usual style, but seems like most of us aren't strict about
> it.)  More of a review comment than a feedback comment.

Done. Updated if's too.

> @@ +430,5 @@
> > +      return;
> > +    while (header.firstChild.nextSibling)
> > +      header.firstChild.nextSibling.remove();
> > +    header.appendChild(document.createTextNode(
> > +      this._strings.searchHeader.replace("%S", this._engineName)));
> 
> This works but it's not great.  See next comment too.
> 
> @@ +445,5 @@
> > +      let span = document.createElementNS(HTML_NS, "span");
> > +      span.setAttribute("class", "searchWithHeaderSearchText");
> > +      span.appendChild(document.createTextNode(" " + this.input.value + " "));
> > +      searchWithHeader.appendChild(span);
> > +      searchWithHeader.appendChild(document.createTextNode(this._strings.searchWith));
> 
> This is bad for l10n because it assumes how the larger string is constructed
> from its substrings, but that can be different in different languages. 
> Normally we'd just use nsIStringBundle.formatStringFromName, but that's not
> available here.  I think you're going to have to send a message to
> ContentSearch asking for it to call formatStringFromName for you and send
> back the result.  I'm not sure what the state of the art in front-end e10s
> is for this.
> 
> tab-content.js can access strings, so we could avoid a round trip between
> chrome and content by having ContentSearchMediator owning all the strings
> instead of ContentSearch.jsm.  How's that sound?

We discussed this in person: the problem with formatStringFromName is that we have separate styling on the text the user has typed. In any case, this has precedent in the toolbar searchbar's panel UI.

> @@ +469,5 @@
> >  
> >      let entry = document.createElementNS(HTML_NS, "td");
> > +    let img = document.createElementNS(HTML_NS, "div");
> > +    img.setAttribute("class", "historyIcon");
> > +    entry.appendChild(img);
> 
> Hmm, instead of a whole div just for the result icon, can't applying the
> image to the result be done entirely in CSS?  Like with a background-image?

Forgot about this, but thought it was worth putting a patch up anyway.

> @@ +532,5 @@
> >    },
> >  
> > +  _indexOfTableItem: function (elt) {
> > +    if (elt.classList.contains("searchbar-engine-one-off-item")) {
> > +      return this.numSuggestions + this._oneOffButtons.indexOf(elt);
> 
> Would be great to localize all this arithmetic to a small number of
> orthogonal methods whose job is nothing but that.  I don't have a great idea
> off the top of my head, but maybe you can think of something.

I've improved some of this kind of arithmetic, let me know what you think.

> @@ +556,5 @@
> > +
> > +    this._table.addEventListener("mousedown", () => { this._mousedown = true; });
> > +    document.addEventListener("mouseup", () => { delete this._mousedown; });
> > +
> > +    window.addEventListener("beforeunload", () => { this._hideSuggestions(); });
> 
> Is this really necessary?  beforeunload handlers prevent pages from being
> stored in the back-forward cache ("bfcache").  Maybe not a big deal for
> about:home/newtab, but would be good to avoid if possible.  What's the
> problem you're solving?

As discussed in person, I think it's a good UX to immediately hide the dropdown UI when a search loads in the same tab (which is the purpose of this listener). It provides immediate feedback that the search has been performed while the actual search page loads (which can take a few seconds on some providers, depending on the connection).
Attachment #8616319 - Attachment is obsolete: true
Attachment #8617065 - Flags: review?(adw)
Attached patch Patch v2 (obsolete) — — Splinter Review 
Some improvements and cleanup in ContentSearch.jsm. Also cleaned up newtab's search.js and renamed searchSuggestionUI* to contentSearchUI*.

Also I forgot to mention, I have not yet looked at the related tests. That will be the next thing I work on.
Attachment #8617065 - Attachment is obsolete: true
Attachment #8617065 - Flags: review?(adw)
Attachment #8617122 - Flags: review?(adw)
Comment on attachment 8617122 [details] [diff] [review]
Patch v2

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

::: browser/base/content/contentSearchUI.css
@@ +5,5 @@
>  .searchSuggestionTable {
>    background-color: hsla(0,0%,100%,.99);
>    border: 1px solid;
> +  border-top: none;
> +  border-color: rgba(185, 185, 185, 0.90);

nit : combine this border-color property with the shorthand used earlier.

::: browser/themes/shared/search/search-indicator-magnifying-glass.svg
@@ +1,2 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">

nit : Please remove this DOCTYPE.

@@ +1,3 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="24" height="24" viewBox="0 0 24 24">

nit : You can remove the version and the xmlns:xlink attributes (xmlns:xlink is unneeded as there are no xlink:href in this file)

@@ +1,5 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="24" height="24" viewBox="0 0 24 24">
> +
> +  <path fill="#808080" d="M21.7,20.3l-1.4,1.4l-5.4-5.4c-1.3,1-3,1.7-4.9,1.7 c-4.4,0-8-3.6-8-8c0-4.4,3.6-8,8-8c4.4,0,8,3.6,8,8c0,1.8-0.6,3.5-1.7,4.9L21.7,20.3z M10,4c-3.3,0-6,2.7-6,6s2.7,6,6,6s6-2.7,6-6 S13.3,4,10,4z"/>

nit : unnecessary blank lines before and after the path
Comment on attachment 8617122 [details] [diff] [review]
Patch v2

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

Here are some comments on the JS and markup parts.  I'll look at the CSS next and remove the r? for now since there are some substantive questions below.

When you mouse down on a suggestion, the arrow button blinks: it turns gray (blurred) and then quickly turns blue again.  It's pretty annoying.  You can clearly see it by holding down the mouse button before releasing it.  When I'm not trying to reproduce it that way, it only happens sometimes.  I couldn't reproduce with the old UI even for slow-loading search pages, so I presume it's something you can fix.  If it's too hard to fix, and maybe it is because of the careful interaction around mouse clicks you've implemented, we can do that in a follow-up.

(In reply to Nihanth Subramanya [:nhnt11] from comment #9)
> > It looks like you removed the parts that fixed bug 1115616 related to the
> > input and IME.  We'll need to make sure we don't ultimately regress that.
> The search is no longer happening using a form's onsubmit event, so we no
> longer forcibly update the inputbox's value before the submit event fires. I
> think we're fine, let me know if I'm missing something.

Hmm, something's wrong but I can't reproduce it 100% of the time.  I tried with an IME, and sometimes clicking a suggestion while the IME input was not "finalized" (not sure what the right term is) triggered the search immediately, but other times I saw bug 1115616, where clicking a suggestion finalized the IME input but kept the search dropdown open, and then a second click would work normally and search for the input that was finalized.

::: browser/base/content/contentSearchUI.js
@@ +31,5 @@
>   *        positions should be "static"), or the parent's position should be the
>   *        top left of the page.
> + * @param searchParams
> + *        An object containing parameters that will be passed in the generated
> + *        search events' data. It should at least specify the healthReportKey.

Let's document the two parameters that we expect clients to be using right now.  Doesn't preclude us from adding more in the future of course.

Now that I think about it, I think we should enforce that both searchParams are non-null.  As in, `throw new Error()` if either is null.  There shouldn't be any case in the browser where either the FHR key or the search purpose isn't specified, since both are important pieces of information.

I think that might have implications for what the constructor's params should be.  Personally, when I see a function that has some non-default params followed by a params object that defaults to {}, it makes me think that the params in the params object are all optional.  Maybe that's just me.  I'll let you change or not change the params how you like.

@@ +128,4 @@
>        if (i == idx) {
> +        elt.classList.add("selected");
> +        ariaSelectedElt.setAttribute("aria-selected", "true");
> +        ariaParentElt.setAttribute("aria-activedescendant", ariaSelectedElt.id);

Hmm, I don't know whether aria-activedescendant should go on _table or _suggestionsList/_oneOffsTable.  I would guess always _table since it's the top-level container.  I'll flag Marco for feedback.  Anyhow, nice work keeping it updated.

@@ +182,5 @@
>        return;
>      }
> +    let searchText = this.input;
> +    let searchTerms = this.suggestionAtIndex(this.selectedIndex) || searchText.value;
> +    let engineName = (this.selectedIndex > -1 && this._table.querySelector(".selected").engineName) ||

Would be nice to have a selectedEngine (or selectedEngineName) getter that returns this value.

@@ +184,5 @@
> +    let searchText = this.input;
> +    let searchTerms = this.suggestionAtIndex(this.selectedIndex) || searchText.value;
> +    let engineName = (this.selectedIndex > -1 && this._table.querySelector(".selected").engineName) ||
> +                     this.defaultEngine.name;
> +    if (engineName) {

If there's no engine name, something is weirdly wrong, and it should be OK to bail early from the method right here.  No need to addInputValueToFormHistory() below in that case I think.

@@ +192,5 @@
> +        engineName: engineName,
> +        searchString: searchTerms,
> +        originalEvent: {
> +          target: {
> +            ownerDocument: document,

Sorry, I blew it when I OK'ed this ownerDocument change in the other whereToOpenLink bug.  The document lives in the content process, so when you send it over to chrome, it's wrapped in a cross-process object wrapper ("CPOW"), which is like a last-resort way of dealing with e10s that we should avoid.

This is kind of a separate bug, but since you're touching this code, would you mind fixing it?  Fortunately it doesn't look like we need target or ownerDocument since whereToOpenLink doesn't need them.  Am I reading that right?  Maybe they were left over from an earlier version of your patch in the other bug?

@@ +204,5 @@
> +      };
> +      for (let param in this._searchParams) {
> +        eventData[param] = this._searchParams[param];
> +      }
> +  

Nit: Please remove this trailing whitespace here and in the three other lines in this method.

@@ +208,5 @@
> +  
> +      if (searchText.hasAttribute("selection-index")) {
> +        eventData.selection = {
> +          index: searchText.getAttribute("selection-index"),
> +          kind: searchText.getAttribute("selection-kind")

Hmm, we could do away with these attributes on the input now that this prototype completely handles searching.  It looks like they were added only because about:home/newtab had their own search boxes but search suggestions were unified (http://hg.mozilla.org/mozilla-central/rev/57b1a2b98388).

We don't need selection-index at all anymore since it's the same thing as this.selectedIndex.  selection-kind is either "key" or "mouse", and it looks like your patch removes where "mouse" is set, so at the least that should be fixed.  But I think you should pass the kind to onCommand.  _onKeypress would pass "key", and _onMouseUp would pass "key".  What do you think about that?

@@ +375,5 @@
>            break;
>          }
>        }
> +      this._suggestionsList.appendChild(this._makeTableRow(type, suggestions[type][idx],
> +                                        i, searchWords));

Nit: Please align the second line with _makeTableRow's opening paren.

@@ +382,5 @@
> +    if (this._table.hidden) {
> +      this.selectedIndex = -1;
> +      this._table.hidden = false;
> +      this.input.setAttribute("aria-expanded", "true");
> +      this._getSearchEngines();

Why get engines here?  Isn't getting them on construction and then being notified when they change via the CurrentState message enough?

There's a CurrentEngine message that you're not currently listening for (and neither is the current searchSuggestionUI.js).  It's sent when the current engine changes.  I think you should probably listen for that too and then you don't need this?  What happens now with your patch when you change the current engine is that when the dropdown opens, you briefly see the old engine in the header before it's replaced with the new one.  I'm guessing you'll see the old one for as long as it takes for suggestions to arrive, based on this code here.

I think getting the engines here is also causing some flickering of the one-off button icons I'm seeing (on my slow debug build) since getting the engines causes the buttons to be rebuilt.

@@ +556,5 @@
> +      }
> +    });
> +
> +    // If a search is loaded in the same tab, ensure the suggestions dropdown
> +    // is hidden.

Please add something like: ... is hidden immediately, when the page starts loading and not when it first appears, in order to provide timely feedback to the user.

@@ +579,5 @@
> +    this._suggestionsList.setAttribute("class", "searchSuggestionsList");
> +    cell.appendChild(this._suggestionsList);
> +    row.appendChild(cell);
> +    this._table.appendChild(row);
> +    this._suggestionsList.setAttribute("role", "listbox");

role might need to be on _table still, not sure.  Will ask Marco.

@@ +609,4 @@
>      return this._table;
>    },
>  
> +  _addOneOffButtons: function () {

Nit: Could you call this something like _setUpOneOffButtons?  "add" makes it sound like the buttons should not already be there when it's called.

@@ +622,5 @@
> +      return;
> +    }
> +
> +    let enginesPerRow = Math.floor(421 / 49);
> +    let buttonWidth = Math.floor(421 / enginesPerRow);

Assuming that the dropdown's width is 421 isn't great, even if that's the case right now for about:home/newtab.  And actually the dropdown may be wider than that.  Its minWidth is the width of the input but its maxWidth is based on the window's width (see _onMsgSuggestions).  Can't you use _table.getBoundingClientRect() or something?

And please move the magic 49 number up top to a const.

::: browser/base/content/newtab/newTab.xul
@@ +1,1 @@
>  <?xml version="1.0" encoding="UTF-8"?>

We may need to remove the logo box since we're removing the useNewUI part and the logo box and panel are part of the old UI.  I'll check with Stephen, but I would guess that we ought to do that.

::: browser/base/content/newtab/search.js
@@ +34,5 @@
>    search: function (event) {
>      if (event) {
>        event.preventDefault();
>      }
> +    this._contentSearchController.onCommand(event);

Sorry, I wasn't clear.  Having clients call controller.search() makes more sense than onCommand().  I was suggesting an _onCommand internal to the controller that sends ManageEngines if the manage-button is selected but otherwise calls search().

::: browser/modules/ContentSearch.jsm
@@ +228,5 @@
>    _onMessageSearch: function (msg, data) {
>      this._ensureDataHasProperties(data, [
>        "engineName",
>        "searchString",
> +      "healthReportKey",

This is relevant to my comment earlier about making both search params required.
Attachment #8617122 - Flags: review?(adw)
Comment on attachment 8617122 [details] [diff] [review]
Patch v2

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

Marco, would you mind making sure that we're using aria attributes correctly?  You looked at a similar patch for when search suggestions on about:home originally landed in bug 612453.  This probably isn't the final patch, but the aria parts should be final.

First I'll describe what the DOM looks like and then I'll ask my two specific questions.

We have an html:input with an html:table "attached" to it that's styled to look like the one-off search dropdown.  The table has three children: another table to hold search suggestions, another table to hold the search provider buttons, and an html:button to open the search preferences.  Each of the suggestions and search provider buttons can be highlighted/selected.

::: browser/base/content/contentSearchUI.js
@@ +128,4 @@
>        if (i == idx) {
> +        elt.classList.add("selected");
> +        ariaSelectedElt.setAttribute("aria-selected", "true");
> +        ariaParentElt.setAttribute("aria-activedescendant", ariaSelectedElt.id);

Should aria-activedescendant go on the two inner tables or on the outer table?

@@ +579,5 @@
> +    this._suggestionsList.setAttribute("class", "searchSuggestionsList");
> +    cell.appendChild(this._suggestionsList);
> +    row.appendChild(cell);
> +    this._table.appendChild(row);
> +    this._suggestionsList.setAttribute("role", "listbox");

Should role=listbox go on the inner table for suggestions or on the outer table?
Attachment #8617122 - Flags: a11y-review?(mzehe)
Comment on attachment 8617122 [details] [diff] [review]
Patch v2

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

More comments, this should be everything.

::: browser/base/content/abouthome/aboutHome.css
@@ +180,2 @@
>    padding: 6px 0;
> +  -moz-padding-start: 49px;

Just a note for posterity that the point of this is to center the snippet under the search input, now that the input is wider.

::: browser/base/content/contentSearchUI.css
@@ +29,3 @@
>  }
>  
> +.headerRow,

contentSearchUI/searchSuggestionUI is designed to be able to be dropped into any page, so it's probably not a good idea to use selectors that aren't somehow "namespaced" to it.  In other words, I think each selector in this file should start with "contentSearch" or "searchSuggestion".  But it's probably OK for terms in the selector after the first term not to be namespaced.

@@ +103,5 @@
> +  margin: 0;
> +  padding: 0;
> +  border: none;
> +  background: none;
> +  background-image: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAAWCAYAAAABxvaqAAAABmJLR0QA/wD/AP+gvaeTAAAACXBIWXMAAAsTAAALEwEAmpwYAAAAB3RJTUUH3gofECQNNVW2/AAAABBJREFUGFdjOHPmzH8GehEA/KpKg9YTf4AAAAAASUVORK5CYII=');

Does the original one-off UI use a data URI like this?

@@ +119,5 @@
> +  border-bottom: 1px solid #ccc;
> +}
> +
> +.searchOneOffItem.end-of-row {
> +  background-image: none;

Could you use row:last-child instead?

::: browser/locales/en-US/chrome/browser/search.properties
@@ +34,5 @@
>  cmd_addFoundEngineMenu=Add search engine
> +
> +# LOCALIZATION NOTE (searchFor, searchWith):
> +# These two strings are used to build the header above the list of one-click
> +# search providers:  "Search for <used typed keywords> with:"

used typed -> user-typed

::: browser/modules/AboutHome.jsm
@@ +21,5 @@
>    "resource://gre/modules/FxAccounts.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "Promise",
>    "resource://gre/modules/Promise.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> +  "resource://gre/modules/PlacesUtils.jsm");

Did you mean to add this?
(In reply to Drew Willcoxon :adw from comment #13)
> Marco, would you mind making sure that we're using aria attributes
> correctly?  You looked at a similar patch for when search suggestions on
> about:home originally landed in bug 612453.  This probably isn't the final
> patch, but the aria parts should be final.
Not at all! Thanks for reaching out! :)

> We have an html:input with an html:table "attached" to it that's styled to
> look like the one-off search dropdown.  The table has three children:
> another table to hold search suggestions, another table to hold the search
> provider buttons, and an html:button to open the search preferences.  Each
> of the suggestions and search provider buttons can be highlighted/selected.
Is that done similarly to the search box in the navigation toolbar of the main Firefox UI, e. g. up and down arrow from the input selects search suggestions, and tab moves to the one-off buttons and finally to the search preferences button?

> ::: browser/base/content/contentSearchUI.js
> @@ +128,4 @@
> >        if (i == idx) {
> > +        elt.classList.add("selected");
> > +        ariaSelectedElt.setAttribute("aria-selected", "true");
> > +        ariaParentElt.setAttribute("aria-activedescendant", ariaSelectedElt.id);
> 
> Should aria-activedescendant go on the two inner tables or on the outer
> table?
If the actual keyboard focus never leaves the html:input, aria-activedescendant should go on that input. Because all aria-activedescendant does is fire fake focus events, but the reference must always be the item with actual keyboard focus.

> @@ +579,5 @@
> > +    this._suggestionsList.setAttribute("class", "searchSuggestionsList");
> > +    cell.appendChild(this._suggestionsList);
> > +    row.appendChild(cell);
> > +    this._table.appendChild(row);
> > +    this._suggestionsList.setAttribute("role", "listbox");
> 
> Should role=listbox go on the inner table for suggestions or on the outer
> table?
On the inner. The outer groups not only the search suggestions, but also the one-off buttons together, so that would be incorrect. The ohter might be good to have a role of "presentation", and the table that contains the one-off buttons gets a role of "group". Are the one-off buttons themselves true html:button elements or faked ones done with html:span?
Going to leave the a11y-review flag for me until I've looked at a try build.
(In reply to Marco Zehe (:MarcoZ) from comment #15)
> > We have an html:input with an html:table "attached" to it that's styled to
> > look like the one-off search dropdown.  The table has three children:
> > another table to hold search suggestions, another table to hold the search
> > provider buttons, and an html:button to open the search preferences.  Each
> > of the suggestions and search provider buttons can be highlighted/selected.
> Is that done similarly to the search box in the navigation toolbar of the
> main Firefox UI, e. g. up and down arrow from the input selects search
> suggestions, and tab moves to the one-off buttons and finally to the search
> preferences button?

Navigation with the tab key has not yet been implemented, but the goal is to mimic the search box in the navigation toolbar as much as possible.

> > @@ +579,5 @@
> > > +    this._suggestionsList.setAttribute("class", "searchSuggestionsList");
> > > +    cell.appendChild(this._suggestionsList);
> > > +    row.appendChild(cell);
> > > +    this._table.appendChild(row);
> > > +    this._suggestionsList.setAttribute("role", "listbox");
> > 
> > Should role=listbox go on the inner table for suggestions or on the outer
> > table?
> On the inner. The outer groups not only the search suggestions, but also the
> one-off buttons together, so that would be incorrect. The ohter might be
> good to have a role of "presentation", and the table that contains the
> one-off buttons gets a role of "group". Are the one-off buttons themselves
> true html:button elements or faked ones done with html:span?

They are true html:button elements.

Thanks for the feedback!
Comment on attachment 8617122 [details] [diff] [review]
Patch v2

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

Just tried this earlier, nice work !

Here are a couple of issues I have found :
- One-off search popup lacks box-shadow like in the mockup ( 0 0 4px hsla(210,4%,10%,.2) )
- clicking the magnifier doesn't open the one-off search popup, which isn't consistent with the navbar searchbar
- Old new tab search UI is still there (but I'm guessing it's going to be removed in a future patch)
(In reply to Tim Nguyen [:ntim] from comment #18)
> Just tried this earlier, nice work !
Thanks! :)
> Here are a couple of issues I have found :
> - One-off search popup lacks box-shadow like in the mockup ( 0 0 4px
> hsla(210,4%,10%,.2) )
Wow, did you figure out those values by trial and error? :)
I was going to needinfo? shorlander on the UX bug for the values for the exact colors of borders/shadows/stuff used in the mockup.
> - clicking the magnifier doesn't open the one-off search popup, which isn't
> consistent with the navbar searchbar
Ah yes, I'm aware of this. It might be pushed to a follow-up, and in any case, I will be requesting the icons magnifying glass first :)
> - Old new tab search UI is still there (but I'm guessing it's going to be
> removed in a future patch)
Yup, stay tuned!
(In reply to Nihanth Subramanya [:nhnt11] from comment #19)
> (In reply to Tim Nguyen [:ntim] from comment #18)
> > Just tried this earlier, nice work !
> Thanks! :)
> > Here are a couple of issues I have found :
> > - One-off search popup lacks box-shadow like in the mockup ( 0 0 4px
> > hsla(210,4%,10%,.2) )
> Wow, did you figure out those values by trial and error? :)
> I was going to needinfo? shorlander on the UX bug for the values for the
> exact colors of borders/shadows/stuff used in the mockup.
Those values are from the Windows arrow panel styling which looks pretty similar (and was designed by shorlander as well) ;)
Attached patch Patch v3 (obsolete) — — Splinter Review 
Stuff that's new in this patch:
- Clicking the default engine header fires a search with the default engine.
- Update newTab.xul and newTab.css to match mockup.
- Removed remnants of the search engine logo thing and the search panel in newTab.xul and search.js (I think it still needs to be removed from chrome though)
- "Hidden" one off buttons were still showing up, because I wasn't using the pref that controls this (I thought getVisibleEngines() did the trick). This is now fixed. ContentSearchService listens for a change in the pref and broadcasts a message when it changes.
- Fixed a couple of almost unnoticeable UI bugs.
- Addressed review comments (including Marco's and Tim's)

(In reply to Drew Willcoxon :adw from comment #12)
> When you mouse down on a suggestion, the arrow button blinks: it turns gray
> (blurred) and then quickly turns blue again.  It's pretty annoying.  You can
> clearly see it by holding down the mouse button before releasing it.  When
> I'm not trying to reproduce it that way, it only happens sometimes.  I
> couldn't reproduce with the old UI even for slow-loading search pages, so I
> presume it's something you can fix.  If it's too hard to fix, and maybe it
> is because of the careful interaction around mouse clicks you've
> implemented, we can do that in a follow-up.

Fixed with a new keepfocus attribute, added a comment too.

> (In reply to Nihanth Subramanya [:nhnt11] from comment #9)
> > > It looks like you removed the parts that fixed bug 1115616 related to the
> > > input and IME.  We'll need to make sure we don't ultimately regress that.
> > The search is no longer happening using a form's onsubmit event, so we no
> > longer forcibly update the inputbox's value before the submit event fires. I
> > think we're fine, let me know if I'm missing something.
> 
> Hmm, something's wrong but I can't reproduce it 100% of the time.  I tried
> with an IME, and sometimes clicking a suggestion while the IME input was not
> "finalized" (not sure what the right term is) triggered the search
> immediately, but other times I saw bug 1115616, where clicking a suggestion
> finalized the IME input but kept the search dropdown open, and then a second
> click would work normally and search for the input that was finalized.

I found an issue that caused suggestions to be re-fetched (and the list re-populated, causing deselection) when the IME input was finalized. I've fixed this in this patch, and am not experiencing any issues with IME input myself. Please let me know if you still encounter those issues.

> ::: browser/base/content/contentSearchUI.js
> @@ +31,5 @@
> >   *        positions should be "static"), or the parent's position should be the
> >   *        top left of the page.
> > + * @param searchParams
> > + *        An object containing parameters that will be passed in the generated
> > + *        search events' data. It should at least specify the healthReportKey.
> 
> Let's document the two parameters that we expect clients to be using right
> now.  Doesn't preclude us from adding more in the future of course.
> 
> Now that I think about it, I think we should enforce that both searchParams
> are non-null.  As in, `throw new Error()` if either is null.  There
> shouldn't be any case in the browser where either the FHR key or the search
> purpose isn't specified, since both are important pieces of information.
> 
> I think that might have implications for what the constructor's params
> should be.  Personally, when I see a function that has some non-default
> params followed by a params object that defaults to {}, it makes me think
> that the params in the params object are all optional.  Maybe that's just
> me.  I'll let you change or not change the params how you like.

I removed the params object and just included both in the constructor arguments.

> @@ +128,4 @@
> >        if (i == idx) {
> > +        elt.classList.add("selected");
> > +        ariaSelectedElt.setAttribute("aria-selected", "true");
> > +        ariaParentElt.setAttribute("aria-activedescendant", ariaSelectedElt.id);
> 
> Hmm, I don't know whether aria-activedescendant should go on _table or
> _suggestionsList/_oneOffsTable.  I would guess always _table since it's the
> top-level container.  I'll flag Marco for feedback.  Anyhow, nice work
> keeping it updated.

I've updated it as Marco advised.

> @@ +192,5 @@
> > +        engineName: engineName,
> > +        searchString: searchTerms,
> > +        originalEvent: {
> > +          target: {
> > +            ownerDocument: document,
> 
> Sorry, I blew it when I OK'ed this ownerDocument change in the other
> whereToOpenLink bug.  The document lives in the content process, so when you
> send it over to chrome, it's wrapped in a cross-process object wrapper
> ("CPOW"), which is like a last-resort way of dealing with e10s that we
> should avoid.
> 
> This is kind of a separate bug, but since you're touching this code, would
> you mind fixing it?  Fortunately it doesn't look like we need target or
> ownerDocument since whereToOpenLink doesn't need them.  Am I reading that
> right?  Maybe they were left over from an earlier version of your patch in
> the other bug?

In the whereToOpenLink bug this is set to null, not the document. You didn't blow it up afaik ;)
This was a remnant from a previous WIP, I've just removed the target object altogether in this.

> @@ +208,5 @@
> > +  
> > +      if (searchText.hasAttribute("selection-index")) {
> > +        eventData.selection = {
> > +          index: searchText.getAttribute("selection-index"),
> > +          kind: searchText.getAttribute("selection-kind")
> 
> Hmm, we could do away with these attributes on the input now that this
> prototype completely handles searching.  It looks like they were added only
> because about:home/newtab had their own search boxes but search suggestions
> were unified (http://hg.mozilla.org/mozilla-central/rev/57b1a2b98388).
> 
> We don't need selection-index at all anymore since it's the same thing as
> this.selectedIndex.  selection-kind is either "key" or "mouse", and it looks
> like your patch removes where "mouse" is set, so at the least that should be
> fixed.  But I think you should pass the kind to onCommand.  _onKeypress
> would pass "key", and _onMouseUp would pass "key".  What do you think about
> that?

I've used aEvent instanceof MouseEvent/KeyboardEvent since we have access to the event object.

> @@ +382,5 @@
> > +    if (this._table.hidden) {
> > +      this.selectedIndex = -1;
> > +      this._table.hidden = false;
> > +      this.input.setAttribute("aria-expanded", "true");
> > +      this._getSearchEngines();
> 
> Why get engines here?  Isn't getting them on construction and then being
> notified when they change via the CurrentState message enough?

Um, no idea why this line got added. I've removed it.

> There's a CurrentEngine message that you're not currently listening for (and
> neither is the current searchSuggestionUI.js).  It's sent when the current
> engine changes.  I think you should probably listen for that too and then
> you don't need this?  What happens now with your patch when you change the
> current engine is that when the dropdown opens, you briefly see the old
> engine in the header before it's replaced with the new one.  I'm guessing
> you'll see the old one for as long as it takes for suggestions to arrive,
> based on this code here.
>
> I think getting the engines here is also causing some flickering of the
> one-off button icons I'm seeing (on my slow debug build) since getting the
> engines causes the buttons to be rebuilt.

I'm listening for it now, it should work correctly.

> @@ +622,5 @@
> > +      return;
> > +    }
> > +
> > +    let enginesPerRow = Math.floor(421 / 49);
> > +    let buttonWidth = Math.floor(421 / enginesPerRow);
> 
> Assuming that the dropdown's width is 421 isn't great, even if that's the
> case right now for about:home/newtab.  And actually the dropdown may be
> wider than that.  Its minWidth is the width of the input but its maxWidth is
> based on the window's width (see _onMsgSuggestions).  Can't you use
> _table.getBoundingClientRect() or something?
> 
> And please move the magic 49 number up top to a const.

Thanks for catching this, I'm now using the input box's width.

> @@ +103,5 @@
> > +  margin: 0;
> > +  padding: 0;
> > +  border: none;
> > +  background: none;
> > +  background-image: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAAWCAYAAAABxvaqAAAABmJLR0QA/wD/AP+gvaeTAAAACXBIWXMAAAsTAAALEwEAmpwYAAAAB3RJTUUH3gofECQNNVW2/AAAABBJREFUGFdjOHPmzH8GehEA/KpKg9YTf4AAAAAASUVORK5CYII=');
> 
> Does the original one-off UI use a data URI like this?

Yes, https://mxr.mozilla.org/mozilla-central/source/browser/themes/linux/searchbar.css#165.

> @@ +119,5 @@
> > +  border-bottom: 1px solid #ccc;
> > +}
> > +
> > +.searchOneOffItem.end-of-row {
> > +  background-image: none;
> 
> Could you use row:last-child instead?

No, because if the last child is somewhere in the middle of the row, we still want to show that vertical line.

> ::: browser/modules/AboutHome.jsm
> @@ +21,5 @@
> >    "resource://gre/modules/FxAccounts.jsm");
> >  XPCOMUtils.defineLazyModuleGetter(this, "Promise",
> >    "resource://gre/modules/Promise.jsm");
> > +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> > +  "resource://gre/modules/PlacesUtils.jsm");
> 
> Did you mean to add this?

It's no longer needed, forgot to remove it.
Attachment #8617122 - Attachment is obsolete: true
Attachment #8617122 - Flags: a11y-review?(mzehe)
Attachment #8620736 - Flags: review?(adw)
Attachment #8620736 - Flags: a11y-review?(mzehe)
Forgot to mention, the table is now positioned in a way that doesn't break when the window is resized or the page is zoomed.
Attached patch Patch v3.1 (obsolete) — — Splinter Review 
Removed bitrot from jar.mn's
Attachment #8620736 - Attachment is obsolete: true
Attachment #8620736 - Flags: review?(adw)
Attachment #8620736 - Flags: a11y-review?(mzehe)
Attachment #8620740 - Flags: review?(adw)
Attachment #8620740 - Flags: a11y-review?(mzehe)
Comment on attachment 8620740 [details] [diff] [review]
Patch v3.1

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

Fantastic!  Some comments below, mostly about search.js.  r+ with those fixed.

(In reply to Nihanth Subramanya [:nhnt11] from comment #21)
> - "Hidden" one off buttons were still showing up, because I wasn't using the
> pref that controls this (I thought getVisibleEngines() did the trick).

Huh, I wonder why that's a separate pref instead of using the visible-engines mechanism.  Interesting.

(In reply to Nihanth Subramanya [:nhnt11] from comment #22)
> Forgot to mention, the table is now positioned in a way that doesn't break
> when the window is resized or the page is zoomed.

Nice!

::: browser/base/content/newtab/newTab.xul
@@ +1,1 @@
>  <?xml version="1.0" encoding="UTF-8"?>

You can remove newtab-search-panel too and all its CSS from newTab.css.

@@ +12,5 @@
>  <!DOCTYPE window [
>    <!ENTITY % newTabDTD SYSTEM "chrome://browser/locale/newTab.dtd">
>    %newTabDTD;
>    <!ENTITY % searchBarDTD SYSTEM "chrome://browser/locale/searchbar.dtd">
>    %searchBarDTD;

Can remove this DTD.

@@ +110,3 @@
>            <input type="text" name="q" value="" id="newtab-search-text"
>                   maxlength="256" dir="auto"/>
> +          <input id="newtab-search-submit" type="button" value="" onclick="search(event)"/>

You're adding a click listener via JS too, so please remove this inline one.

::: browser/base/content/newtab/search.js
@@ +3,1 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,

I'm realizing just now that there's a lot of stuff in this file we can remove, since contentSearchUI handles most aspects of searching.

@@ +5,5 @@
>  #endif
>  
>  let gSearch = {
>  
>    currentEngineName: null,

this

@@ +17,2 @@
>      window.addEventListener("ContentSearchService", this);
>      this._send("GetState");

both of these lines

@@ +26,3 @@
>    },
>  
>    manageEngines: function () {

this whole method

@@ +46,1 @@
>      if (this._initialStateReceived) {

both onState and onCurrentState

@@ +48,5 @@
>        this._setCurrentEngine(data.currentEngine);
>      }
>    },
>  
>    onCurrentEngine: function (engineName) {

this method

@@ +54,5 @@
>        this._setCurrentEngine(engineName);
>      }
>    },
>  
>    onFocusInput: function () {

Focusing input should be a part of contentSearchUI.js too, but please don't do it now since it's not strictly related to this bug and this patch is enough work already. :-)

@@ +64,1 @@
>      "manage",

this line ("manage")

@@ +66,5 @@
>    ],
>  
>    _nodes: {},
>  
>    _initWhenInitalStateReceived: function () {

This method too.  We can add the click listener right in init now.  That's the only thing from this method that still needs to happen.

@@ +73,5 @@
>      this._initialStateReceived = true;
>      this._initWhenInitalStateReceived = function () {};
>    },
>  
>    _send: function (type, data=null) {

this method

@@ +86,2 @@
>    // Converts favicon array buffer into data URI of the right size and dpi.
>    _getFaviconURIFromBuffer: function (buffer) {

this method

@@ +94,1 @@
>    _setCurrentEngine: function (engine) {

This method.  _contentSearchController should be created in init.

::: browser/modules/ContentSearch.jsm
@@ +57,5 @@
>   *     Removes an entry from the search form history.
>   *     data: the entry, a string
>   *   Search
>   *     Performs a search.
> + *     data: { engineName, searchString, healthReportKey, [searchPurpose] }

Please update this line since searchPurpose isn't optional anymore.

@@ +232,5 @@
>    _onMessageSearch: function (msg, data) {
>      this._ensureDataHasProperties(data, [
>        "engineName",
>        "searchString",
> +      "healthReportKey",

Please add searchPurpose too.
Attachment #8620740 - Flags: review?(adw) → review+
So I wanted to give this an accessibility review, but I don't see any one-off buttons. The search suggestions work, but one-off buttons don't appear either in about:home or about:newtab. I used the try server build from comment #24.
Comment on attachment 8620740 [details] [diff] [review]
Patch v3.1

The search input is missing a label. Screen readers will only say something like "edit" or "textbox". The previous search UI had a proper label, although it had been done by associating the image's alternative text to the input via aria-labelledby.

Same goes for the Search button. It needs a label, or screen readers will only call it "button". Right now, it has no semantic information that allows screen readers to know what this button does. How do you indicate this visually? If you do it through CSS generated background images, be aware that these will disappear as soon as you switch Windows to high contrast mode, leaving those sighted people without any semantics, too.
Attachment #8620740 - Flags: a11y-review?(mzehe) → a11y-review-
Rank: 25
Flags: firefox-backlog+
Priority: -- → P2
QA Contact: petruta.rasa
Attached patch Patch v4 (obsolete) — — Splinter Review 
Address the a11y comments and a couple of other fixes from what I found while writing tests.
Attachment #8620740 - Attachment is obsolete: true
Attachment #8629127 - Flags: review?(adw)
Attached patch Tests (obsolete) — — Splinter Review 
Update content search related UI tests.
Attachment #8629128 - Flags: review?(adw)
I just realized interdiff won't work because I rebased the patch. I'll try and make a list of changes and post them here.
Comment on attachment 8629127 [details] [diff] [review]
Patch v4

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

::: browser/themes/shared/search/search-indicator-magnifying-glass.svg
@@ +1,2 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24">

Please add a license header to this file.
Turns out Patch v4 doesn't address a11y comments after all, I think those changes got lost. I'll upload another patch with the aria-labels.
Comment on attachment 8629127 [details] [diff] [review]
Patch v4

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

No need for an interdiff, but if/when you post a new patch with accessibility changes, then an interdiff would be helpful.

::: browser/base/content/browser.js
@@ +3458,5 @@
>        if (!aSearchBar || document.activeElement != aSearchBar.textbox.inputField) {
>          let url = gBrowser.currentURI.spec.toLowerCase();
>          let mm = gBrowser.selectedBrowser.messageManager;
> +        if (url === "about:home" ||
> +            (url === "about:newtab" && NewTabUtils.allPages.enabled)) {

Probably shouldn't fix this in this bug.  Let's file a new one.

::: browser/base/content/contentSearchUI.js
@@ +236,5 @@
> +      };
> +    }
> +
> +    this._sendMsg("Search", eventData);
> +  

Nit: please remove this trailing space.

::: browser/modules/ContentSearch.jsm
@@ +442,5 @@
>      let state = {
>        engines: [],
>        currentEngine: yield this._currentEngineObj(),
>      };
> +    let pref = Preferences.get("browser.search.hiddenOneOffs");

Could you just use Services.prefs.getCharPref() so you don't have to import Preferences.jsm?  Preferences.jsm isn't really buying you anything here.
Attachment #8629127 - Flags: review?(adw) → review+
(In reply to Drew Willcoxon :adw from comment #33)
> Comment on attachment 8629127 [details] [diff] [review]
> Patch v4
> ::: browser/base/content/browser.js
> @@ +3458,5 @@
> >        if (!aSearchBar || document.activeElement != aSearchBar.textbox.inputField) {
> >          let url = gBrowser.currentURI.spec.toLowerCase();
> >          let mm = gBrowser.selectedBrowser.messageManager;
> > +        if (url === "about:home" ||
> > +            (url === "about:newtab" && NewTabUtils.allPages.enabled)) {
> 
> Probably shouldn't fix this in this bug.  Let's file a new one.

This change is necessary in this patch because ContentSearchUIController handles focusInput, and the signal needs to come from ContentSearch.jsm.

I know it also fixes that bug as a side affect, but I don't know why ContentSearch.focsuInput() isn't working on the newtab page in the current release (i.e. this isn't a real fix for the existing bug).
I suppose we should also fix the existing bad behavior in another bug and uplift it?
Comment on attachment 8629128 [details] [diff] [review]
Tests

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

Thanks for updating all these tests.  I know it's a giant pain.

Could you please paste your latest tryserver results here in the bug?

::: browser/base/content/test/general/browser_aboutHome.js
@@ +17,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "AboutHomeUtils",
>    "resource:///modules/AboutHome.jsm");
>  
>  const TEST_CONTENT_HELPER = "chrome://mochitests/content/browser/browser/base/content/test/general/aboutHome_content_script.js";
> +const TEST_ENGINE_PREFIX = "browser_searchSuggestionEngine";

Doesn't look like this is used anywhere.

@@ +114,3 @@
>  
> +    let mm = Cc["@mozilla.org/globalmessagemanager;1"].
> +      getService(Ci.nsIMessageListenerManager);

Doesn't look like mm is actually used anymore, so there's no need to redefine it...?

@@ +651,5 @@
>      });
>    }
>  });
>  
> +function promiseSearchEngineChange(newEngineName) {

Could you please call this promiseContentSearchChange?  There are multiple changes related to search going on, and this name made me think that we're waiting for Services.search.currentEngine to change, or maybe waiting for the engine to be fully registered with the search service after adding it.

::: browser/base/content/test/general/browser_contentSearchUI.js
@@ +161,1 @@
>    yield deferred.promise;

Maybe yield Promise.all([...]) would be safest...

@@ +219,5 @@
> +  };
> +  SimpleTest.isDeeply(eventData, mesg, "Search event data");
> +
> +  yield promiseTab();
> +  yield msg("init");

Maybe promiseTab should do the msg("init")?  Is there any promiseTab caller that doesn't want to init the page?

@@ +475,5 @@
> +    info("Adding test search engines");
> +    let engine1 = yield promiseNewEngine(TEST_ENGINE_BASENAME);
> +    let engine2 = yield promiseNewEngine(TEST_ENGINE_2_BASENAME);
> +    Services.search.currentEngine = engine1;
> +    for (let engine of currentEngines)

Nit: braces

@@ +503,5 @@
>    });
>    return addDeferred.promise;
>  }
> +
> +function sleep(t) {

Doesn't look like this is used.

::: browser/base/content/test/general/contentSearchUI.js
@@ +14,5 @@
>  });
>  
>  let messageHandlers = {
>  
> +  init: function(name) {

Looks like this param isn't used, neither here nor by the chrome side.

@@ +166,5 @@
>  }
>  
> +function waitForContentSearchEvent(messageType, cb) {
> +  let mm = content.SpecialPowers.Cc["@mozilla.org/globalmessagemanager;1"].
> +    getService(content.SpecialPowers.Ci.nsIMessageListenerManager);

Since you're using the global message manager, you could catch messages unrelated to the tab that the script is running in.  You should just use the addMessageListener() global function to add a listener for this tab.  I could be wrong, but I'm pretty sure that even the messages that ContentSearch sends to the global message manager get dispatched to each tab's message manager, so there's no danger of missing messages...
Attachment #8629128 - Flags: review?(adw) → review+
Attached patch Patch v5 (obsolete) — — Splinter Review 
Address review comments and add aria-labels to search input boxes/submit buttons.

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36ba82a02d57

Marco, can you try the build from this try push and let me know if it's any better?

I tried using the OS X screen reader with the one-off buttons and it reads the search engine name and says "button". I couldn't reproduce the case where the one off buttons don't appear on any platform.
Attachment #8629127 - Attachment is obsolete: true
Attachment #8630704 - Flags: review?(adw)
Attachment #8630704 - Flags: a11y-review?(mzehe)
Attached patch Tests v2 (obsolete) — — Splinter Review 
Address review comments. Carrying forward r+.
Attachment #8629128 - Attachment is obsolete: true
Attachment #8630706 - Flags: review+
(In reply to Drew Willcoxon :adw from comment #35)
> Comment on attachment 8629128 [details] [diff] [review]
> Tests
> @@ +166,5 @@
> >  }
> >  
> > +function waitForContentSearchEvent(messageType, cb) {
> > +  let mm = content.SpecialPowers.Cc["@mozilla.org/globalmessagemanager;1"].
> > +    getService(content.SpecialPowers.Ci.nsIMessageListenerManager);
> 
> Since you're using the global message manager, you could catch messages
> unrelated to the tab that the script is running in.  You should just use the
> addMessageListener() global function to add a listener for this tab.  I
> could be wrong, but I'm pretty sure that even the messages that
> ContentSearch sends to the global message manager get dispatched to each
> tab's message manager, so there's no danger of missing messages...

As discussed in person, this is listening for the outgoing Search message, not an incoming one - so it's not tab-specific.
Attachment #8630704 - Flags: review?(adw) → review+
Comment on attachment 8630704 [details] [diff] [review]
Patch v5

Almost there! This now has the correct labels on the buttons, *but* the tab key misbehaves.

1. Focus the search field and start typing.
2. As soon as the field expands to show search results and the one-off buttons, press Tab.

Expected: Search suggestions should remain visible, and the first one-off button should be focused.
Actual: Search suggestions and one-off buttons disappear, and focus goes straight to the Submit Search button.

So while these now have correct labels, it is not possible to act on the one-off buttons as a keyboard user.

Note that on the Mac, you may have to tell the Tab key to focus *all* controls, not just inputs, in System Preferences, keyboard Settings, Shortcuts tab.
Attachment #8630704 - Flags: a11y-review?(mzehe) → a11y-review-
(In reply to Marco Zehe (:MarcoZ) from comment #40)
> Comment on attachment 8630704 [details] [diff] [review]
> Patch v5

Thanks for the review!

> Almost there! This now has the correct labels on the buttons, *but* the tab
> key misbehaves.
> 
> 1. Focus the search field and start typing.
> 2. As soon as the field expands to show search results and the one-off
> buttons, press Tab.
> 
> Expected: Search suggestions should remain visible, and the first one-off
> button should be focused.
> Actual: Search suggestions and one-off buttons disappear, and focus goes
> straight to the Submit Search button.
> 
> So while these now have correct labels, it is not possible to act on the
> one-off buttons as a keyboard user.
> 
> Note that on the Mac, you may have to tell the Tab key to focus *all*
> controls, not just inputs, in System Preferences, keyboard Settings,
> Shortcuts tab.

Ah, I am aware that keyboard navigation is not yet up to parity with the main searchbar UI (in the toolbar).
I was going to file a follow-up for this to avoid making this patch even larger (there are a few interesting keyboard navigation features to implement).

If that's OK with you, it looks like this is almost ready to land!
Flags: needinfo?(mzehe)
Comment on attachment 8630704 [details] [diff] [review]
Patch v5

a11y-r=me with the explanation/prospect given in comment #41. Please file the follow-up bug and CC me on it, thanks!
Flags: needinfo?(mzehe)
Attachment #8630704 - Flags: a11y-review- → a11y-review+
Attached patch Patch v6 (obsolete) — — Splinter Review 
Use values from new specs posted in attachment 8631786 [details] on bug 1106057.
Attachment #8630704 - Attachment is obsolete: true
Attachment #8631830 - Flags: review?(adw)
Attachment #8631830 - Flags: review?(adw) → review+
Attached patch Patch v7 — — Splinter Review 
Fix an intermittent test failure caused by ContentSearchUIController sometimes receiving a CurrentEngine message before a State message.

r=adw over IRC.
Attachment #8631830 - Attachment is obsolete: true
Attachment #8631867 - Flags: review+
Depends on: 1182338
https://hg.mozilla.org/mozilla-central/rev/5d8717ff74c4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
This was backed out in comment 46
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Tests v3 — — Splinter Review 
- Test failure fixes (browser_newtab_unpin.js and browser_mozLoop_context.js)
- Override ContentSearch._onMessage{Search, ManageEngines} to do nothing, this way we can register a cleanup function to remove each tab we open within promiseTab.
- Remove form history additions at the end of the search

Green try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e0260bf1096
Attachment #8630706 - Attachment is obsolete: true
Attachment #8634827 - Flags: review?(adw)
Comment on attachment 8634827 [details] [diff] [review]
Tests v3

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

Yay!

::: browser/base/content/test/general/contentSearchUI.js
@@ +112,5 @@
> +                     content.document.getElementById("contentSearchSettingsButton")];
> +      row = allElts[eltIdx];
> +    }
> +    let event = arg.modifiers || {};
> +    content.synthesizeMouseAtCenter(row, event);

Could you please add a comment saying that since we don't pass an event type here, synthesizeMouseAtCenter defaults to sending a mousedown followed by a mouseup?  I had to go look that up because I couldn't find where you're setting the event type in this case.
Attachment #8634827 - Flags: review?(adw) → review+
Prevents uninterruptible reflows in the newtab page, which regress browser_tabopen_reflows.js.

Green try push, bc3 retriggered a few times: https://treeherder.mozilla.org/#/jobs?repo=try&revision=110e5e94e376

Hopefully this is the last regression.
Attachment #8635751 - Flags: review?(adw)
Comment on attachment 8635751 [details] [diff] [review]
Delay setting up one off buttons till suggestions table is unhidden

This new _pendingOneOffRefresh variable needs a comment explaining what it's used for / why we need it.
Comment on attachment 8635751 [details] [diff] [review]
Delay setting up one off buttons till suggestions table is unhidden

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

r+ with Florian's comment.
Attachment #8635751 - Flags: review?(adw) → review+
Can someone show me the results (screenshot) of this patch?

http://hg.mozilla.org/mozilla-central/diff/b2a96402ed45/browser/locales/en-US/chrome/browser/search.properties

   15.16 +searchFor=Search for 
   15.17 +searchWith= with:

That space after = is not relevant (it should be ignored by the parser), and most important why are you concatenating strings instead of doing something like "Search for #1 with:" and replace #1?
Flags: needinfo?(nhnt11)
I guess there's the same issue with searchWithHeader: "Search with:" is concatenated with the searchengine's name?

This looks really broken from a localization point of view, especially considering the strings look similar to what we're already using in the search bar.
Depends on: 1185845
Per IRC, let's continue the discussion in bug 1185845.
Flags: needinfo?(nhnt11)
Blocks: 1186281
Blocks: 1186325
:nhnt11, seems this caused bug 1186325 - can you take a look at this  ?
Flags: needinfo?(nhnt11)
Flags: needinfo?(nhnt11)
Depends on: 1188618
Depends on: 1188782
No longer depends on: 1188782
Depends on: 1189101
https://hg.mozilla.org/mozilla-central/diff/b2a96402ed45/browser/locales/en-US/chrome/browser/aboutHome.dtd
> -<!ENTITY abouthome.searchEngineButton.label "Search">
> +<!ENTITY abouthome.search.placeholder    "Search">

Where is abouthome.search.placeholder string used?
(In reply to Stefan Plewako [:stef] from comment #62)
> https://hg.mozilla.org/mozilla-central/diff/b2a96402ed45/browser/locales/en-
> US/chrome/browser/aboutHome.dtd
> > -<!ENTITY abouthome.searchEngineButton.label "Search">
> > +<!ENTITY abouthome.search.placeholder    "Search">
> 
> Where is abouthome.search.placeholder string used?
Flags: needinfo?(nhnt11)
Depends on: 1190488
(In reply to Stefan Plewako [:stef] from comment #62)
> https://hg.mozilla.org/mozilla-central/diff/b2a96402ed45/browser/locales/en-
> US/chrome/browser/aboutHome.dtd
> > -<!ENTITY abouthome.searchEngineButton.label "Search">
> > +<!ENTITY abouthome.search.placeholder    "Search">
> 
> Where is abouthome.search.placeholder string used?

Filed bug 1190488.
Flags: needinfo?(nhnt11)
Without this patch, Bug 1113731 (I think) could be reproduced in about:home and about:newtab by entering some text, selecting a suggestion using the mouse and pressing Enter. In this case, the text field was updated with the new search string after pressing Enter.

Now, the search is still performed after the suggestion hovered by mouse, but the text field is not updated.

(ux bug 1142334 to determine the correct action)

Should I open a new bug for this considering the old behavior is also wrong? Thank you!
Flags: needinfo?(nhnt11)
Hmm, considering the implementation and UX have changed almost completely since then, and the problem now is slightly different, I think it would be clearer to file a new bug describing the current problem, and close the old bug.  Could you do that please?
Flags: needinfo?(nhnt11)
Thanks, Drew. I opened bug 1196677 and let the other one opened since it is about the search toolbar.
Depends on: 1206974
Blocks: 1159672
Depends on: 1206709
Depends on: 1357523
Depends on: 1379460
Depends on: 1381659
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: