Skip to content

Commit

Permalink
[Android][Natsu] Introduce KeyboardExtensionStates for header in sheet
Browse files Browse the repository at this point in the history
KeyboardExtensionState determines what components are displayed
in the keyboard accessory. In V2, we would like to move the sheet header
from the BAR to the VISIBLE_SHEET. New states are required to achieve
this.

We cannot simply replace the existing states because V1 still needs the
bar while the sheets are shown (it still displays the tab switcher).
Meanwhile in V2 the BAR is not required while the sheet is displayed.

Followup CLs about this topic will clean up the sheet header in the BAR
and also do styling / A11y, if required.

Bug: 1420520
Change-Id: I722d807234d1162409643d0737c6db608500f83e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4306717
Commit-Queue: Adem Derinel <derinel@google.com>
Reviewed-by: Friedrich Horschig <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1116463}
  • Loading branch information
deephand authored and Chromium LUCI CQ committed Mar 13, 2023
1 parent f8bd479 commit d95f882
Show file tree
Hide file tree
Showing 19 changed files with 271 additions and 128 deletions.
Expand Up @@ -16,8 +16,7 @@ found in the LICENSE file.
android:paddingTop="@dimen/keyboard_accessory_image_top_padding"
android:paddingBottom="@dimen/keyboard_accessory_image_top_padding"
android:contentDescription="@string/keyboard_accessory_sheet_hide"
android:background="?attr/selectableItemBackground"
android:visibility="gone" />
android:background="?attr/selectableItemBackground" />

<TextView
android:id="@+id/sheet_title"
Expand All @@ -26,7 +25,6 @@ found in the LICENSE file.
android:textAppearance="@style/TextAppearance.TextLarge.Primary"
android:layout_height="wrap_content"
android:layout_width="match_parent"
android:background="?android:attr/selectableItemBackground"
android:visibility="gone" />
android:background="?android:attr/selectableItemBackground" />

</merge>
Expand Up @@ -52,8 +52,9 @@ public void initialize(WindowAndroid windowAndroid, BottomSheetController sheetC
barStub.setShouldInflateOnBackgroundThread(true);
sheetStub.setShouldInflateOnBackgroundThread(true);
initialize(windowAndroid, new KeyboardAccessoryCoordinator(mMediator, mMediator, barStub),
new AccessorySheetCoordinator(sheetStub), sheetController, backPressManager,
keyboardDelegate, new ConfirmationDialogHelper(windowAndroid.getContext()));
new AccessorySheetCoordinator(sheetStub, mMediator), sheetController,
backPressManager, keyboardDelegate,
new ConfirmationDialogHelper(windowAndroid.getContext()));
}

@VisibleForTesting
Expand Down

Large diffs are not rendered by default.

Expand Up @@ -7,9 +7,12 @@
import static org.chromium.chrome.browser.keyboard_accessory.ManualFillingProperties.KeyboardExtensionState.EXTENDING_KEYBOARD;
import static org.chromium.chrome.browser.keyboard_accessory.ManualFillingProperties.KeyboardExtensionState.FLOATING_BAR;
import static org.chromium.chrome.browser.keyboard_accessory.ManualFillingProperties.KeyboardExtensionState.FLOATING_SHEET;
import static org.chromium.chrome.browser.keyboard_accessory.ManualFillingProperties.KeyboardExtensionState.FLOATING_SHEET_V2;
import static org.chromium.chrome.browser.keyboard_accessory.ManualFillingProperties.KeyboardExtensionState.HIDDEN;
import static org.chromium.chrome.browser.keyboard_accessory.ManualFillingProperties.KeyboardExtensionState.REPLACING_KEYBOARD;
import static org.chromium.chrome.browser.keyboard_accessory.ManualFillingProperties.KeyboardExtensionState.REPLACING_KEYBOARD_V2;
import static org.chromium.chrome.browser.keyboard_accessory.ManualFillingProperties.KeyboardExtensionState.WAITING_TO_REPLACE;
import static org.chromium.chrome.browser.keyboard_accessory.ManualFillingProperties.KeyboardExtensionState.WAITING_TO_REPLACE_V2;
import static org.chromium.chrome.browser.keyboard_accessory.ManualFillingProperties.StateProperty.BAR;
import static org.chromium.chrome.browser.keyboard_accessory.ManualFillingProperties.StateProperty.FLOATING;
import static org.chromium.chrome.browser.keyboard_accessory.ManualFillingProperties.StateProperty.HIDDEN_SHEET;
Expand Down Expand Up @@ -43,7 +46,7 @@ class ManualFillingProperties {
* Properties that a given state enforces. Must be between 0x0 and 0x100.
* @see KeyboardExtensionState
*/
@IntDef({BAR, VISIBLE_SHEET, HIDDEN_SHEET})
@IntDef({BAR, VISIBLE_SHEET, HIDDEN_SHEET, FLOATING})
@Retention(RetentionPolicy.SOURCE)
public @interface StateProperty {
int BAR = 0x1; // Any state either shows it or hides it - there is no neutral stance.
Expand All @@ -60,9 +63,13 @@ class ManualFillingProperties {
* e.g. for <code> int FLOATING_BAR = BAR | HIDDEN_SHEET | FLOATING; </code>
* The state FLOATING_BAR must close the sheet but show the bar. To satisfy the FLOATING
* property, the state will ensure that the keyboard can not affect it.
*
* TODO(crbug/1420520): Replace REPLACING_KEYBOARD, WAITING_TO_REPLACE, FLOATING_SHEET states
* with REPLACING_KEYBOARD_V2, WAITING_TO_REPLACE_V2, FLOATING_SHEET_V2 once the legacy
* accessory is cleaned up.
*/
@IntDef({HIDDEN, EXTENDING_KEYBOARD, WAITING_TO_REPLACE, REPLACING_KEYBOARD, FLOATING_BAR,
FLOATING_SHEET})
FLOATING_SHEET, REPLACING_KEYBOARD_V2, WAITING_TO_REPLACE_V2, FLOATING_SHEET_V2})
@Retention(RetentionPolicy.SOURCE)
public @interface KeyboardExtensionState {
int HIDDEN = HIDDEN_SHEET; // == 4
Expand All @@ -71,6 +78,9 @@ class ManualFillingProperties {
int WAITING_TO_REPLACE = BAR; // == 1
int FLOATING_BAR = BAR | HIDDEN_SHEET | FLOATING; // == 13
int FLOATING_SHEET = BAR | VISIBLE_SHEET | FLOATING; // == 11
int REPLACING_KEYBOARD_V2 = VISIBLE_SHEET; // == 2
int WAITING_TO_REPLACE_V2 = 0;
int FLOATING_SHEET_V2 = VISIBLE_SHEET | FLOATING; // == 10
}

static PropertyModel createFillingModel() {
Expand Down
Expand Up @@ -37,8 +37,8 @@ a number of signals. One example:
1. The component is in the HIDDEN state.
1. The signal `showWhenKeyboardIsVisible()` sets the state to `FLOATING_BAR`.
1. The component checks in `meetsStatePreconditions()` whether the set state
fulfills all state-dependent preconditions (e.g. "is VR mode active?" if so,
it would transition into the `HIDDEN` state instead.)
fulfills all state-dependent preconditions (if so, it would transition into
the `HIDDEN` state instead.)
1. In `enforceStateProperties`, the filling component modifies the subcomponents
according to the new state which means it:
1. shows the keyboard accessory bar
Expand Down Expand Up @@ -70,17 +70,27 @@ since these untethered states either:
* couldn't show a keyboard anyway (because multi-window/hardware suppresses it
but Chrome doesn't know that beforehand)

| ID | State | Accessory Bar | Fallback Sheet | Floats | Transition into*
|--------|--------------------|--------------------------|-----------------------------------------|---------|-
| 0x0100 | HIDDEN | Hidden | Hidden | N/A | FLOATING_BAR, REPLACING_KEYBOARD
| 0x0101 | EXTENDING_KEYBOARD | **Visible** | Hidden | No | WAITING_TO_REPLACE
| 0x0001 | WAITING_TO_REPLACE | **Visible** | N/A — waits for keyboard to (dis)appear | No | REPLACING_KEYBOARD
| 0x0011 | REPLACING_KEYBOARD | **Visible** as title bar | **Visible** | No | FLOATING_SHEET
| 0x1101 | FLOATING_BAR | **Visible** | Hidden | **Yes** |FLOATING_SHEET
| 0x1011 | FLOATING_SHEET | **Visible** as title bar | **Visible** | **Yes** | FLOATING_BAR
| ID | State | Accessory Bar | Fallback Sheet | Floats | Transition into*
|--------|-----------------------|--------------------------|-----------------------------------------|---------|-
| 0x0100 | HIDDEN | Hidden | Hidden | N/A | FLOATING_BAR, REPLACING_KEYBOARD, REPLACING_KEYBOARD_V2
| 0x0101 | EXTENDING_KEYBOARD | **Visible** | Hidden | No | WAITING_TO_REPLACE, WAITING_TO_REPLACE_V2
| 0x0001 | WAITING_TO_REPLACE | **Visible** | N/A — waits for keyboard to (dis)appear | No | REPLACING_KEYBOARD
| 0x0000 | WAITING_TO_REPLACE_V2 | Hidden | N/A — waits for keyboard to (dis)appear | No | REPLACING_KEYBOARD_V2
| 0x0011 | REPLACING_KEYBOARD | **Visible** as title bar | **Visible** | No | FLOATING_SHEET
| 0x0010 | REPLACING_KEYBOARD_V2 | Hidden | **Visible** | No | FLOATING_SHEET_V2
| 0x1101 | FLOATING_BAR | **Visible** | Hidden | **Yes** | FLOATING_SHEET, FLOATING_SHEET_V2
| 0x1011 | FLOATING_SHEET | **Visible** as title bar | **Visible** | **Yes** | FLOATING_BAR
| 0x1010 | FLOATING_SHEET_V2 | Hidden | **Visible** | **Yes** | FLOATING_BAR

\* Excluding HIDDEN and EXTENDING_KEYBOARD which can be entered from any state.

In V2 (See [Versioning](#versioning) below), the sheet selector component
(KeyboardAccessoryTabLayoutComponent) is not visible to the user. Therefore
REPLACING_KEYBOARD, FLOATING_SHEET and WAITING_TO_REPLACE states are not
usable. States ending with V2 remove the bar requirement from those states and
are available only if V2 is enabled. V2 states must not be used when V2 is not
enabled. They must be replaced with non V2 equivalents once V2 is launched.

### Using providers to push data

The manual filling component cannot verify the correctness of displayed
Expand Down
Expand Up @@ -291,6 +291,7 @@ public void onPropertyChanged(
public void onActiveTabChanged(Integer activeTab) {
mModel.set(KEYBOARD_TOGGLE_VISIBLE, activeTab != null);
if (activeTab == null) {
if (ChromeFeatureList.isEnabled(ChromeFeatureList.AUTOFILL_KEYBOARD_ACCESSORY)) return;
mSheetVisibilityDelegate.onCloseAccessorySheet();
return;
}
Expand All @@ -309,6 +310,8 @@ private void onKeyboardRequested() {
}

private void closeSheet() {
assert !ChromeFeatureList.isEnabled(ChromeFeatureList.AUTOFILL_KEYBOARD_ACCESSORY)
: "The bar cannot close the sheet when AUTOFILL_KEYBOARD_ACCESSORY is enabled. It must be closed by the sheet.";
assert mTabSwitcher.getActiveTab() != null;
ManualFillingMetricsRecorder.recordSheetTrigger(
mTabSwitcher.getActiveTab().getRecordingType(), AccessorySheetTrigger.MANUAL_CLOSE);
Expand Down
Expand Up @@ -56,23 +56,26 @@ public interface SheetVisibilityDelegate {
* parts up.
* @param sheetStub A {@link AsyncViewStub} for the accessory sheet layout.
*/
public AccessorySheetCoordinator(AsyncViewStub sheetStub) {
this(AsyncViewProvider.of(sheetStub, R.id.keyboard_accessory_sheet_container));
public AccessorySheetCoordinator(
AsyncViewStub sheetStub, SheetVisibilityDelegate sheetVisibilityDelegate) {
this(AsyncViewProvider.of(sheetStub, R.id.keyboard_accessory_sheet_container),
sheetVisibilityDelegate);
}

/**
* Constructor that allows to mock the {@link AsyncViewProvider}.
* @param viewProvider A provider for the accessory.
*/
@VisibleForTesting
AccessorySheetCoordinator(ViewProvider<AccessorySheetView> viewProvider) {
AccessorySheetCoordinator(ViewProvider<AccessorySheetView> viewProvider,
SheetVisibilityDelegate sheetVisibilityDelegate) {
PropertyModel model = AccessorySheetProperties.defaultPropertyModel().build();

LazyConstructionPropertyMcp.create(
model, VISIBLE, viewProvider, AccessorySheetViewBinder::bind);

AccessorySheetMetricsRecorder.registerAccessorySheetModelMetricsObserver(model);
mMediator = new AccessorySheetMediator(model);
mMediator = new AccessorySheetMediator(model, sheetVisibilityDelegate);
}

/**
Expand Down
Expand Up @@ -18,7 +18,11 @@
import androidx.recyclerview.widget.RecyclerView;
import androidx.viewpager.widget.ViewPager;

import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.keyboard_accessory.AccessorySheetTrigger;
import org.chromium.chrome.browser.keyboard_accessory.ManualFillingMetricsRecorder;
import org.chromium.chrome.browser.keyboard_accessory.data.KeyboardAccessoryData;
import org.chromium.chrome.browser.keyboard_accessory.sheet_component.AccessorySheetCoordinator.SheetVisibilityDelegate;
import org.chromium.ui.modelutil.PropertyKey;
import org.chromium.ui.modelutil.PropertyModel;
import org.chromium.ui.modelutil.PropertyObservable;
Expand All @@ -38,10 +42,15 @@ public void onScrolled(RecyclerView recyclerView, int dx, int dy) {
mModel.set(TOP_SHADOW_VISIBLE, recyclerView.canScrollVertically(-1));
}
};
private final SheetVisibilityDelegate mSheetVisibilityDelegate;

AccessorySheetMediator(PropertyModel model) {
AccessorySheetMediator(PropertyModel model, SheetVisibilityDelegate sheetVisibilityDelegate) {
mModel = model;
mModel.addObserver(this);
mSheetVisibilityDelegate = sheetVisibilityDelegate;
if (ChromeFeatureList.isEnabled(ChromeFeatureList.AUTOFILL_KEYBOARD_ACCESSORY)) {
mModel.set(SHOW_KEYBOARD_CALLBACK, this::onKeyboardRequested);
}
}

@Nullable
Expand Down Expand Up @@ -91,6 +100,16 @@ void setActiveTab(int position) {
mModel.set(ACTIVE_TAB_INDEX, position);
}

private void onKeyboardRequested() {
// Return early if the button was clicked twice and the active tab was already reset.
if (mModel.get(ACTIVE_TAB_INDEX) == NO_ACTIVE_TAB) return;
ManualFillingMetricsRecorder.recordSheetTrigger(
mModel.get(TABS).get(mModel.get(ACTIVE_TAB_INDEX)).getRecordingType(),
AccessorySheetTrigger.MANUAL_CLOSE);
mModel.set(ACTIVE_TAB_INDEX, NO_ACTIVE_TAB);
mSheetVisibilityDelegate.onCloseAccessorySheet();
}

@Override
public void onPropertyChanged(PropertyObservable<PropertyKey> source, PropertyKey propertyKey) {
if (propertyKey == VISIBLE) {
Expand Down
Expand Up @@ -51,7 +51,7 @@ protected void onFinishInflate() {
mKeyboardToggle.setImageDrawable(
AppCompatResources.getDrawable(getContext(), R.drawable.ic_arrow_back_24dp));
mSheetTitle = findViewById(R.id.sheet_title);
// TODO(crbug/1420520): make this visible when the sheet header is moved to the sheet.
findViewById(R.id.sheet_header).setVisibility(View.VISIBLE);
}

// Ensure that sub components of the sheet use the RTL direction:
Expand Down
Expand Up @@ -37,6 +37,7 @@
import org.chromium.base.test.util.CallbackHelper;
import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.CriteriaHelper;
import org.chromium.base.test.util.DisabledTest;
import org.chromium.chrome.browser.ChromeWindow;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.flags.ChromeSwitches;
Expand Down Expand Up @@ -254,6 +255,7 @@ public void testPressingBackButtonHidesAccessoryWithAutofillSuggestions()

@Test
@MediumTest
@DisabledTest(message = "crbug.com/1420520")
public void testSheetHasMinimumSizeWhenTriggeredBySuggestion() throws TimeoutException {
MultiWindowUtils.getInstance().setIsInMultiWindowModeForTesting(true);
loadTestPage(MultiWindowKeyboard::new);
Expand Down
Expand Up @@ -189,10 +189,6 @@ public void testAccessorySheetHiddenWhenRefocusingField() throws TimeoutExceptio
whenDisplayed(withChild(withId(R.id.keyboard_accessory_sheet_frame))).check((view, e) -> {
accessorySheetView.set(view);
});
// The accessory bar is now pushed up by the accessory.
CriteriaHelper.pollUiThread(() -> {
return accessoryMargins.get().bottomMargin == accessorySheetView.get().getHeight();
});

mHelper.focusPasswordField();
mHelper.waitForKeyboardAccessoryToBeShown();
Expand Down Expand Up @@ -527,4 +523,4 @@ public void testInfobarReopensOnPressingBack() throws TimeoutException {

whenDisplayed(withText(kInfoBarText));
}
}
}
Expand Up @@ -5,9 +5,9 @@
package org.chromium.chrome.browser.keyboard_accessory.sheet_component;

import static androidx.test.espresso.Espresso.onView;
import static androidx.test.espresso.action.ViewActions.click;
import static androidx.test.espresso.assertion.ViewAssertions.doesNotExist;
import static androidx.test.espresso.assertion.ViewAssertions.matches;
import static androidx.test.espresso.matcher.ViewMatchers.isAssignableFrom;
import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed;
import static androidx.test.espresso.matcher.ViewMatchers.isRoot;
import static androidx.test.espresso.matcher.ViewMatchers.withId;
Expand Down Expand Up @@ -36,11 +36,8 @@
import android.widget.LinearLayout;
import android.widget.TextView;

import androidx.test.espresso.UiController;
import androidx.test.espresso.ViewAction;
import androidx.test.filters.MediumTest;

import org.hamcrest.Matcher;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
Expand Down Expand Up @@ -240,26 +237,7 @@ public void testHeader() {
mModel.set(VISIBLE, true);
});

onView(isRoot()).check(waitForView(withId(R.id.show_keyboard), ViewUtils.VIEW_GONE));

// TODO(crbug/1420520): remove the anonymous ViewAction with click() once the header is
// enabled.
onView(withId(R.id.show_keyboard)).perform(new ViewAction() {
@Override
public Matcher<View> getConstraints() {
return isAssignableFrom(View.class);
}

@Override
public void perform(UiController uiController, View view) {
view.performClick();
}

@Override
public String getDescription() {
return "Click the hidden view";
}
});
onViewWaiting(withId(R.id.show_keyboard)).perform(click());

verify(runnable, times(1)).run();

Expand Down
Expand Up @@ -68,7 +68,8 @@ private void openLayoutInAccessorySheet(
mModel = new AccessorySheetTabItemsModel();
AccessorySheetCoordinator accessorySheet =
new AccessorySheetCoordinator(mActivityTestRule.getActivity().findViewById(
R.id.keyboard_accessory_sheet_stub));
R.id.keyboard_accessory_sheet_stub),
null);
accessorySheet.setTabs(new KeyboardAccessoryData.Tab[] {new KeyboardAccessoryData.Tab(
"Passwords", null, null, layout, AccessoryTabType.ALL, listener)});
accessorySheet.setHeight(
Expand Down
Expand Up @@ -67,7 +67,8 @@ public void setUp() throws InterruptedException {

AccessorySheetCoordinator accessorySheet =
new AccessorySheetCoordinator(mActivityTestRule.getActivity().findViewById(
R.id.keyboard_accessory_sheet_stub));
R.id.keyboard_accessory_sheet_stub),
null);
accessorySheet.setTabs(new KeyboardAccessoryData.Tab[] {new KeyboardAccessoryData.Tab(
"Addresses", null, null, R.layout.address_accessory_sheet,
AccessoryTabType.ADDRESSES, new KeyboardAccessoryData.Tab.Listener() {
Expand Down
Expand Up @@ -90,7 +90,8 @@ public void setUp() throws InterruptedException {
mModel = new AccessorySheetTabItemsModel();
AccessorySheetCoordinator accessorySheet =
new AccessorySheetCoordinator(mActivityTestRule.getActivity().findViewById(
R.id.keyboard_accessory_sheet_stub));
R.id.keyboard_accessory_sheet_stub),
null);
accessorySheet.setTabs(new KeyboardAccessoryData.Tab[] {new KeyboardAccessoryData.Tab(
"Credit Cards", null, null, R.layout.credit_card_accessory_sheet,
AccessoryTabType.CREDIT_CARDS, new KeyboardAccessoryData.Tab.Listener() {
Expand Down
Expand Up @@ -76,7 +76,8 @@ public void setUp() throws InterruptedException {

AccessorySheetCoordinator accessorySheet =
new AccessorySheetCoordinator(mActivityTestRule.getActivity().findViewById(
R.id.keyboard_accessory_sheet_stub));
R.id.keyboard_accessory_sheet_stub),
null);
accessorySheet.setTabs(new KeyboardAccessoryData.Tab[] {new KeyboardAccessoryData.Tab(
"Passwords", null, null, R.layout.password_accessory_sheet,
AccessoryTabType.ALL, new KeyboardAccessoryData.Tab.Listener() {
Expand Down

0 comments on commit d95f882

Please sign in to comment.