Closed Bug 694291 Opened 12 years ago Closed 6 years ago

Removal of additional search field in Navigation Bar

Categories

(Firefox :: Search, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: shwin.lakes, Assigned: Paolo)

Details

(Whiteboard: [fxsearch])

Attachments

(2 files)

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.
You can remove it by Customize + Drag/Drop, but I fear you already know this ...
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.
Component: General → Search
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.
Assignee: nobody → paolo.mozmail
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
Whiteboard: [fxsearch]
OS: Linux → All
Hardware: x86_64 → All
Forgot to add that the pref will be off by default for now, so there will be no user-visible change.
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!
Assignee: paolo.mozmail → nobody
Status: ASSIGNED → NEW
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).
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).
I see, thanks for clarifying the scope of this, it makes sense if Shield is providing an easy pref-switching path.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
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.
Flags: needinfo?(past)
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.
Flags: needinfo?(past)
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).
Attachment #8872396 - Flags: review?(past) → review-
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.
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.
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 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/).
Attachment #8872396 - Flags: review?(past) → review+
(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!
Pushed by paolo.mozmail@amadzone.org:
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
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.
https://hg.mozilla.org/mozilla-central/rev/4448cfe7eed1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(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.
You need to log in before you can comment on or make changes to this bug.