Closed Bug 1350737 Opened 7 years ago Closed 7 years ago

Scrolling behaviour of tabs tray changed

Categories

(Firefox for Android Graveyard :: General, defect)

52 Branch
All
Android
defect
Not set
normal

Tracking

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

VERIFIED FIXED
Firefox 55
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- unaffected
firefox53 --- wontfix
firefox54 --- verified
firefox55 --- verified

People

(Reporter: JanH, Assigned: twointofive)

References

Details

(Keywords: regression)

Attachments

(2 files)

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.
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
See Also: → 1350718
(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.
Assignee: nobody → twointofive
Status: NEW → ASSIGNED
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+
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
Attached patch Fx54 patchSplinter Review
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?
https://hg.mozilla.org/mozilla-central/rev/3cf6b363d1b3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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.
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+
Verified as fixed in build 54.0b12 testing was based on description from comment 1;
Device: Huawei Honor (Android 5.1.1);
Removing qe-verify flag based on comments 14 and 11.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: