Bug 1350718 - 1. Scroll to a tab added to the tabs tray by an external app. r=maliu
authorTom Klein <twointofive@gmail.com>
Tue, 28 Mar 2017 11:29:52 -0500
changeset 558332 a3ea0d4a3c2e691fac654ff4e53fbf85ebfbf8bf
parent 558331 2b6c014551e0a04474d0104b8ef6e58cced543cd
child 558333 63b5915ed9bcd57c128f2424ae68f2226e4cdf15
push id52860
push userbmo:walkingice0204@gmail.com
push dateFri, 07 Apr 2017 13:29:26 +0000
reviewersmaliu
bugs1350718, 1353226
milestone55.0a1
Bug 1350718 - 1. Scroll to a tab added to the tabs tray by an external app. r=maliu 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. MozReview-Commit-ID: BazXFwT0B8v
mobile/android/base/java/org/mozilla/gecko/tabs/TabsGridLayout.java
mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayout.java
mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayoutAdapter.java
mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java
--- a/mobile/android/base/java/org/mozilla/gecko/tabs/TabsGridLayout.java
+++ b/mobile/android/base/java/org/mozilla/gecko/tabs/TabsGridLayout.java
@@ -45,13 +45,14 @@ abstract class TabsGridLayout extends Ta
     @Override
     protected boolean addAtIndexRequiresScroll(int index) {
         final GridLayoutManager layoutManager = (GridLayoutManager) getLayoutManager();
         final int spanCount = layoutManager.getSpanCount();
         final int firstVisibleIndex = layoutManager.findFirstVisibleItemPosition();
         // When you add an item at the first visible position to a GridLayoutManager and there's
         // room to scroll, RecyclerView scrolls the new position to anywhere from near the bottom of
         // its row to completely offscreen (for unknown reasons), so we need to scroll to fix that.
-        // We also scroll when the item being added is the only item on the final row.
-        return index == firstVisibleIndex ||
-                (index == getAdapter().getItemCount() - 1 && index % spanCount == 0);
+        // We also always need to scroll if the new tab is a new last tab on a row by itself; more
+        // generally, another app can open a new last tab with the tabs tray open and the scroll at
+        // an arbitrary position, so we need to always scroll in that more general case as well.
+        return index == firstVisibleIndex || index == getAdapter().getItemCount() - 1;
     }
 }
--- a/mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayout.java
+++ b/mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayout.java
@@ -100,17 +100,19 @@ public abstract class TabsLayout extends
     @Override
     public void onTabChanged(Tab tab, Tabs.TabEvents msg, String data) {
         if (msg != Tabs.TabEvents.RESTORED && tab.getType() != type) {
             return;
         }
 
         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".
+                tabIndex = tabIndex == -1 ? tabsAdapter.getItemCount() : tabIndex;
                 tabsAdapter.notifyTabInserted(tab, tabIndex);
                 if (addAtIndexRequiresScroll(tabIndex)) {
                     // (The SELECTED tab is updated *after* this call to ADDED, so don't just call
                     // updateSelectedPosition().)
                     scrollToPosition(tabIndex);
                 }
                 break;
 
@@ -127,18 +129,19 @@ public abstract class TabsLayout extends
             case RECORDING_CHANGE:
             case AUDIO_PLAYING_CHANGE:
                 tabsAdapter.notifyTabChanged(tab);
                 break;
         }
     }
 
     /**
-     * Addition of a tab at selected positions (dependent on LayoutManager) will result in a tab
-     * being added out of view - return true if {@code index} is such a position.
+     * Addition of a tab at selected positions (dependent on LayoutManager) can result in a tab
+     * being added out of view - return true if {@code index} is such a position.  This should be
+     * called only after the add has occurred.
      */
     abstract protected boolean addAtIndexRequiresScroll(int index);
 
     protected int getSelectedAdapterPosition() {
         return tabsAdapter.getPositionForTab(Tabs.getInstance().getSelectedTab());
     }
 
     @Override
--- a/mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayoutAdapter.java
+++ b/mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayoutAdapter.java
@@ -79,28 +79,29 @@ public class TabsLayoutAdapter
 
     /* package */ void notifyTabChanged(Tab tab) {
         final int position = getPositionForTab(tab);
         if (position != -1) {
             notifyItemChanged(position);
         }
     }
 
+    /**
+     * Insert {@code tab} in the tabs list at the specified {@code index}.
+     * @param index 0 <= index <= current tab count
+     */
     /* package */ void notifyTabInserted(Tab tab, int index) {
         if (index >= 0 && index <= tabs.size()) {
             tabs.add(index, tab);
             notifyItemInserted(index);
         } else {
-            // Add to the end.
+            // The index is out of bounds; add to the end.
             tabs.add(tab);
             notifyItemInserted(tabs.size() - 1);
-            // index == -1 is a valid way to add to the end, the other cases are errors.
-            if (index != -1) {
-                Log.e(LOGTAG, "Tab was inserted at an invalid position: " + Integer.toString(index));
-            }
+            Log.e(LOGTAG, "Tab was inserted at an invalid position: " + Integer.toString(index));
         }
     }
 
     /* package */ boolean moveTab(int fromPosition, int toPosition) {
         final int fromTabId = tabs.get(fromPosition).getId();
         final int toTabId = tabs.get(toPosition).getId();
         JavaUtil.moveInList(tabs, fromPosition, toPosition);
         notifyItemMoved(fromPosition, toPosition);
--- a/mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java
+++ b/mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java
@@ -106,11 +106,14 @@ public class TabsListLayout extends Tabs
             }, cascadeDelay);
 
             cascadeDelay += ANIMATION_CASCADE_DELAY;
         }
     }
 
     @Override
     protected boolean addAtIndexRequiresScroll(int index) {
+        // Scroll if we're adding a new first tab (from a close undo) or if we're adding a new last
+        // tab (needed both for close undo and for when a new last tab is added by another app
+        // opening a link in Fennec where Fennec loads with the tabs tray already open).
         return index == 0 || index == getAdapter().getItemCount() - 1;
     }
 }