1042960 - Create UI elements for facet bar

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

Right now our "terror script" rips out the whole results page header, including the facets. We should see if we can add our own native version of these facets, or if not, we should consider updating the script to leave the facets in place.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: XXHDPI - Mobile - Web 3.png

Attaching a standing mockup just to give a quick reference (especially to how the facets look:)

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Just wanted to mention that the facets have the underline which should be the search engine's brand color, in the example, I used Yahoo!'s purple.

Comment 3

[Eric Edens [:eedens]]

(In reply to Anthony Lam (:antlam) from comment #2)
> Just wanted to mention that the facets have the underline which should be
> the search engine's brand color, in the example, I used Yahoo!'s purple.

Cool. Can you post the hex values of the colors you'd like for Bing, Yahoo, and Google?

Comment 4

[Eric Edens [:eedens]]

Also, what is the "More" tab?

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Eric Edens [:eedens] from comment #3)
> Cool. Can you post the hex values of the colors you'd like for Bing, Yahoo,
> and Google?

Bing: #F4BD27

Yahoo: #400090

Google: #1E6FEA

> Also, what is the "More" tab?

Just a placeholder for now since some engines have quite a bit of facets (more than we have space for). It was essentially a way to group overflow but I'm not sure if we even need that right now. We might have to revisit that depending on how much we want to pull at that thread.

Comment 6

[Eric Edens [:eedens]]

Thanks!

> Just a placeholder for now 

For right now, all three engines support news. So we could have these four facets:
 Web
 Images
 Video
 News

Later we could explore the overflow group. Does that sound alright?

A couple of questions:
 - How are the buttons sized horizontally? 25% of the width?
 - How does the size change in landscape mode?

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Eric Edens [:eedens] from comment #6)
> For right now, all three engines support news. So we could have these four
> facets:
>  Web
>  Images
>  Video
>  News
> 
> Later we could explore the overflow group. Does that sound alright?

Yeah, that sounds alright for now!

> A couple of questions:
>  - How are the buttons sized horizontally? 25% of the width?

Yeah that's what I'm thinking.

>  - How does the size change in landscape mode?

If it's % based for now it should just scale, we can consider fixing the width of the buttons later which should allow us to expose additional facets (the ones in the "more" category)

Comment 8

[Eric Edens [:eedens]]

Narrowing the scope of this bug to UI elements (which is what we've been discussing here). The new cover bug for facets is bug 1056302.

Comment 9

[Eric Edens [:eedens]]

Hey Anthony --

How do the buttons behave when pressed?

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_search_facet_down.png

Can we switch the background color to #FAFAFA on tap?

Comment 11

[Eric Edens [:eedens]]

Attachment: APK 1: Individual Buttons

Hey Anthony, 

Here's the first pass at the buttons -- I did my best to grab the values from [8461129: XXHDPI - Mobile - Web 3.png]. Are you alright with the colors and the dimensions?

(Nothing happens when the buttons are clicked)

If you're alright with the individual buttons, I'd like to handle the button bar next. The tricky part will be handling sizing / padding in different corners cases:

- Text overflowing the 25% allotment (eg: internationalized words)
- Less than four buttons
- More than four buttons

To simplify things, how would you feel about a horizontal scroll with fixed horizontal padding on each button? The Google Play store is a good example of this: http://cdn.redmondpie.com/wp-content/uploads/2013/04/nexus_4_2013-4-26-18-13-2.png

Comment 12

[:Margaret Leibovic]

(In reply to Eric Edens [:eedens] from comment #11)
> Created attachment 8477621 [details]
> APK 1: Individual Buttons
> 
> Hey Anthony, 
> 
> Here's the first pass at the buttons -- I did my best to grab the values
> from [8461129: XXHDPI - Mobile - Web 3.png]. Are you alright with the colors
> and the dimensions?
> 
> (Nothing happens when the buttons are clicked)
> 
> If you're alright with the individual buttons, I'd like to handle the button
> bar next. The tricky part will be handling sizing / padding in different
> corners cases:
> 
> - Text overflowing the 25% allotment (eg: internationalized words)
> - Less than four buttons
> - More than four buttons

Since we'll be specifying these facets ourselves right now, I don't think we should worry about such robust UI support at the moment. For a first pass, I think we can just limit the UI to four buttons. Even with four buttons, low resolution devices and localization will probably still give us trouble, but I think we should just punt that to a follow-up.

I think it would be good to get some basic UI in place so that we can iterate on the search plugin side of this support.

Comment 13

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8477621 [details]
APK 1: Individual Buttons

Looks good! (except that underline should be the Yahoo! purple? since it was the engine I was using :)

I'd also agree with Margaret, I don't think we need to progress that far for this. I'd say there's other elements that could use the polishing efforts more.

Comment 14

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #13)
> Comment on attachment 8477621 [details]
> APK 1: Individual Buttons
> 
> Looks good! (except that underline should be the Yahoo! purple? since it was
> the engine I was using :)

We'll need to wait until we integrate with open search plugins before we can do this. But let's file a follow-up bug for this.

Comment 15

[Eric Edens [:eedens]]

Attachment: bug-1042960-wip.patch

From the previous GH review, this adds FacetBar.java, which is a radio group that manages the state of the facet buttons.

This patch includes sample code in the post search fragment that shows how the view is instantiated. For landing, I'll make a patch that doesn't touch PostSearchFragment.java and search_fragment_post_search.xml.

Lastly, I expect we'll need more handler code when implementing the UI later. For example, when adding a new facet, if the facet is set as checked, the onCheckChange listener will be fired. I'm not sure if that'll be the correct behavior. So we can address that when implementing the UI.

Comment 16

[Eric Edens [:eedens]]

Comment on attachment 8480719 [details] [diff] [review]
bug-1042960-wip.patch

Hey Lucas --

This is my first custom onDraw method. Does FacetButton.onDraw look alright to you?

Thanks!

Comment 17

[:Margaret Leibovic]

Comment on attachment 8480719 [details] [diff] [review]
bug-1042960-wip.patch

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

This looks like it's on the right track! I'm curious to hear what lucasr has to say as well.

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +40,5 @@
>      public View onCreateView(LayoutInflater inflater, ViewGroup container,
>                               Bundle savedInstanceState) {
>          View mainView = inflater.inflate(R.layout.search_fragment_post_search, container, false);
>  
> +        // Temporary code to illustrate how the facet bar is instantiated.

Why temporary? How do you intend to actually add this view?

If the buttons don't work yet (and they won't with just this patch), we can land this but hide the view until the backend bits are hooked up (or we should switch gears and work on the backend bits before landing this).

@@ +50,5 @@
> +        // Facets could come from open search plugins.
> +        facetBar.addFacet("Web", true);  // Indicates that "Web" is currently selected.
> +        facetBar.addFacet("Images");
> +        facetBar.addFacet("Video");
> +        facetBar.addFacet("News");

I wonder if it would be better for performance if we have an API that lets us batch these additions. Although if the view isn't attached yet, I would assume layout passes wouldn't be happening, so this probably isn't actually bad at all.

::: mobile/android/search/java/org/mozilla/search/ui/FacetBar.java
@@ +1,1 @@
> +package org.mozilla.search.ui;

Nit: Remember to add license header.

@@ +80,5 @@
> +     */
> +    private static class FacetButton extends RadioButton {
> +
> +        // Default thickness is 11dp.
> +        private static final float UNDERLINE_THICKNESS = 11 * Resources.getSystem().getDisplayMetrics().density;

Instead of doing this calculation ourselves, can we declare this value in dimens.xml and use getDimensionPixelSize to get the correct value?

::: mobile/android/search/res/drawable/facet_button_background.xml
@@ +6,5 @@
> +    <!--facet button is pressed (omitting currently-selected facet)-->
> +    <item
> +        android:state_pressed="true"
> +        android:state_checked="false"
> +        android:drawable="@drawable/facet_button_background_pressed"/>

Do you need to create new drawable files for this? Can you just specify the color directly instead?

::: mobile/android/search/res/layout/search_fragment_post_search.xml
@@ +15,5 @@
>          android:layout_height="@dimen/progress_bar_height"
>          android:progressDrawable="@drawable/progressbar"/>
>  
> +    <!--Sample implementation of facet bar.-->
> +    <!--Height is determined by values/search_styles/FacetButtonStyle.-->

It seems like it would be clearer if the height was just determined here (alternately: what if other consumers wanted to change the height?). Can we just put that style declaration in here?

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 8480719 [details] [diff] [review]
bug-1042960-wip.patch

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

Looks good with the suggested changes.

::: mobile/android/search/java/org/mozilla/search/ui/FacetBar.java
@@ +106,5 @@
> +        @Override
> +        protected void onDraw(Canvas canvas) {
> +            super.onDraw(canvas);
> +            if (isChecked()) {
> +                canvas.drawLine(getMeasuredWidth(), getMeasuredHeight(), 0, getMeasuredHeight(), underlinePaint);

When onDraw() is called, a full layout pass has already been done on the view. This means you should use getWidth()/getHeight() instead. Don't you have to account for the stroke width to avoid painting the line partially outside view bounds?

Comment 19

[Eric Edens [:eedens]]

Comment on attachment 8480719 [details] [diff] [review]
bug-1042960-wip.patch

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

::: mobile/android/search/java/org/mozilla/search/PostSearchFragment.java
@@ +40,5 @@
>      public View onCreateView(LayoutInflater inflater, ViewGroup container,
>                               Bundle savedInstanceState) {
>          View mainView = inflater.inflate(R.layout.search_fragment_post_search, container, false);
>  
> +        // Temporary code to illustrate how the facet bar is instantiated.

I was hoping to land the UI elements hidden, and then use bug 1056302 for wiring everythign together.

So the landing patch for this wouldn't include PostSearchFragment and its associated xml layout.

@@ +50,5 @@
> +        // Facets could come from open search plugins.
> +        facetBar.addFacet("Web", true);  // Indicates that "Web" is currently selected.
> +        facetBar.addFacet("Images");
> +        facetBar.addFacet("Video");
> +        facetBar.addFacet("News");

Hmm, I hadn't thought about this yet, or done any profiling. If we see performance regressions, we can always do a followup?

::: mobile/android/search/java/org/mozilla/search/ui/FacetBar.java
@@ +1,1 @@
> +package org.mozilla.search.ui;

Thanks :)

@@ +80,5 @@
> +     */
> +    private static class FacetButton extends RadioButton {
> +
> +        // Default thickness is 11dp.
> +        private static final float UNDERLINE_THICKNESS = 11 * Resources.getSystem().getDisplayMetrics().density;

I like that idea!

@@ +106,5 @@
> +        @Override
> +        protected void onDraw(Canvas canvas) {
> +            super.onDraw(canvas);
> +            if (isChecked()) {
> +                canvas.drawLine(getMeasuredWidth(), getMeasuredHeight(), 0, getMeasuredHeight(), underlinePaint);

Thanks! And you were right: half of the line was getting clipped.

::: mobile/android/search/res/drawable/facet_button_background.xml
@@ +6,5 @@
> +    <!--facet button is pressed (omitting currently-selected facet)-->
> +    <item
> +        android:state_pressed="true"
> +        android:state_checked="false"
> +        android:drawable="@drawable/facet_button_background_pressed"/>

That's what I had hoped since it would be simpler.  Apparently, though, background selectors require drawables and not colors (http://stackoverflow.com/a/5295522)

Comment 20

[Eric Edens [:eedens]]

Attachment: bug-1042960.patch

This patch includes fixes from the feedback, and removes the facet buttons from the UI. In bug 1056302, we can expose the working buttons.

Comment 21

[Eric Edens [:eedens]]

Attachment: bug-1042960.v2.patch

Rebased against Bug 1057634 & Bug 1057629

Comment 22

[:Margaret Leibovic]

Update: we decided that we don't strictly need native facets for a v1 release, but we do want to keep making progress on this.

Comment 23

[:Margaret Leibovic]

Comment on attachment 8481512 [details] [diff] [review]
bug-1042960.v2.patch

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

Looks like a good first version to land.

::: mobile/android/search/java/org/mozilla/search/ui/FacetBar.java
@@ +108,5 @@
> +        protected void onDraw(Canvas canvas) {
> +            super.onDraw(canvas);
> +            if (isChecked()) {
> +                // Translate the line upward so that it isn't clipped by the button's boundary.
> +                final float yPos = getHeight() - underlinePaint.getStrokeWidth() / 2;

Why does this need to be divided by 2?

::: mobile/android/search/strings/search_strings.xml.in
@@ +19,5 @@
> +
> +    <string name="search_facet_web">&search_facet_web;</string>
> +    <string name="search_facet_image">&search_facet_image;</string>
> +    <string name="search_facet_video">&search_facet_video;</string>
> +    <string name="search_facet_news">&search_facet_news;</string>

These strings aren't used. Let's remove them for now.

Comment 24

[Eric Edens [:eedens]]

Comment on attachment 8481512 [details] [diff] [review]
bug-1042960.v2.patch

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

::: mobile/android/search/java/org/mozilla/search/ui/FacetBar.java
@@ +108,5 @@
> +        protected void onDraw(Canvas canvas) {
> +            super.onDraw(canvas);
> +            if (isChecked()) {
> +                // Translate the line upward so that it isn't clipped by the button's boundary.
> +                final float yPos = getHeight() - underlinePaint.getStrokeWidth() / 2;

The center of the line gets painted at the bottom of the button, with half of the stroke going up, and the other half getting clipped. Dividing the stroke's width by 2 accounts for the amount that is being clipped. I'll add a comment to make that clearer.

::: mobile/android/search/strings/search_strings.xml.in
@@ +19,5 @@
> +
> +    <string name="search_facet_web">&search_facet_web;</string>
> +    <string name="search_facet_image">&search_facet_image;</string>
> +    <string name="search_facet_video">&search_facet_video;</string>
> +    <string name="search_facet_news">&search_facet_news;</string>

SGTM

Comment 25

[Eric Edens [:eedens]]

Attachment: bug-1042960.v3.patch

Fresh patch for landing:
 - Added comments to explain division for line offset
 - Removed unused string resources

Comment 26

[:Margaret Leibovic]

Comment on attachment 8484340 [details] [diff] [review]
bug-1042960.v3.patch

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

Looks good. I'll land this when fx-team re-opens.

Comment 27

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/b76337ab75bd

Comment 28

[:Margaret Leibovic]

https://github.com/mozilla/fennec-search/commit/31e7cda6d428ea815e4e983f5dae22e5c6a5e28a

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b76337ab75bd