Skip to content

Commit

Permalink
[af] Reland "Do not clear autofill selection if no suggestion is sele…
Browse files Browse the repository at this point in the history
…cted"

This is a reland of 4a696ac

Patch #1 contains the patch originally landed, please check the diff
against it to review the fix.

The original patch broke interactive UI tests on ChromeOS, because of
a mouse exit event being fired when the dropdown was closed; event
handling could happen after Hide() set the delegate to false.

TBR=groby@chromium.org

Original change's description:
> This fixes crbug.com/835019, in which the current suggestion may not
> be cleared when the mouse exits the popup area, by reusing the
> OnMouseExited handler implemented by AutofillPopupBaseView and
> overriden by AutofillPopupViewNativeViews as empty.
>
> The current implementation posts a task that will be a no-op if the
> mouse leaves the popup and there is no suggestion selected, so this
> is a safe change.
>
> Bug: 835019
> Change-Id: I99339d377090721719ed6c7d026f7832a0503e4d
> Reviewed-on: https://chromium-review.googlesource.com/1070788
> Commit-Queue: Fabio Tirelo <ftirelo@chromium.org>
> Reviewed-by: Rachel Blum <groby@chromium.org>
> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
> Reviewed-by: Evan Stade <estade@chromium.org>
> Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#564070}

Bug: 835019
Change-Id: I99339d377090721719ed6c7d026f7832a0503e4d
Reviewed-on: https://chromium-review.googlesource.com/1087008
Commit-Queue: Fabio Tirelo <ftirelo@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564549}
  • Loading branch information
Fabio Tirelo authored and Commit Bot committed Jun 5, 2018
1 parent 774e03c commit 360d661
Show file tree
Hide file tree
Showing 13 changed files with 44 additions and 1 deletion.
4 changes: 4 additions & 0 deletions chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,10 @@ void AutofillPopupControllerImpl::SelectionCleared() {
SetSelectedLine(base::nullopt);
}

bool AutofillPopupControllerImpl::HasSelection() const {
return selected_line_.has_value();
}

