Commit 8b10e3c6 authored by Friedrich Horschig's avatar Friedrich Horschig Committed by Commit Bot

Revert "Remove redundant OnEndEditingTextfield event"

This reverts commit 76648dac.

Reason for revert: As described in crbug.com/1056527, Android WebView still relies on this. An upstream fix is too risky so I'll revert this CL for now.

Bug: 1056527

Original change's description:
> Remove redundant OnEndEditingTextfield event
>
> The event OnEndEditingTextField seems to be redundant (esp. with focus
> changes) and isn't correct in all cases: If users interact with native
> UI like an autofill dropdown, the event will try to close this piece of
> UI although this isn't always necessary.
>
> This CL therefore removes the event which allows dropdowns to remain
> open after interaction if they require it (e.g. see use case in linked
> bug).
>
> Bug: 1043963
> Change-Id: I5c7a6ede34fb574bd3f91d4b16fbb4e2a78a8d24
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2034527
> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
> Reviewed-by: Alex Gough <ajgo@chromium.org>
> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
> Commit-Queue: Friedrich [CET] <fhorschig@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#738105}

TBR=vasilii@chromium.org,dvadym@chromium.org,fhorschig@chromium.org,ajgo@chromium.org

Bug: 1043963
Change-Id: I3bf88ec43fa6ae9bf0d11f295bc1b0debbba71ce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2087331Reviewed-by: default avatarFriedrich [CET] <fhorschig@chromium.org>
Commit-Queue: Friedrich [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746712}
parent 15e85bb0
...@@ -149,6 +149,7 @@ void AutofillPopupControllerImpl::Show( ...@@ -149,6 +149,7 @@ void AutofillPopupControllerImpl::Show(
->RegisterKeyPressHandler( ->RegisterKeyPressHandler(
base::Bind(&AutofillPopupControllerImpl::HandleKeyPressEvent, base::Bind(&AutofillPopupControllerImpl::HandleKeyPressEvent,
base::Unretained(this))); base::Unretained(this)));
pinned_until_update_ = false;
delegate_->OnPopupShown(); delegate_->OnPopupShown();
DCHECK_EQ(suggestions_.size(), elided_values_.size()); DCHECK_EQ(suggestions_.size(), elided_values_.size());
...@@ -231,9 +232,10 @@ AutofillPopupControllerImpl::GetUnelidedSuggestions() const { ...@@ -231,9 +232,10 @@ AutofillPopupControllerImpl::GetUnelidedSuggestions() const {
void AutofillPopupControllerImpl::Hide(PopupHidingReason reason) { void AutofillPopupControllerImpl::Hide(PopupHidingReason reason) {
// If the reason for hiding is only stale data or a user interacting with // If the reason for hiding is only stale data or a user interacting with
// native Chrome UI (kFocusChanged), the popup might be kept open. // native Chrome UI (kFocusChanged/kEndEditing), the popup might be kept open.
if (pinned_until_update_ && (reason == PopupHidingReason::kStaleData || if (pinned_until_update_ && (reason == PopupHidingReason::kStaleData ||
reason == PopupHidingReason::kFocusChanged)) { reason == PopupHidingReason::kFocusChanged ||
reason == PopupHidingReason::kEndEditing)) {
return; // Don't close the popup while waiting for an update. return; // Don't close the popup while waiting for an update.
} }
if (delegate_) { if (delegate_) {
......
...@@ -701,8 +701,9 @@ TEST_F(AutofillPopupControllerUnitTest, DontHideWhenWaitingForData) { ...@@ -701,8 +701,9 @@ TEST_F(AutofillPopupControllerUnitTest, DontHideWhenWaitingForData) {
EXPECT_CALL(*autofill_popup_view(), Hide).Times(0); EXPECT_CALL(*autofill_popup_view(), Hide).Times(0);
autofill_popup_controller_->PinViewUntilUpdate(); autofill_popup_controller_->PinViewUntilUpdate();
// Hide() will not work for stale data. // Hide() will not work for stale data or when focusing native UI.
autofill_popup_controller_->DoHide(PopupHidingReason::kStaleData); autofill_popup_controller_->DoHide(PopupHidingReason::kStaleData);
autofill_popup_controller_->DoHide(PopupHidingReason::kEndEditing);
// Check the expectations now since TearDown will perform a successful hide. // Check the expectations now since TearDown will perform a successful hide.
Mock::VerifyAndClearExpectations(delegate()); Mock::VerifyAndClearExpectations(delegate());
......
...@@ -117,6 +117,8 @@ class FakeContentAutofillDriver : public mojom::AutofillDriver { ...@@ -117,6 +117,8 @@ class FakeContentAutofillDriver : public mojom::AutofillDriver {
void DidPreviewAutofillFormData() override {} void DidPreviewAutofillFormData() override {}
void DidEndTextFieldEditing() override {}
void SetDataList(const std::vector<base::string16>& values, void SetDataList(const std::vector<base::string16>& values,
const std::vector<base::string16>& labels) override {} const std::vector<base::string16>& labels) override {}
......
...@@ -105,6 +105,8 @@ class FakeContentAutofillDriver : public mojom::AutofillDriver { ...@@ -105,6 +105,8 @@ class FakeContentAutofillDriver : public mojom::AutofillDriver {
void DidPreviewAutofillFormData() override {} void DidPreviewAutofillFormData() override {}
void DidEndTextFieldEditing() override {}
void SetDataList(const std::vector<base::string16>& values, void SetDataList(const std::vector<base::string16>& values,
const std::vector<base::string16>& labels) override {} const std::vector<base::string16>& labels) override {}
......
...@@ -285,6 +285,10 @@ void ContentAutofillDriver::DidPreviewAutofillFormData() { ...@@ -285,6 +285,10 @@ void ContentAutofillDriver::DidPreviewAutofillFormData() {
autofill_handler_->OnDidPreviewAutofillFormData(); autofill_handler_->OnDidPreviewAutofillFormData();
} }
void ContentAutofillDriver::DidEndTextFieldEditing() {
autofill_handler_->OnDidEndTextFieldEditing();
}
void ContentAutofillDriver::SetDataList( void ContentAutofillDriver::SetDataList(
const std::vector<base::string16>& values, const std::vector<base::string16>& values,
const std::vector<base::string16>& labels) { const std::vector<base::string16>& labels) {
......
...@@ -116,6 +116,7 @@ class ContentAutofillDriver : public AutofillDriver, ...@@ -116,6 +116,7 @@ class ContentAutofillDriver : public AutofillDriver,
void DidFillAutofillFormData(const FormData& form, void DidFillAutofillFormData(const FormData& form,
base::TimeTicks timestamp) override; base::TimeTicks timestamp) override;
void DidPreviewAutofillFormData() override; void DidPreviewAutofillFormData() override;
void DidEndTextFieldEditing() override;
void SetDataList(const std::vector<base::string16>& values, void SetDataList(const std::vector<base::string16>& values,
const std::vector<base::string16>& labels) override; const std::vector<base::string16>& labels) override;
void SelectFieldOptionsDidChange(const FormData& form) override; void SelectFieldOptionsDidChange(const FormData& form) override;
......
...@@ -69,6 +69,9 @@ interface AutofillDriver { ...@@ -69,6 +69,9 @@ interface AutofillDriver {
// Sent when a form is previewed with Autofill suggestions. // Sent when a form is previewed with Autofill suggestions.
DidPreviewAutofillFormData(); DidPreviewAutofillFormData();
// Sent when a text field is done editing.
DidEndTextFieldEditing();
// Informs browser of data list values for the current field. // Informs browser of data list values for the current field.
SetDataList(array<mojo_base.mojom.String16> values, SetDataList(array<mojo_base.mojom.String16> values,
array<mojo_base.mojom.String16> labels); array<mojo_base.mojom.String16> labels);
......
...@@ -319,6 +319,7 @@ void AutofillAgent::TextFieldDidEndEditing(const WebInputElement& element) { ...@@ -319,6 +319,7 @@ void AutofillAgent::TextFieldDidEndEditing(const WebInputElement& element) {
password_generation_agent_->ShouldIgnoreBlur()) { password_generation_agent_->ShouldIgnoreBlur()) {
return; return;
} }
GetAutofillDriver()->DidEndTextFieldEditing();
password_autofill_agent_->DidEndTextFieldEditing(); password_autofill_agent_->DidEndTextFieldEditing();
if (password_generation_agent_) if (password_generation_agent_)
password_generation_agent_->DidEndTextFieldEditing(element); password_generation_agent_->DidEndTextFieldEditing(element);
......
...@@ -311,6 +311,10 @@ bool AutofillExternalDelegate::RemoveSuggestion(const base::string16& value, ...@@ -311,6 +311,10 @@ bool AutofillExternalDelegate::RemoveSuggestion(const base::string16& value,
return false; return false;
} }
void AutofillExternalDelegate::DidEndTextFieldEditing() {
manager_->client()->HideAutofillPopup(PopupHidingReason::kEndEditing);
}
void AutofillExternalDelegate::ClearPreviewedForm() { void AutofillExternalDelegate::ClearPreviewedForm() {
driver_->RendererShouldClearPreviewedForm(); driver_->RendererShouldClearPreviewedForm();
} }
......
...@@ -94,6 +94,10 @@ class AutofillExternalDelegate : public AutofillPopupDelegate { ...@@ -94,6 +94,10 @@ class AutofillExternalDelegate : public AutofillPopupDelegate {
const std::vector<base::string16>& data_list_values, const std::vector<base::string16>& data_list_values,
const std::vector<base::string16>& data_list_labels); const std::vector<base::string16>& data_list_labels);
// Inform the delegate that the text field editing has ended. This is
// used to help record the metrics of when a new popup is shown.
void DidEndTextFieldEditing();
// Returns the delegate to its starting state by removing any page specific // Returns the delegate to its starting state by removing any page specific
// values or settings. // values or settings.
void Reset(); void Reset();
......
...@@ -572,6 +572,17 @@ TEST_F(AutofillExternalDelegateUnitTest, ExternalDelegateClearPreviewedForm) { ...@@ -572,6 +572,17 @@ TEST_F(AutofillExternalDelegateUnitTest, ExternalDelegateClearPreviewedForm) {
POPUP_ITEM_ID_AUTOCOMPLETE_ENTRY); POPUP_ITEM_ID_AUTOCOMPLETE_ENTRY);
} }
// Test that the popup is hidden once we are done editing the autofill field.
TEST_F(AutofillExternalDelegateUnitTest,
ExternalDelegateHidePopupAfterEditing) {
EXPECT_CALL(autofill_client_, ShowAutofillPopup);
test::GenerateTestAutofillPopup(external_delegate_.get());
EXPECT_CALL(autofill_client_,
HideAutofillPopup(autofill::PopupHidingReason::kEndEditing));
external_delegate_->DidEndTextFieldEditing();
}
// Test that the driver is directed to accept the data list after being notified // Test that the driver is directed to accept the data list after being notified
// that the user accepted the data list suggestion. // that the user accepted the data list suggestion.
TEST_F(AutofillExternalDelegateUnitTest, TEST_F(AutofillExternalDelegateUnitTest,
......
...@@ -102,6 +102,9 @@ class AutofillHandler { ...@@ -102,6 +102,9 @@ class AutofillHandler {
// Invoked when preview autofill value has been shown. // Invoked when preview autofill value has been shown.
virtual void OnDidPreviewAutofillFormData() = 0; virtual void OnDidPreviewAutofillFormData() = 0;
// Invoked when textfeild editing ended
virtual void OnDidEndTextFieldEditing() = 0;
// Invoked when popup window should be hidden. // Invoked when popup window should be hidden.
virtual void OnHidePopup() = 0; virtual void OnHidePopup() = 0;
......
...@@ -86,6 +86,8 @@ void AutofillHandlerProxy::OnDidFillAutofillFormData( ...@@ -86,6 +86,8 @@ void AutofillHandlerProxy::OnDidFillAutofillFormData(
void AutofillHandlerProxy::OnDidPreviewAutofillFormData() {} void AutofillHandlerProxy::OnDidPreviewAutofillFormData() {}
void AutofillHandlerProxy::OnDidEndTextFieldEditing() {}
void AutofillHandlerProxy::OnHidePopup() {} void AutofillHandlerProxy::OnHidePopup() {}
void AutofillHandlerProxy::OnSetDataList( void AutofillHandlerProxy::OnSetDataList(
......
...@@ -26,6 +26,7 @@ class AutofillHandlerProxy : public AutofillHandler { ...@@ -26,6 +26,7 @@ class AutofillHandlerProxy : public AutofillHandler {
const base::TimeTicks timestamp) override; const base::TimeTicks timestamp) override;
void OnDidPreviewAutofillFormData() override; void OnDidPreviewAutofillFormData() override;
void OnDidEndTextFieldEditing() override;
void OnHidePopup() override; void OnHidePopup() override;
void OnSetDataList(const std::vector<base::string16>& values, void OnSetDataList(const std::vector<base::string16>& values,
const std::vector<base::string16>& labels) override; const std::vector<base::string16>& labels) override;
......
...@@ -1416,6 +1416,10 @@ void AutofillManager::OnCreditCardFetched(bool did_succeed, ...@@ -1416,6 +1416,10 @@ void AutofillManager::OnCreditCardFetched(bool did_succeed,
credit_card_field_, *credit_card, cvc); credit_card_field_, *credit_card, cvc);
} }
void AutofillManager::OnDidEndTextFieldEditing() {
external_delegate_->DidEndTextFieldEditing();
}
bool AutofillManager::IsAutofillEnabled() const { bool AutofillManager::IsAutofillEnabled() const {
return IsAutofillProfileEnabled() || IsAutofillCreditCardEnabled(); return IsAutofillProfileEnabled() || IsAutofillCreditCardEnabled();
} }
......
...@@ -226,6 +226,7 @@ class AutofillManager : public AutofillHandler, ...@@ -226,6 +226,7 @@ class AutofillManager : public AutofillHandler,
void OnDidFillAutofillFormData(const FormData& form, void OnDidFillAutofillFormData(const FormData& form,
const base::TimeTicks timestamp) override; const base::TimeTicks timestamp) override;
void OnDidPreviewAutofillFormData() override; void OnDidPreviewAutofillFormData() override;
void OnDidEndTextFieldEditing() override;
void OnHidePopup() override; void OnHidePopup() override;
void OnSetDataList(const std::vector<base::string16>& values, void OnSetDataList(const std::vector<base::string16>& values,
const std::vector<base::string16>& labels) override; const std::vector<base::string16>& labels) override;
......
...@@ -23,7 +23,8 @@ enum class PopupType { ...@@ -23,7 +23,8 @@ enum class PopupType {
enum class PopupHidingReason { enum class PopupHidingReason {
kAcceptSuggestion, // A suggestion was accepted. kAcceptSuggestion, // A suggestion was accepted.
kAttachInterstitialPage, // An interstitial page displaces the popup. kAttachInterstitialPage, // An interstitial page displaces the popup.
kFocusChanged, // Focus removed from field. kEndEditing, // A field isn't edited anymore but remains focused for now.
kFocusChanged, // Focus removed from field. Follows kEndEditing.
kContentAreaMoved, // Scrolling or zooming into the page displaces popup. kContentAreaMoved, // Scrolling or zooming into the page displaces popup.
kNavigation, // A navigation on page or frame level. kNavigation, // A navigation on page or frame level.
kNoSuggestions, // The popup is or would become empty. kNoSuggestions, // The popup is or would become empty.
......
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