TabGridDialog selection mode polish

* Tint the toolbar menu button properly.
* When rotation happens, update the position of selection editor
  PopupWindow instead of re-showing it.
* Calculate the dialog position instead of using getGlobalVisibleRect
  which doesn't deterministically happen early enough when rotating.
* Pull out positioning-related logics from TabSelectionEditorLayout
  to TabSelectionEditorMediator and add tests.

Bug: 1011269, 1011271
Change-Id: Iff3b5d993030f7798981b5ff5d945b28a2655188
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1846246
Commit-Queue: Yue Zhang <yuezhanggg@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Mei Liang <meiliang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706608}

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabGridDialogParent.java

@@ -31,6 +31,7 @@ import android.widget.RelativeLayout;
 import android.widget.TextView;
 
 import androidx.annotation.IntDef;
+import androidx.annotation.NonNull;
 
 import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.ContextUtils;
@@ -47,7 +48,7 @@ import java.lang.annotation.RetentionPolicy;
  * Parent for TabGridDialog component.
  */
 public class TabGridDialogParent
-        implements TabSelectionEditorLayout.TabSelectionEditorLayoutPositionProvider {
+        implements TabSelectionEditorMediator.TabSelectionEditorPositionProvider {
     private static final int DIALOG_ANIMATION_DURATION = 300;
     private static final int DIALOG_ALPHA_ANIMATION_DURATION = 150;
     private static final int CARD_FADE_ANIMATION_DURATION = 50;
@@ -654,19 +655,17 @@ public class TabGridDialogParent
     }
 
     /**
-     * {@link TabSelectionEditorLayout.TabSelectionEditorLayoutPositionProvider} implementation.
+     * {@link TabSelectionEditorMediator.TabSelectionEditorPositionProvider} implementation.
      * Returns a {@link Rect} that indicates the current position of dialog.
      */
     @Override
+    @NonNull
     public Rect getSelectionEditorPositionRect() {
-        Rect positionRect = new Rect();
-        mDialogContainerView.getGlobalVisibleRect(positionRect);
+        // Get the status bar height as offset.
         Rect parentRect = new Rect();
         mParent.getGlobalVisibleRect(parentRect);
-
-        // Offset by the status bar height.
-        positionRect.offset(0, parentRect.top);
-        return positionRect;
+        return new Rect(mSideMargin, mTopMargin + parentRect.top, mCurrentScreenWidth - mSideMargin,
+                mCurrentScreenHeight - mTopMargin);
     }
 
     /**

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabGroupUiToolbarView.java

@@ -103,6 +103,9 @@ public class TabGroupUiToolbarView extends FrameLayout {
         ApiCompatibilityUtils.setImageTintList(mLeftButton, tint);
         ApiCompatibilityUtils.setImageTintList(mRightButton, tint);
         if (mTitleTextView != null) mTitleTextView.setTextColor(tint);
+        if (mMenuButton != null) {
+            ApiCompatibilityUtils.setImageTintList(mMenuButton, tint);
+        }
     }
 
     /**

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabSelectionEditorCoordinator.java

@@ -74,7 +74,8 @@ class TabSelectionEditorCoordinator {
 
     public TabSelectionEditorCoordinator(Context context, View parentView,
             TabModelSelector tabModelSelector, TabContentManager tabContentManager,
-            TabSelectionEditorLayout.TabSelectionEditorLayoutPositionProvider positionProvider) {
+            @Nullable TabSelectionEditorMediator
+                    .TabSelectionEditorPositionProvider positionProvider) {
         mContext = context;
         mParentView = parentView;
         mTabModelSelector = tabModelSelector;
@@ -87,15 +88,14 @@ class TabSelectionEditorCoordinator {
                 .inflate(R.layout.tab_selection_editor_layout, null)
                 .findViewById(R.id.selectable_list);
         mTabSelectionEditorLayout.initialize(mParentView, mTabListCoordinator.getContainerView(),
-                mTabListCoordinator.getContainerView().getAdapter(), mSelectionDelegate,
-                positionProvider);
+                mTabListCoordinator.getContainerView().getAdapter(), mSelectionDelegate);
         mSelectionDelegate.setSelectionModeEnabledForZeroItems(true);
 
         mTabSelectionEditorLayoutChangeProcessor = PropertyModelChangeProcessor.create(
                 mModel, mTabSelectionEditorLayout, TabSelectionEditorLayoutBinder::bind);
 
-        mTabSelectionEditorMediator = new TabSelectionEditorMediator(
-                mContext, mTabModelSelector, this::resetWithListOfTabs, mModel, mSelectionDelegate);
+        mTabSelectionEditorMediator = new TabSelectionEditorMediator(mContext, mTabModelSelector,
+                this::resetWithListOfTabs, mModel, mSelectionDelegate, positionProvider);
     }
 
     /**

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabSelectionEditorLayout.java

@@ -14,8 +14,10 @@ import android.view.ViewGroup;
 import android.view.ViewTreeObserver;
 import android.widget.PopupWindow;
 
+import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 
+import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.browser.widget.selection.SelectableListLayout;
 import org.chromium.chrome.browser.widget.selection.SelectionDelegate;
 import org.chromium.chrome.tab_ui.R;
@@ -27,22 +29,10 @@ class TabSelectionEditorLayout extends SelectableListLayout<Integer> {
     private final PopupWindow mWindow;
     private TabSelectionEditorToolbar mToolbar;
     private View mParentView;
-    private TabSelectionEditorLayoutPositionProvider mPositionProvider;
     private ViewTreeObserver.OnGlobalLayoutListener mParentLayoutListener;
+    private @Nullable Rect mPositionRect;
     private boolean mIsInitialized;
 
-    /**
-     * An interface to provide the {@link Rect} used to position the selection editor layout on
-     * screen.
-     */
-    public interface TabSelectionEditorLayoutPositionProvider {
-        /**
-         * This method fetches the {@link Rect} used to position the selection editor layout.
-         * @return The {@link Rect} indicates where to show the selection editor layout.
-         */
-        Rect getSelectionEditorPositionRect();
-    }
-
     // TODO(meiliang): inflates R.layout.tab_selection_editor_layout in
     // TabSelectionEditorCoordinator.
     public TabSelectionEditorLayout(Context context, AttributeSet attrs) {
@@ -60,28 +50,15 @@ class TabSelectionEditorLayout extends SelectableListLayout<Integer> {
      * @param adapter The adapter that provides views that represent items in the recycler view.
      * @param selectionDelegate The {@link SelectionDelegate} that will inform the toolbar of
      *                            selection changes.
-     * @param positionProvider The {@link TabSelectionEditorLayoutPositionProvider} that provides
-     *         the position rect to show the selection editor layout.
      */
     void initialize(View parentView, RecyclerView recyclerView, RecyclerView.Adapter adapter,
-            SelectionDelegate<Integer> selectionDelegate,
-            @Nullable TabSelectionEditorLayoutPositionProvider positionProvider) {
+            SelectionDelegate<Integer> selectionDelegate) {
         mIsInitialized = true;
         initializeRecyclerView(adapter, recyclerView);
         mToolbar =
                 (TabSelectionEditorToolbar) initializeToolbar(R.layout.tab_selection_editor_toolbar,
                         selectionDelegate, 0, 0, 0, null, false, true);
         mParentView = parentView;
-        mPositionProvider = positionProvider;
-        // Re-fetch the position rect and re-show the selection editor in case there is a global
-        // layout change on parent view, e.g. re-layout caused by orientation change.
-        mParentLayoutListener = () -> {
-            if (mWindow.isShowing() && mPositionProvider != null) {
-                hide();
-                show();
-            }
-        };
-        mParentView.getViewTreeObserver().addOnGlobalLayoutListener(mParentLayoutListener);
     }
 
     /**
@@ -89,14 +66,14 @@ class TabSelectionEditorLayout extends SelectableListLayout<Integer> {
      */
     public void show() {
         assert mIsInitialized;
-        if (mPositionProvider == null) {
+        if (mPositionRect == null) {
             mWindow.showAtLocation(mParentView, Gravity.CENTER, 0, 0);
             return;
         }
-        Rect rect = mPositionProvider.getSelectionEditorPositionRect();
-        mWindow.setWidth(rect.width());
-        mWindow.setHeight(rect.height());
-        mWindow.showAtLocation(mParentView, Gravity.NO_GRAVITY, rect.left, rect.top);
+        mWindow.setWidth(mPositionRect.width());
+        mWindow.setHeight(mPositionRect.height());
+        mWindow.showAtLocation(
+                mParentView, Gravity.NO_GRAVITY, mPositionRect.left, mPositionRect.top);
     }
 
     /**
@@ -114,6 +91,32 @@ class TabSelectionEditorLayout extends SelectableListLayout<Integer> {
         return mToolbar;
     }
 
+    /**
+     * Register a {@link ViewTreeObserver.OnGlobalLayoutListener} handling TabSelectionEditor
+     * related changes when parent view global layout changed.
+     */
+    public void registerGlobalLayoutListener(ViewTreeObserver.OnGlobalLayoutListener listener) {
+        if (mParentView == null || listener == null) return;
+        if (mParentLayoutListener != null) {
+            mParentView.getViewTreeObserver().removeOnGlobalLayoutListener(mParentLayoutListener);
+        }
+        mParentLayoutListener = listener;
+        mParentView.getViewTreeObserver().addOnGlobalLayoutListener(listener);
+    }
+
+    /**
+     * Update the {@link Rect} used to show the selection editor. If the editor is currently
+     * showing, update its positioning.
+     * @param rect  The {@link Rect} to update mPositionRect.
+     */
+    public void updateTabSelectionEditorPositionRect(@NonNull Rect rect) {
+        assert rect != null;
+        mPositionRect = rect;
+        if (mWindow.isShowing()) {
+            mWindow.update(rect.left, rect.top, rect.width(), rect.height());
+        }
+    }
+
     /**
      * Destroy any members that needs clean up.
      */
@@ -122,4 +125,9 @@ class TabSelectionEditorLayout extends SelectableListLayout<Integer> {
             mParentView.getViewTreeObserver().removeOnGlobalLayoutListener(mParentLayoutListener);
         }
     }
+
+    @VisibleForTesting
+    Rect getPositionRectForTesting() {
+        return mPositionRect;
+    }
 }

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabSelectionEditorLayoutBinder.java

@@ -4,6 +4,8 @@
 
 package org.chromium.chrome.browser.tasks.tab_management;
 
+import android.graphics.Rect;
+
 import org.chromium.ui.modelutil.PropertyKey;
 import org.chromium.ui.modelutil.PropertyModel;
 
@@ -49,6 +51,17 @@ public class TabSelectionEditorLayoutBinder {
                 == propertyKey) {
             view.getToolbar().setActionButtonEnablingThreshold(model.get(
                     TabSelectionEditorProperties.TOOLBAR_ACTION_BUTTON_ENABLING_THRESHOLD));
+        } else if (TabSelectionEditorProperties.SELECTION_EDITOR_POSITION_RECT == propertyKey) {
+            Rect positionRect =
+                    model.get(TabSelectionEditorProperties.SELECTION_EDITOR_POSITION_RECT);
+            if (positionRect == null) {
+                return;
+            }
+            view.updateTabSelectionEditorPositionRect(positionRect);
+        } else if (TabSelectionEditorProperties.SELECTION_EDITOR_GLOBAL_LAYOUT_LISTENER
+                == propertyKey) {
+            view.registerGlobalLayoutListener(model.get(
+                    TabSelectionEditorProperties.SELECTION_EDITOR_GLOBAL_LAYOUT_LISTENER));
         }
     }
 }

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabSelectionEditorMediator.java

