Scrolling behaviour of tabs tray changed

VERIFIED FIXED in Firefox 54

Status

()

Firefox for Android
General
VERIFIED FIXED
a year ago
3 months ago

People

(Reporter: JanH, Assigned: Tom Klein)

Tracking

({regression})

52 Branch
Firefox 55
All
Android
regression
Points:
---

Firefox Tracking Flags

(firefox52 wontfix, firefox-esr52 unaffected, firefox53 wontfix, firefox54 verified, firefox55 verified)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Previously, each time you opened the tabs tray it would scroll such as to position the current tab at the top of the screen (or as far as possible for tabs near the end of the tabs list). This had the side effect that if the selected tab was near the end of the list, opening a new tab would cause the list to scroll up the next time it was opened, meaning that you could directly switch to the new tab if necessary.

With the current implementation however, the list remembers its scrolling position, so even if a new tab was added directly below the current tab (would be even more relevant if we do bug 1299905), you now have to scroll the tabs tray in order to see and select the new tab.

This was probably caused by bug 1116415 or something similar, although I'll still need to confirm it.
(Assignee)

Comment 1

a year ago
Yes, caused by bug 1116415.  I didn't set out to change this behavior, but I also know this isn't a scenario I was on the watch for (so maybe a test, or maybe just a code comment on maintaining this behavior, would be helpful to prevent this happening again :).

Just to make sure I'm thinking the same thing:
1) Open more tabs than will fit on screen in the tabs tray (and fill the last row with tabs if the tabs tray is in a grid mode);
2) Scroll to the bottom;
3) Open one of the last visible tabs, and from that open tab open a new background tab (e.g. long click on a link and choose "Open in new tab");
4) Reopen the tabs tray.

AR: In list mode the new tab isn't visible at all, you have to scroll to be able to select it; in grid mode only the top of the title of the new tab is visible.

ER: In all such cases the new tab and its thumbnail should be visible (at least as much as possible, given that the selected tab is situated in the top row of visible tabs).

The grid mode behavior in this scenario is actually unchanged; we didn't have ER even prior to switching to RecyclerView (which happened for grid modes in 53 (current beta)).

The fix should be to change https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayout.java#173 to

((LinearLayoutManager) getLayoutManager()).scrollToPositionWithOffset(selected, 0);

I don't have strong feelings on this, but aside from this one scenario, I actually prefer the current (RecyclerView) behavior where it remembers your scroll position.  Particularly if you have a bunch of tabs open on a tablet where there might be four or more visible rows, my first reaction when there are a bunch of rows and my selected tab is in the first row is to scroll the selected tab closer to the center [caveat: I only use tablets on emulators, so maybe it's just me].  (We could also just fix this for the list case, which is maybe what Jan had in mind?)
Blocks: 1116415
(Reporter)

Updated

a year ago
See Also: → bug 1350718
(Reporter)

Comment 2

a year ago
(In reply to Tom Klein from comment #1)
> Just to make sure I'm thinking the same thing:

From your description I'd say we are.

> I don't have strong feelings on this, but aside from this one scenario, I
> actually prefer the current (RecyclerView) behavior where it remembers your
> scroll position.

Yeah, it's debatable and also a matter of what you're used to, but I've noticed it quite a bit now when opening new tabs but not immediately switching to them - even though at the moment this only works when the current tab is near the end of the tab list.
Too late for 53, but we could still maybe take a patch for 54.
status-firefox53: affected → wontfix
status-firefox54: affected → fix-optional
Comment hidden (mozreview-request)
Assignee: nobody → twointofive
Status: NEW → ASSIGNED

Comment 5

a year ago
mozreview-review
Comment on attachment 8858532 [details]
Bug 1350737 - Make the selected tab's row the first visible row when the tabs tray opens.

https://reviewboard.mozilla.org/r/130504/#review133692
Attachment #8858532 - Flags: review?(s.kaspari) → review+

Comment 6

a year ago
Pushed by twointofive@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3cf6b363d1b3
Make the selected tab's row the first visible row when the tabs tray opens. r=sebastian
(Assignee)

Comment 7

a year ago
Created attachment 8859223 [details] [diff] [review]
Fx54 patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1116415.
[User impact if declined]: If you open a new tab from the currently selected tab and then return to the tabs tray, the new tab isn't visible in some cases where it was prior to bug 1116415 (so you have to scroll a little to be able to select it).
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: Steps to reproduce in comment 1.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: We just change from "scrollToPosition" to "scrollToPositionWithOffset".
[String changes made/needed]: None.
Attachment #8859223 - Flags: approval-mozilla-aurora?
Comment on attachment 8859223 [details] [diff] [review]
Fx54 patch

54 was merged to Beta today.
Attachment #8859223 - Attachment description: Aurora patch → Fx54 patch
Attachment #8859223 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3cf6b363d1b3
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
status-firefox-esr52: --- → unaffected
Hi Mihai,
can you help verify if the fix is expected in the latest nightly?
Flags: qe-verify+
Flags: needinfo?(mihai.ninu)
Verified as fixed on the latest Nightly build 55.0a1 (2017-04-24). This issue was tested using a Samsung Galaxy S6 Edge (Android 6.0) in both portrait and landscape.
status-firefox55: fixed → verified
Flags: needinfo?(mihai.ninu)
Comment on attachment 8859223 [details] [diff] [review]
Fx54 patch

Fix a regression related to scrolling behavior of tabs and was verified. Beta54+. Should be in 54 beta 3.
Attachment #8859223 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 13

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/a11c3d6524f9
status-firefox54: fix-optional → fixed
Verified as fixed in build 54.0b12 testing was based on description from comment 1;
Device: Huawei Honor (Android 5.1.1);
status-firefox54: fixed → verified
Removing qe-verify flag based on comments 14 and 11.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.