Closed Bug 1181284 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed
fennec + ---

People

(Reporter: pwd.mozilla, Assigned: vivek)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
OS: Unspecified → Android
Hardware: Unspecified → All
Paul, which list does this apply to? All of them?
Flags: needinfo?(pwd.mozilla)
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.
Flags: needinfo?(pwd.mozilla)
(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.
Phone portrait: pull-to-refresh works
Phone landscape: pull-to-refresh works
Tablet portrait: pull-to-refresh works
Tablet landscape: nothing
Summary: No obvious way to force sync on tablet → [tablet] Pull-to-refresh does not work on landscape Synced Tabs panel
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
(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)?
Flags: needinfo?(vivekb.balakrishnan)
I'm not aware of any other open bug to fix this.
Yes, lets repurpose this bug to fix the issue.
Flags: needinfo?(vivekb.balakrishnan)
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Bug 1181284 - Enable swipe to refresh on client list based on scroll position r?mcomella.
Attachment #8636883 - Flags: review?(michael.l.comella)
Assignee: nobody → vivekb.balakrishnan
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?
Attachment #8636883 - Flags: review?(michael.l.comella)
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.
Bug 1181284 - Enable swipe to refresh on remote panel list based on scroll position r?mcomella.
Attachment #8640684 - Flags: review?(michael.l.comella)
This patch works as expected, though I was unable to test a number of devices that would force a scroll on these pages.
Attachment #8640684 - Flags: review?(michael.l.comella) → review+
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 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
Attachment #8636883 - Flags: review?(michael.l.comella) → review+
tracking-fennec: ? → +
url:        https://hg.mozilla.org/integration/fx-team/rev/d59e6ed60e5aa0437c91f01a1bc0a8033c319f78
changeset:  d59e6ed60e5aa0437c91f01a1bc0a8033c319f78
user:       vivek <vivekb.balakrishnan@gmail.com>
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.
https://hg.mozilla.org/mozilla-central/rev/d59e6ed60e5a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.