Skip to content

Commit

Permalink
Revert "suggest: fix shifting suggestion window"
Browse files Browse the repository at this point in the history 
This reverts commit 7dc39e7.

Reason for revert: Causes diacritics window to pop up on the top-left corner.

https://screenshot.googleplex.com/4ogxKR6U4uMtBsE.png
Bisect log: https://paste.googleplex.com/6667540586561536

Original change's description:
> suggest: fix shifting suggestion window
>
> 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}

Bug: b:241321719
Change-Id: Ic00f81ee39433c6c524f9337d72b9c67640009ce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3826337
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: jhtin <jhtin@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Reviewed-by: Bao-Duy Tran <tranbaoduy@chromium.org>
Reviewed-by: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1034213}
  • Loading branch information
jhtin authored and Chromium LUCI CQ committed Aug 11, 2022
1 parent b8afc01 commit 3a31378
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 112 deletions.
59 changes: 14 additions & 45 deletions chrome/browser/ash/input_method/assistive_window_controller.cc
View file
Expand Up @@ -23,8 +23,6 @@
namespace ash {
namespace input_method {

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

namespace {
gfx::NativeView GetParentView() {
gfx::NativeView parent = nullptr;
Expand All @@ -47,7 +45,6 @@ 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 @@ -104,7 +101,6 @@ 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 @@ -147,25 +143,24 @@ 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;
if (suggestion_window_view_)
suggestion_window_view_->SetAnchorRect(bounds_.caret);
if (grammar_suggestion_window_)
// 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_) {
grammar_suggestion_window_->SetBounds(bounds_.caret);
if (pending_suggestion_timer_ && pending_suggestion_timer_->IsRunning()) {
pending_suggestion_timer_->FireNow();
pending_suggestion_timer_ = nullptr;
}
}

Expand All @@ -177,18 +172,13 @@ 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;
// 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));
suggestion_window_view_->Show(details);
}

void AssistiveWindowController::SetButtonHighlighted(
Expand Down Expand Up @@ -256,11 +246,6 @@ 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 @@ -307,22 +292,6 @@ 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: 0 additions & 8 deletions chrome/browser/ash/input_method/assistive_window_controller.h
View file
Expand Up @@ -6,10 +6,7 @@
#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 @@ -76,8 +73,6 @@ 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 @@ -88,9 +83,6 @@ 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: 3 additions & 55 deletions chrome/browser/ash/input_method/assistive_window_controller_unittest.cc
View file
Expand Up @@ -16,7 +16,6 @@
#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 @@ -32,8 +31,6 @@ 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 @@ -56,10 +53,7 @@ class TestAccessibilityView : public ui::ime::AssistiveAccessibilityView {

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

AssistiveWindowControllerTest() = default;
~AssistiveWindowControllerTest() override = default;

void SetUp() override {
Expand Down Expand Up @@ -111,11 +105,6 @@ 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 @@ -126,42 +115,10 @@ 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 @@ -171,18 +128,16 @@ 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 @@ -191,18 +146,16 @@ 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 @@ -211,13 +164,10 @@ 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 @@ -244,9 +194,7 @@ 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: 0 additions & 4 deletions ui/base/ime/ash/ime_assistive_window_handler_interface.h
View file
Expand Up @@ -30,10 +30,6 @@ 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 3a31378

Please sign in to comment.