959567 - [User Story] Implement search suggestions opt-in/out UI

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Comment 1

[Mike de Boer [:mikedeboer]]

(estimation done based on the assumption that the opt-in/out is managed from the preferences pane.)

Comment 3

[:shell escalante]

this was #5 on the list and below the minimum needed for a v1 - so putting as a p2 (we would release initial feature without it)

Comment 4

[Marco Bonardo [:mak]]

would it be ok if for now we just add a "Search Suggestions" checkbox to Options / Privacy / Location Bar?
We don't have a mock-up for this specific part, but we need to hook the implementation to a pref and expose that. The above point looks the most coherent one.
I might even do that in a separate bug if it's fine for you, but we still want to implement a more visible option here.

Comment 5

[Stephen Horlander [:shorlander]]

(In reply to Marco Bonardo [::mak] from comment #4)
> would it be ok if for now we just add a "Search Suggestions" checkbox to
> Options / Privacy / Location Bar?
> We don't have a mock-up for this specific part, but we need to hook the
> implementation to a pref and expose that. The above point looks the most
> coherent one.
> I might even do that in a separate bug if it's fine for you, but we still
> want to implement a more visible option here.

Yeah, that works for me as we are implementing as long as we are ok that it will most likely change before release.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Stephen Horlander [:shorlander] from comment #5)
> Yeah, that works for me as we are implementing as long as we are ok that it
> will most likely change before release.

Note that regardless, we need to land strings before the Aurora merge for any release UI we might want.

Comment 7

[Stephen Horlander [:shorlander]]

Attachment: Search Suggestions Notification - i01

Designs for Search Suggestions notification.

Top:
- When user focuses Awesome Bar the notification appears
- "Learn more…" will go to a help page explaining the new feature
- "Change Settings" will go to the appropriate place in Settings/Preferences to disable it
- "OK!" will close the notification

Bottom:
- If user starts typing results appear below the notification
- Should probably decide on how many interactions with the Awesome Bar to persist it. Around 3–5.

Comment 8

[Dave Townsend [:mossop]]

Drew, we talked a couple of weeks ago about you taking this once the UX was done. Can you make this a priority now?

Comment 9

[Drew Willcoxon :adw]

Sure.

Comment 10

[Drew Willcoxon :adw]

Attachment: WIP

Here's the start of a patch.  It adds a <notification> to the top of the popup.  Works surprisingly well.

Notes from an IRC chat with Stephen:

* The mockup shows the popup stretching across the width of the browser, but we won't block this bug on that, so the notification should go within the popup.

* Therefore the notfication's text is likely to wrap, which is OK.

Comment 11

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #10)
> * The mockup shows the popup stretching across the width of the browser, but
> we won't block this bug on that, so the notification should go within the
> popup.

This will be part of the new awesomebar style, I made a patch for that while on the whistler bus, but clearly requires more work.

Comment 12

[Drew Willcoxon :adw]

Attachment: WIP v2

It'd be helpful to have some feedback on only the <content> addition to urlbar-rich-result-popup and its CSS.  As discussed on IRC, I think there are a few options for adding the notification to the popup, and this is the least hacky.  (Strings are hardcoded for now.)

There's a weird problem where the scroll position of the listbox shifts down a little after you type, so that the first result is partially hidden.  It's especially bad when the window is narrow and the text in the notification has to wrap.  Any idea what might be happening?

Comment 13

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #12)
> It'd be helpful to have some feedback on only the <content> addition to
> urlbar-rich-result-popup and its CSS.  As discussed on IRC, I think there
> are a few options for adding the notification to the popup, and this is the
> least hacky.  (Strings are hardcoded for now.)

Well, if we could show the notification at the bottom of the popup, that would simplify a lot of stuff. Maybe we could ask Stephen how much he cares about having this at the top, I suspect the reason is that when the user starts typing it would be pushed down by the results and that would look janky.

That said, your solution looks ok to me. The other choices don't look much better.

> There's a weird problem where the scroll position of the listbox shifts down
> a little after you type, so that the first result is partially hidden.  It's
> especially bad when the window is narrow and the text in the notification
> has to wrap.  Any idea what might be happening?

I cannot reproduce this issue on Windows 8.1, it looks correct here, might be something OS X only?

The autocomplete-rich-result-popup that urlbar-rich-result-popup derives from, has various hacks to calculate the height of the rlb, maybe you could try giving the notification the same exact height as one of the richlistitems. It would also make sense from a UI rhythm point of view. The various calls to ensureElementIsVisible and adjustHeight in autocomplete.xml could be involved.

The other thing I would look at is th offset in richlistbox ensureElementIsVisible implementation:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/richlistbox.xml#209

I hope we can avoid entering the ScrollboxObject rabbit hole...

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8631325 [details] [diff] [review]
WIP v2

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

I only evaluated the approach, not the code.

Comment 15

[Stephen Horlander [:shorlander]]

Attachment: notifcation-info.svg

Notification Icon

Comment 16

[Drew Willcoxon :adw]

Attachment: WIP v3

This incorporates Stephen's icon and adds a slide-up animation when you click the OK button.  I think an animation is necessary when there are results in popup.  Without it, the notification disappears and your mouse is now over the first result.  The animation has made this patch a little trickier.  I'd wanted to post a for-review patch today but I don't think that's going to happen.

Comment 17

[Drew Willcoxon :adw]

Shell or Kev, who's responsible for writing and putting up the page that the "Learn more" link in the UI will link to?  Can we get that started?  I'm tentatively using this URL in the code:

https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/search-suggestions

Comment 18

[Drew Willcoxon :adw]

Attachment: for-review patch

This feels pretty solid.  I'll work on a test next, but in the meantime we can start review I think.

Comment 19

[Drew Willcoxon :adw]

Shell, did you mean to clear the needinfo?  Could you see comment 17 please?

Comment 20

[Drew Willcoxon :adw]

Attachment: for-review patch v1.1

Whoops, forgot to hg add browser/themes/shared/urlbarSearchSuggestionsOptOut.inc.css.

Comment 21

[Drew Willcoxon :adw]

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

(still without a new test)

Comment 22

[Drew Willcoxon :adw]

Attachment: test

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

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 8633568 [details] [diff] [review]
for-review patch v1.1

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

So, there are some general issues:

1. when switching tabs, the locationbar gets focus, and we show for a brief while the notification (it disappears immediately). it's flickery and looks broken.
2. when I click Change Settings, the notification disappears, and then appears again for a brief while. it should just go away.
3. I don't know if it's a build issue, but on Windows it looks quite bad, very different from the mock-up, it is taller, the font is smaller and the button is completely wrong.

::: browser/base/content/urlbarBindings.xml
@@ +968,5 @@
>        <handler event="focus" phase="capturing"><![CDATA[
>          this._hideURLTooltip();
>          this._clearFormatting();
> +
> +        // Show the search suggestions opt-out notification if necessary.

Could you please factor this out to an helper method and just invoke that here, for readability

@@ +970,5 @@
>          this._clearFormatting();
> +
> +        // Show the search suggestions opt-out notification if necessary.
> +        let optOutCount =
> +          this._prefs.getIntPref("searchSuggestionsOptOutCount");

I think we should cache this in a field, rather than reading the pref at every focus change.
Then the most common case would just hit the field (must be kept in sync with the pref when we decrease it, but that's trivial)

@@ +1482,5 @@
>    <binding id="urlbar-rich-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-rich-result-popup">
> +
> +    <content ignorekeys="true" level="top" consumeoutsideclicks="never">
> +      <xul:hbox anonid="search-suggestions-opt-out" align="center"
> +                style="visibility: collapse;">

why not collapsed="true"? is this for the animation?

@@ +1626,5 @@
>            // to avoid impacting startup / new window performance
>            aInput.popup.hidden = false;
>  
> +          let optOutVisible =
> +            this.searchSuggestionsOptOut.style.visibility == "visible";

please refactor the additions into an helper method so openAutocompletePopup stays readable.

@@ +1636,5 @@
> +            // Show the footer only if there are results.  The popup may be
> +            // open even when there are no results in order to show the search
> +            // suggestions opt out.
> +            this.footer.collapsed = this._matchCount == 0;
> +            this._lastMatchCountOnOpen = this._matchCount;

I don't understand this _lastMatchCountOnOpen thing, is it just to unhide the footer? could you add a comment to it?

@@ +1649,5 @@
> +            // Show the opt out.  Set the learn-more link href first.
> +            let link = this.searchSuggestionsOptOutLearnMoreLink;
> +            if (!link.hasAttribute("href")) {
> +              let url = Services.urlFormatter.formatURL(
> +                Services.prefs.getCharPref("app.support.baseURL") +

I imagine we need the url here, file a product management bug for it and make it a P1?

@@ +1689,5 @@
> +            self.style.removeProperty("maxHeight");
> +            self.removeAttribute("height");
> +            self.footer.collapsed = self._matchCount == 0;
> +            if (!self._matchCount) {
> +              self.closePopup();

do we really have to hide the footer if we are already closing the popup?
Also, why would the footer be visible at this point if we had already hidden it before? Who will make it visible again?

@@ +1691,5 @@
> +            self.footer.collapsed = self._matchCount == 0;
> +            if (!self._matchCount) {
> +              self.closePopup();
> +            }
> +          }, true);

.bind(this)?

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +399,5 @@
>  
> +<!ENTITY urlbar.searchSuggestionsOptOut.text "You can now see Search Suggestions when typing in the Awesome Bar!">
> +<!ENTITY urlbar.searchSuggestionsOptOut.learnMore "Learn more…">
> +<!ENTITY urlbar.searchSuggestionsOptOut.changeSettings "Change Settings">
> +<!ENTITY urlbar.searchSuggestionsOptOut.ok "OK!">

Why does this differ from the other notifications where we have just a cleaner X button to dismiss the notification?

I think my problem is mostly with the "OK!" text, first it shouts too much in my face, OK is strong enough even without an exclamation point. It's also non-standard, dialogs use "OK" (http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/dialogOverlay.dtd#8) and we should try to not create new standards for confirm buttons. Imo, at a minimum, the exclamation point must go.

My other problem is that I HOPE localizers will leave this untouched, but I don't trust that too much (http://mxr.mozilla.org/l10n-central/search?string=ENTITY%20okButton.label)
While a bad translation like "Dismiss", "Close", "Continue" is sort of harmless in a dialog, here it might confuse the user, is he Dismissing search suggestions? is he Closing the popup or the browser?
It looks like a footgun to me, could you please bring this to shorlander attention before proceeding and see if he has alternative ideas to make this hard to get wrong when localized?

While [translate] in the translation bar executes an operation and thus makes sense to have a big green button, here the user is just stating he saw the notice, so I'd just go for the usual X.

::: browser/themes/linux/jar.mn
@@ +95,5 @@
>    skin/classic/browser/search-engine-placeholder.png        (../shared/search/search-engine-placeholder.png)
>    skin/classic/browser/badge-add-engine.png                 (../shared/search/badge-add-engine.png)
>    skin/classic/browser/search-indicator-badge-add.png       (../shared/search/search-indicator-badge-add.png)
>    skin/classic/browser/search-history-icon.svg              (../shared/search/history-icon.svg)
> +  skin/classic/browser/search-suggestions-opt-out-info.svg  (../shared/search/search-suggestions-opt-out-info.svg)

I think the idea was to add this svg icon as a generic notification icon, reusable by multiple UIs.
Could you move it to the main shared folder with a more generic name? the original notification-info.svg name SGTM, or even just info.svg since we have warning.png...

Comment 24

[Marco Bonardo [:mak]]

Comment on attachment 8633669 [details] [diff] [review]
test

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

Did you already check others tests on Try to be sure they are not influenced by this new behavior (we now show the popup when there are no results)
note that eventually we can set the pref to 0 globally for all mochitests.

::: browser/base/content/test/general/browser_urlbarSearchSuggestionsOptOut.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

nit: no need for a license header in tests (they default to PD)

@@ +139,5 @@
> +add_task(function* cleanup() {
> +  Services.prefs.clearUserPref(SUGGEST_ALL_PREF);
> +  Services.prefs.clearUserPref(SUGGEST_URLBAR_PREF);
> +  Services.prefs.clearUserPref(OPT_OUT_COUNT_PREF);
> +});

better do this in a registerCleanupFunction

@@ +165,5 @@
> +  return new Promise(resolve => {
> +    gURLBar.popup.addEventListener("transitionend", function onEnd() {
> +      gURLBar.popup.removeEventListener("transitionend", onEnd, true);
> +      // Let the urlbar handled the transitionend first.
> +      executeSoon(resolve);

promises resolve on the next tick already, is this really needed?

Comment 25

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] from comment #23)
> 3. I don't know if it's a build issue, but on Windows it looks quite bad,
> very different from the mock-up, it is taller, the font is smaller and the
> button is completely wrong.

I just tested on Windows 7, and it looks how it's supposed to look except for the OK button, which has a border that it shouldn't have.  I don't know what you mean by taller.  It has to be taller, if the text wraps, to fit everything.  See a previous comment where I mentioned that Stephen and I had talked about that.

I also don't know what you mean by the font is smaller.  To me it looks how it's supposed to look.

> @@ +1482,5 @@
> >    <binding id="urlbar-rich-result-popup" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete-rich-result-popup">
> > +
> > +    <content ignorekeys="true" level="top" consumeoutsideclicks="never">
> > +      <xul:hbox anonid="search-suggestions-opt-out" align="center"
> > +                style="visibility: collapse;">
> 
> why not collapsed="true"? is this for the animation?

Yes.

> @@ +1636,5 @@
> > +            // Show the footer only if there are results.  The popup may be
> > +            // open even when there are no results in order to show the search
> > +            // suggestions opt out.
> > +            this.footer.collapsed = this._matchCount == 0;
> > +            this._lastMatchCountOnOpen = this._matchCount;
> 
> I don't understand this _lastMatchCountOnOpen thing, is it just to unhide
> the footer? could you add a comment to it?

It's to avoid doing this.removeAttribute("height") unnecessarily.  We could just always remove the attribute, but that would defeat the height caching that _invalidate does.

removeAttribute("height") is necessary because the popup can now be open when the match count goes from zero to non-zero.  I found that if I didn't remove the attribute, then the popup would remain only tall enough to show one result.

I'll add a comment.

> > +            // Show the opt out.  Set the learn-more link href first.
> > +            let link = this.searchSuggestionsOptOutLearnMoreLink;
> > +            if (!link.hasAttribute("href")) {
> > +              let url = Services.urlFormatter.formatURL(
> > +                Services.prefs.getCharPref("app.support.baseURL") +
> 
> I imagine we need the url here, file a product management bug for it and
> make it a P1?

I needinfoed Shell about it but she hasn't responded yet...  I'll file another bug.

> @@ +1691,5 @@
> > +            self.footer.collapsed = self._matchCount == 0;
> > +            if (!self._matchCount) {
> > +              self.closePopup();
> > +            }
> > +          }, true);
> 
> .bind(this)?

Then I need to define the function not inline with the addEventListener, which is annoying and ugly.  It's just easier to use self.

> ::: browser/locales/en-US/chrome/browser/browser.dtd
> @@ +399,5 @@
> >  
> > +<!ENTITY urlbar.searchSuggestionsOptOut.text "You can now see Search Suggestions when typing in the Awesome Bar!">
> > +<!ENTITY urlbar.searchSuggestionsOptOut.learnMore "Learn more…">
> > +<!ENTITY urlbar.searchSuggestionsOptOut.changeSettings "Change Settings">
> > +<!ENTITY urlbar.searchSuggestionsOptOut.ok "OK!">
> 
> Why does this differ from the other notifications where we have just a
> cleaner X button to dismiss the notification?

You should needinfo Stephen about this whole part of your comment, not me, if you really want to object to the design.

> I think the idea was to add this svg icon as a generic notification icon,
> reusable by multiple UIs.

Where did we decide that that's the idea?  I'm not opposed to it and in fact I think it's a good idea, but...

Comment 26

[Drew Willcoxon :adw]

Attachment: patch v2

Still need to figure out what's up with the OK button on Windows, but this addresses your other technical comments (except for info SVG path), and I think it's a better patch for it.

Comment 27

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #25)
> > @@ +1691,5 @@
> > > +            self.footer.collapsed = self._matchCount == 0;
> > > +            if (!self._matchCount) {
> > > +              self.closePopup();
> > > +            }
> > > +          }, true);
> > 
> > .bind(this)?
> 
> Then I need to define the function not inline with the addEventListener,
> which is annoying and ugly.  It's just easier to use self.

Afaik
addEventListener("something", function () {}.bind(this), true);
works fine

> > I think the idea was to add this svg icon as a generic notification icon,
> > reusable by multiple UIs.
> 
> Where did we decide that that's the idea?  I'm not opposed to it and in fact
> I think it's a good idea, but...

Well, just by looking at how it's named and the fact it's not specific at all for search suggestions, made me guess the purpose was more general.

Comment 28

[Marco Bonardo [:mak]]

Stephen, could you please help us clarifying some things about the design:

1. Was the info svg icon intended to be generic and reusable in future, or is it specific just for this notification? I think it would be a good idea to have it in the main shared theme folder to allow reuse for any info notification.

2. I don't understand the huge green "OK!" button for various reasons:

2a. Differently from the huge [translate] button in the translate bar, that executes an action, this button doesn't do anything, it's just dismissing the notification. Since it's just dismissing a notification, why is it not a simple X as for all of the other notification bars we have?

2b. If we really want to keep this huge green button, can we at least make it "OK" instead of "OK!" since the exclamation point looks like shouting and it's not standard (dialog buttons and most system buttons use just "OK").

2c. I fear localizers will go mad about this, just look at the dialogs OK button translations (http://mxr.mozilla.org/l10n-central/search?string=ENTITY%20okButton.label). While a bad translation like "Dismiss", "Close", "Continue" is sort of harmless in a dialog, here it might confuse the user. Am I dismissing search suggestions? What am I closing, the awesomebar popup? the browser? Continue doing what? It looks like a footgun to me.

Comment 29

[Marco Bonardo [:mak]]

Attachment: Windows screenshot

So this is what I meant:

1. The button looks weird, but we know it still needs fixes.
2. even if the text is on one line, the bar is much taller. Note, it might be due to the button pushing it.
3. In the mockup the notification text is as big as the awesomebar entries text, here it looks smaller, by measuring it is 1px smaller.
4. Something I don't know if it's a bug or not in the mock-up, the Change settings text should be smaller than the notification text. Basically the current font size in your patch looks correct for Change settings but not for the notification text.
5. the mock-up doesn't have a right margin, after the button, but I honestly think your implementation looks better, with even margin both sides.

Comment 30

[Marco Bonardo [:mak]]

Comment on attachment 8634991 [details] [diff] [review]
patch v2

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

This looks good, modulo:
1. comment 28 (please ping shorlander on IRC today)
2. comment 29
3. I'm still building on Linux, will eventually follow-up if there are issues.

I also wonder if we should land this with the pref set to 0, and change the default value to 3 when we fix the prefs pane in bug 1181173 (I'm ignoring the learn more link for now, cause it might take more time). Currently users would think the only way to disable suggestions is to disable them globally, that is not what we want. I don't have a strong opinion about this though.

::: browser/base/content/urlbarBindings.xml
@@ +929,5 @@
> +            this._searchSuggestionsOptOutCount > 0 &&
> +            this._prefs.getBoolPref("suggest.searches") &&
> +            Services.prefs.getBoolPref("browser.search.suggest.enabled") &&
> +            this._prefs.getBoolPref("unifiedcomplete");
> +          if (showOptOut) {

a "> 0" could be more reliable (someone might thing it's a good idea to set it to -1 to disable something)

@@ +969,5 @@
>            this._hideURLTooltip();
>            this.formatValue();
> +          if (!gBrowser.selectedBrowser._urlbarFocused) {
> +            // When _urlbarFocused is true, tabbrowser will close the popup if
> +            // it's open after this code here runs, so in that case do not show

"...tabbrowser would close the autocomplete popup if it's being opened here,"

Is there a reason to not move the if as an early bailout inside _maybeShowSearchSuggestionsOptOut and keep this cleaner? in the end that's already named maybeShow, that presumes there are conditions...

@@ +1710,5 @@
> +
> +          let popupHeightPx =
> +            (this.getBoundingClientRect().height - optOutHeight) + "px";
> +          this.style.height = popupHeightPx;
> +          this.style.maxHeight = popupHeightPx;

Is maxHeight actually needed here? I think I read an article about it being needed to animate height in some cases, but then I couldn't reproduce any issue without it.

Comment 31

[Marco Bonardo [:mak]]

Linux has the same theming issues with font size (the notification font is right, change settings should maybe be smaller) and the button also looks weird, as on Windows.

Comment 32

[Drew Willcoxon :adw]

Thanks for posting the screenshot.  I see what you mean.

(In reply to Marco Bonardo [::mak] from comment #30)
> I also wonder if we should land this with the pref set to 0, and change the
> default value to 3 when we fix the prefs pane in bug 1181173 (I'm ignoring
> the learn more link for now, cause it might take more time). Currently users
> would think the only way to disable suggestions is to disable them globally,
> that is not what we want. I don't have a strong opinion about this though.

That might make sense.  This makes me think that the change-settings link in the urlbar should open the privacy pref pane, where the location bar preferences are, not the search pane, where the global preference is (until bug 1181173 is fixed).

> ::: browser/base/content/urlbarBindings.xml
> @@ +929,5 @@
> > +            this._searchSuggestionsOptOutCount > 0 &&
> > +            this._prefs.getBoolPref("suggest.searches") &&
> > +            Services.prefs.getBoolPref("browser.search.suggest.enabled") &&
> > +            this._prefs.getBoolPref("unifiedcomplete");
> > +          if (showOptOut) {
> 
> a "> 0" could be more reliable (someone might thing it's a good idea to set
> it to -1 to disable something)

What do you mean?  if (showOptOut > 0)?  showOptOpt will always be a boolean since each of the terms in the conjunction are booleans.  And the first term does do > 0.

> @@ +1710,5 @@
> > +
> > +          let popupHeightPx =
> > +            (this.getBoundingClientRect().height - optOutHeight) + "px";
> > +          this.style.height = popupHeightPx;
> > +          this.style.maxHeight = popupHeightPx;
> 
> Is maxHeight actually needed here? I think I read an article about it being
> needed to animate height in some cases, but then I couldn't reproduce any
> issue without it.

You're right, it doesn't seem to be necessary.  It did seem to be necessary in an earlier version.

(In reply to Marco Bonardo [::mak] from comment #27)
> > Then I need to define the function not inline with the addEventListener,
> > which is annoying and ugly.  It's just easier to use self.
> 
> Afaik
> addEventListener("something", function () {}.bind(this), true);
> works fine

Yes, but what I mean is that the listener needs to be removed once it fires, and to do that you need a reference to it, which you lose when you call bind() inline like that since bind() returns a new function.

Anyhow, the latest patch uses a reference to an arrow function.

Comment 33

[Drew Willcoxon :adw]

Attachment: patch v3

This still conforms to Stephen's spec.  He's out until Monday.  If you're able to review this in the meantime I think that would be good since it does what he specified.  I'll make whatever changes we decide on after we hear from him of course.

I tested on all three platforms, but please do continue to test too.

Summary of changes from previous patch:

* Sets browser.urlbar.searchSuggestionsOptOutCount to 0
* Moves the gBrowser.selectedBrowser._urlbarFocused check from focus handler to _maybeShowSearchSuggestionsOptOut
* openPreferences("paneSearch") -> openPreferences("privacy")
* Removes maxHeight change
* Adds a localization note to the OK button string
* Moves the SVG file to shared/info.svg
* Styling fixes to match the spec more closely:
  * Makes the change-settings link font smaller (same as .ac-action-text etc.)
  * Makes OK button padding/size smaller
  * Makes OK button's right margin smaller to match spec (even though you thought the earlier patch looked better)

Comment 34

[Drew Willcoxon :adw]

Attachment: patch v3.1

Whoops, the recent changes broke the count-goes-to-zero behavior.  The opt out wasn't being hidden.  This fixes it by having the urlbar listen for popuphidden when the count becomes zero and then calls dismissSearchSuggestionsOptOut on the popup.  Not sure what the best way to do it now is, but I think this probably makes sense since the API is that the urlbar tells the popup when to show and hide the notification.  With the older patches, the popup had a shouldShowOptOut property that the urlbar would set to false in this case, and then the popup would handle it the next time it opened.

Comment 35

[Drew Willcoxon :adw]

And _showSearchSuggestionsOptOut probably shouldn't be underscored.

Comment 36

[Drew Willcoxon :adw]

Attachment: test v2

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

Addresses comments, hopefully fixes timeouts by making sure the popup is closed when the test ends.

Comment 37

[Marco Bonardo [:mak]]

Comment on attachment 8635594 [details] [diff] [review]
patch v3.1

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

::: browser/base/content/urlbarBindings.xml
@@ +1649,5 @@
> +          // The popup may already be open if it's showing the search
> +          // suggestions opt-out notification.  In that case, its height and
> +          // footer visibility need to be updated.
> +          if (this.popupOpen) {
> +            this.removeAttribute("height");

I have a lot of doubts regarding this.
Afaik this attribute is used by consumers who don't want the panel to grow/shrink dynamically, the awesomebar is not one of those. _invalidate doesn't do any caching of the height attribute, it rather sets the height property on the richlistbox (and I'm changing that to use css height property to avoid flickering). The only setAttribute in autocomplete.xml is about tree-based autocomplete.
Also browser.xul is not setting an height attribute afaict.

So either this is not doing anything, or something I'm unaware of is setting this attribute, and I'd like to know what.

Comment 38

[Drew Willcoxon :adw]

Attachment: patch v4

You're right, it doesn't seem to be necessary.  This patch removes the two places where removeAttribute("height") was called.  I had problems getting the popup to resize in earlier patches and added them somewhere along the way.

I also did a quick mxr search for setAttribute("height" and didn't find anything related to the urlbar popup.

The previous try run hit some infrastructure problem and didn't even start, so here's a new one with this patch:

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

Comment 39

[Tim Nguyen :ntim]

Comment on attachment 8636120 [details] [diff] [review]
patch v4

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

::: browser/themes/shared/info.svg
@@ +1,5 @@
> +<?xml version="1.0"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="16" height="16" viewBox="0 0 16 16">

nit : version and xmlns:xlink attributes aren't needed (there are no xlink:href attributes in this file)

Comment 40

[Marco Bonardo [:mak]]

this seems to make browser_compartments.js go mad :(
By the log, looks like the test is doing something wrong, it times out after
"Not looking for total user time on this platform, we're done"
At that point it should just exit, but it keeps printing stuff, so it's not waiting for all of its pending stuff. Indeed the isShuttingDown protection that has been added there is very poor.
Moreover I'm not sure why this doesn't go through the usual automatic requestLongerTimeout thing, it should not suddenly timeout.

Comment 41

[Marco Bonardo [:mak]]

Comment on attachment 8636120 [details] [diff] [review]
patch v4

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

::: browser/base/content/urlbarBindings.xml
@@ +1709,5 @@
> +      <method name="_hideSearchSuggestionsOptOutWithAnimation">
> +        <body>
> +          <![CDATA[
> +          let optOutHeight = this.searchSuggestionsOptOut.
> +                             getBoundingClientRect().height;

nit: please align on dots

@@ +1843,5 @@
>            controller.stopSearch();
>          }
>        ]]></handler>
>  
> +      <handler event="mousedown"><![CDATA[

this looks a little bit scary, can we at least limit handling it on the specific target that has the problem?

Comment 42

[Stephen Horlander [:shorlander]]

(In reply to Marco Bonardo [::mak] from comment #28)
> Stephen, could you please help us clarifying some things about the design:
> 
> 1. Was the info svg icon intended to be generic and reusable in future, or
> is it specific just for this notification? I think it would be a good idea
> to have it in the main shared theme folder to allow reuse for any info
> notification.

It could be reusable but I didn't design it with that in mind. We have had a bug open for a while to update notification bar. I didn't want to block on it.

> 2. I don't understand the huge green "OK!" button for various reasons:
> 
> 2a. Differently from the huge [translate] button in the translate bar, that
> executes an action, this button doesn't do anything, it's just dismissing
> the notification. Since it's just dismissing a notification, why is it not a
> simple X as for all of the other notification bars we have?

An X icon is easy to miss and isn't really an affirmative action. The button is basically "Yeah, I read this and I am ok with it."

> 2b. If we really want to keep this huge green button, can we at least make
> it "OK" instead of "OK!" since the exclamation point looks like shouting and
> it's not standard (dialog buttons and most system buttons use just "OK").

Sure, we can just use OK. Or something like "Got it". Trying to make our options a little more human.

Comment 43

[Drew Willcoxon :adw]

(In reply to Tim Nguyen [:ntim] from comment #39)

Thanks, Tim.

(In reply to Marco Bonardo [::mak] from comment #40)
> this seems to make browser_compartments.js go mad :(

I'll try to figure out the problem there.

(In reply to Marco Bonardo [::mak] from comment #41)
> @@ +1843,5 @@
> >            controller.stopSearch();
> >          }
> >        ]]></handler>
> >  
> > +      <handler event="mousedown"><![CDATA[
> 
> this looks a little bit scary, can we at least limit handling it on the
> specific target that has the problem?

It is scary.  The popup's change-settings link and OK button weren't responding to clicks on Linux and I couldn't figure out why.  I noticed that browser-search-autocomplete-result-popup was doing this same mousedown thing, and that fixed it: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#1393

I'll expand the comment about it.

(In reply to Stephen Horlander [:shorlander] (Away until 7/20) from comment #42)
> Sure, we can just use OK. Or something like "Got it". Trying to make our
> options a little more human.

I'll go with OK then since I agree with Marco about the exclamation point.

Comment 44

[Marco Bonardo [:mak]]

The scrollbar problem with flex is bug 413336.

Basically the richlistbox thinks it can render with a given size, but it doesn't take into account wrapped text. Here the notification bar text is wrapping and causing the bug.
Either we figure how to fix that longstanding bug, or we need a workaround to avoid the notification box growing due to text wrapping. At least now we know what's causing that.

On the other side, removing flex breaks the panel growing/shrinking on OS X, this might be a panel bug.

Comment 45

[Marco Bonardo [:mak]]

Attachment: workaround.diff

this should allow you to avoid the flex, while still having the rlb sizing properly on Mac. It seems to be working fine here, please test it.

Comment 46

[Drew Willcoxon :adw]

Attachment: patch v4 unbitrotted

Comment 47

[Drew Willcoxon :adw]

Attachment: imperfect attempt at popup height fixes

This is the best I could come up with so far.  The only problem I could find is that when the listbox needs to shrink while the popup is open, it shrinks within the popup, and then the popup shrinks.  It may be hard to notice at first but once you see it it's pretty ugly and you can't not see it.  (I recorded a video to make sure.)  It's as if the popup transition and listbox transition aren't synced.  I also tried resizing/transitioning only the popup height, hoping that the listbox would resize with it, but then neither the popup nor the listbox resizes, like the listbox is forcing the popup to be a certain size even though I removed all its size styles and attributes.

Comment 48

[Drew Willcoxon :adw]

For posterity and anyone following along, the latest patch had been working fine until bug 1047613 landed.  From comment 44 and on, Marco and I are trying to fix popup sizing problems, on OS X especially.

Comment 49

[Drew Willcoxon :adw]

Attachment: changes on top of r+'ed v4

OK, after trying and failing to modify autocomplete-rich-result-popup to work right when the richlistbox is not flexed, I tried the opposite direction of changing autocomplete-rich-result-popup the least amount possible and having urlbar-rich-result-popup do its own thing when the opt out is shown.  It works well (tested on Windows and OS X), but the popup's height doesn't animate when the opt out is shown.  I can live with that!

A good thing about this approach is that the code paths for both urlbar and autocomplete are pretty much the same as before this patch when the opt out is not shown -- i.e., 99% of the time.  When I was trying to fix autocomplete-rich-result-popup instead, there were tons of changes no matter the opt-out status.

This patch makes one unrelated change to adjustHeight in autocomplete-rich-result-popup: when the popup needs to shrink and animate, it collapses unused items *before* the transition starts, not after it finishes.  IMO it looks better because now the popup's scrollbar doesn't pop out after the animation finishes.  It pops out before, which is more pleasant IMO.  But I can revert that part if you disagree.

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

Comment 50

[Drew Willcoxon :adw]

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

Comment 51

[Drew Willcoxon :adw]

Latest try looks good so far!  No browser_compartments.js Windows failures anymore.  Marco, if you're OK with the new patch, we can probably land it.

Comment 52

[Mike Hommey [:glandium]]

This is opt-in, isn't it? Did I miss a discussion about opt-in vs opt-out?

Comment 53

[Drew Willcoxon :adw]

It's opt-out, that was the plan all along.  See this and related bugs.  Why were you assuming it's opt-in?

Comment 54

[Mike Hommey [:glandium]]

Because that's what it looked like in the first mock-up attached to this bug, and it didn't seem to have been mock-ups for opt-out.

Comment 55

[Marco Bonardo [:mak]]

(In reply to Mike Hommey [:glandium] from comment #54)
> Because that's what it looked like in the first mock-up attached to this
> bug, and it didn't seem to have been mock-ups for opt-out.

Hi mike, please bring the discussion to firefox-dev where we can digress more freely, the opt-in opt-out doesn't matter from a coding point of view, since the code is mostly the same in both cases. I'm sure we can clarify most of the doubts in the discussion.
Please note there has not been a final call from product management yet (bug 959565).
Note the mock-up is opt-out.

Comment 56

[Mike Hommey [:glandium]]

Err, sorry, I got opt-out/opt-in backwards. I was thinking opt-out and wrote out-in. I think landing it opt-out before any discussion happens is wrong, even if "that was the plan all along" (especially so, in fact). But then, it's already enabled on nightlies without any opt-in/opt-out option, so I guess the harm is already done...

(In reply to Marco Bonardo [::mak] from comment #55)
> Hi mike, please bring the discussion to firefox-dev where we can digress
> more freely

Sorry, but I don't think /I/ should be the one to start the discussion on firefox-dev. It should be started by the people who want to make this change, and I'd argue it should have been started long ago.

Comment 57

[Marco Bonardo [:mak]]

Nightly is opt-out already, there is a very simple checkbox in Options/Privacy/Location Bar to disable search suggestions. What is missing is just the notification with the direct link to the prefs pane, for non-technical users.

I honestly disagree we need public discussion _way before_ features hit Nightly (Minefield was a so much better name), since I like to trust PM, UX and engineering are more than able to evaluate risks and benefits after 10 years of Firefox releases! However, let's stop discussing in the wrong place. Tomorrow I will prepare a nice description of the feature, get it through some eyes and post to firefox-dev (I hope by Tuesday). Thank you for your feedback.

Comment 58

[Marco Bonardo [:mak]]

I think I'm still able to reproduce the popup getting fixed height bug when I shrink the window and the optout text wraps :(

Comment 59

[Drew Willcoxon :adw]

Sigh, I can too.  Looks like I didn't make the window small enough in when I tested the latest patch.

The problem seems to be that when adjustHeight is called, there's only one match, and after more matches are added, adjustHeight isn't called again while the popup is open.  So the listbox continues to show only one row.  I don't understand why it's only a problem for me when I make the window very narrow.

So I tried calling adjustHeight from _appendCurrentResult but the results are pretty bad.  The popup flickers, even when adjustHeight is called in the _adjustHeightTimeout.

Comment 60

[Drew Willcoxon :adw]

If it's only a problem when the window is very narrow, maybe we can live with that?  Kinda tired of working on this bug. :-/

Comment 61

[Dave Townsend [:mossop]]

(In reply to Drew Willcoxon :adw from comment #60)
> If it's only a problem when the window is very narrow, maybe we can live
> with that?  Kinda tired of working on this bug. :-/

How narrow are we talking? Is there a screenshot that shows the issue?

I'm in favour of getting this landed and filing a follow-up for that issue if it isn't common. We may want to ping Neil to take a look at the core XUL issue.

Comment 62

[Marco Bonardo [:mak]]

Also, I'm a little bit worried by having to use getComputedStyle() at every adjustHeight... You said the problem was with having multiple transitions.
I know that sucks, but in the end the opt out is only shown 3 or leass times to the user, vs autocomplete popup that is shown everyday, could we just remove animations from the opt-out and look at them separately? Would that help solving the craziness?

Comment 63

[Marco Bonardo [:mak]]

(I'm worried about reflows)

Comment 64

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #59)
> Sigh, I can too.  Looks like I didn't make the window small enough in when I
> tested the latest patch.
> 
> The problem seems to be that when adjustHeight is called, there's only one
> match, and after more matches are added, adjustHeight isn't called again
> while the popup is open.

This is very strange, adjustHeight should be invoked when we add more results, we should clear the previous pending timers and issue a new one.
Is _invalidate() called? Should be invoked from http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1538

Comment 65

[Drew Willcoxon :adw]

_invalidate gets called, but the popup has a height attribute, so it doesn't call adjustHeight.  Something keeps setting the height attribute, which is causing at least half my confusion in this bug I think.

Comment 66

[Drew Willcoxon :adw]

Attachment: changes on top of r+'ed v4 -- v2

This seems to work, could you please test it?

* Changes the height attribute to fixedheight to allow the consumer to specify a fixed height
* Removes the height attribute on adjustHeight()
* Checks a dontanimate attribute instead of getComputedStyle()

Comment 67

[Marco Bonardo [:mak]]

Comment on attachment 8639942 [details] [diff] [review]
changes on top of r+'ed v4 -- v2

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

::: browser/base/content/urlbarBindings.xml
@@ +1679,5 @@
>                "search-suggestions"
>              );
>              link.setAttribute("href", url);
>            }
> +          this.richlistbox.flex = 0;

please add a comment explaining that this is a hack needed cause the richlistbox otherwise is scrolled to the wrong position when the notification text wraps.

::: toolkit/content/widgets/autocomplete.xml
@@ +1081,5 @@
>            // behavior that grows and shrinks it on result change, a consumer
> +          // can set the fixedheight attribute. In such a case, instead of
> +          // adjusting the richlistbox height, we just need to collapse unused
> +          // items.
> +          if (!this.hasAttribute("fixedheight")) {

as discussed on IRC, this was added for XUL Fennec, that is a dead project, let's just remove the conditional branch

@@ +1159,5 @@
>              // Calculate the height to have the first row to last row shown
>              height = this._rowHeight * numRows;
>            }
>  
> +          let animate = this.getAttribute("dontanimate") != "true";

please reintroduce the cached this._rlbAnimated to support consumers not having transitions

Comment 68

[Drew Willcoxon :adw]

Attachment: changes on top of r+'ed v4 -- v3

I'm posting a couple of new patches for r? just to make sure I didn't miss anything.  Feel free to rubber-stamp if you want.

This patch makes the changes you requested.

Comment 69

[Drew Willcoxon :adw]

Attachment: s/opt out/notification/

This s/opt out/notfication/ everywhere liked we talked about since it's a mystery whether this'll be opt out or what.

Comment 70

[Drew Willcoxon :adw]

Attachment: combined final patch

Here's a combined patch, everything that should land including the test and the very first patch r+'ed a while ago.

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

Comment 71

[Marco Bonardo [:mak]]

Comment on attachment 8640189 [details] [diff] [review]
changes on top of r+'ed v4 -- v3

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

::: browser/base/content/urlbarBindings.xml
@@ +1688,5 @@
> +          // must wrap.  Work around them by removing the flex.
> +          //
> +          // But without flexing the listbox, the popup's height animation
> +          // sometimes fails to complete, leaving the popup too tall.  Work
> +          // around that problem by disabling the popup's height animation.

this technically disables the richlistbox animation, not the popup's.

::: toolkit/content/widgets/autocomplete.xml
@@ +1156,5 @@
>              height = this._rowHeight * numRows;
>            }
>  
> +          let animate = this._rlbAnimated &&
> +                        this.getAttribute("dontanimate") != "true";

why not just !hasAttribute?

Comment 72

[Drew Willcoxon :adw]

Like we talked about today, when I land this, I'll land it with the show-notification counter set to 3.

(In reply to Marco Bonardo [::mak] from comment #71)
> this technically disables the richlistbox animation, not the popup's.

Right, thanks.

> why not just !hasAttribute?

To me that's insufficient because maybe the attribute's value is "false".  No reason for anybody to make it false, but no reason not to check it either.

Comment 73

[:shell escalante]

needing info to Kev to write the "learn more"

Comment 74

[Mike Hommey [:glandium]]

(In reply to Marco Bonardo [::mak] from comment #57)
> Tomorrow I will prepare a nice description of the feature, get it
> through some eyes and post to firefox-dev (I hope by Tuesday). Thank you for
> your feedback.

This hasn't happened yet.

Comment 75

[Marco Bonardo [:mak]]

(In reply to Mike Hommey [:glandium] from comment #74)
> This hasn't happened yet.

Sorry but I was asked to wait for the final decision about opt-in/out, that decision changed in the meanwhile, and we are still lacking UX for the new notification.  Since I wanted to write a decent post about the whole feature, with screens and explanations, I am basically lacking information at this point, I hope today things will unblock.

Comment 76

[Drew Willcoxon :adw]

Attachment: combined patch + old test - new test

Changes to match the new spec.  This includes the old test and does not update it yet, so please ignore that part.  I'll work on updating it now and post another patch.

Next I'll post a diff between this and the previous combined patch, which should make this easier to review.

Comment 77

[Drew Willcoxon :adw]

Attachment: diff between old and new combined patches

Comment 78

[Drew Willcoxon :adw]

Attachment: new test

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

Comment 79

[Drew Willcoxon :adw]

Attachment: strings-only patch

For insurance especially due to our time zone difference, here's a strings-only patch that we could land on 42 ASAP.

Comment 80

[Stephen Horlander [:shorlander]]

Attachment: Search Suggest Opt-In Flow - 01

Flow for a Search Suggest Opt-In

Comment 81

[Drew Willcoxon :adw]

Attachment: strings-only patch 2

Spec changed.

Comment 82

[Drew Willcoxon :adw]

Attachment: final combined patch question mark question mark

This includes the new test, which is different from the test patch I posted earlier today.  Code changes from patch posted earlier today:

* Changes for the newest spec in comment 80
  * String changes (same as in strings-only patch 2)
  * CSS tweaks on buttons, give them a min-width so they're the same size
  * Start a new search when user clicks Yes to show suggestions immediately

* Remove the !this._prefs.getBoolPref("suggest.searches") term from the `showNotification` definition in _maybeShowSearchSuggestionsNotification.  The decision to show the notification shouldn't depend on whether urlbar suggestions are on, it should only depend on whether the user made a choice yet.

* Disable the notification in testing/profiles/prefs_general.js so as not to break other tests (see try link in comment 78)

Comment 83

[Drew Willcoxon :adw]

Attachment: diff between old opt-out and newest combined patches

Diff between the newest combined patch and the old opt-out combined patch, excluding test differences.

Comment 84

[Drew Willcoxon :adw]

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

Comment 85

[Drew Willcoxon :adw]

Comment on attachment 8644058 [details] [diff] [review]
final combined patch question mark question mark

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

::: browser/base/content/test/general/browser_urlbarSearchSuggestionsNotification.js
@@ +11,5 @@
> +  registerCleanupFunction(function () {
> +    Services.search.currentEngine = oldCurrentEngine;
> +    Services.prefs.clearUserPref(SUGGEST_ALL_PREF);
> +    Services.prefs.clearUserPref(SUGGEST_URLBAR_PREF);
> +    Services.prefs.clearUserPref(CHOICE_PREF);

prefs_general.js adds user_pref()s, not pref()s, so this needs to be:

> --- a/browser/base/content/test/general/browser_urlbarSearchSuggestionsNotification.js
> +++ b/browser/base/content/test/general/browser_urlbarSearchSuggestionsNotification.js
> @@ -7,17 +7,20 @@ const TEST_ENGINE_BASENAME = "searchSugg
>  add_task(function* prepare() {
>    let engine = yield promiseNewEngine(TEST_ENGINE_BASENAME);
>    let oldCurrentEngine = Services.search.currentEngine;
>    Services.search.currentEngine = engine;
>    registerCleanupFunction(function () {
>      Services.search.currentEngine = oldCurrentEngine;
>      Services.prefs.clearUserPref(SUGGEST_ALL_PREF);
>      Services.prefs.clearUserPref(SUGGEST_URLBAR_PREF);
> -    Services.prefs.clearUserPref(CHOICE_PREF);
> +
> +    // Disable the notification for future tests so it doesn't interfere with
> +    // them.  clearUserPref() won't work because by default the pref is false.
> +    Services.prefs.setBoolPref(CHOICE_PREF, true);
>  
>      // Make sure the popup is closed for the next test.
>      gURLBar.blur();
>      Assert.ok(!gURLBar.popup.popupOpen, "popup should be closed");
>    });
>  });
>  
>  add_task(function* focus_allSuggestionsDisabled() {

That fixes the remaining failure locally for me.

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

Comment 86

[Kev Needham [:kev]]

Just to reiterate from bug 1189409 and reply to comment #17 here, the URL we'll use on SUMO is:

https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/suggestions

Thanks all.

Comment 87

[Drew Willcoxon :adw]

> diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml
> --- a/browser/base/content/urlbarBindings.xml
> +++ b/browser/base/content/urlbarBindings.xml
> @@ -1655,18 +1655,17 @@ file, You can obtain one at http://mozil
>          <parameter name="aInput"/>
>          <parameter name="aElement"/>
>          <body>
>            <![CDATA[
>            // Set the learn-more link href.
>            let link = this.searchSuggestionsNotificationLearnMoreLink;
>            if (!link.hasAttribute("href")) {
>              let url = Services.urlFormatter.formatURL(
> -              Services.prefs.getCharPref("app.support.baseURL") +
> -              "search-suggestions"
> +              Services.prefs.getCharPref("app.support.baseURL") + "suggestions"
>              );
>              link.setAttribute("href", url);
>            }
>  
>            // With the notification shown, the listbox's height can sometimes be
>            // too small when it's flexed, as it normally is.  Also, it can start
>            // out slightly scrolled down.  Both problems appear together, most
>            // often when the popup is very narrow and the notification's text

Comment 88

[Marco Bonardo [:mak]]

Comment on attachment 8644058 [details] [diff] [review]
final combined patch question mark question mark

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

it looks great to me from a code point of view, I couldn't yet test this cause I need to clobber, so I will convert to an r+ as soon as I get a testable build.

You can attach the final version in the meanwhile, if you wish.

::: browser/base/content/test/general/browser_urlbarSearchSuggestionsNotification.js
@@ +181,5 @@
> +  return new Promise(resolve => {
> +    gURLBar.popup.addEventListener("transitionend", function onEnd() {
> +      gURLBar.popup.removeEventListener("transitionend", onEnd, true);
> +      // The urlbar needs to handle the transitionend first, but that happens
> +      // naturally since promises are resolved on the next tick.

this is actually not completely true, now DOM promises are resolved at the end of the current tick, in microtasks.
For this case it's likely to not make a difference, but you could update the comment.

::: browser/base/content/urlbarBindings.xml
@@ +1645,5 @@
> +
> +      <method name="_updateFooterVisibility">
> +        <body>
> +          <![CDATA[
> +          this.footer.collapsed = this._matchCount == 0;

I assume this will work fine with bug 1191520 cause it will unhide nothing (no children with bug 1191520 in Aurora)

Comment 89

[Drew Willcoxon :adw]

Try with fresh tree:

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

Comment 90

[Drew Willcoxon :adw]

I should land this with browser.urlbar.suggest.searches=false, right?  Bug 1188281 disabled UnifiedComplete on non-Nightly, which disables search suggestions on non-Nightly -- but on Nightly, by default you get search suggestions *plus* the opt-in notification, which is wrong.

Comment 91

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw (Away 8/10–14) from comment #90)
> I should land this with browser.urlbar.suggest.searches=false, right?  Bug
> 1188281 disabled UnifiedComplete on non-Nightly, which disables search
> suggestions on non-Nightly -- but on Nightly, by default you get search
> suggestions *plus* the opt-in notification, which is wrong.

yes, suggest.searches = false

Comment 92

[Drew Willcoxon :adw]

Attachment: final patch

One final try with browser.urlbar.suggest.searches=false:

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

Comment 93

[Drew Willcoxon :adw]

Sigh, need this too:

> diff --git a/toolkit/components/places/UnifiedComplete.js b/toolkit/components/places/UnifiedComplete.js
> --- a/toolkit/components/places/UnifiedComplete.js
> +++ b/toolkit/components/places/UnifiedComplete.js
> @@ -31,17 +31,17 @@ const PREF_RESTRICT_SWITCHTAB =     [ "r
>  const PREF_RESTRICT_SEARCHES =      [ "restrict.searces",       "$" ];
>  const PREF_MATCH_TITLE =            [ "match.title",            "#" ];
>  const PREF_MATCH_URL =              [ "match.url",              "@" ];
> 
>  const PREF_SUGGEST_HISTORY =        [ "suggest.history",        true ];
>  const PREF_SUGGEST_BOOKMARK =       [ "suggest.bookmark",       true ];
>  const PREF_SUGGEST_OPENPAGE =       [ "suggest.openpage",       true ];
>  const PREF_SUGGEST_HISTORY_ONLYTYPED = [ "suggest.history.onlyTyped", false ];
> -const PREF_SUGGEST_SEARCHES =       [ "suggest.searches",       true ];
> +const PREF_SUGGEST_SEARCHES =       [ "suggest.searches",       false ];
> 
>  const PREF_MAX_CHARS_FOR_SUGGEST =  [ "maxCharsForSearchSuggestions", 20];
> 
>  // Match type constants.
>  // These indicate what type of search function we should be using.
>  const MATCH_ANYWHERE = Ci.mozIPlacesAutoComplete.MATCH_ANYWHERE;
>  const MATCH_BOUNDARY_ANYWHERE = Ci.mozIPlacesAutoComplete.MATCH_BOUNDARY_ANYWHERE;
>  const MATCH_BOUNDARY = Ci.mozIPlacesAutoComplete.MATCH_BOUNDARY;

And the previous try is triggering outside-connection errors in browser_urlbar_autoFill_backspaced.js that weren't there before.  Don't know what that's about.  Maybe this fixes it.

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

Comment 94

[Drew Willcoxon :adw]

> diff --git a/toolkit/components/places/tests/unifiedcomplete/test_enabled.js b/toolkit/components/places/tests/unifiedcomplete/test_enabled.js
> --- a/toolkit/components/places/tests/unifiedcomplete/test_enabled.js
> +++ b/toolkit/components/places/tests/unifiedcomplete/test_enabled.js
> @@ -54,11 +54,15 @@ add_task(function* test_sync_enabled() {
>    }
>    Assert.equal(Services.prefs.getBoolPref("browser.urlbar.autocomplete.enabled"), true);
> 
>    // Disable autocoplete again, then re-enable it and check suggest prefs
>    // have been reset.
>    Services.prefs.setBoolPref("browser.urlbar.autocomplete.enabled", false);
>    Services.prefs.setBoolPref("browser.urlbar.autocomplete.enabled", true);
>    for (let type of types.filter(t => t != "history")) {
> -    Assert.equal(Services.prefs.getBoolPref("browser.urlbar.suggest." + type), true);
> +    if (type == "searches") {
> +      Assert.equal(Services.prefs.getBoolPref("browser.urlbar.suggest." + type), false);
> +    } else {
> +      Assert.equal(Services.prefs.getBoolPref("browser.urlbar.suggest." + type), true);
> +    }
>    }
>  });

Comment 95

[Drew Willcoxon :adw]

> diff --git a/testing/marionette/client/marionette/tests/unit/unit-tests.ini b/testing/marionette/client/marionette/tests/unit/unit-tests.ini
> --- a/testing/marionette/client/marionette/tests/unit/unit-tests.ini
> +++ b/testing/marionette/client/marionette/tests/unit/unit-tests.ini
> @@ -142,16 +142,17 @@ browser = false
>  b2g = false
>  [test_set_window_size.py]
>  b2g = false
>  skip-if = os == "linux" # Bug 1085717
>  [test_with_using_context.py]
> 
>  [test_modal_dialogs.py]
>  b2g = false
> +skip-if = true # Disabled so bug 959567 can land
>  [test_key_actions.py]
>  [test_mouse_action.py]
>  b2g = false
>  [test_teardown_context_preserved.py]
>  b2g = false
>  [test_file_upload.py]
>  b2g = false
>  skip-if = os == "win" # http://bugs.python.org/issue14574

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

Comment 96

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4e2489f7cc7d

Comment 97

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e2489f7cc7d

Comment 98

[Petruta Horea [:phorea], Desktop QA]

Drew, considering bug 1182792, should the opt-in/out prompt be shown in private windows? This may be confusing for users, as after selecting Yes, no suggestions will be displayed.

Comment 99

[Drew Willcoxon :adw]

Thanks Petruta, you're right.  I filed bug 1196032.