@@ -6,10 +6,12 @@ package org.chromium.chrome.browser.tasks.tab_management;
 
 import android.content.Context;
 import android.content.res.ColorStateList;
+import android.graphics.Rect;
 import android.support.annotation.ColorInt;
 import android.support.v7.content.res.AppCompatResources;
 import android.view.View;
 
+import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 
 import org.chromium.base.ApiCompatibilityUtils;
@@ -47,6 +49,19 @@ class TabSelectionEditorMediator
         void resetWithListOfTabs(@Nullable List<Tab> tabs);
     }
 
+    /**
+     * An interface to provide the {@link Rect} used to position the selection editor on screen.
+     */
+    public interface TabSelectionEditorPositionProvider {
+        /**
+         * This method fetches the {@link Rect} used to position the selection editor layout.
+         * @return The {@link Rect} indicates where to show the selection editor layout. This Rect
+         * should never be null.
+         */
+        @NonNull
+        Rect getSelectionEditorPositionRect();
+    }
+
     private final Context mContext;
     private final TabModelSelector mTabModelSelector;
     private final ResetHandler mResetHandler;
@@ -54,6 +69,7 @@ class TabSelectionEditorMediator
     private final SelectionDelegate<Integer> mSelectionDelegate;
     private final TabModelSelectorTabModelObserver mTabModelObserver;
     private final TabModelSelectorObserver mTabModelSelectorObserver;
