1180944 - Implement one-off searches from Awesomebar

Status: VERIFIED FIXED

(Firefox :: Search, defect, P2)

Description

[Tim Nguyen :ntim]

Prototype add-on : http://t.co/DIaaSE9QrJ

Comment 1

[Tim Nguyen :ntim]

Are there plans to implement this ? I'd love to do this. The only mockup I've seen so far is this one : http://people.mozilla.org/~shorlander/Flare/Flare-Search-Unification-%28OSX%29-03.png

Comment 2

[Florian Quèze [:florian]]

I don't know, but Dave (currently in vacations) or Stephen should know :).

Comment 3

[Marco Bonardo [:mak]]

yes, it's planned, see http://people.mozilla.org/~shorlander/mockups/AwesomeResults/AwesomeResults-01.html

Comment 4

[Marco Bonardo [:mak]]

the footer has been hidden in bug 1191520 until this bug is done.

Comment 6

[Łukasz]

Please enter the search icon on the gray bar. Like in searchbox.
http://zapodaj.net/images/ec579dbcd2f86.png

Comment 7

[Łukasz]

https://bugzilla.mozilla.org/show_bug.cgi?id=1206749

Comment 8

[Marco Bonardo [:mak]]

I think this shoulk be re-prioritized based on the fact we stated consistency is a priority

Comment 9

[:shell escalante]

to avoid complications - moving forward with experiment... and then implement as soon as done - or limit experiment to specific version number and implement in other.

Comment 11

[Drew Willcoxon :adw]

Attachment: Bug 1180944 - Implement one-off searches from Awesomebar

Work in progress.  This factors out the one-off code from the browser-search-autocomplete-result-popup binding in search.xml into its own binding and uses it in both the searchbar and awesomebar.

The major things left to do are:

* Seamless key handling in the awesomebar

* Change the one-off UI design to match Stephen's spec?  Although I think I'd like to leave that for a follow-up, after we have a single implementation that's used in both places.

* Make sure current searchbar one-off tests still work, write new tests for the awesomebar

Review commit: https://reviewboard.mozilla.org/r/59074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59074/

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 8762257 [details]
Bug 1180944 - Implement one-off searches from Awesomebar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59074/diff/1-2/

Comment 13

[Drew Willcoxon :adw]

That commit:

* Integrates the new binding in the searchbar binding (in addition to integrating it in the urlbar, which the previous commit did)

* Implements key handling that works in both the urlbar and searchbar

* Fixes existing urlbar and searchbar tests, which all pass locally for me

Still need to do:

* Write/update tests for one-offs in the urlbar, probably

* Figure out telemetry: either move it to the new binding so it's in one place, or update the urlbar binding so that it logs telemetry in the new places like the searchbar already does

* Update the one-off UI to match the spec, but I still think it would be a good idea to do that in a follow-up bug or at least a follow-up patch, to lessen the review burden

Comment 14

[Drew Willcoxon :adw]

Try with latest commit (fixes existing tests, no new tests):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c655e11c2678

Comment 15

[Drew Willcoxon :adw]

Oh one other thing the latest commit does is in the urlbar, when the currently selected item is "foo -- Search with Search Engine" and you select a one-off (by mouse over for example), the item changes to show the selected engine to show you what will happen when you press Return or click the mouse on the one-off.  That's something that the searchbar doesn't have to worry about.

For example, if the item is "foo -- Search with Google" and you mouse over the Yahoo one-off, then the item changes to "foo -- Search with Yahoo".

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 8762257 [details]
Bug 1180944 - Implement one-off searches from Awesomebar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59074/diff/2-3/

Comment 17

[Drew Willcoxon :adw]

Comment on attachment 8762257 [details]
Bug 1180944 - Implement one-off searches from Awesomebar

Florian, since everything I'm factoring out is code you wrote, would you mind reviewing the browser/components/search/ parts of this?  Feel free to look at the other parts too of course, but I'll ask Marco to look at those since they deal with the urlbar and we've been tag-teaming that recently.

The only thing left to do is to change the UI so it matches Stephen's spec.  As I say, I'd like to leave that for a follow-up patch, after this initial implementation is r+'ed, since this patch is big enough already.

Summary of changes: I factored out the one-offs into a one-offs binding in search.xml.  I tried to make it generic enough that it could be used in both the urlbar and searchbar.  You can see the main API points at the top of the binding.  Consumers set popup, textbox, and telemetryID properties, and the binding takes it from there.  It adds event listeners to the popup and textbox to update itself properly.

One hairy part is the handleKeyPress method.  Both the urlbar and searchbar use the one-offs in conjunction with a list, and basically they behave the same, so I wanted a single method that both could call given a key event to update the selection in both the popup list and the one-offs.  They have slightly different needs though because most of the time the urlbar wants there to always be a selection.  One consequence of that is that I added an allowEmptySelection param.

Comment 18

[Drew Willcoxon :adw]

Comment on attachment 8762257 [details]
Bug 1180944 - Implement one-off searches from Awesomebar

Marco, would you mind reviewing the non-search parts of this?

browser/base/content/
toolkit/components/places/
toolkit/content/widgets/autocomplete.xml

Feel free to look at the other parts too of course.

I added a vbox to the urlbar panel that's bound to the one-offs binding.

I'm not totally sure how we want to handle telemetry.  This patch still records a search engine action (BrowserSearch.recordSearchInTelemetry()) when you click a searchengine item or when you press Return while a search engine item is selected.  In addition, (a) it records a search engine action when you click a one-off and when you select "search in new tab" from the one-off context menu, and (b) if a searchengine item's URL is loaded and a one-off is selected, then one-off telemetry is also recorded (BrowserSearch.recordOneoffSearchInTelemetry()).

I factored out the URL-loading code into _loadURL so that there's one place where the one-off telemetry could be recorded.

I update all the params.engineName values when the selected one-off changes.  I factored out the moz-action creation code into PlacesUtils so I could call it (instead of duplicating it yet again) when doing this.  I also had to change the initial "type" attribute on richlistitems to "originalType", since we modify the "type" attribute.

Comment 19

[Florian Quèze [:florian]]

