694291 - Removal of additional search field in Navigation Bar

Status: RESOLVED FIXED

(Firefox :: Search, enhancement, P1)

Description

[Aswin Lakshman]

Attachment: Firefox: Showing unecessary search field

The Firefox user interface is unnecessarily cluttered with the inclusion of both an address bar, and a dedicated search bar. This dedicated search bar becomes essentially redundant, as the address bar also exhibits the behaviour of the search bar when a “non” address is entered into it.

This is an illogical design, and in order to improve cohesion and maintain a clean design, the dedicated search bar should be removed.

Reproducible: Always

Steps to Reproduce:
1.	Open the web browser
2.	Inspect the navigation toolbar

Actual Result:
Please refer to attachment. Firefox is shown with its unnecessarily cluttered Navigation bar.
 
Expected Results:
Firefox should possess a cleaner design, incorporating the two bars (address, and search) into one, as seen in browsers such as Chrome by Google.

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

You can remove it by Customize + Drag/Drop, but I fear you already know this ...

Comment 2

[Aswin Lakshman]

That may be so, however I am of the opinion that one should not have to do that. Firefox should posses a nice clean design "straight out of the box". Your average user will not know this.

There is no reason to have multiple ways to do the same thing. Especially if they are right next to each other.

Comment 3

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

We considered this feature request and we are going to add a pref to toggle the display of the search bar so that it will be easier to study how this change impacts our users.

Comment 4

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

Forgot to add that the pref will be off by default for now, so there will be no user-visible change.

Comment 5

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

I was too quick to assign this. It's not yet at the top of Paolo's list, so if anyone else has more cycles, have at it!

Comment 6

[Marco Bonardo [:mak]]

I'm not sure I understand why we would want to add a pref to hide the searchbar, a Shield add-on can do that trivially, and for the product needs it would be better to actually move the search bar to the customization palette (and could be done behind one of the photon prefs).

Comment 7

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

