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

RESOLVED FIXED in Firefox 42

Status

()

Firefox for Android
Awesomescreen
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Paul [pwd], Assigned: vivek)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 42
All
Android
Points:
---

Firefox Tracking Flags

(firefox42 fixed, fennec+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

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

Updated

3 years ago
OS: Unspecified → Android
Hardware: Unspecified → All
Paul, which list does this apply to? All of them?
Flags: needinfo?(pwd.mozilla)
(Reporter)

Comment 2

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

Comment 4

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

Comment 5

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

Comment 7

3 years ago
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)
Blocks: 1157964
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
(Assignee)

Comment 8

3 years ago
Created 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.
Attachment #8636883 - Flags: review?(michael.l.comella)
Assignee: nobody → vivekb.balakrishnan
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.

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?
(Assignee)

Updated

3 years ago
Attachment #8636883 - Flags: review?(michael.l.comella)
(Assignee)

Comment 10

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

Comment 11

3 years ago
Created attachment 8640684 [details]
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.
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: ? → +
(Assignee)

Comment 15

3 years ago
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
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.