Closed Bug 1350718 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox for Android :: General, defect)

52 Branch
All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
fennec + ---
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- verified
firefox55 --- verified

People

(Reporter: wlevine, Assigned: twointofive)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(6 files)

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.
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.
Blocks: 1116415
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Keywords: regression
OS: Unspecified → Android
Hardware: Unspecified → All
See Also: → 1350737
Summary: When opening a link from another app, the new tab is not opened automatically → When opening a link from another app with the tabs tray open, the tabs tray doesn't scroll to show the new tab
Blocks: 1350994
Assignee: nobody → twointofive
See Also: → 1353226
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.
Attachment #8854686 - Flags: review?(s.kaspari) → review?(max)
Attachment #8854687 - Flags: review?(s.kaspari) → review?(max)
@max: Can you take this review?

@tom: Thanks for looking into this! :)
Comment on attachment 8854687 [details]
Bug 1350718 - 2. Remove the add animation for the TabsLayout list.

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

LGTM
Attachment #8854687 - Flags: review?(max) → review+
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
Attachment #8854686 - Flags: review?(max) → review+
tracking-fennec: ? → +
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
Attachment #8855410 - Flags: review?(max) → review+
Pushed by twointofive@gmail.com:
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
Too late for 53 at this point, but maybe worth uplifting to Aurora for Fx54 still?
Flags: needinfo?(twointofive)
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.
Flags: needinfo?(twointofive)
Attachment #8854686 - Flags: approval-mozilla-aurora?
Hi Will,
Can you help check if this issue is fixed as expected in the latest nightly?
Flags: qe-verify+
Flags: needinfo?(wlevine)
Hi Mihai,
Can you help verify this fix on the latest nightly?
Flags: needinfo?(wlevine) → needinfo?(mihai.ninu)
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.
Flags: needinfo?(mihai.ninu)
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+.
Attachment #8854686 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Flags: needinfo?(twointofive)
Flags: needinfo?(twointofive)
(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!
Flags: needinfo?(ihsiao)
Flags: needinfo?(ihsiao)
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.