1350718 - When opening a link from another app with the tabs tray open, the tabs tray doesn't scroll to show the new tab

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Will Levine]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170317213149

Steps to reproduce:

0) Set Firefox as your default browser so that when you click a link in any app, Firefox will open the link.
1) Open Firefox and press the tab button (the button that shows the number of currently open tabs, located between the address bar and the ... icon). This opens what I call the 'tab list view', which shows all your open tabs.
2) Switch to another app and click on a link in that app.


Actual results:

Firefox opens the link in a new tab, but it remains in the 'tab list view' and it doesn't automatically show the new tab. I have to scroll down the tab list and select the tab I just opened.


Expected results:

Firefox opens the link in a new tab and automatically shows the new tab.

This is something that regressed in a recent update. It used to work properly. I run into this bug a lot because my typical behavior is that when I'm done with a website I'll close the tab (by switching to the tab list view), and then switch to another app without closing the tab list. Whenever I end up returning to Firefox by clicking a link, the tab list view stays open.

Comment 1

[Jan Henning [:JanH]]

Unless I'm doing something wrong, I can't quite reproduce your expected behaviour even going back to Firefox 43 in that the tabs tray remains open when an external app opens a new tab.
What is different though is that after opening an external tab, the tabs tray no longer automatically scrolls to show the new tab.

From the look of things, it seems the broke around the time of bug 1116415 as well.

However the behaviour you mention is worth considering as well, so I've opened a new bug for it.

Comment 2

[Tom Klein]

Attachment: Bug 1350718 - 1. Scroll to a tab added to the tabs tray by an external app.

If another app opens a link in Fennec, and Fennec restores itself in a state
where the tabs tray is already open, we need to scroll to the newly added tab
since it gets added offscreen (not to mention the scroll position restored when
we open is unconstrained (it's whatever the user left it at before they switched
apps)).

This introduces one small change in behavior:
1) Use a gridded tabs tray;
2) Fill more tabs than will fit in the tray;
3) Put more than one tab on the last row;
4) Scroll so that the last row is partially, but not fully, hidden;
5) Close the last tab and then undo the close.
In that case we now scroll the last row fully into view, whereas previously we
maintained the old (partially hidden) scroll position.  (If you undo close any
tab other than the last on the final row then you still get the old behavior.)

Note that this fixes the case where the other app adds a *new* tab in Fennec
with the tabs tray open; it's (currently) also possible to open a link in an
already existing tab with the tabs tray open - that's bug 1353226.

Review commit: https://reviewboard.mozilla.org/r/126644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126644/

Comment 3

[Tom Klein]

Attachment: Bug 1350718 - 2. Remove the add animation for the TabsLayout list.

Based on what I'm seeing, if you call scrollToPosition and that causes you to
"scroll into view" (remember, scrollToPosition doesn't actually scroll, it just
redraws the new position) one or more positions, then RecyclerView runs the add
animation on all those views "scrolled onto screen", which, for the list view's
slide-in-from-the-right add animation, looks silly (I think).  [Caveat:
RecyclerView sometimes keeps one offscreen view ready to go, which doesn't seem
to get the add animation.]

In non open-tab-from-another-app-with-the-tabs-tray-already-open operations this
was never an issue because either those animations are hidden by the panel being
animated into view when the panel opens and we scroll to the selected position
[at least that's my guess], or we only scroll by at most one, as in the case of
a tab close or undo close.  But in the
open-a-tab-and-scroll-to-it-while-the-tabs-tray-is-already-open case that we can
get with opening a tab from another app, the add animation runs for however many
tabs "need to be added" between the current position and the new tab; sometimes
the animation still gets hidden if the new tabs get added quickly enough when
fennec reloads [again, my guess], but on my device I always see the animations
if I open a tab in tab queue and then reopen Fennec by hand, whereas on an
emulator I see the animations in additional external-app-open cases as well.

Review commit: https://reviewboard.mozilla.org/r/126646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126646/

Comment 4

[Tom Klein]

There are a couple things going on here:

1) There was a bug in TabsLayout.onChanged:ADDED: when a tab gets added we sends its position as data, with -1 meaning "new last tab".  In the added case, tabsAdapter.notifyTabInserted handled the -1 case, but addAtIndexRequiresScroll did not, which prevented us from scrolling in this bug.  Additionally in the grid case we were formerly only scrolling a new last tab if it was added on a new line; now we need to scroll for a new last tab at any position (that introduces a minor change in behavior, as explained in the commit message).

2) Now that we can get a scrollToPosition which covers a change in position greater than 1 (formerly we only handled tab close undo), the animation we have for adding a tab in the list view no longer works as well.  The issue is that, when using scrollToPosition (as opposed to scrolling by hand...), RecyclerView runs the add animation on as many tabs as "need to be added" to "scroll the position into view".  I use quotes because scrollToPosition doesn't actually scroll, it just redraws, but it sort of pretends it's scrolling as much as it can.  In our case I think that makes us look somewhat broken in some cases - see https://youtu.be/1drp1Xk8j0k for example, where we add one tab from an external app, but when Fennec opens we get three add animations (out of the four tabs scrolled into view) because we needed to "scroll" three places to get the new tab in view.  (You can still tell this is happening with the default fade-in animation if you're paying close attention, but it's now much less obvious.)

I'm not too beat up about losing the add animation; the only situation in which it previously showed up was a tab close undo.  Note though that bug 1350994 may wind up removing the scenario of this bug, in which case we could, if we wanted, keep the add animation.  Do we want to wait for a decision on that bug, or are we eager to get a fix uplifted on this one?  (Even if we wait and the scenario of this bug becomes disallowed, I'll still need to fix the -1 bug from 1) above, but as far as I know that's not affecting any other scenario at the moment.)

Additional notes:
a) The patches here fix the case where the external app causes a *new* tab to be added in Fennec; the case where we open a link in an existing tab from an external app is bug 1353226.
b) This bug isn't an issue with the tab strip; namely if you have the tab strip open, then add a new tab from another app in such a way that the tab strip is still open when you return to fennec, then the tab strip will scroll to the new tab.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

@max: Can you take this review?

@tom: Thanks for looking into this! :)

Comment 6

[Max Liu [:maliu]]

Comment on attachment 8854687 [details]
Bug 1350718 - 2. Remove the add animation for the TabsLayout list.

https://reviewboard.mozilla.org/r/126646/#review129878

LGTM

Comment 7

[Max Liu [:maliu]]

Comment on attachment 8854686 [details]
Bug 1350718 - 1. Scroll to a tab added to the tabs tray by an external app.

https://reviewboard.mozilla.org/r/126644/#review129884

::: mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayout.java:109
(Diff revision 1)
>          }
>  
>          switch (msg) {
>              case ADDED:
> -                final int tabIndex = Integer.parseInt(data);
> +                int tabIndex = Integer.parseInt(data);
> +                // A tabIndex of -1 means "add a new last tab".

Since "-1" is kinda magic number, is it better if we define another const and reference to it? Just like Tabs.INVALID_TAB_ID. Probably better for us to trace where does it goes when magic happenes. 

Tabs.loadUrl seems to be the starting point of it(-1).
https://dxr.mozilla.org/mozilla-central/rev/720b9177c6856c1c4339d0fac1bf5149c0d53950/mobile/android/base/java/org/mozilla/gecko/Tabs.java#1025

Comment 8

[Tom Klein]

Attachment: Bug 1350718 - Post: Name the special value -1 used to indicate a new tab should be appended.

Review commit: https://reviewboard.mozilla.org/r/127246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127246/

Comment 9

[Tom Klein]

Comment on attachment 8854686 [details]
Bug 1350718 - 1. Scroll to a tab added to the tabs tray by an external app.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126644/diff/1-2/

Comment 10

[Tom Klein]

Comment on attachment 8854687 [details]
Bug 1350718 - 2. Remove the add animation for the TabsLayout list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126646/diff/1-2/

Comment 11

[Max Liu [:maliu]]

Comment on attachment 8855410 [details]
Bug 1350718 - Post: Name the special value -1 used to indicate a new tab should be appended.

https://reviewboard.mozilla.org/r/127246/#review130144

LGTM

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3ea0d4a3c2e
1. Scroll to a tab added to the tabs tray by an external app. r=maliu
https://hg.mozilla.org/integration/autoland/rev/63b5915ed9bc
2. Remove the add animation for the TabsLayout list. r=maliu
https://hg.mozilla.org/integration/autoland/rev/d544c4b2b56b
Post: Name the special value -1 used to indicate a new tab should be appended. r=maliu

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3ea0d4a3c2e
https://hg.mozilla.org/mozilla-central/rev/63b5915ed9bc
https://hg.mozilla.org/mozilla-central/rev/d544c4b2b56b

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3ea0d4a3c2e
https://hg.mozilla.org/mozilla-central/rev/63b5915ed9bc
https://hg.mozilla.org/mozilla-central/rev/d544c4b2b56b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Too late for 53 at this point, but maybe worth uplifting to Aurora for Fx54 still?

Comment 16

[Tom Klein]

Comment on attachment 8854686 [details]
Bug 1350718 - 1. Scroll to a tab added to the tabs tray by an external app.

Approval Request Comment
[Feature/Bug causing the regression]: 1116415
[User impact if declined]: In certain cases we won't scroll to a tab that was just opened by the user from an external app.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Steps to reproduce in comment 0.
[List of other uplifts needed for the feature/fix]: Please uplift all three commits on this bug.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: The fix was to properly handle a case where we use "-1" to mean "new last tab": we now simply scroll in a case where we always intended to before but did not.  The animation change is sort of new: we're reverting the add animation that was added in bug 1314322 and are now back to using the default add animation, but the default is now running through the 1314322 code, so is new in that sense.  Testing looks good though, and if there was an issue it would just be of the form "animations look a little jerky" when we undo close a tab.
[String changes made/needed]: None.

Comment 17

[Gerry Chang [:gchang]]

Hi Will,
Can you help check if this issue is fixed as expected in the latest nightly?

Comment 18

[Gerry Chang [:gchang]]

Hi Mihai,
Can you help verify this fix on the latest nightly?

Comment 19

[u549602]

Verified as fixed on the latest Nightly build 55.0a1 (2017-04-13)on a Samsung galaxy S6 Edge.

So, i've tried two scenarios that might work with the STR at Comm 0 
1. Opening an external link into Firefox while the tab tray (Will's 'tab list view' as i understood)was displayed and with tab queue OFF. 

Result: With multiple tabs in view, when opening external link in browser, the user is still in tab tray view but with focus on the external link even if he left the app at the top of the list, he will be redirected at the bottom with the tab in focus. 

2.  Opening an external link into Firefox while the tab tray (Will's 'tab list view' as i understood)was displayed and with tab queue ON. When tapping on Open Now or from  the Android "awaiting tab"notification, the same behavior as at the first scenario can be observed.

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8854686 [details]
Bug 1350718 - 1. Scroll to a tab added to the tabs tray by an external app.

Fix a regression related to open a link from another app and was verified. Aurora54+.

Comment 21

[Iris Hsiao [:ihsiao]]

Could you help to fix the conflicts
merging mobile/android/base/java/org/mozilla/gecko/Tabs.java
merging mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayout.java
2017-04-14 17:59:33.579 FileMerge[17944:6328436] Unable to load platform at path /Applications/Xcode.app/Contents/Developer/Platforms/AppleTVOS.platform
2017-04-14 17:59:33.580 FileMerge[17944:6328436] Unable to load platform at path /Applications/Xcode.app/Contents/Developer/Platforms/AppleTVSimulator.platform
2017-04-14 17:59:33.581 FileMerge[17944:6328436] Unable to load platform at path /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform
2017-04-14 17:59:33.582 FileMerge[17944:6328436] Unable to load platform at path /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform
2017-04-14 17:59:33.587 FileMerge[17944:6328436] Unable to load platform at path /Applications/Xcode.app/Contents/Developer/Platforms/WatchOS.platform
2017-04-14 17:59:33.588 FileMerge[17944:6328436] Unable to load platform at path /Applications/Xcode.app/Contents/Developer/Platforms/WatchSimulator.platform

Comment 22

[Tom Klein]

Attachment: Aurora patch part 1

Comment 23

[Tom Klein]

Attachment: Aurora patch part 2

Comment 24

[Tom Klein]

Attachment: Aurora patch part 3

Comment 25

[Tom Klein]

(In reply to Iris Hsiao [:ihsiao] from comment #21)
> Could you help to fix the conflicts

I've created three new patches for aurora.  Thanks!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/23e8c43d48b3
https://hg.mozilla.org/releases/mozilla-aurora/rev/5fd6b9f3af92
https://hg.mozilla.org/releases/mozilla-aurora/rev/0c01486f2381

Comment 27

[Bogdan Surd, QA [:bsurd, NI]]

Verified as fixed in the 54.0b2 Beta build on a Nexus 6 (Android 7.0)

Using the provided STR in Comment 0 and from Comment 19 the issue is no longer reproducible and the behavior works as expected.