1181284 - [tablet] Pull-to-refresh does not work on landscape Synced Tabs panel

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, 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:

On phone you can pull-to-refresh, on tablet you can't do that. Pressing refresh in the action bar doesn't do anything either.

Comment 1

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

Paul, which list does this apply to? All of them?

Comment 2

[Paul [pwd]]

Let's say you send a tab to a device, on phone you'd simply scroll to "Synced Tabs" within the AwesomeScreen and pull-to-refresh and this would update the synced tabs list, history and fetch any tabs you've been sent. There's no way to do the same on tablet. Hopefully that answers your question.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Paul [pwd] from comment #2)
> Let's say you send a tab to a device, on phone you'd simply scroll to
> "Synced Tabs" within the AwesomeScreen and pull-to-refresh and this would
> update the synced tabs list, history and fetch any tabs you've been sent.
> There's no way to do the same on tablet. Hopefully that answers your
> question.

I thought we supported pull-to-refresh in the tabs list of Synced Tabs.  A code search suggests we do:

https://dxr.mozilla.org/mozilla-central/search?q=remote_tabs_refresh_layout&case=true

Paul, can you verify that you cannot swipe to refresh:
1) in portrait mode;
2) and in landscape mode, on either the list of devices or the list of tabs.

Comment 4

[Paul [pwd]]

Phone portrait: pull-to-refresh works
Phone landscape: pull-to-refresh works
Tablet portrait: pull-to-refresh works
Tablet landscape: nothing

Comment 5

[Vivek Balakrishnan[:vivek]]

pull to refresh was disabled in tablet landscape mode as it was interfering with scrolling.
Refer https://bugzilla.mozilla.org/show_bug.cgi?id=1136699#c4

Comment 6

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

(In reply to Vivek Balakrishnan[:vivek] from comment #5)
> pull to refresh was disabled in tablet landscape mode as it was interfering
> with scrolling.
> Refer https://bugzilla.mozilla.org/show_bug.cgi?id=1136699#c4

Is there a bug open to fix this, was it decided to be too much effort, or should we repurpose this one to fix the issue (though, not saying anyone will work on it)?

Comment 7

[Vivek Balakrishnan[:vivek]]

I'm not aware of any other open bug to fix this.
Yes, lets repurpose this bug to fix the issue.

Comment 8

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1181284 - Enable swipe to refresh on client list based on scroll position r?mcomella.

Bug 1181284 - Enable swipe to refresh on client list based on scroll position r?mcomella.

Comment 9

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

Comment on attachment 8636883 [details]
MozReview Request: Bug 1181284 - Enable swipe to refresh on client list based on scroll position r?mcomella.

https://reviewboard.mozilla.org/r/13743/#review12725

I can't test this because I don't want to spend the time adding numerous devices to my sync account (I'd be happy if you sent me an account though! :P).

How does this feel? When you scroll down and scroll back up to the top, does everything work as expected? When you scroll down and scroll back up to the top and continue to scroll, what happens? Does the swipe layout *just work*?

::: mobile/android/base/home/RemoteTabsSplitPlaneFragment.java:80
(Diff revision 1)
> +                mRefreshLayout.setEnabled(visibleItemCount > 0 && view.getChildAt(0).getTop() >= 0);

`view.getChildAt(0)` will return the first *visible* view, not the first view in the list - perhaps you want to get the view from the adapter?

Also, I'd expect `...getTop() <= 0` for the view being at the top.

::: mobile/android/base/home/RemoteTabsSplitPlaneFragment.java:79
(Diff revision 1)
> +                // Enable swipe to refresh iff there are some list items and the first item is visible.

The comments indicate that the first view needs to be visible - do you mean it needs to be visible and at the top?

Comment 10

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8636883 [details]
MozReview Request: Bug 1181284 - Enable swipe to refresh on client list based on scroll position r?mcomella.

Bug 1181284 - Enable swipe to refresh on client list based on scroll position r?mcomella.

Comment 11

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1181284 - Enable swipe to refresh on remote panel list based on scroll position r?mcomella.

Bug 1181284 - Enable swipe to refresh on remote panel list based on scroll position r?mcomella.

Comment 12

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

This patch works as expected, though I was unable to test a number of devices that would force a scroll on these pages.

Comment 13

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

Comment on attachment 8640684 [details]
MozReview Request: Bug 1181284 - Enable swipe to refresh on remote panel list based on scroll position r?mcomella.

https://reviewboard.mozilla.org/r/14355/#review12993

Looks good - just address my comment and fold these patches up.

::: mobile/android/base/home/RemoteTabsSplitPlaneFragment.java:387
(Diff revision 1)
> +            if (listView.getCount() <= 0) {

This if block can be in ACTION_DOWN, right? I think that'd save us a few cycles.

Comment 14

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

Comment on attachment 8636883 [details]
MozReview Request: Bug 1181284 - Enable swipe to refresh on client list based on scroll position r?mcomella.

https://reviewboard.mozilla.org/r/13743/#review12995

Comment 15

[Vivek Balakrishnan[:vivek]]

url:        https://hg.mozilla.org/integration/fx-team/rev/d59e6ed60e5aa0437c91f01a1bc0a8033c319f78
changeset:  d59e6ed60e5aa0437c91f01a1bc0a8033c319f78
user:       vivek <[e-mail]>
date:       Wed Jul 29 23:08:38 2015 +0300
description:
Bug 1181284 - Enable swipe to refresh on remote panel list based on scroll position r=mcomella.

Comment 16

[Carsten Book [:Tomcat]]

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