+    private final TabSelectionEditorPositionProvider mPositionProvider;
     private TabSelectionEditorActionProvider mActionProvider;
 
     private final View.OnClickListener mNavigationClickListener = new View.OnClickListener() {
@@ -81,12 +97,15 @@ class TabSelectionEditorMediator
 
     TabSelectionEditorMediator(Context context, TabModelSelector tabModelSelector,
             ResetHandler resetHandler, PropertyModel model,
-            SelectionDelegate<Integer> selectionDelegate) {
+            SelectionDelegate<Integer> selectionDelegate,
+            @Nullable TabSelectionEditorMediator
+                    .TabSelectionEditorPositionProvider positionProvider) {
         mContext = context;
         mTabModelSelector = tabModelSelector;
         mResetHandler = resetHandler;
         mModel = model;
         mSelectionDelegate = selectionDelegate;
+        mPositionProvider = positionProvider;
 
         mModel.set(
                 TabSelectionEditorProperties.TOOLBAR_NAVIGATION_LISTENER, mNavigationClickListener);
@@ -144,6 +163,14 @@ class TabSelectionEditorMediator
         // Default action for action button is to group selected tabs.
         mActionProvider = new TabSelectionEditorActionProvider(mTabModelSelector, this,
                 TabSelectionEditorActionProvider.TabSelectionEditorAction.GROUP);
+
+        if (mPositionProvider != null) {
+            mModel.set(TabSelectionEditorProperties.SELECTION_EDITOR_GLOBAL_LAYOUT_LISTENER,
+                    ()
+                            -> mModel.set(
+                                    TabSelectionEditorProperties.SELECTION_EDITOR_POSITION_RECT,
+                                    mPositionProvider.getSelectionEditorPositionRect()));
+        }
     }
 
     private boolean isEditorVisible() {
@@ -157,6 +184,10 @@ class TabSelectionEditorMediator
     public void show(List<Tab> tabs) {
         mResetHandler.resetWithListOfTabs(tabs);
         mSelectionDelegate.setSelectionModeEnabledForZeroItems(true);
+        if (mPositionProvider != null) {
+            mModel.set(TabSelectionEditorProperties.SELECTION_EDITOR_POSITION_RECT,
+                    mPositionProvider.getSelectionEditorPositionRect());
+        }
         mModel.set(TabSelectionEditorProperties.IS_VISIBLE, true);
     }
 

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabSelectionEditorProperties.java

@@ -5,7 +5,9 @@
 package org.chromium.chrome.browser.tasks.tab_management;
 
 import android.content.res.ColorStateList;
+import android.graphics.Rect;
 import android.view.View;
+import android.view.ViewTreeObserver;
 
 import org.chromium.ui.modelutil.PropertyKey;
 import org.chromium.ui.modelutil.PropertyModel;
@@ -45,8 +47,17 @@ public class TabSelectionEditorProperties {
     public static final PropertyModel.WritableIntPropertyKey TOOLBAR_TEXT_APPEARANCE =
             new PropertyModel.WritableIntPropertyKey();
 
+    public static final PropertyModel
+            .WritableObjectPropertyKey<Rect> SELECTION_EDITOR_POSITION_RECT =
+            new PropertyModel.WritableObjectPropertyKey<>();
+
+    public static final PropertyModel.WritableObjectPropertyKey<
+            ViewTreeObserver.OnGlobalLayoutListener> SELECTION_EDITOR_GLOBAL_LAYOUT_LISTENER =
+            new PropertyModel.WritableObjectPropertyKey<>();
+
     public static final PropertyKey[] ALL_KEYS = new PropertyKey[] {IS_VISIBLE,
             TOOLBAR_ACTION_BUTTON_LISTENER, TOOLBAR_ACTION_BUTTON_TEXT,
             TOOLBAR_ACTION_BUTTON_ENABLING_THRESHOLD, TOOLBAR_NAVIGATION_LISTENER, PRIMARY_COLOR,
-            TOOLBAR_BACKGROUND_COLOR, TOOLBAR_GROUP_BUTTON_TINT, TOOLBAR_TEXT_APPEARANCE};
+            TOOLBAR_BACKGROUND_COLOR, TOOLBAR_GROUP_BUTTON_TINT, TOOLBAR_TEXT_APPEARANCE,
+            SELECTION_EDITOR_POSITION_RECT, SELECTION_EDITOR_GLOBAL_LAYOUT_LISTENER};
 }

chrome/android/features/tab_ui/javatests/src/org/chromium/chrome/browser/tasks/tab_management/TabSelectionEditorLayoutBinderTest.java

@@ -4,16 +4,26 @@
 
 package org.chromium.chrome.browser.tasks.tab_management;
 
+import static junit.framework.Assert.assertEquals;
+import static junit.framework.Assert.assertFalse;
+import static junit.framework.Assert.assertTrue;
+
+import android.graphics.Rect;
 import android.support.annotation.NonNull;
 import android.support.test.annotation.UiThreadTest;
 import android.support.test.filters.MediumTest;
 import android.support.test.filters.SmallTest;
 import android.support.v7.widget.RecyclerView;
 import android.view.ViewGroup;
+import android.view.ViewTreeObserver;
 import android.widget.Button;
 import android.widget.LinearLayout;
 import android.widget.TextView;
 
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
 import org.chromium.chrome.browser.widget.selection.SelectionDelegate;
 import org.chromium.chrome.tab_ui.R;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
@@ -21,17 +31,11 @@ import org.chromium.chrome.test.ui.DummyUiActivityTestCase;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.ui.modelutil.PropertyModel;
 import org.chromium.ui.modelutil.PropertyModelChangeProcessor;
-import org.junit.Assert;
-import org.junit.Test;
-import org.junit.runner.RunWith;
 
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.concurrent.atomic.AtomicBoolean;
 
-import static junit.framework.Assert.assertFalse;
-import static junit.framework.Assert.assertTrue;
-
 /**
  * Tests for {@link TabSelectionEditorLayoutBinder}.
  */
@@ -41,21 +45,20 @@ public class TabSelectionEditorLayoutBinderTest extends DummyUiActivityTestCase
     private PropertyModel mModel = new PropertyModel(TabSelectionEditorProperties.ALL_KEYS);
     private PropertyModelChangeProcessor mMCP;
     private SelectionDelegate<Integer> mSelectionDelegate = new SelectionDelegate<>();
+    private ViewGroup mParentView;
 
     @Override
     public void setUpTest() throws Exception {
         super.setUpTest();
 
-        ViewGroup view = new LinearLayout(getActivity());
-        TabSelectionEditorLayout.TabSelectionEditorLayoutPositionProvider positionProvider =
-                () -> null;
+        mParentView = new LinearLayout(getActivity());
 
         TestThreadUtils.runOnUiThreadBlocking(() -> {
-            getActivity().setContentView(view);
+            getActivity().setContentView(mParentView);
             mEditorLayoutView =
                     (TabSelectionEditorLayout) getActivity().getLayoutInflater().inflate(
                             R.layout.tab_selection_editor_layout, null);
-            mEditorLayoutView.initialize(view, null, new RecyclerView.Adapter() {
+            mEditorLayoutView.initialize(mParentView, null, new RecyclerView.Adapter() {
                 @NonNull
                 @Override
                 public RecyclerView.ViewHolder onCreateViewHolder(
@@ -70,7 +73,7 @@ public class TabSelectionEditorLayoutBinderTest extends DummyUiActivityTestCase
                 public int getItemCount() {
                     return 0;
                 }
-            }, mSelectionDelegate, positionProvider);
+            }, mSelectionDelegate);
         });
         mMCP = PropertyModelChangeProcessor.create(
                 mModel, mEditorLayoutView, TabSelectionEditorLayoutBinder::bind);
@@ -121,4 +124,30 @@ public class TabSelectionEditorLayoutBinderTest extends DummyUiActivityTestCase
         mSelectionDelegate.setSelectedItems(selectedItem);
         assertFalse(button.isEnabled());
     }
-}
+
\ No newline at end of file
+    @Test
+    @SmallTest
+    @UiThreadTest
+    public void testSetPositionRect() {
+        Assert.assertNull(mEditorLayoutView.getPositionRectForTesting());
+
+        Rect rect = new Rect();
+        mModel.set(TabSelectionEditorProperties.SELECTION_EDITOR_POSITION_RECT, rect);
+
+        assertEquals(rect, mModel.get(TabSelectionEditorProperties.SELECTION_EDITOR_POSITION_RECT));
+    }
+
+    @Test
+    @SmallTest
+    @UiThreadTest
+    public void testRegisterGlobalLayoutListener() {
+        AtomicBoolean globalLayoutChanged = new AtomicBoolean();
+        globalLayoutChanged.set(false);
+
+        ViewTreeObserver.OnGlobalLayoutListener listener = () -> globalLayoutChanged.set(true);
+        mModel.set(TabSelectionEditorProperties.SELECTION_EDITOR_GLOBAL_LAYOUT_LISTENER, listener);
+        mParentView.getViewTreeObserver().dispatchOnGlobalLayout();
+
+        assertTrue(globalLayoutChanged.get());
+    }
+}