Commit f12d4ba6 authored by Friedrich Horschig's avatar Friedrich Horschig Committed by Commit Bot

[Android] Close accessory when suggestions were filled

This is a reland of https://chromium-review.googlesource.com/1126244
which was reverted because it promoted a legacy bug which triggered
a 2D keyboard in VR mode.

The keyboard accessory is not enabled in VR mode.
There is a test to ensure that resetting that the sheet doesn't
trigger the keyboard unnecessarily anymore.

Original change's description:
> This CL causes the accessory sheet to close when a user selected a valid
> filling suggestions.
> There is no indicator of the negative case yet (i.e. error during
> filling).

TBR=rouslan@chromium.org

Bug: 856026, 857592, 865749
Change-Id: I71abca9366cfa96789ffe7538204f5435332e4ac
Reviewed-on: https://chromium-review.googlesource.com/1146361
Commit-Queue: Friedrich Horschig <fhorschig@chromium.org>
Reviewed-by: default avatarAldo Culquicondor <acondor@chromium.org>
Reviewed-by: default avatarVasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: default avatarFriedrich Horschig <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578343}
parent affe21c6
......@@ -55,6 +55,12 @@ public class KeyboardAccessoryCoordinator {
* Called when the whole accessory should be hidden (e.g. due to selecting a suggestion).
*/
void onCloseKeyboardAccessory();
/**
* Called when the normal keyboard should be brought back (e.g. the user dismissed a bottom
* sheet manually because they didn't find a suitable suggestion).
*/
void onOpenKeyboard();
}
/**
......@@ -102,6 +108,10 @@ public class KeyboardAccessoryCoordinator {
return tabViewBinder;
}
public void closeActiveTab() {
mMediator.closeActiveTab();
}
/**
* Called by the {@link LazyViewBinderAdapter} as soon as the view is inflated so it can be
* initialized. This call happens before the {@link KeyboardAccessoryViewBinder} is called for
......
......@@ -83,7 +83,6 @@ class KeyboardAccessoryMediator
void dismiss() {
mModel.setActiveTab(null);
mVisibilityDelegate.onCloseKeyboardAccessory();
if (mModel.getAutofillSuggestions() != null) {
mModel.getAutofillSuggestions().dismiss();
mModel.setAutofillSuggestions(null);
......@@ -91,6 +90,10 @@ class KeyboardAccessoryMediator
updateVisibility();
}
void closeActiveTab() {
mModel.setActiveTab(null);
}
@VisibleForTesting
KeyboardAccessoryModel getModelForTesting() {
return mModel;
......@@ -126,6 +129,13 @@ class KeyboardAccessoryMediator
return;
}
if (propertyKey == KeyboardAccessoryModel.PropertyKey.ACTIVE_TAB) {
Integer activeTab = mModel.activeTab();
if (activeTab == null) {
mVisibilityDelegate.onCloseAccessorySheet();
return;
}
mVisibilityDelegate.onChangeAccessorySheet(activeTab);
mVisibilityDelegate.onOpenAccessorySheet();
return;
}
if (propertyKey == KeyboardAccessoryModel.PropertyKey.TAB_SELECTION_CALLBACKS) {
......@@ -140,10 +150,7 @@ class KeyboardAccessoryMediator
@Override
public void onTabSelected(TabLayout.Tab tab) {
boolean hadNoActiveTab = mModel.activeTab() == null;
mModel.setActiveTab(tab.getPosition());
mVisibilityDelegate.onChangeAccessorySheet(tab.getPosition());
if (hadNoActiveTab) mVisibilityDelegate.onOpenAccessorySheet();
}
@Override
......@@ -153,11 +160,9 @@ class KeyboardAccessoryMediator
public void onTabReselected(TabLayout.Tab tab) {
if (mModel.activeTab() == null) {
mModel.setActiveTab(tab.getPosition());
mVisibilityDelegate.onChangeAccessorySheet(tab.getPosition());
mVisibilityDelegate.onOpenAccessorySheet();
} else {
mModel.setActiveTab(null);
mVisibilityDelegate.onCloseAccessorySheet();
mVisibilityDelegate.onOpenKeyboard();
}
}
......
......@@ -91,7 +91,9 @@ class KeyboardAccessoryModel extends PropertyObservable<KeyboardAccessoryModel.P
return mVisible;
}
void setActiveTab(Integer activeTab) {
@SuppressWarnings("ReferenceEquality") // No action if both are null or exact same object.
void setActiveTab(@Nullable Integer activeTab) {
if (activeTab == mActiveTab) return;
if (null != mActiveTab && mActiveTab.equals(activeTab)) return;
mActiveTab = activeTab;
notifyPropertyChanged(PropertyKey.ACTIVE_TAB);
......
......@@ -54,6 +54,21 @@ public class ManualFillingCoordinator {
return mMediator.handleBackPress();
}
/**
* Requests to close the active tab in the keyboard accessory. If there is no active tab, this
* is a NoOp.
*/
public void closeAccessorySheet() {
mMediator.getKeyboardAccessory().closeActiveTab();
}
/**
* Tries to reopen the keyboard which will implicitly show the keyboard accessory bar again.
*/
public void openKeyboard() {
mMediator.onOpenKeyboard();
}
void registerActionProvider(Provider<KeyboardAccessoryData.Action> actionProvider) {
mMediator.registerActionProvider(actionProvider);
}
......
......@@ -186,7 +186,6 @@ class ManualFillingMediator
@Override
public void onChangeAccessorySheet(int tabIndex) {
assert mActivity != null : "ManualFillingMediator needs initialization.";
mAccessorySheet.setActiveTab(tabIndex);
}
......@@ -199,9 +198,7 @@ class ManualFillingMediator
@Override
public void onCloseAccessorySheet() {
assert mActivity != null : "ManualFillingMediator needs initialization.";
mAccessorySheet.hide();
UiUtils.showKeyboard(mActivity.getCurrentFocus());
}
@Override
......@@ -211,6 +208,12 @@ class ManualFillingMediator
UiUtils.hideKeyboard(mActivity.getCurrentFocus());
}
@Override
public void onOpenKeyboard() {
assert mActivity != null : "ManualFillingMediator needs initialization.";
UiUtils.showKeyboard(mActivity.getCurrentFocus());
}
private AccessoryState getOrCreateAccessoryState(Tab tab) {
AccessoryState state = mModel.get(tab);
if (state != null) return state;
......
......@@ -73,6 +73,16 @@ class PasswordAccessoryBridge {
mActionProvider.notifyObservers(generationAction);
}
@CalledByNative
private void closeAccessorySheet() {
mManualFillingCoordinator.closeAccessorySheet();
}
@CalledByNative
private void openKeyboard() {
mManualFillingCoordinator.openKeyboard();
}
@CalledByNative
private void destroy() {
mItemProvider.notifyObservers(new Item[] {}); // There are no more items available!
......
......@@ -111,6 +111,10 @@ public class AutofillKeyboardAccessoryIntegrationTest {
@Test
@MediumTest
@Feature({"keyboard-accessory"})
@DisabledTest(message = "crbug.com/854224")
// TODO(fhorschig): Figure out why this test exists. If a keyboard is shown, the accessory
// should be there. If there is no keyboard, there shouldn't be an accessory. Looks more like a
// keyboard test than an accessory test.
public void testAutofocusedFieldDoesNotShowKeyboardAccessory()
throws ExecutionException, InterruptedException, TimeoutException {
loadTestPage(false);
......
......@@ -4,20 +4,24 @@
package org.chromium.chrome.browser.autofill.keyboard_accessory;
import static android.support.test.espresso.Espresso.onView;
import static android.support.test.espresso.action.ViewActions.click;
import static android.support.test.espresso.assertion.ViewAssertions.matches;
import static android.support.test.espresso.contrib.RecyclerViewActions.scrollToPosition;
import static android.support.test.espresso.matcher.ViewMatchers.assertThat;
import static android.support.test.espresso.matcher.ViewMatchers.isDisplayed;
import static android.support.test.espresso.matcher.ViewMatchers.withChild;
import static android.support.test.espresso.matcher.ViewMatchers.withId;
import static android.support.test.espresso.matcher.ViewMatchers.withText;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;
import static org.chromium.chrome.browser.autofill.keyboard_accessory.ManualFillingTestHelper.selectTabAtPosition;
import static org.chromium.chrome.browser.autofill.keyboard_accessory.ManualFillingTestHelper.whenDisplayed;
import android.support.test.espresso.Espresso;
import android.support.test.espresso.matcher.BoundedMatcher;
import android.support.test.filters.SmallTest;
import android.text.method.PasswordTransformationMethod;
......@@ -187,6 +191,30 @@ public class PasswordAccessoryIntegrationTest {
assertThat(clicked.get().getCaption(), equalTo("mayapark@gmail.com"));
}
@Test
@SmallTest
@EnableFeatures({ChromeFeatureList.PASSWORDS_KEYBOARD_ACCESSORY})
@FlakyTest(message = "crbug.com/855617")
public void testDisplaysEmptyStateMessageWithoutSavedPasswords()
throws InterruptedException, TimeoutException {
mHelper.loadTestPage(false);
// Focus the field to bring up the accessory.
mHelper.clickPasswordField();
mHelper.waitForKeyboard();
// Click the tab to show the sheet and hide the keyboard.
whenDisplayed(withId(R.id.tabs)).perform(selectTabAtPosition(0));
mHelper.waitForKeyboardToDisappear();
whenDisplayed(withId(R.id.keyboard_accessory_sheet));
onView(withText(containsString("No saved passwords"))).check(matches(isDisplayed()));
Espresso.pressBack();
mHelper.waitToBeHidden(withId(R.id.keyboard_accessory_sheet));
mHelper.waitToBeHidden(withId(R.id.keyboard_accessory));
}
/**
* Matches any {@link TextView} which applies a {@link PasswordTransformationMethod}.
* @return The matcher checking the transformation method.
......
......@@ -214,4 +214,27 @@ public class KeyboardAccessoryControllerTest {
mCoordinator.addTab(mTestTab);
assertThat(mModel.isVisible(), is(true));
}
@Test
public void testClosingTabDismissesOpenSheet() {
mModel.setActiveTab(0);
mModel.addObserver(mMockPropertyObserver);
assertThat(mModel.activeTab(), is(0));
// Closing the active tab should reset the tab which should trigger the visibility delegate.
mCoordinator.closeActiveTab();
assertThat(mModel.activeTab(), is(nullValue()));
verify(mMockPropertyObserver)
.onPropertyChanged(mModel, KeyboardAccessoryModel.PropertyKey.ACTIVE_TAB);
verify(mMockVisibilityDelegate).onCloseAccessorySheet();
}
@Test
public void testClosingTabIsNoOpForAlreadyClosedTab() {
mModel.setActiveTab(null);
mModel.addObserver(mMockPropertyObserver);
mCoordinator.closeActiveTab();
verifyNoMoreInteractions(mMockPropertyObserver, mMockVisibilityDelegate);
}
}
\ No newline at end of file
......@@ -64,6 +64,16 @@ void PasswordAccessoryViewAndroid::OnItemsAvailable(
base::android::ToJavaIntArray(env, item_types));
}
void PasswordAccessoryViewAndroid::CloseAccessorySheet() {
Java_PasswordAccessoryBridge_closeAccessorySheet(
base::android::AttachCurrentThread(), java_object_);
}
void PasswordAccessoryViewAndroid::OpenKeyboard() {
Java_PasswordAccessoryBridge_openKeyboard(
base::android::AttachCurrentThread(), java_object_);
}
void PasswordAccessoryViewAndroid::OnAutomaticGenerationStatusChanged(
bool available) {
if (!available && java_object_.is_null())
......@@ -71,7 +81,7 @@ void PasswordAccessoryViewAndroid::OnAutomaticGenerationStatusChanged(
JNIEnv* env = base::android::AttachCurrentThread();
Java_PasswordAccessoryBridge_onAutomaticGenerationStatusChanged(
env, java_object_, available /* available */);
env, java_object_, available);
}
void PasswordAccessoryViewAndroid::OnFaviconRequested(
......
......@@ -29,6 +29,8 @@ class PasswordAccessoryViewAndroid : public PasswordAccessoryViewInterface {
// PasswordAccessoryViewInterface:
void OnItemsAvailable(const std::vector<AccessoryItem>& items) override;
void OnAutomaticGenerationStatusChanged(bool available) override;
void CloseAccessorySheet() override;
void OpenKeyboard() override;
// Called from Java via JNI:
void OnFaviconRequested(
......
......@@ -454,9 +454,7 @@ void ChromePasswordManagerClient::PasswordWasAutofilled(
const std::vector<const autofill::PasswordForm*>* federated_matches) const {
#if defined(OS_ANDROID)
// Either #passwords-keyboards-accessory or #experimental-ui must be enabled.
if (!base::FeatureList::IsEnabled(
password_manager::features::kPasswordsKeyboardAccessory) &&
!base::FeatureList::IsEnabled(features::kExperimentalUi)) {
if (!PasswordAccessoryController::AllowedForWebContents(web_contents())) {
return; // No need to even create the bridge if it's not going to be used.
}
// If an accessory exists already, |CreateForWebContents| is a NoOp
......@@ -700,9 +698,7 @@ void ChromePasswordManagerClient::AutomaticGenerationStatusChanged(
return;
#if defined(OS_ANDROID)
// Either #passwords-keyboards-accessory or #experimental-ui must be enabled.
if (!base::FeatureList::IsEnabled(
password_manager::features::kPasswordsKeyboardAccessory) &&
!base::FeatureList::IsEnabled(features::kExperimentalUi)) {
if (!PasswordAccessoryController::AllowedForWebContents(web_contents())) {
if (available) {
ShowPasswordGenerationPopup(ui_data.value(),
false /* is_manually_triggered */);
......@@ -1076,9 +1072,7 @@ void ChromePasswordManagerClient::FocusedInputChanged(bool is_fillable,
bool is_password_field) {
#if defined(OS_ANDROID)
// Either #passwords-keyboards-accessory or #experimental-ui must be enabled.
if (!base::FeatureList::IsEnabled(
password_manager::features::kPasswordsKeyboardAccessory) &&
!base::FeatureList::IsEnabled(features::kExperimentalUi)) {
if (!PasswordAccessoryController::AllowedForWebContents(web_contents())) {
return; // No need to even create the bridge if it's not going to be used.
}
if (is_fillable) { // If not fillable, update existing an accessory only.
......
......@@ -13,6 +13,7 @@
#include "chrome/browser/password_manager/password_generation_dialog_view_interface.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/passwords/manage_passwords_view_utils.h"
#include "chrome/browser/vr/vr_tab_helper.h"
#include "chrome/grit/generated_resources.h"
#include "components/autofill/content/browser/content_autofill_driver.h"
#include "components/autofill/content/browser/content_autofill_driver_factory.h"
......@@ -21,9 +22,11 @@
#include "components/password_manager/content/browser/content_password_manager_driver.h"
#include "components/password_manager/content/browser/content_password_manager_driver_factory.h"
#include "components/password_manager/core/browser/password_manager_driver.h"
#include "components/password_manager/core/common/password_manager_features.h"
#include "components/strings/grit/components_strings.h"
#include "content/public/browser/web_contents.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/ui_base_features.h"
using autofill::PasswordForm;
using Item = PasswordAccessoryViewInterface::AccessoryItem;
......@@ -100,6 +103,19 @@ PasswordAccessoryController::PasswordAccessoryController(
PasswordAccessoryController::~PasswordAccessoryController() = default;
// static
bool PasswordAccessoryController::AllowedForWebContents(
content::WebContents* web_contents) {
DCHECK(web_contents) << "Need valid WebContents to attach controller to!";
if (vr::VrTabHelper::IsInVr(web_contents)) {
return false; // TODO(crbug.com/865749): Reenable if works for VR keyboard.
}
// Either #passwords-keyboards-accessory or #experimental-ui must be enabled.
return base::FeatureList::IsEnabled(
password_manager::features::kPasswordsKeyboardAccessory) ||
base::FeatureList::IsEnabled(features::kExperimentalUi);
}
// static
void PasswordAccessoryController::CreateForWebContentsForTesting(
content::WebContents* web_contents,
......@@ -153,14 +169,19 @@ void PasswordAccessoryController::OnAutomaticGenerationStatusChanged(
void PasswordAccessoryController::OnFilledIntoFocusedField(
autofill::FillingStatus status) {
// TODO(crbug/853766): Record success rate.
// TODO(fhorschig): Update UI by hiding the sheet or communicating the error.
if (status != autofill::FillingStatus::SUCCESS)
return; // TODO(crbug/853766): Record success rate.
view_->CloseAccessorySheet(); // The sheet's purpose is fulfilled.
view_->OpenKeyboard(); // Bring up the keyboard for the still focused field.
}
void PasswordAccessoryController::RefreshSuggestionsForField(
const url::Origin& origin,
bool is_fillable,
bool is_password_field) {
// When new suggestions are requested, the keyboard will pop open. To avoid
// that any open sheet is pushed up, close it now. Doesn't affect the bar.
view_->CloseAccessorySheet();
// TODO(crbug/853766): Record CTR metric.
if (is_fillable) {
current_origin_ = origin;
......
......@@ -63,6 +63,10 @@ class PasswordAccessoryController
// Methods called by the client:
// -----------------------------
// Returns true, if the accessory controller may exist for |web_contents|.
// Otherwise (e.g. if VR is enabled), it returns false.
static bool AllowedForWebContents(content::WebContents* web_contents);
// Saves credentials for an origin so that they can be used in the sheet.
void SavePasswordsForOrigin(
const std::map<base::string16, const autofill::PasswordForm*>&
......
......@@ -33,11 +33,13 @@
#include "ui/base/l10n/l10n_util.h"
namespace {
using autofill::FillingStatus;
using autofill::PasswordForm;
using autofill::password_generation::PasswordGenerationUIData;
using base::ASCIIToUTF16;
using base::UTF16ToWide;
using testing::_;
using testing::AnyNumber;
using testing::ByMove;
using testing::ElementsAre;
using testing::Mock;
......@@ -62,6 +64,8 @@ class MockPasswordAccessoryView : public PasswordAccessoryViewInterface {
MOCK_METHOD1(OnFillingTriggered, void(const base::string16& textToFill));
MOCK_METHOD0(OnViewDestroyed, void());
MOCK_METHOD1(OnAutomaticGenerationStatusChanged, void(bool));
MOCK_METHOD0(CloseAccessorySheet, void());
MOCK_METHOD0(OpenKeyboard, void());
private:
DISALLOW_COPY_AND_ASSIGN(MockPasswordAccessoryView);
......@@ -254,6 +258,7 @@ class PasswordAccessoryControllerTest : public ChromeRenderViewHostTestHarness {
std::make_unique<StrictMock<MockPasswordAccessoryView>>(),
mock_dialog_factory_.Get(), favicon_service());
NavigateAndCommit(GURL(kExampleSite));
EXPECT_CALL(*view(), CloseAccessorySheet()).Times(AnyNumber());
}
PasswordAccessoryController* controller() {
......@@ -396,6 +401,32 @@ TEST_F(PasswordAccessoryControllerTest, ProvidesEmptySuggestionsMessage) {
/*is_password_field=*/false);
}
// TODO(fhorschig): Check for recorded metrics here or similar to this.
TEST_F(PasswordAccessoryControllerTest, ClosesViewOnSuccessfullFillingOnly) {
// If the filling wasn't successful, no call is expected.
EXPECT_CALL(*view(), CloseAccessorySheet()).Times(0);
controller()->OnFilledIntoFocusedField(FillingStatus::ERROR_NOT_ALLOWED);
controller()->OnFilledIntoFocusedField(FillingStatus::ERROR_NO_VALID_FIELD);
// If the filling completed successfully, let the view know.
EXPECT_CALL(*view(), CloseAccessorySheet());
EXPECT_CALL(*view(), OpenKeyboard());
controller()->OnFilledIntoFocusedField(FillingStatus::SUCCESS);
}
// TODO(fhorschig): Check for recorded metrics here or similar to this.
TEST_F(PasswordAccessoryControllerTest, ClosesViewWhenRefreshingSuggestions) {
// Ignore Items - only the closing calls are interesting here.
EXPECT_CALL(*view(), OnItemsAvailable(_)).Times(AnyNumber());
EXPECT_CALL(*view(), CloseAccessorySheet());
EXPECT_CALL(*view(), OpenKeyboard()).Times(0); // Don't touch the keyboard!
controller()->RefreshSuggestionsForField(
url::Origin::Create(GURL(kExampleSite)),
/*is_fillable=*/false,
/*is_password_field=*/false);
}
TEST_F(PasswordAccessoryControllerTest, RelaysAutomaticGenerationAvailable) {
EXPECT_CALL(*view(), OnAutomaticGenerationStatusChanged(true));
controller()->OnAutomaticGenerationStatusChanged(
......@@ -407,6 +438,7 @@ TEST_F(PasswordAccessoryControllerTest, RelaysAutmaticGenerationUnavailable) {
controller()->OnAutomaticGenerationStatusChanged(false, base::nullopt,
nullptr);
}
// Tests that if AutomaticGenerationStatusChanged(true) is called for different
// password forms, the form and field signatures used for password generation
// are updated.
......
......@@ -71,6 +71,12 @@ class PasswordAccessoryViewInterface {
// in the keyboard accessory.
virtual void OnAutomaticGenerationStatusChanged(bool available) = 0;
// Called to inform the view that the accessory sheet should be closed now.
virtual void CloseAccessorySheet() = 0;
// Called to inform the view that the accessory sheet should be closed now.
virtual void OpenKeyboard() = 0;
private:
friend class PasswordAccessoryController;
// Factory function used to create a concrete instance of this view.
......
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