Closed Bug 1449317 Opened 2 years ago Closed 2 years ago

Update the default string in the address bar

Categories

(Firefox :: Address Bar, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox60 + verified
firefox61 + verified

People

(Reporter: verdi, Assigned: standard8)

References

(Depends on 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(2 files)

In Bug 1444504 we landed the string "Search with %S or enter address" where %S is the name of the user's current search engine. Now we'd like to make this the new default string for the address bar.
it would be easier to track this if it wouldn't be confidential. Since a decision has been made, can we open it?
Priority: -- → P1
Whiteboard: [fxsearch]
I'm opening the bug.

We want to proceed with this change. 

We know from user research that we have a small portion of users who do not understand that the Awesome bar can also be used for external search. This is a small "nudge"
Group: mozilla-employee-confidential
Assignee: nobody → standard8
Status: NEW → ASSIGNED
How does the placeholder "Enter search terms and addresses here" that was added in that bug relate to this? Is that obsolete now?
Flags: needinfo?(mverdi)
Yes, that’s obsolete. We’re not going to use it.
Flags: needinfo?(mverdi)
Comment on attachment 8966297 [details]
Bug 1449317 - Update the default string in the address bar.

Florian, can you take a look at this patch please, and see what you think? I'm a bit concerned it is going to affect startup, as currently BrowserSearch.init() is called from within onLoad.

Are there tests that I should run other than the Talos Tpaint tests?

If onLoad is too early, would _delayedStartup still be enough, or are we going to have to find another solution for getting the engine's name?
Attachment #8966297 - Flags: feedback?(florian)
Comment on attachment 8966297 [details]
Bug 1449317 - Update the default string in the address bar.

f- because accessing Services.search.currentEngine during startup will cause a synchronous initialization of the search service (causing a fair amount of main thread I/O).

(In reply to Mark Banner (:standard8) from comment #6)

> Are there tests that I should run other than the Talos Tpaint tests?

browser/base/content/test/performance/browser_startup.js should fail and tell you that you are loading the search service too early.

> If onLoad is too early, would _delayedStartup still be enough, or are we
> going to have to find another solution for getting the engine's name?

onLoad is too late to update the placeholder, we should have it at first paint, ie. after DOMContentLoaded.
But _delayedStartup is too early to use the search service, which hasn't finished initializing asynchronously (it can sometimes take more than 30s).

So yes, if you really want to implement this behavior, you'll need to cache the name of the default in a pref at shutdown to be able to use it at the next startup. And find a solution for the first startup case.
Attachment #8966297 - Flags: feedback?(florian) → feedback-
Verdi, please see the previous comments, I think I need some suggestions as to the best way forward in terms of UX. From what Florian's describing here's a few extra thoughts about proposed implementations:

* Store the name of the engine in a pref & use that at startup.

=> This could cause a few mistakes where we'd display the wrong engine name for a while, for example:

--- upgrading from a build with one default engine to a build with a different default engine, it would not detect that until after the search service had started. Similar situations could likely occur with switching locales or distribution builds to non-distribution builds.
--- if the absearch server triggers a change in the default search engine that gets applied at the next startup.


* On first startup, we wouldn't know which engine to use.

=> We'd have to display some other static string, and then either:

--- update the string once the service has initialised (which may distract the user).
--- find some other way to get the string to update.


Finally, have we also thought about what happens if an engine has a long name? Most of the default shipped engines have quite short names, https://de.pons.com/ has some which are quite long.
Flags: needinfo?(mverdi)
(In reply to Mark Banner (:standard8) from comment #8)
> Verdi, please see the previous comments, I think I need some suggestions as
> to the best way forward in terms of UX. From what Florian's describing
> here's a few extra thoughts about proposed implementations:
> 
> * Store the name of the engine in a pref & use that at startup.
> 

That works for me.

> => This could cause a few mistakes where we'd display the wrong engine name
> for a while, for example:
> 
> --- upgrading from a build with one default engine to a build with a
> different default engine, it would not detect that until after the search
> service had started. Similar situations could likely occur with switching
> locales or distribution builds to non-distribution builds.
> --- if the absearch server triggers a change in the default search engine
> that gets applied at the next startup.
> 

It would be good to estimate how often this happens. Seems like it would be fairly rare. 

> 
> * On first startup, we wouldn't know which engine to use.
> 
> => We'd have to display some other static string, and then either:
> 
> --- update the string once the service has initialised (which may distract
> the user).
> --- find some other way to get the string to update.
> 

Yes let's do that. We can use the current "Search or enter address" as the placeholder if we don't know the search engine by the time we have to display this. Then we can switch it to the dynamic string the next time we have to display this after having figured out what the search engine is.

> 
> Finally, have we also thought about what happens if an engine has a long
> name? Most of the default shipped engines have quite short names,
> https://de.pons.com/ has some which are quite long.

It's hard to tell what we do know because the string is short. I would hope we would fade it out like we do with long URLs.
Flags: needinfo?(mverdi)
(In reply to Verdi [:verdi] from comment #9)

> > Finally, have we also thought about what happens if an engine has a long
> > name? Most of the default shipped engines have quite short names,
> > https://de.pons.com/ has some which are quite long.
> 
> It's hard to tell what we do know because the string is short. I would hope
> we would fade it out like we do with long URLs.

Some engines can have pretty confusing (or even deceptive) names. Especially engines from search hijacking. Would it make sense to always use the fallback string when the current engine isn't a built-in engine?
(In reply to Florian Quèze [:florian] from comment #10)
 
> Some engines can have pretty confusing (or even deceptive) names. Especially
> engines from search hijacking. Would it make sense to always use the
> fallback string when the current engine isn't a built-in engine?

Yes that makes sense - let's do that. We'll only add in the name of the current search engine if it's one we ship with.
Comment on attachment 8966297 [details]
Bug 1449317 - Update the default string in the address bar.

Florian, can you run your eyes over this briefly, and check it looks roughly like what you expect, and I'm not doing anything super bad?

I might reorganise the functions a bit on Monday, as I'm feeling that it is a little more complex than it feels it needs to be.

I'm also not convinced the placeholder is always updating when entering urls and then clearing them without switching tabs. Could be a xul thing, but I'll look closer again on Monday.
Attachment #8966297 - Flags: feedback?(florian)
Comment on attachment 8966297 [details]
Bug 1449317 - Update the default string in the address bar.

https://reviewboard.mozilla.org/r/235026/#review242328

::: browser/base/content/browser.js:3865
(Diff revision 2)
> +  _updateURLBarPlaceholder(engine) {
> +    if (!engine) {
> +      throw new Error("Expected an engine to be specified");
> +    }
> +
> +    if (Services.search.getDefaultEngines().includes(engine)) {

This is more expensive than I would like. _isDefault isn't exposed unfortunately. It's possible to abuse engine.identifier with a null check to get this information in a cheaper way. Not sure it's better :-/.

::: browser/base/content/browser.js:3883
(Diff revision 2)
> +      this._setURLBarPlaceholder(name);
> +      return;
> +    }
> +
> +    function updateURLBar() {
> +      if (gURLBar.value != "") {

Why isn't "if (gURLBar.value)" enough? (same question a few lines above)

::: browser/base/content/browser.js:3890
(Diff revision 2)
> +        gURLBar.removeEventListener("input", this._placeholderUpdateListener);
> +        gBrowser.tabContainer.removeEventListener("TabSelect", this._placeholderUpdateListener);
> +      }
> +    }
> +
> +    this._placeholderUpdateListener = updateURLBar.bind(this);

use an arrow function instead of .bind(this). And this function doesn't need to be saved on the 'this' object, a local variable would be enough.

::: browser/base/content/browser.js:3902
(Diff revision 2)
> +    if (name) {
> +      gURLBar.setAttribute("placeholder",
> +        gBrowserBundle.formatStringFromName("urlbar.placeholder",
> +          [name], 1));
> +    } else {
> +      gURLBar.setAttribute("placeholder", this._originalPlaceholder);

nit: put the string in a variable to avoid duplicating "gURLBar.setAttribute("placeholder",".
Attachment #8966297 - Flags: feedback?(florian) → feedback+
Comment on attachment 8966297 [details]
Bug 1449317 - Update the default string in the address bar.

https://reviewboard.mozilla.org/r/235026/#review242660

::: browser/base/content/browser.js:3755
(Diff revision 3)
>  };
>  
>  const BrowserSearch = {
>    init() {
>      Services.obs.addObserver(this, "browser-search-engine-modified");
> +    // Asynchronously initialise the search service if necessary, to get the

nit: Are we using GB or US spelling for our comments? (initialise vs initialize)

::: browser/base/content/browser.js:3757
(Diff revision 3)
>  const BrowserSearch = {
>    init() {
>      Services.obs.addObserver(this, "browser-search-engine-modified");
> +    // Asynchronously initialise the search service if necessary, to get the
> +    // current engine for working out the placeholder.
> +    Services.search.init(rv => {

This will load the search service from browser.xul's onload, which is before first paint and should fail https://searchfox.org/mozilla-central/rev/bd326a2a6b729dc62a5aee57354a97ceac4d1dc0/browser/base/content/test/performance/browser_startup.js#59-62
Can this be done from something called by _delayedStartup?

::: browser/base/content/browser.js:3793
(Diff revision 3)
>        // engine, then the engine needs to be removed from the corresponding
>        // browser's offered engines.
>        this._removeMaybeOfferedEngine(engineName);
>        break;
> +    case "engine-current":
> +      this._updateURLBarPlaceholder(engine);

How sure are you that this isn't going to be fired while initializing the search service?

::: browser/base/content/browser.js:3885
(Diff revision 3)
> +      Services.prefs.setStringPref("browser.urlbar.placeholderName", engineName);
> +    } else {
> +      Services.prefs.clearUserPref("browser.urlbar.placeholderName");
> +    }
> +
> +    if (delayUpdate) {

I think we should force delayUpdate to false if there's currently an url displayed.

::: browser/base/content/browser.js:3899
(Diff revision 3)
> +   * seeing it, e.g. when there is a value entered in the bar, or if there is
> +   * a tab switch to a tab which has a url loaded.
> +   *
> +   * @param {String} name The name of the engine to put in the placeholder.
> +   */
> +  _delayUpdateURLBarPlaceholder(name) {

given that this has only one caller, I would inline it.

::: browser/base/content/test/urlbar/browser_urlbarPlaceholder.js:53
(Diff revision 3)
> +
> +add_task(async function test_delayed_update_placeholder() {
> +  // Since we can't easily test for startup changes, we'll at least test the delay
> +  // of update for the placeholder works.
> +
> +  let urlTab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:mozilla");

I think it would make sense to have the test verify that calling _updateURLBarPlaceholder(extraEngine, true) while the about:mozilla tab is selected updates the placeholder immediately.
Attachment #8966297 - Flags: review?(florian)
Comment on attachment 8966297 [details]
Bug 1449317 - Update the default string in the address bar.

https://reviewboard.mozilla.org/r/235026/#review242660

> nit: Are we using GB or US spelling for our comments? (initialise vs initialize)

Bah, I'm normally very good at keeping to US for code and GB for bugs ;-)
Hi Mark, What is ETA for landing this bug and uplift?

Thanks
Shilpi
Flags: needinfo?(standard8)
Comment on attachment 8966297 [details]
Bug 1449317 - Update the default string in the address bar.

Asking for ux-review, builds available from

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c08209537802097f6fa9056a0c35a506a453590e
Flags: needinfo?(standard8)
Attachment #8966297 - Flags: ui-review?(mverdi)
(In reply to Shilpi Gupta [:shilpi] from comment #19)
> Hi Mark, What is ETA for landing this bug and uplift?

As soon as reviews are complete, hopefully today.
Comment on attachment 8966297 [details]
Bug 1449317 - Update the default string in the address bar.

https://reviewboard.mozilla.org/r/235026/#review243058

Looks good to me.

::: browser/base/content/test/urlbar/browser_urlbarPlaceholder.js:23
(Diff revisions 3 - 4)
>  
>  add_task(async function setup() {
>    extraEngine = await promiseNewSearchEngine(TEST_ENGINE_BASENAME);
>  
> +  // Force display of a tab with a URL bar, to clear out any possible placeholder
> +  // initialisation listeners that happen on startup.

More GB spelling ;).

::: browser/base/content/browser.js:3897
(Diff revision 4)
> +    }
> +
> +    // Only delay if requested, and we're not displaying text in the URL bar
> +    // currently.
> +    if (delayUpdate && !gURLBar.value) {
> +      this._value++;

Don't forget to remove this debugging code.
Attachment #8966297 - Flags: review?(florian) → review+
Verdi found an issue on initial startup - the placeholder would never change from default, and I found that a variable name was wrong. I've fixed that & extended the test for it.
Comment on attachment 8966297 [details]
Bug 1449317 - Update the default string in the address bar.

This looks good given the fix comment 24.
Attachment #8966297 - Flags: ui-review?(mverdi) → ui-review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82d5734d2fea
Update the default string in the address bar. r=florian
Depends on: 1454776
Backout by toros@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf7ed4125f1a
Backed out changeset 82d5734d2fea for valgrind-test failures on a CLOSED TREE
I've re-reviewed the javascript, and I don't think I'm doing anything bad. A re-run try push on latest master still fails as well:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1941a5dc4ef4ca250827bbddb959e83a0220693&selectedJob=174274378

So I think this must be something in javascript land, needinfo to people suggested on the valgrind page, if you could help us out here that would be great, as I don't understand what's going on.
Flags: needinfo?(standard8)
Flags: needinfo?(n.nethercote)
Flags: needinfo?(jseward)
Depends on: 1454999
Note: I've filed bug 1454999 on the valgrind issue, help wanted please.
(In reply to Mark Banner (:standard8) from comment #30)
> Note: I've filed bug 1454999 on the valgrind issue, help wanted please.

I've replied in that bug. Looks like a GC issue.
Flags: needinfo?(n.nethercote)
Flags: needinfo?(jseward)
Comment on attachment 8969292 [details] [diff] [review]
Update the default string in the address bar (patch for beta).

This is the patch for beta, there was a little bit of additional work in browser.js to add the observer as that wasn't in 61.
Attachment #8969292 - Attachment description: Update the default string in the address bar → Update the default string in the address bar (patch for beta).
Attachment #8969292 - Flags: review?(florian)
Attachment #8969292 - Flags: review?(florian) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13bfd624b92e
Update the default string in the address bar. r=florian
QEVerify:

This is all about testing the placeholder string in the address bar.

Things to test:

- On startup the placeholder string shouldn't flicker.
- On first start on a fresh profile, it will be "Search or enter address"
- Changing the default search engine, visiting a url, or switching to a tab where a url is shown, will update the placeholder to "Search with <search engine name> or enter address".
--> In the cases where a url is displayed, you'll need to switch to a tab with no url (or open a new tab) to see the placeholder.
- On subsequent starts then it should be "Search with <search engine name> or enter address" where the name is the last selected default search engine name.
Flags: qe-verify+
Comment on attachment 8969292 [details] [diff] [review]
Update the default string in the address bar (patch for beta).

Approval Request Comment
[Feature/Bug causing the regression]: Update the placeholder in the address bar to better prompt users for the intention of the unified address bar.
[User impact if declined]: Product have requested this for 60, I believe drivers are aware. We'd like to get this done earlier to get feedback sooner.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Just landed on inbound (it was delayed by another bug - the other bug is not required for beta).
[Needs manual test from QE? If yes, steps to reproduce]: Yes, see previous comment
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Minor changes to a well tested area of code, small code change with its own tests.
[String changes made/needed]: None
Attachment #8969292 - Flags: approval-mozilla-beta?
This sounds pretty risky for late beta, and I would normally not take this patch a week before we build the release candidate. I would worry about possible issues in startup, possible crashes, whether this appears correctly on first use vs. afterwards, and I am not really sure what else to expect (l10n issues, ltr, etc?) 

Is this really crucial for 60? And, if so, can you help us stay on top of, say, startup crashes and incoming bugs over the next couple of weeks before the release?
Flags: needinfo?(standard8)
After discussion with Javaun in email, sounds like the product team really wants this in 60 if we can manage it.  

This hasn't landed on mozilla-central yet but it should get merged this evening.  If all goes well on m-c let's uplift to beta for the beta 15 build tomorrow. If we're going to try this for 60 I'd rather have beta feedback from users as early as possible before we build the release candidate.
Comment on attachment 8969292 [details] [diff] [review]
Update the default string in the address bar (patch for beta).

approved for 60.0b15 per email
Attachment #8969292 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/13bfd624b92e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #39)
> This sounds pretty risky for late beta, and I would normally not take this
> patch a week before we build the release candidate. I would worry about
> possible issues in startup, possible crashes, whether this appears correctly
> on first use vs. afterwards, and I am not really sure what else to expect
> (l10n issues, ltr, etc?) 

I'd be really surprised and worried if it gave us startup crashes. This is standard javascript/xul code and we're not doing anything fancy or new.

My biggest concern would be display issues - but they should only affect the placeholder itself, and there are tests covering it. These would hopefully be picked up quickly by the nightly & beta communities.
Flags: needinfo?(standard8)
I verified all of the scenarios from Comment 37, and they work as expected but I have some remarks:

- when a new empty tab is opened, clicking on the show history button from the search bar makes the dropdown flicker (see: https://www.screencast.com/t/PeFz2OOH )
- there are some search engines that can be added from the "Page actions" button, which if I select as default search engines, their name doesn't appear in the placeholder (so only "Search or enter address" text appears). Ex: yahoo
- there are also some that cannot be added from the "Page actions", but can be added from Options->One click Search Engines->Find more search engines that behave the same (no engine name shown in the placeholder). Ex: dogpile

Mark, can you take a look at these scenarios and tell me if they are in scope of this issue or should I create new bugs for these?
Until then, I will not mark the issue as Verified.
Flags: needinfo?(standard8)
(In reply to David Olah from comment #45)
> I verified all of the scenarios from Comment 37, and they work as expected
> but I have some remarks:
> 
> - when a new empty tab is opened, clicking on the show history button from
> the search bar makes the dropdown flicker (see:
> https://www.screencast.com/t/PeFz2OOH )

That's normal - an existing issue with new profiles (there's no history, so it can't actually display anything).

> - there are some search engines that can be added from the "Page actions"
> button, which if I select as default search engines, their name doesn't
> appear in the placeholder (so only "Search or enter address" text appears).
> Ex: yahoo

Sorry, I forgot to mention. Search engines that aren't in the built-in engines list won't have the name in the placeholder, and it should appear as "Search or enter address".

Hence, what you are seeing here is fine.

> - there are also some that cannot be added from the "Page actions", but can
> be added from Options->One click Search Engines->Find more search engines
> that behave the same (no engine name shown in the placeholder). Ex: dogpile

Yes, these are treated in the same way, so again this is expected.
Flags: needinfo?(standard8)
Based on Comment 45 and Comment 46 I am marking the issue as verified.
Status: RESOLVED → VERIFIED
I can confirm this issue is fixed, I verified using Fx 61.0a1 (build ID: 20180427102245), and Fx 60.0b16, on Windows 10 x64, mac os 10.13 and Ubuntu 16.06 LTS x86.
Depends on: 1463858
Depends on: 1503161
Regressions: 1591119
You need to log in before you can comment on or make changes to this bug.