Closed
Bug 1350737
Opened 7 years ago
Closed 7 years ago
Scrolling behaviour of tabs tray changed
Categories
(Firefox for Android Graveyard :: General, defect)
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)
59 bytes,
text/x-review-board-request
|
sebastian
:
review+
|
Details |
5.86 KB,
patch
|
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 2•7 years 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.
Comment 3•7 years ago
|
||
Too late for 53, but we could still maybe take a patch for 54.
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Assignee: nobody → twointofive
Status: NEW → ASSIGNED
Comment 5•7 years 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+
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
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 8•7 years ago
|
||
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3cf6b363d1b3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•7 years ago
|
status-firefox-esr52:
--- → unaffected
Comment 10•7 years ago
|
||
Hi Mihai, can you help verify if the fix is expected in the latest nightly?
Flags: qe-verify+
Flags: needinfo?(mihai.ninu)
Comment 11•7 years ago
|
||
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 12•7 years ago
|
||
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•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a11c3d6524f9
Comment 14•7 years ago
|
||
Verified as fixed in build 54.0b12 testing was based on description from comment 1; Device: Huawei Honor (Android 5.1.1);
Comment 15•6 years ago
|
||
Removing qe-verify flag based on comments 14 and 11.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•