Commit 978a453a authored by Mei Liang's avatar Mei Liang Committed by Commit Bot

Fix TabSelectionEditor no show issue after using the system back button

crrev.com/c/2399314 uses a focusable PopupWindow for accessibility
support. The focusable PopupWindow catches the system back button event
and dismisses the PopupWindow directly without going through the
TabSelectionEditorMediator#hide, where it updates the
TabSelectionEditorProperties#IS_VISIBLE property from true to false. The
TabSelectionEditorProperties#IS_VISIBLE remains to be true. Then
TabSelectionEditorMediator#show is called again to show the
TabSelectionEditor has no effects. Since there is no property change,
the binder will not get triggered.

This CL fixes the issue by setting the TabSelectionEditorMediator#hide
as a Runnable to the TabSelectionEditorProperties, then binding it to
the TabSelectionEditorLayout and triggering the Runnable when the
PopupWindow is dismissed.

Everything introduced in this CL is gated by by Finch parameter
"enable_launch_polish" under flag "enable-tab-grid-layout". Most of the
code is verified to be behind the gating function
TabUiFeatureUtilities#isLaunchPolishEnabled by formal equivalence
checking tool here: http://crrev.com/c/1934235. The changes in
following files can't be verified by the tool, but it is expected and
no-op without the flags:
  * The new property in TabSelectionEditorProperties

Bug: 1132478
Change-Id: I774d13714445e76da9c9ef4573412b62bf042039
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2432969
Commit-Queue: Mei Liang <meiliang@chromium.org>
Reviewed-by: default avatarWei-Yin Chen (陳威尹) <wychen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811025}
parent b4f91407
......@@ -33,6 +33,7 @@ class TabSelectionEditorLayout extends SelectableListLayout<Integer> {
private ViewTreeObserver.OnGlobalLayoutListener mParentLayoutListener;
private @Nullable Rect mPositionRect;
private boolean mIsInitialized;
private Runnable mEditorHideController;
// TODO(meiliang): inflates R.layout.tab_selection_editor_layout in
// TabSelectionEditorCoordinator.
......@@ -46,6 +47,9 @@ class TabSelectionEditorLayout extends SelectableListLayout<Integer> {
// up the TalkBack. Focusable PopupWindow always focuses the first item in the content
// view and announces the first item twice under Talkback mode.
mWindow.setFocusable(true);
mWindow.setOnDismissListener(() -> {
if (mEditorHideController != null) mEditorHideController.run();
});
}
}
......@@ -149,4 +153,10 @@ class TabSelectionEditorLayout extends SelectableListLayout<Integer> {
Rect getPositionRectForTesting() {
return mPositionRect;
}
void setEditorHideController(Runnable hideController) {
if (!TabUiFeatureUtilities.isLaunchPolishEnabled()) return;
mEditorHideController = hideController;
}
}
......@@ -66,6 +66,9 @@ public class TabSelectionEditorLayoutBinder {
== propertyKey) {
view.getToolbar().setActionButtonDescriptionResourceId(model.get(
TabSelectionEditorProperties.TOOLBAR_ACTION_BUTTON_DESCRIPTION_RESOURCE_ID));
} else if (TabUiFeatureUtilities.isLaunchPolishEnabled()
&& TabSelectionEditorProperties.DISMISS_HANDLER == propertyKey) {
view.setEditorHideController(model.get(TabSelectionEditorProperties.DISMISS_HANDLER));
}
}
}
......@@ -178,6 +178,10 @@ class TabSelectionEditorMediator
TabSelectionEditorProperties.SELECTION_EDITOR_POSITION_RECT,
mPositionProvider.getSelectionEditorPositionRect()));
}
if (TabUiFeatureUtilities.isLaunchPolishEnabled()) {
mModel.set(TabSelectionEditorProperties.DISMISS_HANDLER, this::hide);
}
}
private boolean isEditorVisible() {
......
......@@ -59,10 +59,13 @@ public class TabSelectionEditorProperties {
.WritableIntPropertyKey TOOLBAR_ACTION_BUTTON_DESCRIPTION_RESOURCE_ID =
new PropertyModel.WritableIntPropertyKey();
public static final PropertyModel.WritableObjectPropertyKey<Runnable> DISMISS_HANDLER =
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,
SELECTION_EDITOR_POSITION_RECT, SELECTION_EDITOR_GLOBAL_LAYOUT_LISTENER,
TOOLBAR_ACTION_BUTTON_DESCRIPTION_RESOURCE_ID};
TOOLBAR_ACTION_BUTTON_DESCRIPTION_RESOURCE_ID, DISMISS_HANDLER};
}
......@@ -15,6 +15,7 @@ import android.view.View;
import android.view.ViewGroup;
import android.widget.Button;
import androidx.test.espresso.Espresso;
import androidx.test.filters.MediumTest;
import org.junit.After;
......@@ -538,6 +539,7 @@ public class TabSelectionEditorTest {
assertEquals("Group 2 selected tabs", actionButton.getContentDescription());
}
// This is a regression test for crbug.com/1132478.
@Test
@MediumTest
// clang-format off
......@@ -571,6 +573,24 @@ public class TabSelectionEditorTest {
mTabSelectionEditorLayout.getToolbar().getNavigationContentDescription());
}
@Test
@MediumTest
// clang-format off
@EnableFeatures({ChromeFeatureList.TAB_GRID_LAYOUT_ANDROID + "<Study"})
@CommandLineFlags.Add({"force-fieldtrials=Study/Group",
"force-fieldtrial-params=Study.Group:enable_launch_polish/true"})
public void testEditorHideCorrectly() {
// clang-format on
prepareBlankTab(2, false);
List<Tab> tabs = getTabsInCurrentTabModel();
TestThreadUtils.runOnUiThreadBlocking(() -> mTabSelectionEditorController.show(tabs));
mRobot.resultRobot.verifyTabSelectionEditorIsVisible();
Espresso.pressBack();
TestThreadUtils.runOnUiThreadBlocking(() -> mTabSelectionEditorController.show(tabs));
mRobot.resultRobot.verifyTabSelectionEditorIsVisible();
}
private List<Tab> getTabsInCurrentTabModel() {
List<Tab> tabs = new ArrayList<>();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment