Closed Bug 1049600 Opened 6 years ago Closed 6 years ago

Show search engine branding

Categories

(Firefox for Android Graveyard :: Search Activity, defect, P1)

All
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: antlam, Assigned: Margaret, NeedInfo)

References

Details

Attachments

(2 files, 8 obsolete files)

Attached image XXHDPI - Mobile - Pre typing 5.png (obsolete) —
Using this bug to track the "show current search engine being used" UI elements on the pre-search stage.
Priority: -- → P1
Attached image XXHDPI - Mobile - First run 2.png (obsolete) —
In light of bug 1045655, this is more along the lines of what it might look like.
Attachment #8468511 - Attachment is obsolete: true
Attached image suggest_rows_covered.png (obsolete) —
Hey Anthony --

I overlayed the button portion of your mock onto the current UI, and the proposed search engine icon covers the fourth suggestion.

I'm not comfortable losing 25% of our suggestions. Are there some alternatives that we could explore? Such as moving the icon to the search bar area?
Flags: needinfo?(alam)
I would agree. :) I think that the icon for search engine and settings should only be there when the space allows. This would mean giving precedence to suggestions, and if there weren't that many or any at all, we could show those.

I realize this isn't a perfect solution since there still exists a screen where the user could be without any visual branding on screen (like a Yahoo!, Bing, or Google logo) but I think this will do for MVP, for now. :\

I have a couple WIP mocks but since this touches on so many areas and we're already keeping tabs on the suggestions (typing) stage of this UI, I'd like to tackle this as a part of that. For now, I'd like to focus on the experience of searching and not so much around having branding. It's also important to not go overboard with it (not that we are right now) since it would also be harder to remove later on.
Flags: needinfo?(alam)
From IRC talk, it sounds like Anthony is still working on these designs, and there are going to be some upcoming meetings regarding partner branding.

Setting needinfo for antlam; no rush -- just ping back when you're ready to move forward.
Flags: needinfo?(alam)
Sorry for the miscommunication! I think that might have been my fault. 

But I think we should land this first and we can iterate later since this is still MVP.

Again, I don't think we need it to be there when the suggestions and history items aren't up. This label isn't as important as the provider icon.
Flags: needinfo?(alam)
Priority: P1 → --
Making this bug P1 again. NI to antlam to help figure out where we can put search engine branding.
Flags: needinfo?(alam)
Priority: -- → P1
Summary: Show which search engine is currently "default" → Show search engine branding in pre-search view
FYI, bug 1059537 will help us get images out of the search plugin XML.
Depends on: 1059537
Attached image prev_branding_overview1.png (obsolete) —
Attaching the latest WIP for this bug. I think we should leverage the icon as a launching point for the contextual menu that users see when they switch engines. You might notice the purple underline, that's branding purple so if they use Google, we could ideally switch it out to blue. Non-supported sites would just use our default orange.

One last thing to call out is the text in the cards are not vertically aligned with the input field in this mock but they are aligned with the suggestions (as per bug 1051973). Wouldn't pay too much mind to that yet - that's still an undecided detail. :)
Attachment #8471138 - Attachment is obsolete: true
Flags: needinfo?(mconnor)
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(alam)
For dynamic underline colors, the work in bug 1042960 could help here -- right now it's inside the private class, org.mozilla.search.ui.FacetBar.FacetButton.
Hardware: x86 → All
Attached image prev_branding_overview2.png (obsolete) —
Attaching the latest screen shot of the design for this bug.

As mentioned, there are concerns about the "typing phase" missing branding but let's start with something first, get it into a build and test that. I definitely get the concerns but we want to avoid scope creeping so I'd like to follow up and solve that screens issue separately.

Also, getting quick access to the contextual menu of Search engines to switch to should be the functionality behind that branding icon.
Attachment #8485086 - Attachment is obsolete: true
^ I MAY have a better solution after talking with Bryan! will work on this more over the weekend. :) stay tuned!
I'm updating the summary to make this bug more general, since it seems like we're going to try to address branding throughout the user flow here.

