1183588 - Update pull-to-refresh pattern to Material Design

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Paul [pwd]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150513191204

Steps to reproduce:

We currently use the ICS/JB/KK pattern, the platform has moved on. Let's switch over to Material Design pattern.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This change seems reasonable to me.

For an example on how this looks, see:

 https://www.google.com/design/spec/patterns/swipe-to-refresh.html#swipe-to-refresh-swipe-to-refresh

Comment 2

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

Sounds good. Would this work for our Panels Mike?

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Anthony Lam (:antlam) from comment #2)
> Sounds good. Would this work for our Panels Mike?

+1 from me.  I'm a little surprised this isn't just on by default.  Do we need to rev the support library version to get this styling?  Do we need to opt-in somewhere in our code?

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Nick Alexander :nalexander (PTO July 17-27) from comment #3)
> +1 from me.  I'm a little surprised this isn't just on by default.  Do we
> need to rev the support library version to get this styling?  Do we need to
> opt-in somewhere in our code?

Apparently we copied the source to GeckoSwipeToRefreshLayout and use that instead [1].

The reasons in the Javadoc are:

45  * GeckoSwipeRefreshLayout is mostly lifted from Android's support library (v4) with these
46  * modifications:
47  *  - Removes elastic "rubber banding" effect when overscrolling the child view.
48  *  - Changes the height of the progress bar to match the height of HomePager's page indicator.
49  *  - Uses a rectangle rather than a circle for the SwipeProgressBar indicator.

Rubber banding is no longer present, the height is no longer relevant, and I'm not sure what the rectangle vs. circle means, but it's probably not relevant.

[1]: https://mxr.mozilla.org/mozilla-central/search?string=swiperefreshlayout&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Vivek said he'd handle this one on IRC.

Comment 6

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback r?nalexander.

Bug 1183588 - Material design swipe refresh pattern changes r?mcomella.

* Replaced GeckoSwipeRefreshLayout with SwipeRefreshLayout from support library.
* Handled refresh cancel in a UI Thread to avoid swipe refresh circle sticking around.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sorry, Vivek - I'll get to this tomorrow.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8638853 [details]
MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback  r?nalexander.

https://reviewboard.mozilla.org/r/14145/#review13101

::: mobile/android/base/home/PanelRefreshLayout.java:74
(Diff revision 1)
> -        setRefreshing(false);
> +                setRefreshing(false);

Why does this solve the problem of the refresh icon sticking around if the refresh is cancelled?

You should probably either add comments to each invocation, or extend SwipeRefreshLayout and override setRefreshing so that it always calls itself in a UIThread.

Comment 9

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8638853 [details]
MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback  r?nalexander.

Bug 1183588 - Material design swipe refresh pattern changes r?mcomella.

* Replaced GeckoSwipeRefreshLayout with SwipeRefreshLayout from support library.
* Annotated the method with appropriate thread annotations.

Support API SwipeRefreshLayout cancels the refresh view by scheduling a scale down animation.
At the end of the animation, the view is set to Gone state.
So, we must ensure that this view update happens in UiThread.

The deprecated GeckoSwipeRefreshLayout got away this because the it cleared
the progress bar with postInvalidate() which invalidates the view through
event loop.

Comment 11

[Vivek Balakrishnan[:vivek]]

Hi Nick,

As part of this bug I've added @WorkerThread annotation to onSyncStarted() and onSyncFinished(). I arrived at this conclusion through the logic of elimination that these callback are not run in MainThread. After digging through the code I find that requestSync is actually executed in a service [1]. Can you please correct me if I'm wrong with those annotations.

1 http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/5.1.1_r1/android/content/ContentResolver.java#1797

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Vivek Balakrishnan[:vivek] from comment #11)
> Hi Nick,
> 
> As part of this bug I've added @WorkerThread annotation to onSyncStarted()
> and onSyncFinished(). I arrived at this conclusion through the logic of
> elimination that these callback are not run in MainThread. After digging
> through the code I find that requestSync is actually executed in a service
> [1]. Can you please correct me if I'm wrong with those annotations.
> 
> 1
> http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/
> android/5.1.1_r1/android/content/ContentResolver.java#1797

The callback is invoked here:

http://androidxref.com/4.4.4_r1/xref/frameworks/base/services/java/com/android/server/content/SyncStorageEngine.java#547

Which is called all over the place, including in things like setBackoff().  It's not clear that setBackoff() couldn't be called from the main UI thread.

Sadly, there's no documentation guarantee of anything.  But we have a point we control: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/sync/FxAccountSyncStatusHelper.java.

As a pre: patch, extract the interface from https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/FirefoxAccounts.java#72 to a new file; document that the methods are always called on a particular thread (which could be the UI thread, if you want that); and then make FxAccountSyncStatusHelper always invoke delegate methods in the right place.

I should have documented the thread assumptions and enforced the threading model from the very beginning.  That's my oversight.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8638853 [details]
MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback  r?nalexander.

Clearing review – I think I'm waiting on the changes from comment 11 and comment 12.

Comment 14

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8638853 [details]
MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback  r?nalexander.

Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback  r?nalexander.

Comment 15

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1183588 - Material design swipe refresh pattern changes r?mcomella.

Bug 1183588 - Material design swipe refresh pattern changes r?mcomella.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8650030 [details]
MozReview Request: Bug 1183588 - Material design swipe refresh pattern changes r?mcomella.

https://reviewboard.mozilla.org/r/16529/#review14963

Ship It!

Comment 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8638853 [details]
MozReview Request: Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback  r?nalexander.

https://reviewboard.mozilla.org/r/14145/#review14965

If it works for you, it works for me: just nits.

::: mobile/android/base/fxa/SyncStatusListener.java:20
(Diff revision 3)
> +     * This is always called in UiThread.

...called on the UiThread.  (Both places.)

::: mobile/android/base/widget/ActivityChooserModel.java:1336
(Diff revision 3)
> -    private class SyncStatusListener implements FirefoxAccounts.SyncStatusListener {
> +    private class SyncStatusDelegate implements SyncStatusListener {

Consider InnerSyncStatusListener or something -- no sense using delegate and listener to refer to the same thing.

Comment 18

[Vivek Balakrishnan[:vivek]]

url:        https://hg.mozilla.org/integration/fx-team/rev/53da6578fad4f05df702ff737bc11a141eae9e0a
changeset:  53da6578fad4f05df702ff737bc11a141eae9e0a
user:       vivek <[e-mail]>
date:       Wed Aug 19 21:16:10 2015 +0300
description:
Bug 1183588 - Pre:Enforce strict threading policy for Sync status change callback  r=nalexander.

Comment 19

[Vivek Balakrishnan[:vivek]]

url:        https://hg.mozilla.org/integration/fx-team/rev/720c7eff9ea0cafb08fe442311bb3d1c353ee977
changeset:  720c7eff9ea0cafb08fe442311bb3d1c353ee977
user:       vivek <[e-mail]>
date:       Wed Aug 19 21:31:50 2015 +0300
description:
Bug 1183588 - Material design swipe refresh pattern changes r=mcomella.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/53da6578fad4
https://hg.mozilla.org/mozilla-central/rev/720c7eff9ea0

Comment 21

[Flaviu Cos]

Verified as fixed in build 43.0a1 2015-08-26;
Devices: 
 Asus Transformer Pad (Android 4.2.1).
 Google Nexus 9 (Android 5.1.1);
 Motorola Razr (Android 4.4.4).