void AutofillPopupControllerImpl::AcceptSuggestion(int index) {
const autofill::Suggestion& suggestion = suggestions_[index];
delegate_->DidAcceptSuggestion(suggestion.value, suggestion.frontend_id,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class AutofillPopupControllerImpl : public AutofillPopupController {
void SetSelectionAtPoint(const gfx::Point& point) override;
bool AcceptSelectedLine() override;
void SelectionCleared() override;
bool HasSelection() const override;
gfx::Rect popup_bounds() const override;
gfx::NativeView container_view() override;
const gfx::RectF& element_bounds() const override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class TestAutofillPopupViewDelegate : public AutofillPopupViewDelegate {
void SetSelectionAtPoint(const gfx::Point& point) override {}
bool AcceptSelectedLine() override { return true; }
void SelectionCleared() override {}
bool HasSelection() const override { return false; }
gfx::Rect popup_bounds() const override { return gfx::Rect(0, 0, 100, 100); }
gfx::NativeView container_view() override { return container_view_; }
const gfx::RectF& element_bounds() const override { return element_bounds_; }
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/ui/autofill/autofill_popup_view_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ class AutofillPopupViewDelegate {
// out of the popup bounds.
virtual void SelectionCleared() = 0;

// Returns true if any of the suggestions is selected.
virtual bool HasSelection() const = 0;

// The actual bounds of the popup.
virtual gfx::Rect popup_bounds() const = 0;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
MOCK_METHOD1(SetSelectionAtPoint, void(const gfx::Point& point));
MOCK_METHOD0(AcceptSelectedLine, bool());
MOCK_METHOD0(SelectionCleared, void());
MOCK_CONST_METHOD0(HasSelection, bool());
MOCK_CONST_METHOD0(popup_bounds, gfx::Rect());
MOCK_METHOD0(container_view, gfx::NativeView());
MOCK_CONST_METHOD0(element_bounds, const gfx::RectF&());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ void Hide() override {}
void SetSelectionAtPoint(const gfx::Point&) override {}
bool AcceptSelectedLine() override { return true; }
void SelectionCleared() override {}
bool HasSelection() const override { return password_selected(); }
gfx::Rect popup_bounds() const override { return popup_bounds_; }
MOCK_METHOD0(container_view, gfx::NativeView());
MOCK_CONST_METHOD0(element_bounds, gfx::RectF&());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,10 @@ void PasswordGenerationPopupControllerImpl::SelectionCleared() {
PasswordSelected(false);
}

bool PasswordGenerationPopupControllerImpl::HasSelection() const {
return password_selected();
}

gfx::NativeView PasswordGenerationPopupControllerImpl::container_view() {
return controller_common_.container_view;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ class PasswordGenerationPopupControllerImpl
void SetSelectionAtPoint(const gfx::Point& point) override;
bool AcceptSelectedLine() override;
void SelectionCleared() override;
bool HasSelection() const override;
void PasswordAccepted() override;
void OnSavedPasswordsLinkClicked() override;
int GetMinimumWidth() override;
Expand Down
4 changes: 4 additions & 0 deletions chrome/browser/ui/views/autofill/autofill_popup_base_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,10 @@ bool AutofillPopupBaseView::OnMouseDragged(const ui::MouseEvent& event) {
}

void AutofillPopupBaseView::OnMouseExited(const ui::MouseEvent& event) {
// There is no need to post a ClearSelection task if no row is selected.
if (!delegate_ || !delegate_->HasSelection())
return;

// Pressing return causes the cursor to hide, which will generate an
// OnMouseExited event. Pressing return should activate the current selection
// via AcceleratorPressed, so we need to let that run first.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include "chrome/browser/ui/views/autofill/autofill_popup_base_view.h"

#include "base/macros.h"
#include "base/run_loop.h"
#include "build/build_config.h"
#include "chrome/browser/ui/autofill/autofill_popup_view_delegate.h"
#include "chrome/browser/ui/browser.h"
Expand Down Expand Up @@ -32,6 +34,8 @@ class MockAutofillPopupViewDelegate : public AutofillPopupViewDelegate {
MOCK_METHOD1(SetSelectionAtPoint, void(const gfx::Point&));
MOCK_METHOD0(AcceptSelectedLine, bool());
MOCK_METHOD0(SelectionCleared, void());
MOCK_CONST_METHOD0(HasSelection, bool());

// TODO(jdduke): Mock this method upon resolution of crbug.com/352463.
MOCK_CONST_METHOD0(popup_bounds, gfx::Rect());
MOCK_METHOD0(container_view, gfx::NativeView());
Expand Down Expand Up @@ -84,6 +88,8 @@ class AutofillPopupBaseViewTest : public InProcessBrowserTest {
test::ScopedMacViewsBrowserMode views_mode_{true};
testing::NiceMock<MockAutofillPopupViewDelegate> mock_delegate_;
AutofillPopupBaseView* view_;

DISALLOW_COPY_AND_ASSIGN(AutofillPopupBaseViewTest);
};

// Flaky on Win and Linux. http://crbug.com/376299
Expand Down Expand Up @@ -154,4 +160,20 @@ IN_PROC_BROWSER_TEST_F(AutofillPopupBaseViewTest, CorrectBoundsTest) {
EXPECT_EQ(expected_point, display_point);
}

IN_PROC_BROWSER_TEST_F(AutofillPopupBaseViewTest, MouseExitedTest) {
for (bool has_selection : {true, false}) {
EXPECT_CALL(mock_delegate_, HasSelection()).WillOnce(Return(has_selection));
EXPECT_CALL(mock_delegate_, SelectionCleared())
.Times(has_selection ? 1 : 0);

ShowView();

ui::MouseEvent exit_event(ui::ET_MOUSE_EXITED, gfx::Point(), gfx::Point(),
ui::EventTimeForNow(), 0, 0);
static_cast<views::View*>(view_)->OnMouseExited(exit_event);

base::RunLoop().RunUntilIdle();
}
}

} // namespace autofill
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ class AutofillPopupViewNativeViews : public AutofillPopupBaseView,
// TODO(crbug.com/831603): Remove these overrides and the corresponding
// methods in AutofillPopupBaseView once deprecation of
// AutofillPopupViewViews is complete.
void OnMouseExited(const ui::MouseEvent& event) override {}
void OnMouseMoved(const ui::MouseEvent& event) override {}

private:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class MockAutofillPopupController : public autofill::AutofillPopupController {
MOCK_METHOD1(SetSelectionAtPoint, void(const gfx::Point& point));
MOCK_METHOD0(AcceptSelectedLine, bool());
MOCK_METHOD0(SelectionCleared, void());
MOCK_CONST_METHOD0(HasSelection, bool());
MOCK_CONST_METHOD0(popup_bounds, gfx::Rect());
MOCK_METHOD0(container_view, gfx::NativeView());
MOCK_CONST_METHOD0(element_bounds, const gfx::RectF&());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class MockAutofillPopupController : public AutofillPopupController {
MOCK_METHOD1(SetSelectionAtPoint, void(const gfx::Point& point));
MOCK_METHOD0(AcceptSelectedLine, bool());
MOCK_METHOD0(SelectionCleared, void());
MOCK_CONST_METHOD0(HasSelection, bool());
MOCK_CONST_METHOD0(popup_bounds, gfx::Rect());
MOCK_METHOD0(container_view, gfx::NativeView());
MOCK_CONST_METHOD0(element_bounds, const gfx::RectF&());
Expand Down

0 comments on commit 360d661

Please sign in to comment.