I just landed bug 1059537, so we can get the icon out of the search plugin now, and I started playing around with putting it in the search bar, but I'll stay tuned for new mock-ups!
Flags: needinfo?(margaret.leibovic)
Summary: Show search engine branding in pre-search view → Show search engine branding
Assignee: nobody → margaret.leibovic
Attached image XXHDPI - Mobile - First run 4.png (obsolete) —
(In reply to :Margaret Leibovic from comment #12)
> I'm updating the summary to make this bug more general, since it seems like
> we're going to try to address branding throughout the user flow here.

Good call! 

Wanted to show a general WIP, I'm working back towards moving the branding at the footer like a "powered by _____" sort of attribution. I know we had these designs filed in another bug but not recalling why we stepped away from that now..
Attached image prev_branding_overview3.png (obsolete) —
Pulling this back once again for V1, let's just get some footer attribution going alongside the colored active state.

With this approach, we will have to be smart with the "typing phase" and show suggestions over the branding icon if/when there are a lot of suggested items. I'm picturing a clear 'display or not' type relationship as opposed to a "how much of this icon image should we cover" to keep it simple for now.

On IRC we also mentioned possibly changing the placeholder text to drive home this message "Search with Yahoo!", "Search with Bing" for instance. Could we try that as a part of this bug too?

Thoughts?
Attachment #8485232 - Attachment is obsolete: true
Flags: needinfo?(margaret.leibovic)
(In reply to Anthony Lam (:antlam) from comment #14)
> Created attachment 8486126 [details]
> prev_branding_overview3.png
> 
> Pulling this back once again for V1, let's just get some footer attribution
> going alongside the colored active state.
> 
> With this approach, we will have to be smart with the "typing phase" and
> show suggestions over the branding icon if/when there are a lot of suggested
> items. I'm picturing a clear 'display or not' type relationship as opposed
> to a "how much of this icon image should we cover" to keep it simple for now.
>
> On IRC we also mentioned possibly changing the placeholder text to drive
> home this message "Search with Yahoo!", "Search with Bing" for instance.
> Could we try that as a part of this bug too?

Sure, that shouldn't be too hard to do.

> Thoughts?

I'll give this a shot. My main concern is that we're taking up some vertical space, but hiding the icon if there isn't room for it seems like a reasonable solution there. Let's just try this, and we can iterate.

It may be a bit hard to change the color of the underline, since that's created in a background drawable, but I'll see what I can do there.
Flags: needinfo?(margaret.leibovic)
Depends on: 1064880
Going back to branding on top right for V1. 

After feedback and discussing with Margaret some more, it offers the best blend of least amount of issues when dealing with states where keyboard is up, and most visible branding/attribution.

I feel like we're spinning our wheels a bit here. Let's try this in practice and iterate from there.
Attachment #8473828 - Attachment is obsolete: true
Attachment #8485983 - Attachment is obsolete: true
Attachment #8486126 - Attachment is obsolete: true
Attached patch Show search engine branding (obsolete) — Splinter Review
This is broken up into multiple commits in the PR here, but I was too lazy to `grunt export` each of those individually :)
https://github.com/mozilla/fennec-search/pull/91

Outstanding issues that need to be addressed:
* EditText#setBackground is API level 16+. I tried using the deprecated setBackgroundDrawable in an if statement for older versions, but my build was failing with an error about deprecation, and I need to investigate how to fix that. 
* Need to add support for getting colors from search plugins. I'm going to file a separate bug to do this, since it will involve modifying the search plugin XML.
* Need to address the issue of search engine icon resolution, but I think that should also go in a separate bug, since a fix for that would involve updating the search plugins (and this is an issue we've had in Fennec for a long time, so I'm worried it might not be totally straightforward to deal with).
Attachment #8489748 - Flags: review?(wjohnston)
Depends on: 1063703
Depends on: 1068407
Comment on attachment 8489748 [details] [diff] [review]
Show search engine branding

liuche can also review this. You two can race to see who reviews it first! :)
Attachment #8489748 - Flags: review?(liuche)
Comment on attachment 8489748 [details] [diff] [review]
Show search engine branding

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

I think this seems fine. Its a bit hard to review because of the way it removed/added files to hg.

::: mobile/android/search/java/org/mozilla/search/autocomplete/ClearableEditText.java
@@ -22,5 @@
> -import org.mozilla.gecko.Telemetry;
> -import org.mozilla.gecko.TelemetryContract;
> -import org.mozilla.search.R;
> -
> -public class ClearableEditText extends FrameLayout {

Is this an hg move? It looks like a delete/add.

::: mobile/android/search/java/org/mozilla/search/autocomplete/SearchBar.java
@@ +129,5 @@
> +    public void setEngine(SearchEngine engine) {
> +        int color = engine.getColor();
> +        if (color == Color.TRANSPARENT) {
> +            // Fall back to default orange if the search engine doesn't specify a color.
> +            color = getResources().getColor(R.color.highlight_orange);

Why not just have getColor return orange for you?

::: mobile/android/search/res/layout/search_bar.xml
@@ +1,5 @@
> +<!-- 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/. -->
> +
> +<merge xmlns:android="http://schemas.android.com/apk/res/android">

Same move/add here?
Attachment #8489748 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #19)
> Comment on attachment 8489748 [details] [diff] [review]
> Show search engine branding
> 
> Review of attachment 8489748 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this seems fine. Its a bit hard to review because of the way it
> removed/added files to hg.

Yeah, I'm sorry about that, I think that's a problem with our `grunt export
Gr, hit enter by accident...

(In reply to :Margaret Leibovic from comment #20)

> > I think this seems fine. Its a bit hard to review because of the way it
> > removed/added files to hg.
> 
> Yeah, I'm sorry about that, I think that's a problem with our `grunt export

` approach. If you look at the pull request, you can see the individual commits, which should be easier to review.

If I had exported each of the commits individually, I think I could have done the rename in hg.
Just noticed I didn't respond to this...

(In reply to Wesley Johnston (:wesj) from comment #19)

> ::: mobile/android/search/java/org/mozilla/search/autocomplete/SearchBar.java
> @@ +129,5 @@
> > +    public void setEngine(SearchEngine engine) {
> > +        int color = engine.getColor();
> > +        if (color == Color.TRANSPARENT) {
> > +            // Fall back to default orange if the search engine doesn't specify a color.
> > +            color = getResources().getColor(R.color.highlight_orange);
> 
> Why not just have getColor return orange for you?

I thought about that, but I decided that SearchEngine shouldn't be worried about the UI, it should just be providing access to the data stored in the search plugins. It may be that we'll want to use the brand color elsewhere, but default to something other than orange.

I also just added a commit that fixes some things for gingerbread:
https://github.com/leibovic/fennec-search/commit/92be8c8c33f3f6b136068ee10ded7bc1f3880401

I'll upload a new patch that does the hg renames instead, since that's better for hg history. It turns out it wasn't that hard - I just did the hg renames manually, then `grunt export` afterwards.
Attachment #8489748 - Attachment is obsolete: true
Attachment #8489748 - Flags: review?(liuche)
Blocks: 1050457
https://hg.mozilla.org/mozilla-central/rev/a482af02dd95
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.