1042937 - Provide visual feedback while search results are loading

Status: VERIFIED FIXED

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

Description

[:Margaret Leibovic]

This issue came up at our check-in meeting this morning, but I'm unclear on what exactly we want to implement as a solution.

ibarlow or antlam, could you elaborate?

Comment 1

[Ian Barlow (:ibarlow)]

We just want to display some kind of preloader animation while the results page is coming in. Anthony is playing around with some designs, but even the 'orange laser show' animation under the title bar could be a good placeholder to start with.

Comment 2

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

I've been working on this today, hopefully I'll have something to post soon. But like Ian said, standard Android loading bar could be a good place holder for the time being (right above the white content area).

Leaving NI on..

Comment 3

[Eric Edens [:eedens]]

Attachment: bug-1042937-wip.patch

I tried the horizontal progress bar, but it was floating in the middle of nowhere (since the background / toolbar are all the same color)

So, as a placeholder, this adds a spinner to the top right of the screen.

Comment 4

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

Attachment: prev_samvp_loadingbarpos.png

Hey Eric,

Maybe this mock will help you out with the position. I have it 68dp from the bottom of the top notification bar and it's 2 dp in height.

Comment 5

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

I'm also experimenting with a centered loading animation since that area that expands from the suggestion stage before this step will direct user attention to that area anyways (plus they can't really do anything until it's loaded anyways)

Comment 6

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

Attachment: Screenshot_2014-07-24-16-48-32.png

attaching current status from the build Eric sent as reference.

Comment 7

[Eric Edens [:eedens]]

Attachment: bug-1042937-wip.v2.patch

Changes:
 - Spinner is now orange bar

Comment 8

[Eric Edens [:eedens]]

Attachment: Screenshot_2014-07-24-17-03-47.png

Updated screenshot after talking with antlam on irc.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8462231 [details] [diff] [review]
bug-1042937-wip.v2.patch

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

Looking good!

::: mobile/android/search/res/layout/search_fragment_post_search.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/. -->
>  
> +<FrameLayout

If you're using a FrameLayout here, that means that the progress bar is actually getting drawn on top of the web view. It may look good for now, but we'll need to watch out for this if we switch to another search engine that doesn't have as much padding at the top of its result view.

A safer bet would probably be to use a LinearLayout, but that would push the WebView content down 3dp.

@@ +17,5 @@
> +        style="@android:style/Widget.ProgressBar.Horizontal"
> +        android:layout_width="match_parent"
> +        android:layout_height="wrap_content"
> +        android:maxHeight="@dimen/progress_bar_height"
> +        android:minHeight="@dimen/progress_bar_height"

Why not just set android:layout_height directly to progress_bar_height?

Comment 10

[Eric Edens [:eedens]]

Attachment: bug-1042937-fix.patch

(In reply to :Margaret Leibovic from comment #9)
> Comment on attachment 8462231 [details] [diff] [review]
> bug-1042937-wip.v2.patch
> 
> Review of attachment 8462231 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good!
> 
> ::: mobile/android/search/res/layout/search_fragment_post_search.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/. -->
> >  
> > +<FrameLayout
> 
> If you're using a FrameLayout here, that means that the progress bar is
> actually getting drawn on top of the web view. It may look good for now, but
> we'll need to watch out for this if we switch to another search engine that
> doesn't have as much padding at the top of its result view.
> 
> A safer bet would probably be to use a LinearLayout, but that would push the
> WebView content down 3dp.


Sounds good


> 
> @@ +17,5 @@
> > +        style="@android:style/Widget.ProgressBar.Horizontal"
> > +        android:layout_width="match_parent"
> > +        android:layout_height="wrap_content"
> > +        android:maxHeight="@dimen/progress_bar_height"
> > +        android:minHeight="@dimen/progress_bar_height"
> 
> Why not just set android:layout_height directly to progress_bar_height?


From what I read, I thought I had to use the min/max height, but you're totally right. Using layout_height works (and is a lot cleaner).

Comment 11

[:Margaret Leibovic]

Comment on attachment 8462243 [details] [diff] [review]
bug-1042937-fix.patch

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

Nice, looks good.

::: mobile/android/search/res/values/search_dimens.xml
@@ +14,5 @@
>      <!-- To create the padding we see around the content of each
>           card, we need to account for the padding of the background -->
>      <dimen name="card_padding_x">38dp</dimen>
>      <dimen name="card_padding_y">23dp</dimen>
> +    <dimen name="progress_bar_height">3dp</dimen>

Nit: I would put a blank line above this. Maybe also move it up higher below the search_bar_height declaration (organizing the dimen declarations by parts of the UI where they're used).

Comment 12

[:Margaret Leibovic]

(Before we land this, make sure you share a build with antlam to get his approval.)

Comment 13

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

(In reply to Eric Edens [:eedens] from comment #8)
> Created attachment 8462234 [details]
> Screenshot_2014-07-24-17-03-47.png
> 
> Updated screenshot after talking with antlam on irc.

This is better! I'm still working on the final one but I think this will do for now

Comment 14

[Eric Edens [:eedens]]

Attachment: bug-1042937-fix.v2.patch

Re-arranged order of dimensions.

Comment 15

[Ian Barlow (:ibarlow)]

Just wanted to check and make sure we're using the indeterminate 'laser show' progress bar, and not the linear one?

Comment 16

[Eric Edens [:eedens]]

Hey Ian --

It's determinate -- when you said "'orange laser show' animation under the title bar", I thought you were referring to the orange loader that we use in fennec under the title bar, which is determinate is well.

I'll swap this out and post a new patch.

Comment 17

[Eric Edens [:eedens]]

Alright, to bring the bug up to speed:
  - 'Laser show' refers to the home panel's GeckoSwipeRefreshLayout, which is a forked from compat v4 library's SwipeProgressBar.
  - SwipeProgressBar does not extend the Android view -- it does its own drawing on a canvas
  - Drawing / layout / state updates are driven by (Gecko)SwipeRefreshLayout.
  - In order to use SwipeProgressBar here, we'd need to write our own ViewGroup to drive its layout and drawing.

Since we're waiting on the final designs, and this would be a placeholder anyway, we should land this patch and revisit when the final designs are finished.

Comment 18

[:Margaret Leibovic]

Given eedens's latest comment, I landed this patch as-is for now:

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

eedens, please file another bug to iterate on the design here. I applied this patch locally to test, and while I appreciated getting some feedback that the WebView was loading, the progress bar feels pretty clunky, so this needs some more design love.

Comment 19

[Eric Edens [:eedens]]

Merged into GH: https://github.com/ericedens/FirefoxSearch/pull/33

I created a followup, bug 1044262, to make things less clunky :) . It's assigned to alam for the designs -- alam, you can pass it back when it's ready!

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Flaviu Cos]

Verified as fixed in build 34.0a1 (2014-08-04);
Device: Samsung Galaxy Nexus (Android 4.2.1).