The point is precisely to avoid having to build a Shield add-on and instead rely on the new Shield preference system, which is much easier to use (also it's much easier to get approval for uplifting a pref flip if necessary). In terms of implementation, I think what you describe is very much what the pref flipping should do, i.e. move the search bar to the customization palette).

Comment 8

[Marco Bonardo [:mak]]

I see, thanks for clarifying the scope of this, it makes sense if Shield is providing an easy pref-switching path.

Comment 9

[:Paolo Amadini]

Attachment: Bug 694291 - Add a preference mirroring the presence of the search widget in the navigation toolbar.

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

Comment 10

[:Paolo Amadini]

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

Comment 11

[:Paolo Amadini]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5f0eea3a6c7578d285bafee0be5352869c6f774f&newProject=try&newRevision=6eef9a6fde61209e947c088ad03428b1aa051ff6&framework=1&showOnlyImportant=0

Comment 12

[:Paolo Amadini]

It occurred to me only now that we need to synchronize the preference to match the widget state if this code takes effect after an update, or if the same profile is used on a different version that does not keep the preference in sync. For the former case, we might add a preference that tells whether we've already migrated or not, but for the latter case I'm not aware of any way to distinguish between when the preference is flipped and this is what we should consider, or the widget is moved and this is what we should consider...

The simplest thing might be to always give priority to the widget state, but in this case we should also make sure that any preference flipping done by experiments takes effect while Firefox is running, both for setup and teardown.

Comment 13

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

Agreed that the migration case is very important. Changing the default value either via an add-on or a patch in the tree should be reflected in the UI. If we weren't interested in the add-on case a UI migration would have sufficed, but it doesn't seem to be particularly useful in this case.

I'm less concerned about downgrading/sharing profiles. Ideally we would handle this case, but if it's twice the effort I'd say don't bother.

Comment 14

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

Comment on attachment 8872396 [details]
Bug 694291 - Add a preference mirroring the presence of the search widget in the navigation toolbar.

https://reviewboard.mozilla.org/r/143904/#review147874

Didn't have a chance to play with the patch yet, but I have some coments and there is the migration case that needs handling so I'll just give you what I have so far.

::: browser/components/customizableui/test/browser_694291_searchbar_preference.js:3
(Diff revision 1)
> +/* 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: public domain license for test files please.

::: browser/components/customizableui/test/browser_694291_searchbar_preference.js:15
(Diff revision 1)
> +function checkDefaults() {
> +  // If the following defaults change, then the DEFAULT_AREA_PLACEMENTS of
> +  // UITelemetry.jsm, the navbarPlacements of CustomizableUI.jsm, and the
> +  // position and attributes of the search-container element in browser.xul
> +  // should also change at the same time.
> +  ok(Services.prefs.getBoolPref(PREF_NAME));

Doesn't this mean that whenever we flip the default value this test must be updated? This would complicate the patch to do so and a simple pref-flipping Shield experiment.

The test would be more robust if we explicitly set the pref to true in the beginning (just like the photon pref) and then proceeded to test both values (as you already do).

Comment 15

[:Paolo Amadini]

I'd note that the default widgets of a new profile only reflect what is already in "browser.xul", and any patch that changes the default must also change "browser.xul". This is because we don't want to move things around unnecessarily in the main interface to improve performance on first startup. This is different from the case of an experiment that is installed afterward for a subset of users, so I believe we should clarify the goals here.

The test is intentionally designed to fail if someone just changes the defaults of the widget or the preference separately. The comment in the test contains a reminder of the required code changes, including obviously modifying the test itself. There are probably many more tests that would fail as well.

Comment 16

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

OK, that makes sense. So let's only ensure that Shield pref flipping works and we will deal with a potential default change in a followup.

Comment 17

[:Paolo Amadini]

Comment on attachment 8872396 [details]
Bug 694291 - Add a preference mirroring the presence of the search widget in the navigation toolbar.

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

Comment 18

[:Paolo Amadini]

Following the considerations above, for the moment I've removed any syncrhonization of the preference value on startup, and I think the rest of the patch is still good. The key feature is that switching the preference to false at runtime will remove the search bar from the toolbar.

Comment 19

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

Comment on attachment 8872396 [details]
Bug 694291 - Add a preference mirroring the presence of the search widget in the navigation toolbar.

https://reviewboard.mozilla.org/r/143904/#review149218

::: browser/components/customizableui/SearchWidgetTracker.jsm:36
(Diff revision 2)
> +    CustomizableUI.addListener(this);
> +    Services.prefs.addObserver(PREF_NAME,
> +                               () => this.syncWidgetWithPreference());

Isn't it necessary to remove the observer and listener at some point?

::: browser/components/customizableui/test/browser_694291_searchbar_preference.js:3
(Diff revision 2)
> +/* 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/. */

Test files should have a public domain dedication (https://www.mozilla.org/en-US/MPL/license-policy/).

Comment 20

[:Paolo Amadini]

(In reply to Panos Astithas [:past] (please needinfo?) from comment #19)
> Isn't it necessary to remove the observer and listener at some point?

Like other similar modules in this folder, this one is loaded until the session terminates, and the platform already does the right thing with reference handling on shutdown.

> Test files should have a public domain dedication
> (https://www.mozilla.org/en-US/MPL/license-policy/).

Ah, I just forgot about addressing this comment from the previous review. Thanks!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4448cfe7eed1
Add a preference mirroring the presence of the search widget in the navigation toolbar. r=past

Comment 22

[Gary [:streetwolf52]]

Might a make a suggestion?  The search bar allows me to click on a search engine and I will be taken there without having to place anything in the search bar itself.  However, the URL bar requires a search argument.  If you want the URL bar to have the same functionality as the search bar perhaps you should allow no search items too.  I make use of this using Google Maps for example.  It will take me right to GM where I most often get directions from the places that I saved in Google Maps. On the URL bar I have to put at least one character on the bar in order to get me to Google Maps.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4448cfe7eed1

Comment 24

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

(In reply to Gary [:streetwolf] from comment #22)
> Might a make a suggestion?  The search bar allows me to click on a search
> engine and I will be taken there without having to place anything in the
> search bar itself.  However, the URL bar requires a search argument.  If you
> want the URL bar to have the same functionality as the search bar perhaps
> you should allow no search items too.  I make use of this using Google Maps
> for example.  It will take me right to GM where I most often get directions
> from the places that I saved in Google Maps. On the URL bar I have to put at
> least one character on the bar in order to get me to Google Maps.

This sounds reasonable, filed bug 1370000.