(In reply to Drew Willcoxon :adw from comment #17)

> The only thing left to do is to change the UI so it matches Stephen's spec.

Which spec is this?

> As I say, I'd like to leave that for a follow-up patch, after this initial
> implementation is r+'ed, since this patch is big enough already.

Sure, the smaller the patches, the better.

Comment 20

[Drew Willcoxon :adw]

(In reply to Florian Quèze [:florian] [:flo] from comment #19)
> Which spec is this?

http://people.mozilla.org/~shorlander/mockups/AwesomeResults/AwesomeResults-01.html from comment 3.

> Sure, the smaller the patches, the better.

Yeah, I probably should have split this one up into a search part and a urlbar part. :-/

Comment 21

[Marco Bonardo [:mak]]

Are we removing the search engine icons colors? Apart from the fact it makes harder to recognize engines quickly, can we actually change how we show their TM-ed logos?

Comment 22

[Drew Willcoxon :adw]

Attachment: Bug 1180944 - Implement one-off searches from Awesomebar.

More big changes for you Marco, sorry...

The previous commit didn't handle engine changes completely right.  For example, if you typed something, hit Tab to select a one-off, typed some more, and hit Return, the search would happen with the current engine, not the selected one-off.

The bottom line is that one-offs make the engineName included in the moz-action URI by UnifiedComplete pretty meaningless.  It can't be a static thing anymore.  So when a new one-off is selected, should we change the value of all searchengine results in the autocomplete controller, or the front end?  (If we don't change them at all, that's kind of basically the same as changing them in the front end since ultimately the URL that's loaded had better match the selected one-off.)

This commit does it in the front end.  The autocomplete popup now has an overrideSearchEngineName property that _adjustAcItem checks.  If the property is non-null, then _adjustAcItem changes its item's url attribute and the engine name in its label.

handleCommand in urlbar now uses the url attribute of the selected popup item when loading URLs.  If there's no selection, then it uses the input value as before.  At first I tried keeping input.value (or _value) up to date for the selected item, but that was a pain.  I think it's simpler to keep the url attributes of items updated and then use those.

But since handleCommand can be called when the popup is already closed, I modified the relevant idl to pass a selected popup index to it.

This commit also funnels all URL loads in urlbar through handleCommand.  And it splits out the continueOperation nested function into its own method called _loadURL.

Since handleEnter ends up calling handleCommand, and handle command needs an event to determine where to open the URL, I modified handleEnter to take an event.

I think these two idl changes make sense in general, not only for this bug.

For the value -> textValue changes in the tests, I made those changes when I was trying to keep input.value up to date.  I'm not sure they're necessary anymore, but they seem like good changes anyway?  Since textValue will always be what the user sees, but value may be a moz-action URI.

Review commit: https://reviewboard.mozilla.org/r/59540/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59540/

Comment 23

[Drew Willcoxon :adw]

Comment on attachment 8762257 [details]
Bug 1180944 - Implement one-off searches from Awesomebar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59074/diff/3-4/

Comment 24

[Drew Willcoxon :adw]

Well I switched from mq to bookmarks and of course I screwed up the review push.  I hate computers.  If you started reviewing the previous commit let me know and I'll try to untangle it from the new commit on Review Board.

Comment 25

[Drew Willcoxon :adw]

Review Board is the worst.  The worst Jerry.  This is the URL that shows the diff slider thing with all the review commits: https://reviewboard.mozilla.org/r/59072/diff/5#index_header

I have no idea how to ask for review for that series of commits as a whole.  Which is what I want to do.

Comment 26

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #25)
> I have no idea how to ask for review for that series of commits as a whole. 
> Which is what I want to do.

what I do usually is just having a series of commits with r=reviewer (even if it warns cause it can rewrite the reviewer on push, I just ignore it, it's dumb) and I just push the range of commits to mozreview (hg push -r c1:cN review). To update patches better to use the evolve extension. I never had a problem with that.
I don't use bookmarks, unless you use bookmarkbinder extension using bookmarks is not useful. I just use heads, one head per feature, use hg wip to check status and rebase when needed.

Comment 27

[Marco Bonardo [:mak]]

my workflow is not particularly advanced, but if you still have issues we can setup a vidyo session and go through it.

Comment 28

[Drew Willcoxon :adw]

Comment on attachment 8762257 [details]
Bug 1180944 - Implement one-off searches from Awesomebar

Clearing review for one of these, didn't mean to ask for both.  The second revision is just an iteration on the first one.

Also according to try it looks like I missed some native callers for the idl I changed, so I need to fix that.

Comment 29

[Drew Willcoxon :adw]

Try with the native implementor (not caller) fixed:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b258c0a59476

Comment 30

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32e7e99ed196

Comment 31

[Marco Bonardo [:mak]]

https://reviewboard.mozilla.org/r/59074/#review57042

it's huge, may take some review passes. So far I have doubts regarding some behaviors that would probably be better to clarify.

::: browser/base/content/test/urlbar/browser_autocomplete_autoselect.js:11
(Diff revision 4)
>  
>  function is_selected(index) {
>    is(gURLBar.popup.richlistbox.selectedIndex, index, `Item ${index + 1} should be selected`);
> +
> +  // This is true because although both the listbox and the one-offs can have
> +  // selections, the test doesn't check that.

I *think* I disagree about the fact both the listbox and one-off can have a selection... Why should be like that? it's bogus enough in the search bar where the treebody selection is completely ignored (so why it exists at all?)

::: browser/base/content/urlbarBindings.xml:448
(Diff revision 4)
> +        <parameter name="triggeringEvent"/>
> +        <parameter name="url"/>
> +        <parameter name="where"/>
> +        <parameter name="overrideParams"/>
> +        <body><![CDATA[
> +          let current = where == "current";

nit: I'm not sure I see the usefulness of this caching, it just seems to hide code for very small benefit. I'd just use where (!=)= "current" in the 3 conditions

::: browser/base/content/urlbarBindings.xml:461
(Diff revision 4)
> +              allowPinnedTabHostChange: true,
> +              allowPopups: url.startsWith("javascript:"),
> +            };
> +          } else {
> +            params = {
> +              allowThirdPartyFixup: true,

nit: since allowThirdPartyFixup is the same, you could compact the code a little bit:
let params = { allowThird... }
if (where == "current") {
  params.indicateError... = true;
  ...
} else {
  params.initiatingDoc = ...;
}

::: browser/base/content/urlbarBindings.xml:472
(Diff revision 4)
> +            for (let key in overrideParams) {
> +              params[key] = overrideParams[key];
> +            }
> +          }
> +
> +          let action = this._parseActionUrl(this.value);

I suppose you want to parse the action before an eventual handleRevert call, and that handleRevert is cause we are opening elsewhere...
But it would be nice if all of this would be in a comment

::: browser/base/content/urlbarBindings.xml:489
(Diff revision 4)
> +              this.handleRevert();
> +            }
> +          }
> +
> +          if (current) {
> +            // Ensure the start of the URL is visible for UX reasons:

nit: s/UX/usability/ and s/:/./

::: browser/base/content/urlbarBindings.xml:495
(Diff revision 4)
> +            this.selectionStart = this.selectionEnd = 0;
> +          }
> +
> +          // Record one-off search telemetry if appropriate.
> +          if (action && action.type == "searchengine") {
> +            this.popup.oneOffSearchButtons.recordTelemetry(triggeringEvent,

Does this work differently from the search bar?
Cause in the searchbar the default engine doesn't have a one-off button.
And "searchengine" actions are often coming from the default engine.
Moreover, "searchengine" action is also used by search suggestions, restyled searches (currently disabled), aliases, so looks like this ends up counting as oneoff some stuff that is not.

If the filtering happens later, I'd rather rename to maybeRecordTelemetry, but off-hand I'd prefer some filtering code to be here cause as-is it sounds error-prone.

::: browser/base/content/urlbarBindings.xml:1392
(Diff revision 4)
>            this.mInput = aInput;
>            this.selectedIndex = this._isFirstResultHeuristic ? 0 : -1;
>            this.view = aInput.controller.QueryInterface(Components.interfaces.nsITreeView);
> +          this.oneOffSearchButtons.popup = this;
> +          this.oneOffSearchButtons.textbox = this.input;
> +          this.oneOffSearchButtons.telemetryID = "urlbar";

nit: s/telemetryID/telemetryOrigin/

::: browser/base/content/urlbarBindings.xml:1619
(Diff revision 4)
> +              break;
> +            }
> +            let url = item.getAttribute("url");
> +            if (url) {
> +              let action = item._parseActionUrl(url);
> +              if (action && action.type == "searchengine") {

Hm, this means we may get search suggestions from google, but then pass them to another engine.
We never thought about it honestly, and I don't know if there are agreements regarding suggestions in the awesomebar.
On the other side, the search bar works differently, one-off buttons only act on the typed value, so off-hand looks like here at a maximum we'd need to update the currently selected row.
Even if I select an entry with the mouse, and directly move to a oneoff, I expect the oneoff to execute a search for the original text in the input field, not what I selected.

::: browser/base/content/urlbarBindings.xml:1635
(Diff revision 4)
> +          }
> +
> +          // The urlbar uses its _value property (via value) to determine where
> +          // to go when you press Return, so update it too.
> +          if (selectedNewURL) {
> +            this.input._value = selectedNewURL;

If I have a mouse selection and click on a one-off (with mouse acrobatics), I don't expect to go to the selected value... On the other side if I selected with the keyboard the value to search is already in the input field.

::: browser/base/content/urlbarBindings.xml:1646
(Diff revision 4)
> +           search buttons and between the one-offs and the listbox.  It returns
> +           true if the keypress was consumed and false if not. -->
> +      <method name="handleKeyPress">
> +        <parameter name="aEvent"/>
> +        <body><![CDATA[
> +          this.oneOffSearchButtons.handleKeyPress(aEvent, this._matchCount,

does this also cover the search settings gear? it should be reachable by keyboard.

::: toolkit/content/widgets/autocomplete.xml:1276
(Diff revision 4)
>              // so that we can use them from the constructor
>              let iconURI = controller.getImageAt(this._currentIndex);
>              item.setAttribute("image", iconURI);
>              item.setAttribute("url", url);
>              item.setAttribute("title", controller.getCommentAt(this._currentIndex));
> -            item.setAttribute("type", controller.getStyleAt(this._currentIndex));
> +            item.setAttribute("originaltype", controller.getStyleAt(this._currentIndex));

what's this change about?

Comment 32

[Drew Willcoxon :adw]

Thanks Marco!

(In reply to Marco Bonardo [::mak] from comment #31)
> ::: browser/base/content/test/urlbar/browser_autocomplete_autoselect.js:11
> (Diff revision 4)
> >  
> >  function is_selected(index) {
> >    is(gURLBar.popup.richlistbox.selectedIndex, index, `Item ${index + 1} should be selected`);
> > +
> > +  // This is true because although both the listbox and the one-offs can have
> > +  // selections, the test doesn't check that.
> 
> I *think* I disagree about the fact both the listbox and one-off can have a
> selection... Why should be like that? it's bogus enough in the search bar
> where the treebody selection is completely ignored (so why it exists at all?)

A couple points:

(1) It's useful for keyboard users.  You can have a selection the main list and then hit Tab or Alt+Up/Down to cycle through the one-offs.  And then you can hit Up/Down to change the main selection.

(2) In this bug I'd prefer to simply factor out the search.xml one-off code and use it in the awesomebar.  I'm completely fine with questioning how one-offs work but I think we should do that in follow-ups -- unless we want different behavior in the searchbar and awesomebar.  In that case then sure, let's decide what the differences for the awesomebar are here in this bug.  But I haven't heard Stephen or anyone mention any differences.

> > +          let current = where == "current";
> 
> nit: I'm not sure I see the usefulness of this caching, it just seems to
> hide code for very small benefit. I'd just use where (!=)= "current" in the
> 3 conditions

Fair enough, I usually do this kind of thing whenever a string is involved just to avoid typos within the string (not necessarily for the patch in question but more for maintenance), which would result in silent logic errors.

> ::: browser/base/content/urlbarBindings.xml:495
> (Diff revision 4)
> > +            this.selectionStart = this.selectionEnd = 0;
> > +          }
> > +
> > +          // Record one-off search telemetry if appropriate.
> > +          if (action && action.type == "searchengine") {
> > +            this.popup.oneOffSearchButtons.recordTelemetry(triggeringEvent,
> 
> Does this work differently from the search bar?
> Cause in the searchbar the default engine doesn't have a one-off button.

That's right, and it's the same here.

> And "searchengine" actions are often coming from the default engine.
> Moreover, "searchengine" action is also used by search suggestions, restyled
> searches (currently disabled), aliases, so looks like this ends up counting
> as oneoff some stuff that is not.

Sorry, I changed this in the newer commit (https://reviewboard.mozilla.org/r/59540/diff/#index_header).  I need to completely redo the review request so you're looking at "diff revisions" and not commits on Review Board.

> ::: browser/base/content/urlbarBindings.xml:1619
> (Diff revision 4)
> > +              break;
> > +            }
> > +            let url = item.getAttribute("url");
> > +            if (url) {
> > +              let action = item._parseActionUrl(url);
> > +              if (action && action.type == "searchengine") {
> 
> Hm, this means we may get search suggestions from google, but then pass them
> to another engine.
> We never thought about it honestly, and I don't know if there are agreements
> regarding suggestions in the awesomebar.

That's true.

> On the other side, the search bar works differently, one-off buttons only
> act on the typed value, so off-hand looks like here at a maximum we'd need
> to update the currently selected row.

I don't think this is true though.  Type something in the search bar, hit the Down key a few times.  The textbox's value is replaced with the newly selected row.  Then Tab to select a one-off, then Return.  You end up doing a search on the one-off engine but using the suggestion that came from your default engine.

But like you say, maybe there's some legal/contractual difference for the awesomebar?

> Even if I select an entry with the mouse, and directly move to a oneoff, I
> expect the oneoff to execute a search for the original text in the input
> field, not what I selected.

Hmm, I need to fix that.  That behavior is different from what the searchbar does.

> ::: browser/base/content/urlbarBindings.xml:1646
> (Diff revision 4)
> > +           search buttons and between the one-offs and the listbox.  It returns
> > +           true if the keypress was consumed and false if not. -->
> > +      <method name="handleKeyPress">
> > +        <parameter name="aEvent"/>
> > +        <body><![CDATA[
> > +          this.oneOffSearchButtons.handleKeyPress(aEvent, this._matchCount,
> 
> does this also cover the search settings gear? it should be reachable by
> keyboard.

Yes.

> ::: toolkit/content/widgets/autocomplete.xml:1276
> (Diff revision 4)
> >              // so that we can use them from the constructor
> >              let iconURI = controller.getImageAt(this._currentIndex);
> >              item.setAttribute("image", iconURI);
> >              item.setAttribute("url", url);
> >              item.setAttribute("title", controller.getCommentAt(this._currentIndex));
> > -            item.setAttribute("type", controller.getStyleAt(this._currentIndex));
> > +            item.setAttribute("originaltype", controller.getStyleAt(this._currentIndex));
> 
> what's this change about?

_adjustAcItem is now potentially called multiple times per list item, since the item's searchengine is updated when the selected one-off changes.  But _adjustAcItem changes the item's type attribute -- it parses the attribute, removes some types, concats the remaining types, and then sets the attribute to that new value.  So every time after the first time that _adjustAcItem is called, the type attribute is missing info that's necessary to set up the item.  So this new originaltype is specifically never changed.

Comment 33

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #32)
> > I *think* I disagree about the fact both the listbox and one-off can have a
> > selection... Why should be like that? it's bogus enough in the search bar
> > where the treebody selection is completely ignored (so why it exists at all?)
> 
> A couple points:
> 
> (1) It's useful for keyboard users.  You can have a selection the main list
> and then hit Tab or Alt+Up/Down to cycle through the one-offs.  And then you
> can hit Up/Down to change the main selection.

I still don't see the point of updating ALL entries  rather than just the selected one...

> (2) In this bug I'd prefer to simply factor out the search.xml one-off code
> and use it in the awesomebar.  I'm completely fine with questioning how
> one-offs work but I think we should do that in follow-ups

sure, that sounds fine. But the patch seems to add more complication than what the searchbar does by trying to keep all the search entries updated, rather than the currently selected one.
The bavior is definitely something that will require follow ups.

> Sorry, I changed this in the newer commit
> (https://reviewboard.mozilla.org/r/59540/diff/#index_header).  I need to
> completely redo the review request so you're looking at "diff revisions" and
> not commits on Review Board.

Hm, I guess I'm lost :D

> > ::: browser/base/content/urlbarBindings.xml:1619
> > On the other side, the search bar works differently, one-off buttons only
> > act on the typed value, so off-hand looks like here at a maximum we'd need
> > to update the currently selected row.
> 
> I don't think this is true though.  Type something in the search bar, hit
> the Down key a few times.  The textbox's value is replaced with the newly
> selected row.  Then Tab to select a one-off, then Return.  You end up doing
> a search on the one-off engine but using the suggestion that came from your
> default engine.

With tab that works, not with the mouse. It's a mess.

> > ::: toolkit/content/widgets/autocomplete.xml:1276
> So this new originaltype is specifically never changed.

For whatever reasons I couldn't find the point where originaltype was used.. maybe a problem with the review chageset. Now I saw it.
I wonder though if this may break other toolkit consumers, and we should rather make our own "parsedType". off-hand I don't think it's being used often in the wild, but if it's easy to do the opposite, maybe we should try.

Comment 34

[Florian Quèze [:florian]]

(In reply to Marco Bonardo [::mak] from comment #33)

> > > ::: browser/base/content/urlbarBindings.xml:1619
> > > On the other side, the search bar works differently, one-off buttons only
> > > act on the typed value, so off-hand looks like here at a maximum we'd need
> > > to update the currently selected row.
> > 
> > I don't think this is true though.  Type something in the search bar, hit
> > the Down key a few times.  The textbox's value is replaced with the newly
> > selected row.  Then Tab to select a one-off, then Return.  You end up doing
> > a search on the one-off engine but using the suggestion that came from your
> > default engine.
> 
> With tab that works, not with the mouse. It's a mess.

It doesn't work with the mouse because (unless you carefully avoid it) you have to hover suggestions before having the mouse above a one-off button to click it. The tab key skips the suggestions, so the only way to have a suggestion selected when using a one-off with tab is to have used both up/down and tab, ie. you are a keyboard user who knows what (s)he is doing.

Comment 35

[Florian Quèze [:florian]]

https://reviewboard.mozilla.org/r/59072/#review57664

This is a first review pass by code inspection only, I haven't tried the patch this time. Comments are mostly about details, the approach seems reasonable, and I haven't seen anything scary.

::: browser/base/content/test/urlbar/browser_urlbarOneOffs.js:206
(Diff revision 5)
> +
> +  gBrowser.removeTab(gBrowser.selectedTab);
> +});
> +
> +
> +function assertState(result, oneOff, textValue=undefined) {

nit: spaces around '='.

::: browser/base/content/urlbarBindings.xml:402
(Diff revision 5)
> +          let mayInheritPrincipal = false;
> +          let postData = null;
> +          let lastLocationChange = gBrowser.selectedBrowser.lastLocationChange;
>            let matchLastLocationChange = true;
> +
> +let action = this._parseActionUrl(url);

Please fix the indent here.

::: browser/components/search/content/search.xml:376
(Diff revision 5)
> +        <body><![CDATA[
> +          var textBox = this._textbox;
> +          var textValue = textBox.value;
> +
>            let selection = this.telemetrySearchDetails;
> -          this.doSearch(textValue, where, aEngine);
> +          this.doSearch(textValue, aWhere, aEngine);

Did you mean to also pass aParams here?

::: browser/components/search/content/search.xml
(Diff revision 5)
> -              this.selectedButton = null;
> -
> -            if (this.selectedButton || aCycleEngines || suggestionsHidden)
> -              return true;
> -
> -            // Set the selectedIndex to something that will make

Why is this code block no longer needed?

::: browser/components/search/content/search.xml:840
(Diff revision 5)
>        })]]></field>
>      </implementation>
>  
>      <handlers>
> +      <handler event="input"><![CDATA[
> +        if (!this.value) {

Do we need this test, and why is it reverted compared to the if (textbox.value) self.removeAttribute("showonlysettings"); we had before?

::: browser/components/search/content/search.xml:1044
(Diff revision 5)
> -            this.selectedButton &&
> -            this.selectedButton.classList.contains("searchbar-engine-one-off-item");
> -          // Typing de-selects the settings or opensearch buttons at the bottom
> -          // of the search panel, as typing shows the user intends to search.
> -          if (this.selectedButton && !isOneOffSelected)
> -            this.selectedButton = null;
> +    </content>
> +  </binding>
> +
> +
> +  <binding id="search-one-offs">
> +

nit: the empty line here doesn't help readability and is inconsistent with the other bindings in the file.

::: browser/components/search/content/search.xml:1097
(Diff revision 5)
> +    </content>
> +
> +    <implementation implements="nsIDOMEventListener">
> +
> +      <!-- Width in pixels of the one-off buttons. -->
> +      <property name="buttonWidth" readonly="true">

Looks like you can use the shorter onget="return 49;" syntax here, and merge the 2 comments.

::: browser/components/search/content/search.xml:1141
(Diff revision 5)
> +            }
> +          }
> +          return val;
> +        ]]></setter>
> +      </property>
> +      <field name="_popup"><![CDATA[

You don't need the CDATA tag here, it can fit on one line:
<field name="_popup">null</field>
This comment also applies to the next few fields.

Also, I tend to find it more readable when the _ prefixed field is right before the property, rather than after it, but it may just be my personal preference.

::: browser/components/search/content/search.xml:1145
(Diff revision 5)
> +      </property>
> +      <field name="_popup"><![CDATA[
> +        null
> +      ]]></field>
> +
> +      <!-- The textbox associated with the one-offs.  May be null/undefined, and

It seems this is set for both the urlbar and the searchbar, so what's the case in which it is null?

::: browser/components/search/content/search.xml:1146
(Diff revision 5)
> +      <field name="_popup"><![CDATA[
> +        null
> +      ]]></field>
> +
> +      <!-- The textbox associated with the one-offs.  May be null/undefined, and
> +           in that case you should update the query property manually. -->



::: browser/components/search/content/search.xml:1152
(Diff revision 5)
> +      <property name="textbox">
> +        <getter><![CDATA[
> +          return this._textbox;
> +        ]]></getter>
> +        <setter><![CDATA[
> +          if (this._textbox != val) {

nit: I would prefer an early return instead of indenting the whole content of the setter (also applies to the "popup" setter).

::: browser/components/search/content/search.xml:1242
(Diff revision 5)
> +        ]]></getter>
> +      </property>
> +
> +      <!-- The number of one-offs, including the add-engine button (if shown)
> +           and the search-settings button. -->
> +      <property name="numButtons" readonly="true">

I don't really see the point of exposing this, when it seemed to only be used by tests, and given that getSelectableButtons is exposed.

::: browser/components/search/content/search.xml:1289
(Diff revision 5)
> +      <!-- This handles events outside the one-off buttons, like on the popup
> +           and textbox. -->
> +      <method name="handleEvent">
> +        <parameter name="event"/>
> +        <body><![CDATA[
> +        switch (event.type) {

nit: I think everywhere else in the file the content of the body is indented.

::: browser/components/search/content/search.xml:1522
(Diff revision 5)
> +          this.dispatchEvent(event);
> +        ]]></body>
> +      </method>
> +
> +      <method name="getSelectableButtons">
> +        <parameter name="aIncludeSettingsButton"/>

This parameter name is misleading: the add-engine open search items aren't the "Settings" button.

::: browser/components/search/content/search.xml:1547
(Diff revision 5)
> +                                                               "search-settings"));
> +          return buttons;
> +        ]]></body>
> +      </method>
> +
> +      <method name="handleSearchCommand">

You have this method that's an almost duplicate of the method with the same name on the other binding. Copy/paste accident?

::: browser/components/search/content/search.xml:1602
(Diff revision 5)
> +               index will cause the last one-off to become selected and this
> +               method to return true.  Only the aForward=false case is affected
> +               because it is always the case that if aForward=true and there
> +               currently is no selection, the first one-off becomes selected and
> +               this method returns true.
> +        @param aIncludeSettingsButton

This was called aCycleEngine, because it was really about cycling through the list of one-off engines (using alt+up/down), excluding everything else, not just the "settings" button.

::: browser/components/search/content/search.xml:1638
(Diff revision 5)
> +              return true;
> +
> +            return false;
> +          }
> +
> +          if (!selectedButton) {

This test will always pass, won't it?

::: browser/components/search/content/search.xml:1721
(Diff revision 5)
> +                  stopEvent = this.advanceSelection(false, true, true);
> +                  if (stopEvent) {
> +                    this.popup.selectedIndex = -1;
> +                  }
> +                }
> +              } else if (this.firstButtonSelected(true)) {

This is the only call for "firstButtonSelected", so maybe this should be inlined?
If not, it should probably be _ prefixed.

::: browser/components/search/content/search.xml:1748
(Diff revision 5)
> +                  }
> +                  if (!allowEmptySelection) {
> +                    this.popup.selectedIndex = -1;
> +                  }
> +                }
> +              } else if (this.lastButtonSelected(true)) {

Same here.

::: browser/components/search/content/search.xml:1939
(Diff revision 5)
>          // require manual hiding.
> -        this.hidePopup();
> +        this.popup.hidePopup();
>  
> -        let searchbar = document.getElementById("searchbar");
> -        searchbar.handleSearchCommand(event, engine);
> +        // Make sure the clicked button is selected.  In practice this should
> +        // always be the case since you have to mouse over the button to click
> +        // it, and the mouseover handler selects the button.  So mostly this is

Isn't the mouseover handler only changing the visible selection without changing the actual selection?

::: browser/components/search/content/search.xml:1949
(Diff revision 5)
>        ]]></handler>
>  
>        <handler event="command"><![CDATA[
>          let target = event.originalTarget;
>          if (target.classList.contains("addengine-item")) {
> -          // On success, hide and reshow the panel to show the new engine.
> +          // On success, hide the panel and tell event listeners to reshow it to

It seems you have now figured out how to update the panel while it's already open, so can we change this to just be a call to your new _rebuild method?

::: browser/components/search/content/search.xml:1955
(Diff revision 5)
> +          // show the new engine.
>            let installCallback = {
> -            onSuccess: function(engine) {
> -              event.target.hidePopup();
> -              BrowserSearch.searchBar.openSuggestionsPanel();
> +            onSuccess: engine => {
> +              this.popup.hidePopup();
> +              let event = document.createEvent("Events");
> +              event.initEvent("OneOffsEngineInstalled", true, false);



::: toolkit/components/autocomplete/nsIAutoCompleteController.idl:76
(Diff revision 5)
> +   *        the user pressed the Return key or a click event if the user clicked
> +   *        a popup item.
>     * @return True if the controller wishes to prevent event propagation and default event
>     */
> -  boolean handleEnter(in boolean aIsPopupSelection);
> +  boolean handleEnter(in boolean aIsPopupSelection,
> +                      in nsIDOMEvent aEvent);

What about tagging this as [optional] in the interface so that you don't have to pass null parameters in all existing callers?

Comment 36

[Florian Quèze [:florian]]

Comment on attachment 8763297 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

(In reply to Florian Quèze [:florian] [:flo] from comment #35)

Please ignore these 2 empty comments, for some reason mozreview didn't let me delete them:

> ::: browser/components/search/content/search.xml:1146
> (Diff revision 5)
> > +      <field name="_popup"><![CDATA[
> > +        null
> > +      ]]></field>
> > +
> > +      <!-- The textbox associated with the one-offs.  May be null/undefined, and
> > +           in that case you should update the query property manually. -->
> 
> 
> 

> ::: browser/components/search/content/search.xml:1955
> (Diff revision 5)
> > +          // show the new engine.
> >            let installCallback = {
> > -            onSuccess: function(engine) {
> > -              event.target.hidePopup();
> > -              BrowserSearch.searchBar.openSuggestionsPanel();
> > +            onSuccess: engine => {
> > +              this.popup.hidePopup();
> > +              let event = document.createEvent("Events");
> > +              event.initEvent("OneOffsEngineInstalled", true, false);
> 
> 
>

Comment 37

[Drew Willcoxon :adw]

Comment on attachment 8762257 [details]
Bug 1180944 - Implement one-off searches from Awesomebar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59074/diff/4-5/

Comment 38

[Drew Willcoxon :adw]

Comment on attachment 8762257 [details]
Bug 1180944 - Implement one-off searches from Awesomebar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59074/diff/5-6/

Comment 39

[Drew Willcoxon :adw]

Comment on attachment 8762257 [details]
Bug 1180944 - Implement one-off searches from Awesomebar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59074/diff/6-7/

Comment 40

[Drew Willcoxon :adw]

Comment on attachment 8762257 [details]
Bug 1180944 - Implement one-off searches from Awesomebar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59074/diff/7-8/

Comment 41

[Drew Willcoxon :adw]

Attachment: Bug 1180944 - Implement one-off searches from Awesomebar.

Review commit: https://reviewboard.mozilla.org/r/61768/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61768/

Comment 42

[Drew Willcoxon :adw]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61768/diff/1-2/

Comment 43

[Drew Willcoxon :adw]

Very sorry for the bugspam.  This review request has two diffs.  The first is the entire diff of the previous review requests/commits that I botched.  The second is the changes I made to address your comments.

Marco, you probably want to look at both diffs since the part I screwed up last time was mostly code for your review.  The first diff will contain some changes you already looked at.

Florian, you should be able to look at only the second diff I think, since the part I screwed up last time had only a small amount of code for you to look at, and I think you looked at it but I'm not sure.

Comment 44

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] from comment #33)
> I still don't see the point of updating ALL entries  rather than just the
> selected one...

It makes the code simpler I think.  Otherwise we'd need to update the selected entry in two places: (1) when the selected one-off changes, and (2) when the selected entry changes.  But let me know what you think after looking at the latest patches.

Comment 45

[Drew Willcoxon :adw]

(In reply to Florian Quèze [:florian] [:flo] from comment #35)
> ::: browser/components/search/content/search.xml
> (Diff revision 5)
> > -              this.selectedButton = null;
> > -
> > -            if (this.selectedButton || aCycleEngines || suggestionsHidden)
> > -              return true;
> > -
> > -            // Set the selectedIndex to something that will make
> 
> Why is this code block no longer needed?

I don't understand what that block is doing.  Do you think it's really necessary anymore?  Also, the urlbar and searchbar have different requirements for selected popup items: the urlbar should always have a selection (unless it's showing the history popup), so setting `popup.selectedIndex = popup.view.rowCount` isn't ever right for it.

> ::: browser/components/search/content/search.xml:840
> (Diff revision 5)
> >        })]]></field>
> >      </implementation>
> >  
> >      <handlers>
> > +      <handler event="input"><![CDATA[
> > +        if (!this.value) {
> 
> Do we need this test, and why is it reverted compared to the if
> (textbox.value) self.removeAttribute("showonlysettings"); we had before?

Hmm, that looks like an error.  I removed the test.

> ::: browser/components/search/content/search.xml:1145
> (Diff revision 5)
> > +      </property>
> > +      <field name="_popup"><![CDATA[
> > +        null
> > +      ]]></field>
> > +
> > +      <!-- The textbox associated with the one-offs.  May be null/undefined, and
> 
> It seems this is set for both the urlbar and the searchbar, so what's the
> case in which it is null?

It is, the comment is just saying "you can leave this null if you want".  It turned out that factoring out the one-off code meant that it's no longer tightly coupled to a textbox, and hypothetically we could use it without a textbox.  That seems interesting and maybe useful down the line, but I can remove the comment altogether if you want.  I did update it to be more clear hopefully.

> ::: browser/components/search/content/search.xml:1547
> (Diff revision 5)
> > +                                                               "search-settings"));
> > +          return buttons;
> > +        ]]></body>
> > +      </method>
> > +
> > +      <method name="handleSearchCommand">
> 
> You have this method that's an almost duplicate of the method with the same
> name on the other binding. Copy/paste accident?

Not an accident, they're different.  One is on the searchbar binding.  It's called when you click the search arrow button and hit Return in the textbox.  The other is on the one-offs binding and is called when you interact with the one-offs.  Neither one depends on the other, so I don't think there's an opportunity to combine them into one method or something like that.

> ::: browser/components/search/content/search.xml:1949
> (Diff revision 5)
> >        ]]></handler>
> >  
> >        <handler event="command"><![CDATA[
> >          let target = event.originalTarget;
> >          if (target.classList.contains("addengine-item")) {
> > -          // On success, hide and reshow the panel to show the new engine.
> > +          // On success, hide the panel and tell event listeners to reshow it to
> 
> It seems you have now figured out how to update the panel while it's already
> open, so can we change this to just be a call to your new _rebuild method?

Great idea.

> ::: toolkit/components/autocomplete/nsIAutoCompleteController.idl:76
> (Diff revision 5)
> > +   *        the user pressed the Return key or a click event if the user clicked
> > +   *        a popup item.
> >     * @return True if the controller wishes to prevent event propagation and default event
> >     */
> > -  boolean handleEnter(in boolean aIsPopupSelection);
> > +  boolean handleEnter(in boolean aIsPopupSelection,
> > +                      in nsIDOMEvent aEvent);
> 
> What about tagging this as [optional] in the interface so that you don't
> have to pass null parameters in all existing callers?

Ah yeah, of course!

Comment 46

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27dfcce3ae80

Comment 47

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4fd3b69be23

Comment 48

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f4eefa11d41

Comment 49

[Drew Willcoxon :adw]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61768/diff/2-3/

Comment 50

[Drew Willcoxon :adw]

Fix for browser_urlbarSearchTelemetry.js failure on Windows 7 VM debug.

Comment 51

[Marco Bonardo [:mak]]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

https://reviewboard.mozilla.org/r/61768/#review59394

In general it seems to work quite well, good work!

Some issues I found while testing:
1. I seem to be unable to open Search settings with keyword
2. The text when no one-off button is selecte reads as "Search for <string> with: ".... and nothing is selected since the current engine does not appear in the list... Quite unclear to me.
3. related to 1. when change search settings is selected, the string still reads as "search for with:"... and indeed it's what happens, but it should open search settings instead
4. This needs Stephen's feedback, and could also happen later ina follow-up. I honestly think the "Change search settings" button should be an icon and not take so much space. It would look infinitely better.
5. If I select with the keyboard a switch to tab entry and then click on an engine, nothing happens. Though looks like if later I select an url and click on an engine, we load the switch entry, or something like that
6. If I select an uri entry and click on a one off, we load the url. Could even make sense to not search for a url, but it's strange to confirm the url by clicking on a search engine icon... Maybe the click should be ignored completely? if the entry is not something we can search?

::: browser/base/content/test/urlbar/browser_bug1070778.js:46
(Diff revision 3)
>    is_selected(0);
>  
>    EventUtils.synthesizeKey("b", {});
>    yield promiseSearchComplete();
>  
> -  is(gURLBar.value, "keyword ab", "urlbar should have expected input");
> +  is(gURLBar.textValue, "keyword ab", "urlbar should have expected input");

Could you please explain the reason for this change (I'm not saying it's wrong, just trying to understand since we have far too many "value" fields)

::: browser/base/content/urlbarBindings.xml:375
(Diff revision 3)
> +                         event.altKey &&
> +                         !isTabEmpty(gBrowser.selectedTab);
> +          if (isMouseEvent || altEnter) {
> +            // Use the standard UI link behaviors for clicks or Alt+Enter
> +            where = isMouseEvent ? whereToOpenLink(event, false, false) : "tab";
> +          }

it may be readable as:

if (isMouseEvent) {
  where = whereToOpenL...
} else if (altEnter) {
  where = "tab";
}

::: browser/base/content/urlbarBindings.xml:543
(Diff revision 3)
> +        <parameter name="openUILinkParams"/>
>          <body><![CDATA[
>            let engine =
> -            Services.search.getEngineByName(action.params.engineName);
> +            typeof(engineOrEngineName) == "string" ?
> +            Services.search.getEngineByName(engineOrEngineName) :
> +            engineOrEngineName;

nit: please indent more the options

::: toolkit/components/autocomplete/nsIAutoCompleteController.idl:10
(Diff revision 3)
>  #include "nsISupports.idl"
>  
>  interface nsIAutoCompleteInput;
> +interface nsIDOMEvent;
>  
> -[scriptable, uuid(ff9f8465-204a-47a6-b3c9-0628b3856684)]
> +[scriptable, uuid(c6dcd364-99a8-48a9-9611-77e6a18ac9f3)]

nit: it's no more needed to rev the uuid.

::: toolkit/components/autocomplete/nsIAutoCompleteInput.idl:131
(Diff revision 3)
>     *
> +   * @param aEvent
> +   *        The event that triggered the enter.  Will be null if it's unknown.
>     * @return True if the user wishes to prevent the enter
>     */
> -  boolean onTextEntered();
> +  boolean onTextEntered(in nsIDOMEvent aEvent);

why not [optional]?

Comment 52

[Drew Willcoxon :adw]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61768/diff/3-4/

Comment 53

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] from comment #51)
> 1. I seem to be unable to open Search settings with keyword

That's fixed with the latest patch.

> 2. The text when no one-off button is selecte reads as "Search for <string>
> with: ".... and nothing is selected since the current engine does not appear
> in the list... Quite unclear to me.

I kind of agree, but that's the current behavior in the searchbar.  This seems more like a question for UX that we should follow up on later.

> 3. related to 1. when change search settings is selected, the string still
> reads as "search for with:"...

That's also the current behavior in the searchbar.  The "search for with:" text is telling you that you can search for your query in one of the following engines.  So it's not wrong, even in this case where the settings button is selected, but as I said I kind of agree that it might be confusing.

> 4. This needs Stephen's feedback, and could also happen later ina follow-up.
> I honestly think the "Change search settings" button should be an icon and
> not take so much space. It would look infinitely better.

Yes, that's what his spec calls for in the urlbar.  I need to ask him whether we should change that in the searchbar too.  I hope so.  I agree that it can happen in a follow-up.

> 5. If I select with the keyboard a switch to tab entry and then click on an
> engine, nothing happens. Though looks like if later I select an url and
> click on an engine, we load the switch entry, or something like that

Hmm, I think maybe the one-offs should be disabled when you select a switch-to-tab entry.  Another question for UX.

But, I can't reproduce this.  Maybe something in the latest patch fixed it?  When I click a one-off after selecting a switch-to-tab with the keyboard, the switch-to-tab tab is selected.  Which seems like the right behavior if we're not disabling one-offs.

> 6. If I select an uri entry and click on a one off, we load the url. Could
> even make sense to not search for a url, but it's strange to confirm the url
> by clicking on a search engine icon... Maybe the click should be ignored
> completely? if the entry is not something we can search?

Yeah, this is a similar question to the switch-to-tab case.  Maybe the one-offs should just be disabled for all non-searchengine results.

> ::: browser/base/content/test/urlbar/browser_bug1070778.js:46
> (Diff revision 3)
> >    is_selected(0);
> >  
> >    EventUtils.synthesizeKey("b", {});
> >    yield promiseSearchComplete();
> >  
> > -  is(gURLBar.value, "keyword ab", "urlbar should have expected input");
> > +  is(gURLBar.textValue, "keyword ab", "urlbar should have expected input");
> 
> Could you please explain the reason for this change (I'm not saying it's
> wrong, just trying to understand since we have far too many "value" fields)

At one point I was overriding urlbar.value with an action URI containing the selected one-off, so that when you press Return, the one-off engine is used.  Which broke all these tests that assumed that the visual value (textValue) is a simple string, not an action URI.  I'm not doing that anymore, so I think maybe I can revert all these changes.

I can do a try run with those changes reverted, but IMO these tests should be checking textValue, not value.  In the future we may want value to always be an action URI for example.  What do you think?

> ::: toolkit/components/autocomplete/nsIAutoCompleteInput.idl:131
> (Diff revision 3)
> >     *
> > +   * @param aEvent
> > +   *        The event that triggered the enter.  Will be null if it's unknown.
> >     * @return True if the user wishes to prevent the enter
> >     */
> > -  boolean onTextEntered();
> > +  boolean onTextEntered(in nsIDOMEvent aEvent);
> 
> why not [optional]?

It seems like it shouldn't be optional to me, even though in some cases it might not be known.  I think ideally it would always be known, if we were to go back and look at all the callers and modify them (and their callers) so that the triggering event is always passed around.

But it's OK with me if you want to make it optional, let me know.

Comment 54

[Drew Willcoxon :adw]

Stephen, some questions for you:

When no one-off is selected, the label above the one-offs reads, "Search for <string> with:".  Marco thinks that's unclear since nothing is selected and the current engine doesn't appear in the list.  What do you think?

When the search settings button is selected, the label still reads "Search for <string> with:", which Marco thinks is also confusing.

Your spec shows a gear icon for the search settings button in the urlbar.  Should that be used in the searchbar too?  http://people.mozilla.org/~shorlander/mockups/AwesomeResults/AwesomeResults-01.html

Your spec also shows the one-off icons in grayscale.  Is that meaningful?  What should we do?

I'm thinking that when a non-search-engine/non-search-suggestion result is selected in the urlbar, like a URL or switch to tab, the one-offs should all be disabled since it doesn't make sense to use them in that case.  It's different from the searchbar where only search suggestions are shown, so the one-offs always make sense.   What do you think?

Comment 55

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw (Away 7/11–7/15) from comment #53)
> I can do a try run with those changes reverted, but IMO these tests should
> be checking textValue, not value.  In the future we may want value to always
> be an action URI for example.  What do you think?

Ok, I guess.

> > ::: toolkit/components/autocomplete/nsIAutoCompleteInput.idl:131
> > (Diff revision 3)
> > >     *
> > > +   * @param aEvent
> > > +   *        The event that triggered the enter.  Will be null if it's unknown.
> > >     * @return True if the user wishes to prevent the enter
> > >     */
> > > -  boolean onTextEntered();
> > > +  boolean onTextEntered(in nsIDOMEvent aEvent);
> > 
> > why not [optional]?
> 
> It seems like it shouldn't be optional to me, even though in some cases it
> might not be known.  I think ideally it would always be known, if we were to
> go back and look at all the callers and modify them (and their callers) so
> that the triggering event is always passed around.

It just seems uncoherent with handleEnter, where it's optional, while this is not. I think those should be coherent, either we need it or we don't.

Comment 56

[Marco Bonardo [:mak]]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

https://reviewboard.mozilla.org/r/61768/#review59982

It looks better. The issues I found don't really break the experience, so we can probably land sooner for testing purposes. Most of those UX problems are also things we can work separately.
That said, I certainly don't want to ship with the Change Search Settings button taking so much space, so that should take some priority, imo.

Comment 57

[Marco Bonardo [:mak]]

To be clear, I didn't look at the search.xml changes, since I don't want to overlap with Florian's review.

Comment 58

[Florian Quèze [:florian]]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

https://reviewboard.mozilla.org/r/61768/#review59986

I didn't do a full review of the code this time, I looked mostly at the interdiff, and tested the patch. Interacting with it is nice, great job! :-)

Things I noticed while testing the patch:

- if I type 'blah' in the urlbar and a completion to 'blah.com/' (with '.com/' selected) appears in the text field, at the bottom of the panel I see "Search for blah.com/ with:" and if I click a one-off button, Firefox loads blah.com instead of querying the engine I clicked. We should search for what the user typed and ignore the completion suggested in the textfield.

- The 'Change Search Settings' button is taking way too much space in the awesomebar panel (which typically has a much larger width than the searchbar panel). Additionally, the sizing of the one-off button has never been perfect and there's an empty area on the right side, of at most (n-1)px (where n is the number of buttons we could fit on the line). On the searchbar panel it's barely visible, but on the awesomebar panel that is much larger, the empty area at the end of the row can be quite noticeable, and in most cases there'll be only one row of one-off buttons. So I think for the awesomebar panel it would make sense to put a settings icon at the end of the last row of one-off engines, and make that button use up all the space we couldn't allocate to normal-sized buttons. For the searchbar panel, where searching, and so search settings, is much more important, I think we should keep the whole button text ('Change Search Settings') visible.

- Should we show opensearch items in the awesomebar panel? It's taking a lot of space for not much value I think, as the green (+) badge is only displayed on the searchbox anyway. When a webpage offers lots of search plugins (try http://de.pons.com/), the layout of the panel explodes (known bug 1113747). It's ugly for the searchbar but not too bad. A website breaking/hiding the urlbar seem a much more serious problem to me, possibly a release blocker.

- Rebuilding the panel immediately when installing a search plugin is nice :-). An unfortunate detail is that we seem to fire the notification that an engine has been added a bit too early, before the icon is available, and we end up showing a generic placeholder icon instead of the new engine's icon. (this can be addressed in a follow-up, that I'm willing to take, as it's likely a search service bug).

r+ as I could be convinced most if not all of the above points can be follow-ups, as long as we get to them before shipping.

::: browser/components/search/content/search.xml:1097
(Diff revision 4)
> +      <property name="popup">
> +        <getter><![CDATA[
> +          return this._popup;
> +        ]]></getter>
> +        <setter><![CDATA[
> +          if (this._popup != val) {

I see you changed the textbox setter with an early return like I suggested; it seems you missed the popup setter.

Comment 59

[Stephen Horlander [:shorlander]]

(In reply to Drew Willcoxon :adw (Away 7/11–7/15) from comment #54)
> When no one-off is selected, the label above the one-offs reads, "Search for
> <string> with:".  Marco thinks that's unclear since nothing is selected and
> the current engine doesn't appear in the list.  What do you think?

The intent is for that label to be a hint that you can search for your current search string with one of the one-off buttons.

I agree it could be confusing in the context of all of the other things that can happen in the awesome bar results. Can we land it as it currently is and reevaluate after we have had a chance to use it for a while?


> When the search settings button is selected, the label still reads "Search
> for <string> with:", which Marco thinks is also confusing.

Can we append the sting with the name of the search engine when it is selected? E.g.

"Search for <sting> with: Wikipedia"?


> Your spec shows a gear icon for the search settings button in the urlbar. 
> Should that be used in the searchbar too? 
> http://people.mozilla.org/~shorlander/mockups/AwesomeResults/AwesomeResults-
> 01.html

I don't think so. We could evaluate whether or not the footer in the searchbar could be revised. But probably not in this bug.


> Your spec also shows the one-off icons in grayscale.  Is that meaningful? 
> What should we do?

That was mostly as a means to de-emphasize them. But it's probably not necessary. We can use the full color versions.


> I'm thinking that when a non-search-engine/non-search-suggestion result is
> selected in the urlbar, like a URL or switch to tab, the one-offs should all
> be disabled since it doesn't make sense to use them in that case.  It's
> different from the searchbar where only search suggestions are shown, so the
> one-offs always make sense.   What do you think?

I don't think I am clear about what you are proposing.  The one-off buttons always search the string you type, when would the type of result be a factor?

Comment 60

[Stephen Horlander [:shorlander]]

Attachment: One-Off Search Display Problem - 01

(In reply to Stephen Horlander [:shorlander] from comment #59)
> (In reply to Drew Willcoxon :adw (Away 7/11–7/15) from comment #54)
> > I'm thinking that when a non-search-engine/non-search-suggestion result is
> > selected in the urlbar, like a URL or switch to tab, the one-offs should all
> > be disabled since it doesn't make sense to use them in that case.  It's
> > different from the searchbar where only search suggestions are shown, so the
> > one-offs always make sense.   What do you think?
> 
> I don't think I am clear about what you are proposing.  The one-off buttons
> always search the string you type, when would the type of result be a factor?

I think I figured out my confusion. Right, I agree the one-off buttons should be disabled with switch-to-tab items or direct navigation items (URLs).

But back to my confusion :)

There is a display problem when you are using this with your mouse instead of your keyboard. The problem, and this is also a problem in the SearchBar, is that if you are using your mouse to select things it's possible to have more than one selection. Or that's the way it appears.

In that screenshot what it looks like it is going to do, and in fact if you scrub the tiles what it says it will do, is search that selection with the hovered search engine. But that is not what actually happens. It instead searches what you have selected with the keyboard.

Actually I think this is really a result of bug 1231393. We need a separate selected state and a hover state. The selected state attributes (e.g. – Search on <Engine>) should not move with mouse hover.

Comment 61

[Drew Willcoxon :adw]

Marco, Florian, I'm off next week so I think it would be wise to land this when I get back so I can jump on any follow-up problems.  But if you want to land it ASAP to have more time to test it, that's OK with me.  Although I'm also still working on incorporating the latest round of comments, including Stephen's.

Comment 62

[Florian Quèze [:florian]]

(In reply to Stephen Horlander [:shorlander] from comment #59)

> > When the search settings button is selected, the label still reads "Search
> > for <string> with:", which Marco thinks is also confusing.
> 
> Can we append the sting with the name of the search engine when it is
> selected? E.g.
> 
> "Search for <sting> with: Wikipedia"?

When a one-off button is selected, we show "Search <engine name>". Of course, this string was chosen for the searchbar where we have less space. In the awesomebar there would be enough space for a full "Search for <string> with <engine>", but unless it's actually confusing, preserving a consistent behavior seems easier.

Comment 63

[Marco Bonardo [:mak]]

What confused me is that this text is used:
1. as a label hinting to the user what he could do, when no one-off is selected
2. as a status text saying what is about to happen, when one-off is selected

I was always interpreting it as a status, and as such "Search for <string> with:", followed by no selection, didn't make any sense. Now that I figured it's trying to be 2 things at once, I understand what's up.
I still think it's confusing both here and in the searchbar, but likely not a blocker since it's already there.

Comment 64

[Drew Willcoxon :adw]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61768/diff/4-5/

Comment 65

[Drew Willcoxon :adw]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

Marco, could you please look at this small urlbarBindings change?  It's to address one of Florian's comments that I'll comment on soon.

Florian, could you please look at the other changes?

Comment 66

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] from comment #55)
> It just seems uncoherent with handleEnter, where it's optional, while this
> is not. I think those should be coherent, either we need it or we don't.

I made this one [optional] too.

Comment 67

[Drew Willcoxon :adw]

(In reply to Florian Quèze [:florian] [:flo] from comment #58)
> - if I type 'blah' in the urlbar and a completion to 'blah.com/' (with
> '.com/' selected) appears in the text field, at the bottom of the panel I
> see "Search for blah.com/ with:" and if I click a one-off button, Firefox
> loads blah.com instead of querying the engine I clicked. We should search
> for what the user typed and ignore the completion suggested in the textfield.

Should be fixed now, by the urlbarBindings.xml change.

> - The 'Change Search Settings' button is taking way too much space

Latest patch uses a gear icon for the settings button in the urlbar panel, as is shown in Stephen's spec, so this is fixed.

I added a new attribute/property to the one-offs binding called "compact" that controls this.  It also changes some other visual design slightly, to match Stephen's spec, by removing the vertical lines on the right edge of dummy one-offs.

> - Should we show opensearch items in the awesomebar panel?

OK, I skip the addengine-items entirely when compact=true.  I think maybe we should ask Stephen for a redesign for these buttons in the urlbar and come back to it in a follow-up.

> - Rebuilding the panel immediately when installing a search plugin is nice
> :-). An unfortunate detail is that we seem to fire the notification that an
> engine has been added a bit too early, before the icon is available, and we
> end up showing a generic placeholder icon instead of the new engine's icon.
> (this can be addressed in a follow-up, that I'm willing to take, as it's
> likely a search service bug).

Left for a follow-up.

> ::: browser/components/search/content/search.xml:1097
> (Diff revision 4)
> > +      <property name="popup">
> > +        <getter><![CDATA[
> > +          return this._popup;
> > +        ]]></getter>
> > +        <setter><![CDATA[
> > +          if (this._popup != val) {
> 
> I see you changed the textbox setter with an early return like I suggested;
> it seems you missed the popup setter.

Fixed.

Comment 68

[Drew Willcoxon :adw]

(In reply to Stephen Horlander [:shorlander] from comment #60)
> I think I figured out my confusion. Right, I agree the one-off buttons
> should be disabled with switch-to-tab items or direct navigation items
> (URLs).
> 
> But back to my confusion :)
> 
> There is a display problem when you are using this with your mouse instead
> of your keyboard.

I left these two issues for follow-ups.

Comment 69

[Marco Bonardo [:mak]]

we should be evaluating delaying the landing until the next merge. There are a couple reason:
1. not enough time for handling regressions/follow-ups
2. give more time to the shield study extension using the old approach

Comment 70

[Florian Quèze [:florian]]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

https://reviewboard.mozilla.org/r/61768/#review63226

r- because the layout breaks with a small window and a long-ish list of engines, see http://i.imgur.com/cJWAgE9.png

When hovering/selecting the settings button from the awesomebar panel, it's odd that it doesn't have the same color as when hovering/selecting other one-off buttons.

If I was implementing this, the first idea I would try would be to make the settings button a one-off button (I would try just moving the DOM node of the existing settings button, so that we don't have to hide it).

My comment below on the linux searchbar.css file applies to all the 3 versions of this file.

::: browser/components/search/content/search.xml:1438
(Diff revisions 4 - 5)
>            let rowCount = Math.ceil(engines.length / enginesPerRow);
>            let height = rowCount * 33; // 32px per row, 1px border.
> -          list.setAttribute("height", height + "px");
> +          height += "px";
> +          list.style.height = height;
> +          list.style.minHeight = height;
> +          list.parentNode.style.height = height;

This deserves a comment to explain why we need to set this on both the list and its parent... if we can't find a cleaner solution.

::: browser/themes/linux/searchbar.css
(Diff revision 5)
>    height: 32px;
>    margin: 0 0;
>    padding: 0 0;
>    background: none;
> -  background-image: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAAWCAYAAAABxvaqAAAABmJLR0QA/wD/AP+gvaeTAAAAFElEQVQI12NgYGAwZmJgYGCgBwEAHpIAXs+dj/cAAAAASUVORK5CYII=');
> -  background-repeat: no-repeat;

I don't undersatnd why you are moving this to a new rule with identical selectors.

Comment 71

[Drew Willcoxon :adw]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61768/diff/5-6/

Comment 72

[Drew Willcoxon :adw]

(In reply to Florian Quèze [:florian] [:flo] from comment #70)
> When hovering/selecting the settings button from the awesomebar panel, it's
> odd that it doesn't have the same color as when hovering/selecting other
> one-off buttons.

Made the same

> If I was implementing this, the first idea I would try would be to make the
> settings button a one-off button

Done

> (I would try just moving the DOM node of the existing settings button, so
> that we don't have to hide it).

That was the first thing I tried, but the code inside the binding couldn't remove() or insertBefore() on `this` -- there are "node not found" errors.  Maybe because of the way the binding is bound via a class selector to vboxes and it's not an element type itself.  I don't know.  At this point I'm really tired of this bug.

> This deserves a comment to explain why we need to set this on both the list
> and its parent... if we can't find a cleaner solution.

I couldn't find a better solution and I don't know why it's necessary and I don't think it's important and I added a comment to that effect.

Comment 73

[Florian Quèze [:florian]]

Attachment: Proposed change

(In reply to Drew Willcoxon :adw from comment #71)
> Comment on attachment 8767056 [details]
> Bug 1180944 - Implement one-off searches from Awesomebar.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/61768/diff/5-6/

This still doesn't look great with multiple rows of one-off buttons: http://i.imgur.com/5a9BmaT.png

When I suggested making the settings button a one-off button, I really meant have the button be inside the description tag so that the layout would work the same as for other one-off buttons. I know you are tired of iterating on this, so rather than review comments with suggestions, I'm attaching the proposed changes, which you can review and integrate in your patch.

Comment 74

[Drew Willcoxon :adw]

That was actually intentional though.  I think having the settings button set apart looks better than having it flow into the one-offs.

Since this is a visual design question, let's ask Stephen.  Stephen, when there are multiple rows of one-offs, the settings button looks like this currently: http://i.imgur.com/5a9BmaT.png  Is that OK?

Comment 75

[Drew Willcoxon :adw]

Oh wait, I think I see.  The one-offs should take up the entire width of the panel except possibly for the final row.  The final row should have the settings button in it, and it should hug the right edge of the panel.  I haven't tried the patch but it looks like that's what it's doing?

Hmm, but what if each row of one-offs is entirely filled?  Where should the settings button be then?  It looks like the patch will place it on the left edge of the panel on a new row?  But shouldn't it be on the right edge always?

Comment 76

[Florian Quèze [:florian]]

(In reply to Drew Willcoxon :adw from comment #75)

> Hmm, but what if each row of one-offs is entirely filled?  Where should the
> settings button be then?  It looks like the patch will place it on the left
> edge of the panel on a new row?  But shouldn't it be on the right edge
> always?

There'll be dummy items added to push it to the right edge. That's why I had to replace engines.length with oneOffCount in a few places, so that the settings button is taken into account for the calculations.

Comment 77

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #76)
> (In reply to Drew Willcoxon :adw from comment #75)
> 
> > Hmm, but what if each row of one-offs is entirely filled?  Where should the
> > settings button be then?  It looks like the patch will place it on the left
> > edge of the panel on a new row?  But shouldn't it be on the right edge
> > always?
> 
> There'll be dummy items added to push it to the right edge. That's why I had
> to replace engines.length with oneOffCount in a few places, so that the
> settings button is taken into account for the calculations.

It'll look like this: http://i.imgur.com/Ec4yJAY.png
Hopefully that's a pretty rare edge case...

And this screenshot made me realize that the left border of the settings button is 1px off :(.

Comment 78

[Drew Willcoxon :adw]

OK, you're right, that does look better.  I'll post a new patch.  I'll leave the needinfo on Stephen since his feedback would be valuable still.

Comment 79

[Drew Willcoxon :adw]

When there's only one row, there's a gap between the settings button and the right edge of the panel, which doesn't look great...

Comment 80

[Florian Quèze [:florian]]

(In reply to Drew Willcoxon :adw from comment #79)
> When there's only one row, there's a gap between the settings button and the
> right edge of the panel, which doesn't look great...

It's not when there's only one row, but just depends on how well or poorly we can divide the available pixels by the number of items we put on each row. It's actually even more visible when there are multiple rows, because you can see the horizontal lines don't touch the right edge of the panel. This bug has existed in the searchbar for as long as we've had one-off buttons. It's more visible in the awesomebar because the panel is typically wider there. I think we could mitigate it by forcing the last item of each row to take all of the remaining space. Seems to me like something that could be done in a follow-up by the way.

Comment 81

[Drew Willcoxon :adw]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61768/diff/6-7/

Comment 82

[Drew Willcoxon :adw]

That incorporates your patch (thanks), fixes the off-by-one border problem on the compact settings button, and increases the width of the last dummy item when there's only one row so that the settings button hugs the panel's edge.

I did notice that there's a gap too when there are multiple rows, like you say.  But I think it's not nearly as big a "problem" then because all the buttons line up vertically, so the settings button doesn't look out of place by having a gap between it and the panel edge.  The rowCount==1 case does look bad IMO.

Comment 83

[Drew Willcoxon :adw]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61768/diff/7-8/

Comment 84

[Drew Willcoxon :adw]

Sorry, forgot to update all css files.

Comment 85

[Drew Willcoxon :adw]

Comment on attachment 8774303 [details] [diff] [review]
Proposed change

f+ and marking obsolete since I incorporated this into the main patch.

Comment 86

[Marco Bonardo [:mak]]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

https://reviewboard.mozilla.org/r/61768/#review63978

Comment 87

[Florian Quèze [:florian]]

(In reply to Drew Willcoxon :adw from comment #67)
> (In reply to Florian Quèze [:florian] [:flo] from comment #58)
> > - if I type 'blah' in the urlbar and a completion to 'blah.com/' (with
> > '.com/' selected) appears in the text field, at the bottom of the panel I
> > see "Search for blah.com/ with:" and if I click a one-off button, Firefox
> > loads blah.com instead of querying the engine I clicked. We should search
> > for what the user typed and ignore the completion suggested in the textfield.
> 
> Should be fixed now, by the urlbarBindings.xml change.

It's fixed when clicking the one-off button, but when selecting the one-off button using the tab key and then pressing enter, we still load blah.com instead of searching for blah.

New issues I've noticed:
- when opening a one-off button's context menu, and clicking "Search in New Tab", we search in the current tab (only from the awesomebar; works fine in the searchbar).
- super edge-case-y issue, steps to reproduce:
1. Type something in the awesomebar
2. Hover the Google one-off button.
3. Press <tab> until another one-off button (eg. wikipedia) is selected.
4. Right click the context menu of the Google one-off button.
5. Activate the "Search in New Tab" item.
Expected result: we should search with Google. (works fine on the searchbar)
Actual result: we search with wikipedia. (only happens from the awesomebar)

Comment 88

[Florian Quèze [:florian]]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

https://reviewboard.mozilla.org/r/61768/#review64018

r=me as the code looks good, but see comment 87. Not sure how much of that should be fixed here before landing, and how much is for follow-ups.

Comment 89

[Drew Willcoxon :adw]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61768/diff/8-9/

Comment 90

[Drew Willcoxon :adw]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

Thanks Florian, sorry for not noticing those problems myself.  This new patch fixes them.  Changes are small but not exactly trivial, so could you please look at them?

Comment 91

[Drew Willcoxon :adw]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

Marco, sorry for asking for review again but I had to make a non-trivial change to urlbarBindings.  It's a change To fix this problem that Florian noticed:

(In reply to Florian Quèze [:florian] [:flo] from comment #87)
> (In reply to Drew Willcoxon :adw from comment #67)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #58)
> > > - if I type 'blah' in the urlbar and a completion to 'blah.com/' (with
> > > '.com/' selected) appears in the text field, at the bottom of the panel I
> > > see "Search for blah.com/ with:" and if I click a one-off button, Firefox
> > > loads blah.com instead of querying the engine I clicked. We should search
> > > for what the user typed and ignore the completion suggested in the textfield.
> > 
> > Should be fixed now, by the urlbarBindings.xml change.
> 
> It's fixed when clicking the one-off button, but when selecting the one-off
> button using the tab key and then pressing enter, we still load blah.com
> instead of searching for blah.

The problem is that in this case, handleEnter on the urlbar is called, which calls handleEnter on the controller, which sets the controller's mSearchString to be the autofilled string.  So we lose the portion that the user typed.  It's only after that that the urlbar's handleCommand is called, so by that point, we don't know what the user typed.  So I used this new _searchStringOnHandleEnter value.

Comment 92

[Drew Willcoxon :adw]

(Re-requesting review through Bugzilla.  I can't figure out how to do it via Review Board.)

Comment 93

[Florian Quèze [:florian]]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

https://reviewboard.mozilla.org/r/61768/#review64464

Ready for another blah.com issue? (sorry!) It's now fixed when clicking the one-off buttons and when pressing enter, but we still load blah.com instead of searching for blah when clicking the "Search in New Tab" context menu item.

Also, the main reason for adding this context menu item was to give users an easy way to search with several one off buttons in a row, so the panel wasn't supposed to close (for the searchbar we didn't manage to keep the panel open, so we reopen it automatically immediately after opening the new tab). On the awesomebar with the current patch clicking the context menu item closes the panel and doesn't reopen it. (Can be a follow-up IMHO, as I don't think this context menu item is very discoverable anyway.)

::: browser/base/content/urlbarBindings.xml:438
(Diff revisions 8 - 9)
> -          let visuallySelectedOneOff =
> +          if (selectedOneOff && selectedOneOff.engine) {
> -            this.popup.oneOffSearchButtons.visuallySelectedButton;
> -          if (visuallySelectedOneOff && visuallySelectedOneOff.engine) {
>              // `url` (which is this.value) may be an autofilled string.  Search
>              // only with the portion that the user typed, if any, by preferring
>              // the autocomplete controller's searchString.

Do you need to update this comment?

::: browser/components/search/content/search.xml:1506
(Diff revisions 8 - 9)
>        </method>
>  
> +      <method name="_buttonIDForEngine">
> +        <parameter name="engine"/>
> +        <body><![CDATA[
> +          return "searchbar-add-engine-" + engine.name.replace(/ /g, '-');

With this change you are replacing all the searchbar-engine-one-off-item- prefixes with searchbar-add-engine-, it doesn't seem to be an intentional change.

::: browser/components/search/content/search.xml:2001
(Diff revisions 8 - 9)
>          if (anonid == "search-one-offs-context-set-default") {
>            let currentEngine = Services.search.currentEngine;
>  
>            // Make the target button of the context menu reflect the current
>            // search engine first. Doing this as opposed to rebuilding all the
>            // one-off buttons avoids flicker.

This whole code path doesn't seem to be working/doing anything in the awesomebar panel.

Tbh, I'm not sure I care. I only discovered this because I was wondering if in a follow-up we could remove this code and replace it with a call to your new rebuild method.

Comment 94

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #93)
> Comment on attachment 8767056 [details]
> Bug 1180944 - Implement one-off searches from Awesomebar.
> 
> https://reviewboard.mozilla.org/r/61768/#review64464
> 
> Ready for another blah.com issue? (sorry!) It's now fixed when clicking the
> one-off buttons and when pressing enter, but we still load blah.com instead
> of searching for blah when clicking the "Search in New Tab" context menu
> item.

After popping the patches, and reapplying them, I can't reproduce this anymore, so let's forget about it for now.

Comment 95

[Drew Willcoxon :adw]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61768/diff/9-10/

Comment 96

[Drew Willcoxon :adw]

(In reply to Florian Quèze [:florian] [:flo] from comment #93)
> Also, the main reason for adding this context menu item was to give users an
> easy way to search with several one off buttons in a row, so the panel
> wasn't supposed to close (for the searchbar we didn't manage to keep the
> panel open, so we reopen it automatically immediately after opening the new
> tab). On the awesomebar with the current patch clicking the context menu
> item closes the panel and doesn't reopen it. (Can be a follow-up IMHO, as I
> don't think this context menu item is very discoverable anyway.)

Hmm, interesting.  The urlbar panel is always closed when you navigate somewhere, so we'd need to set a flag or something specifically in this case to not close it.  I'll leave it for a follow-up.  Marco might have thoughts about that too.

> ::: browser/base/content/urlbarBindings.xml:438
> (Diff revisions 8 - 9)
> > -          let visuallySelectedOneOff =
> > +          if (selectedOneOff && selectedOneOff.engine) {
> > -            this.popup.oneOffSearchButtons.visuallySelectedButton;
> > -          if (visuallySelectedOneOff && visuallySelectedOneOff.engine) {
> >              // `url` (which is this.value) may be an autofilled string.  Search
> >              // only with the portion that the user typed, if any, by preferring
> >              // the autocomplete controller's searchString.
> 
> Do you need to update this comment?

I think it's still accurate since the new _searchStringOnHandleEnter is the autocomplete controller's searchString, even if it's slightly out of date.  I did add a comment where _searchStringOnHandleEnter is set though, which should help I think.

> ::: browser/components/search/content/search.xml:1506
> (Diff revisions 8 - 9)
> >        </method>
> >  
> > +      <method name="_buttonIDForEngine">
> > +        <parameter name="engine"/>
> > +        <body><![CDATA[
> > +          return "searchbar-add-engine-" + engine.name.replace(/ /g, '-');
> 
> With this change you are replacing all the searchbar-engine-one-off-item-
> prefixes with searchbar-add-engine-, it doesn't seem to be an intentional
> change.

Whoops, thanks!

> ::: browser/components/search/content/search.xml:2001
> (Diff revisions 8 - 9)
> >          if (anonid == "search-one-offs-context-set-default") {
> >            let currentEngine = Services.search.currentEngine;
> >  
> >            // Make the target button of the context menu reflect the current
> >            // search engine first. Doing this as opposed to rebuilding all the
> >            // one-off buttons avoids flicker.
> 
> This whole code path doesn't seem to be working/doing anything in the
> awesomebar panel.

It's weird... the first time I tested this, I swear that I also saw that it didn't work or do anything.  Then I restarted and tried again, and it worked: the icon in the button was replaced with the icon of the previous engine.  Maybe I didn't actually rebuild or something the first time.

> Tbh, I'm not sure I care. I only discovered this because I was wondering if
> in a follow-up we could remove this code and replace it with a call to your
> new rebuild method.

I tried calling the rebuild method here, and I don't think it looks good actually.  The buttons rearrange themselves, which is confusing IMO.  Simply replacing the icon is much less disrupting.  (Plus, it didn't actually work 100% right.  When I opened the context menu again, the make-default menu item was disabled.  So that would need some investigation.)

Comment 97

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=990def9b7a89

Comment 98

[Drew Willcoxon :adw]

Some tests failed because when getSelectableButtons iterates over the one-offs list, there's a text node at the beginning, so the node doesn't have a classList.  _rebuild removes text nodes, but the problem is that getSelectableButtons is being called before _rebuild is, because it's being called before the popup opens.  This is now a potential problem because we added the compact settings button directly to the description in a recent patch.

So that try push should fix that.  I'll push to Review Board if/when it comes back green.

Comment 99

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85195b821bfe

Comment 100

[Drew Willcoxon :adw]

Fixes for several searchbar tests that get the one-off buttons that are tripped up by the same change described in comment 98 (adding the compact settings button to the one-offs xul:description).

Comment 101

[Drew Willcoxon :adw]

Latest try looks OK.

Comment 102

[Drew Willcoxon :adw]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61768/diff/10-11/

Comment 103

[Drew Willcoxon :adw]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61768/diff/11-12/

Comment 104

[Drew Willcoxon :adw]

Marco, could you please review this pref change in addition to this change I flagged you on in comment 91: https://reviewboard.mozilla.org/r/61768/diff/8-9/

The urlbar binding tells the urlbar-rich-result-popup binding to enable/disable one-offs instead of urlbar-rich-result-popup just doing it itself, which is a little roundabout.  The reason is that urlbar already has a pref observer.

Comment 105

[Drew Willcoxon :adw]

Oh, this also means that we've removed the footer and its "change search settings" button when the one-offs are disabled -- i.e., on non-Nightly.  Does anyone really think we should keep it?  I sure don't.

Comment 106

[Drew Willcoxon :adw]

Er, but that was Nightly-only, so there's actually no change there.  Nevermind! :-)

Comment 107

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #105)
> Oh, this also means that we've removed the footer and its "change search
> settings" button when the one-offs are disabled

This is a positive thing :D

Btw, I disabled review requests to avoid piling up stuff I couldn't review in the next weeks, but needinfo is open, so if you need any review just needinfo me this week. I'll look at this today.

Comment 108

[Marco Bonardo [:mak]]

Comment on attachment 8767056 [details]
Bug 1180944 - Implement one-off searches from Awesomebar.

https://reviewboard.mozilla.org/r/61768/#review65662

::: browser/base/content/urlbarBindings.xml:1337
(Diff revisions 5 - 12)
> +          } else {
> +            this.oneOffSearchButtons.style.display = "none";
> +            this.oneOffSearchButtons.popup = null;
> +            this.oneOffSearchButtons.textbox = null;
> +            this.oneOffSearchButtons.telemetryOrigin = null;
> +          }

I didn't test it, but please ensure that when disabled the selection looping behavior in the awesomebar is correct.

Comment 109

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f842dc6b5001

Comment 110

[Drew Willcoxon :adw]

One last try push rebased on the current tree...

Comment 111

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/474f38fc48be9f97b49b084ebf0b59293b81cf16
Bug 1180944 - Implement one-off searches from Awesomebar. r=mak,florian

Comment 112

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/474f38fc48be

Comment 113

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/451e84cd0ff9
Implement one-off searches from Awesomebar: Fix eslint warning about inconsistent return usage. r=eslint-fix a=eslint-fix

Comment 114

[avada]

Just tried it in latest central x86. Neither of the buttons function. I get invalid address, nothing at all, or if there's a match a website loading.
Clicking the buttons is no different than pressing enter, it seems.

Comment 115

[6lobe]

I am used to the behaviour that I can select the last suggested entries by pressing the up-arrow-key. Now that selects the new search things that are useless for me.

Going to take a bit of learning to stop instinctively doing that.

Comment 116

[Nawaf Chowdhury Akash]

I have reproduced this bug with nightly 42.0a1 (2015-07-06) on windows 10, 32 bit.

The bug’s fix is now verified on Latest Nightly 51.0a1.

Build ID:20160805030444.
User Agent:Mozilla/5.0 (Windows NT 10.0; rv:51.0) Gecko/20100101 Firefox/51.0.

Comment 117

[Tim Nguyen :ntim]

Err, wrong flag

Comment 118

[Mason Chang [Inactive] [:mchang]]

This is a pretty serious bug in bug 1292767 as I can no longer go to any URL. I confirmed that backing out revision 474f38fc48be fixed it for me.

Comment 119

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

(In reply to Mason Chang [:mchang] from comment #118)
> This is a pretty serious bug in bug 1292767 as I can no longer go to any
> URL. I confirmed that backing out revision 474f38fc48be fixed it for me.

Universal Search is not part of Firefox. It's an experimental add-on.

There's no reason to assume it will always work against the latest Nightly.

If you have problems with it, the problem isn't Nightly, it's the add-on :-)

Comment 120

[:deanis74]

I tried to search, but my Bugzilla search foo is not what it used to be in my youth.  Is there an open issue for the tab key behavior not working like before?  I have removed all search engines so that the Awesomebar looks like the old search.  But when I type something and press Tab, focus moves to an invisible Search Options button instead of cycling through the entries in the dropdown list.  This is contrary the behavior in the other major Windows browsers.

Comment 121

[Justin Dolske [:Dolske]]

(In reply to Dean Tessman from comment #120)
> Is there an open issue for the tab key behavior not working like
> before?

Yes, bug 1292321. See the "depends on" list at the top of this bug.

Comment 122

[Milan Sreckovic [:milan] (needinfo for best results)]

I'm assuming this is not getting backed out due to regressions, and those regressions are handled instead?  Do we have a timeline?  It'd be good if they don't make it to Aurora.

Comment 123

[Drew Willcoxon :adw]

Which regressions?  It's preffed on only on Nightly right now.

Comment 124

[Panos Astithas (he/him) [:past] (please ni?)]

Yes, this feature won't ride the trains yet so we have time to fix any followups.

Comment 125

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Drew Willcoxon :adw from comment #123)
> Which regressions?  It's preffed on only on Nightly right now.

(In reply to Panos Astithas [:past] from comment #124)
> Yes, this feature won't ride the trains yet so we have time to fix any
> followups.

These two comments are contradicting. It *is* riding the trains, the pref exists and is usable in DevEdition (Aurora). Is this expected? Will it keep riding the trains (pref'd off)?

Can you please confirm this is expected? I need to write compatibility code that relies on it existing or not into my add-on. ;)

Comment 126

[Marco Bonardo [:mak]]

The feature won't be enabled by default (will be preffed off), but the code will ride the train and the pref can be flipped manually.