Skip to content

Commit

Permalink
suggest: fix shifting suggestion window
Browse files Browse the repository at this point in the history 
Currently the multi word suggestion window aligns itself underneath the
relevant text in the currently focused input when a suggestion candidate
is triggered and shown to the user. The alignment works very well,
however there is a noticeable "shifting" of the window as the
AssistiveWindowController class attempts to align the suggestion window
based on events received.

This is due to an unexpected ordering of events received to the
AssistiveWindowController. A ShowSuggestion event may be received prior
to SetBounds event (this is because surrounding text and cursor bounds
updates are sent as two distinct events).

To accomodate this, the CL adds a delay before showing a multi word
suggestion to the user. The delay allows the two related events
(ShowSuggestion and SetBounds) to be received before making the
suggestion window visible to the user.

BUG=b:241321719

Change-Id: If14fe976b244d29deea0cd08925808e7a2221e03
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3811193
Commit-Queue: Curtis McMullan <curtismcmullan@chromium.org>
Reviewed-by: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1032789}
  • Loading branch information
@curtismcmullan
curtismcmullan authored and Chromium LUCI CQ committed Aug 8, 2022
1 parent ce6c106 commit 7dc39e7
Show file tree
Hide file tree
Showing 4 changed files with 112 additions and 17 deletions.
59 changes: 45 additions & 14 deletions chrome/browser/ash/input_method/assistive_window_controller.cc
View file
Expand Up @@ -23,6 +23,8 @@
namespace ash {
namespace input_method {

constexpr base::TimeDelta kShowSuggestionDelayMs = base::Milliseconds(5);

namespace {
gfx::NativeView GetParentView() {
gfx::NativeView parent = nullptr;
Expand All @@ -45,6 +47,7 @@ AssistiveWindowController::AssistiveWindowController(
: delegate_(delegate), accessibility_view_(accessibility_view) {}

AssistiveWindowController::~AssistiveWindowController() {
ClearPendingSuggestionTimer();
if (suggestion_window_view_ && suggestion_window_view_->GetWidget())
suggestion_window_view_->GetWidget()->RemoveObserver(this);
if (undo_window_ && undo_window_->GetWidget())
Expand Down Expand Up @@ -101,6 +104,7 @@ void AssistiveWindowController::InitAccessibilityView() {
}

void AssistiveWindowController::OnWidgetDestroying(views::Widget* widget) {
ClearPendingSuggestionTimer();
if (suggestion_window_view_ &&
widget == suggestion_window_view_->GetWidget()) {
widget->RemoveObserver(this);
Expand Down Expand Up @@ -143,24 +147,25 @@ void AssistiveWindowController::AcceptSuggestion(
void AssistiveWindowController::HideSuggestion() {
suggestion_text_ = base::EmptyString16();
confirmed_length_ = 0;
ClearPendingSuggestionTimer();
if (suggestion_window_view_)
suggestion_window_view_->GetWidget()->Close();
if (grammar_suggestion_window_)
grammar_suggestion_window_->GetWidget()->Close();
}

void AssistiveWindowController::SetBounds(const Bounds& bounds) {
if (bounds == bounds_)
return;

bounds_ = bounds;
// Sets suggestion_window_view_'s bounds here for most up-to-date cursor
// position. This is different from UndoWindow because UndoWindow gets cursors
// position before showing.
// TODO(crbug/1112982): Investigate getting bounds to suggester before sending
// show suggestion request.
if (suggestion_window_view_) {
suggestion_window_view_->SetAnchorRect(bounds.caret);
}
if (grammar_suggestion_window_) {
if (suggestion_window_view_)
suggestion_window_view_->SetAnchorRect(bounds_.caret);
if (grammar_suggestion_window_)
grammar_suggestion_window_->SetBounds(bounds_.caret);
if (pending_suggestion_timer_ && pending_suggestion_timer_->IsRunning()) {
pending_suggestion_timer_->FireNow();
pending_suggestion_timer_ = nullptr;
}
}

Expand All @@ -172,13 +177,18 @@ void AssistiveWindowController::FocusStateChanged() {

void AssistiveWindowController::ShowSuggestion(
const ui::ime::SuggestionDetails& details) {
if (!suggestion_window_view_)
// Since there is only one suggestion text in ShowSuggestion, we default to
// vertical layout.
InitSuggestionWindow(ui::ime::SuggestionWindowView::Orientation::kVertical);
suggestion_text_ = details.text;
confirmed_length_ = details.confirmed_length;
suggestion_window_view_->Show(details);
// Delay the showing of a completion suggestion. This is required to solve
// b/241321719, where we receive a ShowSuggestion call prior to a
// corresponding SetBounds call. Delaying allows any relevant SetBounds calls
// to be received before we show the suggestion to the user.
ClearPendingSuggestionTimer();
pending_suggestion_timer_ = std::make_unique<base::OneShotTimer>();
pending_suggestion_timer_->Start(
FROM_HERE, kShowSuggestionDelayMs,
base::BindOnce(&AssistiveWindowController::DisplayCompletionSuggestion,
weak_ptr_factory_.GetWeakPtr(), details));
}

void AssistiveWindowController::SetButtonHighlighted(
Expand Down Expand Up @@ -246,6 +256,11 @@ AssistiveWindowController::WindowOrientationFor(
void AssistiveWindowController::SetAssistiveWindowProperties(
const AssistiveWindowProperties& window) {
window_ = window;

// Make sure any pending timers are cleared before we attempt to show, or
// update, another assistive window.
ClearPendingSuggestionTimer();

switch (window.type) {
case ui::ime::AssistiveWindowType::kUndoWindow:
if (!undo_window_)
Expand Down Expand Up @@ -292,6 +307,22 @@ void AssistiveWindowController::SetAssistiveWindowProperties(
Announce(window.announce_string);
}

void AssistiveWindowController::DisplayCompletionSuggestion(
const ui::ime::SuggestionDetails& details) {
if (!suggestion_window_view_)
InitSuggestionWindow(ui::ime::SuggestionWindowView::Orientation::kVertical);
suggestion_window_view_->SetAnchorRect(bounds_.caret);
suggestion_window_view_->Show(details);
}

void AssistiveWindowController::ClearPendingSuggestionTimer() {
if (pending_suggestion_timer_) {
if (pending_suggestion_timer_->IsRunning())
pending_suggestion_timer_->Stop();
pending_suggestion_timer_ = nullptr;
}
}

void AssistiveWindowController::AssistiveWindowButtonClicked(
const ui::ime::AssistiveWindowButton& button) const {
delegate_->AssistiveWindowButtonClicked(button);
Expand Down
8 changes: 8 additions & 0 deletions chrome/browser/ash/input_method/assistive_window_controller.h
View file
Expand Up @@ -6,7 +6,10 @@
#define CHROME_BROWSER_ASH_INPUT_METHOD_ASSISTIVE_WINDOW_CONTROLLER_H_

#include <memory>
#include <optional>

#include "base/memory/weak_ptr.h"
#include "base/timer/timer.h"
#include "chrome/browser/ash/input_method/assistive_window_properties.h"
#include "chrome/browser/ash/input_method/ui/assistive_accessibility_view.h"
#include "chrome/browser/ash/input_method/ui/assistive_delegate.h"
Expand Down Expand Up @@ -73,6 +76,8 @@ class AssistiveWindowController : public views::WidgetObserver,
void InitUndoWindow();
void InitGrammarSuggestionWindow();
void InitAccessibilityView();
void DisplayCompletionSuggestion(const ui::ime::SuggestionDetails& details);
void ClearPendingSuggestionTimer();

const AssistiveWindowControllerDelegate* delegate_;
AssistiveWindowProperties window_;
Expand All @@ -83,6 +88,9 @@ class AssistiveWindowController : public views::WidgetObserver,
std::u16string suggestion_text_;
size_t confirmed_length_ = 0;
Bounds bounds_;
std::unique_ptr<base::OneShotTimer> pending_suggestion_timer_;

base::WeakPtrFactory<AssistiveWindowController> weak_ptr_factory_{this};
};

} // namespace input_method
Expand Down
58 changes: 55 additions & 3 deletions chrome/browser/ash/input_method/assistive_window_controller_unittest.cc
View file
Expand Up @@ -16,6 +16,7 @@
#include "chrome/browser/ui/views/chrome_layout_provider.h"
#include "chrome/test/base/chrome_ash_test_base.h"
#include "chrome/test/base/testing_profile.h"
#include "content/public/test/browser_task_environment.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/aura/window.h"
#include "ui/base/ime/ash/ime_assistive_window_handler_interface.h"
Expand All @@ -31,6 +32,8 @@ const char16_t kAnnounceString[] = u"announce string";
namespace ash {
namespace input_method {

constexpr size_t kShowSuggestionDelayMs = 5;

class MockDelegate : public AssistiveWindowControllerDelegate {
public:
~MockDelegate() override = default;
Expand All @@ -53,7 +56,10 @@ class TestAccessibilityView : public ui::ime::AssistiveAccessibilityView {

class AssistiveWindowControllerTest : public ChromeAshTestBase {
protected:
AssistiveWindowControllerTest() = default;
AssistiveWindowControllerTest()
: ChromeAshTestBase(std::make_unique<content::BrowserTaskEnvironment>(
base::test::TaskEnvironment::TimeSource::MOCK_TIME)) {}

~AssistiveWindowControllerTest() override = default;

void SetUp() override {
Expand Down Expand Up @@ -105,6 +111,11 @@ class AssistiveWindowControllerTest : public ChromeAshTestBase {
/*disabled_features=*/{});
}

void WaitForSuggestionWindowDelay() {
task_environment()->FastForwardBy(
base::Milliseconds(kShowSuggestionDelayMs + 1));
}

base::test::ScopedFeatureList feature_list_;
std::unique_ptr<AssistiveWindowController> controller_;
std::unique_ptr<MockDelegate> delegate_ = std::make_unique<MockDelegate>();
Expand All @@ -115,10 +126,42 @@ class AssistiveWindowControllerTest : public ChromeAshTestBase {
std::unique_ptr<TestAccessibilityView> accessibility_view_;
};

TEST_F(AssistiveWindowControllerTest, ShowSuggestionDelaysWindowDisplay) {
ui::ime::SuggestionDetails details;
details.text = u"asdf";
details.confirmed_length = 3;

controller_->ShowSuggestion(details);
ui::ime::SuggestionWindowView* window_before_delay =
controller_->GetSuggestionWindowViewForTesting();
WaitForSuggestionWindowDelay();
ui::ime::SuggestionWindowView* window_after_delay =
controller_->GetSuggestionWindowViewForTesting();

EXPECT_EQ(window_before_delay, nullptr);
EXPECT_NE(window_after_delay, nullptr);
}

TEST_F(AssistiveWindowControllerTest,
SetBoundsAfterShowSuggestionCancelsDelay) {
ui::ime::SuggestionDetails details;
details.text = u"asdf";
details.confirmed_length = 3;
gfx::Rect caret_bounds(0, 0, 100, 100);

controller_->ShowSuggestion(details);
controller_->SetBounds(Bounds{.caret = caret_bounds});
ui::ime::SuggestionWindowView* suggestion_window_view =
controller_->GetSuggestionWindowViewForTesting();

EXPECT_NE(suggestion_window_view, nullptr);
}

TEST_F(AssistiveWindowControllerTest, ShowSuggestionSetsConfirmedLength) {
ui::ime::SuggestionDetails details;
details.text = u"asdf";
details.confirmed_length = 3;

controller_->ShowSuggestion(details);

EXPECT_EQ(controller_->GetConfirmedLength(), 3u);
Expand All @@ -128,16 +171,18 @@ TEST_F(AssistiveWindowControllerTest, ConfirmedLength0SetsBoundsToCaretBounds) {
ui::ime::SuggestionDetails details;
details.text = suggestion_;
details.confirmed_length = 0;

controller_->ShowSuggestion(details);
WaitForSuggestionWindowDelay();
ui::ime::SuggestionWindowView* suggestion_view =
controller_->GetSuggestionWindowViewForTesting();

ASSERT_NE(suggestion_view, nullptr);
gfx::Rect current_bounds = suggestion_view->GetAnchorRect();
gfx::Rect caret_bounds(0, 0, 100, 100);
Bounds bounds;
bounds.caret = caret_bounds;
controller_->SetBounds(bounds);

EXPECT_NE(suggestion_view->GetAnchorRect(), current_bounds);
EXPECT_EQ(suggestion_view->GetAnchorRect(), caret_bounds);
}
Expand All @@ -146,16 +191,18 @@ TEST_F(AssistiveWindowControllerTest, ConfirmedLengthNSetsBoundsToCaretBounds) {
ui::ime::SuggestionDetails details;
details.text = suggestion_;
details.confirmed_length = 1;

controller_->ShowSuggestion(details);
WaitForSuggestionWindowDelay();
ui::ime::SuggestionWindowView* suggestion_view =
controller_->GetSuggestionWindowViewForTesting();

ASSERT_NE(suggestion_view, nullptr);
gfx::Rect current_bounds = suggestion_view->GetAnchorRect();
gfx::Rect caret_bounds(0, 0, 100, 100);
Bounds bounds;
bounds.caret = caret_bounds;
controller_->SetBounds(bounds);

EXPECT_NE(suggestion_view->GetAnchorRect(), current_bounds);
EXPECT_EQ(suggestion_view->GetAnchorRect(), caret_bounds);
}
Expand All @@ -164,10 +211,13 @@ TEST_F(AssistiveWindowControllerTest, WindowTracksCaretBounds) {
ui::ime::SuggestionDetails details;
details.text = suggestion_;
details.confirmed_length = 0;

controller_->ShowSuggestion(details);
WaitForSuggestionWindowDelay();
ui::ime::SuggestionWindowView* suggestion_view =
controller_->GetSuggestionWindowViewForTesting();

ASSERT_NE(suggestion_view, nullptr);
gfx::Rect current_bounds = suggestion_view->GetAnchorRect();
gfx::Rect caret_bounds_after_one_key(current_bounds.width() + 1,
current_bounds.height());
Expand All @@ -194,7 +244,9 @@ TEST_F(AssistiveWindowControllerTest,
ui::ime::SuggestionDetails details;
details.text = suggestion_;
details.confirmed_length = 1;

controller_->ShowSuggestion(details);
WaitForSuggestionWindowDelay();

gfx::Rect current_bounds =
controller_->GetSuggestionWindowViewForTesting()->GetAnchorRect();
Expand Down
4 changes: 4 additions & 0 deletions ui/base/ime/ash/ime_assistive_window_handler_interface.h
View file
Expand Up @@ -30,6 +30,10 @@ struct Bounds {
gfx::Rect caret;
// Position of the autocorrect span, empty if not present.
gfx::Rect autocorrect;

bool operator==(const Bounds& rhs) const {
return caret == rhs.caret && autocorrect == rhs.autocorrect;
}
};

// A interface to handle the assistive windows related method call.
Expand Down

0 comments on commit 7dc39e7

Please sign in to comment.