Closed Bug 1162144 Opened 7 years ago Closed 7 years ago

Add link to search settings to AwesomeBar

Categories

(Firefox :: Search, defect, P1)

40 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 42
Iteration:
41.3 - Jun 29
Tracking Status
firefox40 --- affected
firefox42 --- verified

People

(Reporter: kev, Assigned: mossop)

References

Details

(Whiteboard: [suggestions][fxsearch])

User Story

As a user, I will be able to control whether text entered in the location bar is shared with a search provider to receive suggested searches to provide me with better control over what information I share with third parties.

Attachments

(3 files, 4 obsolete files)

Allow users to disable Search Suggests from the Awesomebar (e.g. provide a method where users can get at the search settings, for the purposes of disabling suggests, from the awesomebar)
Rank: 5
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [fxsearch][searchsuggestions] → [suggestions][fxsearch]
Steve - attach design :)
Assignee: nobody → dtownsend
Iteration: --- → 41.1 - May 25
Flags: needinfo?(shorlander)
The Search Settings button is part of the footer which will include the one-off search buttons; e.g. http://people.mozilla.org/~shorlander/Flare/Flare-Search-Unification-%28OSX%29-03.png

We can add the footer and the Search Settings button independently of the rest.
Flags: needinfo?(shorlander) → needinfo?(dtownsend)
Looks good, thanks
Flags: needinfo?(dtownsend)
Seems like there is some gradient in the image. Can you confirm the following colours:

Footer bar background
Footer bar top border
Button hover
Button active
Flags: needinfo?(shorlander)
(In reply to Dave Townsend [:mossop] from comment #4)
> Seems like there is some gradient in the image. Can you confirm the
> following colours:
> 
> Footer bar background
> Footer bar top border
> Button hover
> Button active

Sorry, I left the translucency in the mockup:

Footer bar background — hsla(210,4%,10%,.07); 
Footer bar top border — hsla(210,4%,10%,.14);
Button hover — hsla(210,4%,10%,.07);
Button active — hsla(210,4%,10%,.12);

I pulled these from the panel footer for consistency:
https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUIOverlay.inc.css#488
Flags: needinfo?(shorlander)
Attached patch patch (obsolete) — Splinter Review
Turns out to be pretty straightforward. There isn't a lot in the shared browser.inc stylesheet, should I be putting the rules in the per-platform stylesheets?
Attachment #8609651 - Flags: review?(dao)
Comment on attachment 8609651 [details] [diff] [review]
patch

>+      <hbox class="urlbar-search-footer" flex="1" align="stretch" pack="end">
>+        <button class="urlbar-search-settings" label="&changeSearchSettings.button;"

You can use id instead of class since these aren't supposed to be used elsewhere.

>+.urlbar-search-settings {
>+  -moz-appearance: none;
>+  min-height: 32px;
>+  margin: 0;
>+  color: #666;
>+  padding: 0 20px;

Please add vertical padding and get rid of the min-height.

Behind the hsla background, there's a native background which isn't necessarily white, so instead of #666 you should use a platform color for the text, such as graytext.

> Turns out to be pretty straightforward. There isn't a lot in the shared
> browser.inc stylesheet, should I be putting the rules in the per-platform
> stylesheets?

Yes, browser.inc is only used for defines.
Attachment #8609651 - Flags: review?(dao) → review-
Status: NEW → ASSIGNED
Flags: qe-verify?
Iteration: 41.1 - May 25 → 41.2 - Jun 8
Attached patch patch (obsolete) — Splinter Review
Attachment #8609651 - Attachment is obsolete: true
Attachment #8611432 - Flags: review?(dao)
Comment on attachment 8611432 [details] [diff] [review]
patch

* I can't seem to click the button. When I try to, the popup just closes

* the button seems inaccessible by keyboard

* the styling is broken on Linux (screenshot coming), likely affects Windows too
Attachment #8611432 - Flags: review?(dao) → review-
Attached image screenshot (obsolete) —
Stephen, how do you want keyboard selection to work for this button? In unified autocomplete land when you type the urlbar has focus and there is always at least one item in the dropdown list highlighted. Tabbing keeps focus on the url bar and instead moves up and down through the list. I guess we could make tabbing at the end of the list drop focus from the url bar, deselect the list item and then focus the settings button, I'm wary that that might break assumptions that there is always one list item selected though. Even then what happens after you press tab again?
Flags: needinfo?(shorlander)
(In reply to Dão Gottwald [:dao] from comment #9)
> Comment on attachment 8611432 [details] [diff] [review]
> patch
> 
> * I can't seem to click the button. When I try to, the popup just closes

This is weird. Seems to be ok on windows and OSX. On Linux I can use the button the first time the awesomebar shows but not after that.

> * the button seems inaccessible by keyboard

Depending on when UX decide what the right thing is here it might make sense to take this without keyboard accessibility at first.

> * the styling is broken on Linux (screenshot coming), likely affects Windows
> too

There's a simple fix for this, I forgot the other platforms include a default border for buttons.
Just a question, the toggles to control what's suggested in Autocomplete are under Privacy, so I'd expect a Search Suggestions toggle there. But here we are linking Search prefs to change the engine.
I suppose we are going to have both this link and a link to privacy settings (or a direct toggle for suggestions, duped under privacy), implemented through bug 959567?
Iteration: 41.2 - Jun 8 → 41.3 - Jun 29
(In reply to Marco Bonardo [::mak] from comment #13)
> Just a question, the toggles to control what's suggested in Autocomplete are
> under Privacy, so I'd expect a Search Suggestions toggle there. But here we
> are linking Search prefs to change the engine.
> I suppose we are going to have both this link and a link to privacy settings
> (or a direct toggle for suggestions, duped under privacy), implemented
> through bug 959567?

I believe that this is the intent yes, I'll make sure Kev is on it though
Attached patch patch (obsolete) — Splinter Review
For now this defers on the keyboard accessibility piece and that makes making the button work on linux easy since it was the urlbar losing focus that was causing the popup to close too soon. This adds a test too.
Attachment #8621318 - Flags: review?(dao)
Attached image screenshot
Closer but still not quite there. You'll need to remove the button's default background color, but be aware that this might make the contrast between the gray background and the text color too small.
Attachment #8611432 - Attachment is obsolete: true
Attachment #8612242 - Attachment is obsolete: true
Attachment #8621318 - Flags: review?(dao) → review-
Attached patch patchSplinter Review
Updated with transparent backgrounds on windows and linux. Windows looked fine, linux didn't as you suspected. I used the color #666 for the text there, that matches that of the same button in the search bar drop-down so I assume that is ok. I don't know if there is a platform colour that would work here.
Attachment #8621318 - Attachment is obsolete: true
Attachment #8623221 - Flags: review?(dao)
Comment on attachment 8623221 [details] [diff] [review]
patch

>--- a/browser/themes/linux/browser.css
>+++ b/browser/themes/linux/browser.css

>+#urlbar-search-footer {
>+  border-top: 1px solid hsla(210,4%,10%,.14);
>+  background-color: hsla(210,4%,10%,.07);
>+}
>+
>+#urlbar-search-settings {
>+  -moz-appearance: none;
>+  -moz-user-focus: ignore;
>+  color: #666;
>+  margin: 0;
>+  border: 0;
>+  padding: 8px 20px;
>+  background: transparent;
>+}

#666 isn't guaranteed to be legible here. Let's just use 'color: inherit' on Linux for now.

I assume you'll only want to land this after the next merge since browser.urlbar.suggest.searches is only true for NIGHTLY_BUILD right now...

I think we should also consider linking to about:preferences#privacy since that's where all the other location prefs are.
Attachment #8623221 - Flags: review?(dao) → review+
Landed with that color change. I left the pref pane alone for now, shorlander was talking about maybe moving the suggestions settings around so we'll maybe adjust after that.
Flags: needinfo?(shorlander)
https://hg.mozilla.org/mozilla-central/rev/9976683b3e5a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
I think this footer is a little bit too prominent, should it be the same height as the one in the search bar? It is quite taller
Flags: needinfo?(shorlander)
(In reply to Marco Bonardo [::mak] from comment #22)
> I think this footer is a little bit too prominent, should it be the same
> height as the one in the search bar? It is quite taller

It's consistent with the other panel footers (bookmarks panel, history panel).
(In reply to Tim Nguyen [:ntim] from comment #23)
> (In reply to Marco Bonardo [::mak] from comment #22)
> > I think this footer is a little bit too prominent, should it be the same
> > height as the one in the search bar? It is quite taller
> 
> It's consistent with the other panel footers (bookmarks panel, history
> panel).

then it's the searchbar footer that it's not consistent? From the mockups the locationbar and searchbar footers look the same.
Also the mock-up attached in this same bug states it should be 32px
https://bug1162144.bugzilla.mozilla.org/attachment.cgi?id=8607550
Mine is like 41px
(In reply to Marco Bonardo [::mak] from comment #22)
> I think this footer is a little bit too prominent, should it be the same
> height as the one in the search bar? It is quite taller

Maybe? It's a little hard to evaluate in its current state since it was designed with the search tiles and the rest of the panel changes in mind. But I agree it feels out of place right now.
Flags: needinfo?(shorlander)
Depends on: 1181173
Flags: qe-verify? → qe-verify+
QA Contact: petruta.rasa
Checked with latest Nightly 42.0a1 2015-08-02 under Mac OS X 10.10.4, Win 7 64-bit, Ubuntu 12.04 32-bit using OS's default themes.

* The Change Search Settings button from Location Bar drop-down, opens about:preferences#search page as per design, but instead it should open about:preferences#privacy - Location Bar as stated by User Story and comment 0. (also mentioned in above comments).

* The footer's height for me is 38px on Windows and 33px on Mac.

Considering that bug 959567 will fix these points, I'm marking this bug as verified.
Status: RESOLVED → VERIFIED
(In reply to Petruta Rasa [QA] [:petruta] from comment #26)
> * The Change Search Settings button from Location Bar drop-down, opens
> about:preferences#search page as per design, but instead it should open
> about:preferences#privacy - Location Bar as stated by User Story and comment
> 0. (also mentioned in above comments).

Nope, it should open "search", always. the opt-in/out will open "privacy".

> * The footer's height for me is 38px on Windows and 33px on Mac.
> 
> Considering that bug 959567 will fix these points, I'm marking this bug as
> verified.

no, that bug won't touch anything that has been added here.
(Not sure who I should be asking, so I'm starting with Dave as the assignee for this bug. If you're not the right person, can you please recommend someone?)

I'm planning to add a few customization features for the location bar into my add-on, one of which includes adding the same "change search settings" button into Firefox release/beta and modifying nigthly's existing one for consistency. *BUT* clicking it sends you to the add-on's preferences tab, which has its own set of options of course, which in turn has another button to redirect to Firefox's options-search pane; this will happen in all Firefox versions.

Should I:
- keep the telemetry counting when clicking the button in that case?
- change the argument "urlbar" into something else like "urlbar-awesomer"? 
- do either of those *only* in Firefox release/beta or *also* in nightly builds? 
- take the telemetry call out completely in every case because it's being added/modified by an add-on?

What's the best way to not screw up telemetry results by users of the add-on?
Flags: needinfo?(dtownsend)
(In reply to Luís Miguel [:quicksaver] from comment #28)
> (Not sure who I should be asking, so I'm starting with Dave as the assignee
> for this bug. If you're not the right person, can you please recommend
> someone?)
> 
> I'm planning to add a few customization features for the location bar into
> my add-on, one of which includes adding the same "change search settings"
> button into Firefox release/beta and modifying nigthly's existing one for
> consistency. *BUT* clicking it sends you to the add-on's preferences tab,
> which has its own set of options of course, which in turn has another button
> to redirect to Firefox's options-search pane; this will happen in all
> Firefox versions.
> 
> Should I:
> - keep the telemetry counting when clicking the button in that case?
> - change the argument "urlbar" into something else like "urlbar-awesomer"? 
> - do either of those *only* in Firefox release/beta or *also* in nightly
> builds? 
> - take the telemetry call out completely in every case because it's being
> added/modified by an add-on?
> 
> What's the best way to not screw up telemetry results by users of the add-on?

I have no idea, I'd try asking the #telemetry folks in IRC, but I'm guessing that unless you have a smash hit on your hands it won't be enough to sway results much.
Flags: needinfo?(dtownsend)
You need to log in before you can comment on or make changes to this bug.