Provide visual feedback while search results are loading

VERIFIED FIXED in Firefox 34

Status

()

Firefox for Android
Search Activity
P1
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Margaret, Assigned: eedens)

Tracking

Trunk
Firefox 34
All
Android
Points:
---

Firefox Tracking Flags

(firefox34 verified)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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?
Flags: needinfo?(ibarlow)
Flags: needinfo?(alam)
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.
Flags: needinfo?(ibarlow)
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..
(Reporter)

Updated

3 years ago
Priority: -- → P1
(Assignee)

Updated

3 years ago
Assignee: margaret.leibovic → eric.edens
(Assignee)

Comment 3

3 years ago
Created attachment 8462029 [details] [diff] [review]
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.
Attachment #8462029 - Flags: feedback?(margaret.leibovic)
Created attachment 8462031 [details]
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.
Flags: needinfo?(alam)
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)
Created attachment 8462223 [details]
Screenshot_2014-07-24-16-48-32.png

attaching current status from the build Eric sent as reference.
(Assignee)

Comment 7

3 years ago
Created attachment 8462231 [details] [diff] [review]
bug-1042937-wip.v2.patch

Changes:
 - Spinner is now orange bar
Attachment #8462029 - Attachment is obsolete: true
Attachment #8462029 - Flags: feedback?(margaret.leibovic)
Attachment #8462231 - Flags: feedback?(margaret.leibovic)
(Assignee)

Comment 8

3 years ago
Created attachment 8462234 [details]
Screenshot_2014-07-24-17-03-47.png

Updated screenshot after talking with antlam on irc.
Attachment #8462223 - Attachment is obsolete: true
(Reporter)

Comment 9

3 years ago
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?
Attachment #8462231 - Flags: feedback?(margaret.leibovic) → feedback+
(Assignee)

Comment 10

3 years ago
Created attachment 8462243 [details] [diff] [review]
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).
Attachment #8462231 - Attachment is obsolete: true
Attachment #8462243 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 11

3 years ago
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).
Attachment #8462243 - Flags: review?(margaret.leibovic) → review+
(Reporter)

Comment 12

3 years ago
(Before we land this, make sure you share a build with antlam to get his approval.)
(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
(Assignee)

Comment 14

3 years ago
Created attachment 8462654 [details] [diff] [review]
bug-1042937-fix.v2.patch

Re-arranged order of dimensions.
Attachment #8462243 - Attachment is obsolete: true
Just wanted to check and make sure we're using the indeterminate 'laser show' progress bar, and not the linear one?
(Assignee)

Comment 16

3 years ago
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.
(Assignee)

Comment 17

3 years ago
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.
(Reporter)

Comment 18

3 years ago
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.
(Assignee)

Updated

3 years ago
See Also: → bug 1044262
(Assignee)

Comment 19

3 years ago
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!
https://hg.mozilla.org/mozilla-central/rev/b5ae1c999f27
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34

Comment 21

3 years ago
Verified as fixed in build 34.0a1 (2014-08-04);
Device: Samsung Galaxy Nexus (Android 4.2.1).
Status: RESOLVED → VERIFIED
status-firefox34: --- → verified
You need to log in before you can comment on or make changes to this bug.