Skip to content

Commit

Permalink
Use cached context caret bounds to show mako UI.
Browse files Browse the repository at this point in the history
Currently, we try to anchor the mako UI at the caret bounds for the
active text input client. This has some issues, e.g.

1. When the mako UI is triggered from a freeform prompt, it is
   incorrectly anchored at the freeform text input rather than the
   original text context.
2. If there is no active text input client (e.g. if a native panel chip
   is focused), the mako UI doesn't show up at all.

Fix this by caching the caret bounds when the native panel requests the
current editor context, then using these cached caret bounds to show
subsequent mako UI.

Bug: b:302055240
Change-Id: I1b9ab152b1eefc84c9307c3f6f637b9dc69e915c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4941631
Commit-Queue: Michelle Chen <michellegc@google.com>
Reviewed-by: John Palmer <jopalmer@chromium.org>
Reviewed-by: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1210013}
  • Loading branch information
Michelle authored and Chromium LUCI CQ committed Oct 16, 2023
1 parent 5d78672 commit 4bf37cc
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 18 deletions.
4 changes: 4 additions & 0 deletions chrome/browser/ash/input_method/editor_mediator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,10 @@ void EditorMediator::HandleTrigger(
}
}

void EditorMediator::CacheContextCaretBounds() {
mako_bubble_coordinator_.CacheContextCaretBounds();
}

void EditorMediator::OnTextInserted() {
// After queuing the text to be inserted, closing the mako web ui should
// return the focus back to the original input.
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/ash/input_method/editor_mediator.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ class EditorMediator
absl::optional<std::string_view> preset_query_id = absl::nullopt,
absl::optional<std::string_view> freeform_text = absl::nullopt) override;
EditorMode GetEditorMode() const override;
void CacheContextCaretBounds() override;

// TabletModeObserver:
void OnTabletModeStarting() override;
Expand Down
4 changes: 4 additions & 0 deletions chrome/browser/ash/input_method/editor_panel_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ void EditorPanelManager::BindEditorClient() {

void EditorPanelManager::GetEditorPanelContext(
GetEditorPanelContextCallback callback) {
// Cache the caret bounds of the current text context, so that we can properly
// anchor UI bubbles that are triggered by and shown after the editor panel.
delegate_->CacheContextCaretBounds();

// TODO(b/295059934): Get the panel mode from the editor mediator.
const auto editor_panel_mode = GetEditorPanelMode(delegate_->GetEditorMode());

Expand Down
2 changes: 2 additions & 0 deletions chrome/browser/ash/input_method/editor_panel_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ class EditorPanelManager : public crosapi::mojom::EditorPanelManager {
absl::optional<std::string_view> preset_query_id,
absl::optional<std::string_view> freeform_text) = 0;
virtual EditorMode GetEditorMode() const = 0;

virtual void CacheContextCaretBounds() = 0;
};

explicit EditorPanelManager(Delegate* delegate);
Expand Down
22 changes: 8 additions & 14 deletions chrome/browser/ui/webui/ash/mako/mako_bubble_coordinator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,12 @@ void MakoBubbleCoordinator::LoadConsentUI(Profile* profile) {
return;
}

caret_bounds_ = GetTextInputClient()->GetCaretBounds();
contents_wrapper_ = std::make_unique<BubbleContentsWrapperT<MakoUntrustedUI>>(
GURL(kChromeUIMakoPrivacyURL), profile, kMakoTaskManagerStringID);
contents_wrapper_->ReloadWebContents();
views::BubbleDialogDelegateView::CreateBubble(
std::make_unique<MakoConsentView>(contents_wrapper_.get(),
caret_bounds_.value()))
->Show();
context_caret_bounds_));
}

void MakoBubbleCoordinator::LoadEditorUI(
Expand All @@ -139,16 +137,7 @@ void MakoBubbleCoordinator::LoadEditorUI(
absl::optional<std::string_view> preset_query_id,
absl::optional<std::string_view> freeform_text) {
if (IsShowingUI()) {
// If switching contents (e.g. from consent UI to rewrite UI), close the
// current contents and use the cached caret bounds.
contents_wrapper_->CloseUI();
CHECK(caret_bounds_.has_value());
} else if (const auto* text_input_client = GetTextInputClient()) {
// Otherwise, try to get the caret bounds from the text input client.
caret_bounds_ = text_input_client->GetCaretBounds();
} else {
// Otherwise, don't show mako UI.
return;
}

GURL url = net::AppendOrReplaceQueryParameter(GURL(kChromeUIMakoOrcaURL),
Expand All @@ -164,7 +153,7 @@ void MakoBubbleCoordinator::LoadEditorUI(
contents_wrapper_->ReloadWebContents();
views::BubbleDialogDelegateView::CreateBubble(
std::make_unique<MakoRewriteView>(contents_wrapper_.get(),
caret_bounds_.value()));
context_caret_bounds_));
}

void MakoBubbleCoordinator::ShowUI() {
Expand All @@ -177,7 +166,6 @@ void MakoBubbleCoordinator::CloseUI() {
if (contents_wrapper_) {
contents_wrapper_->CloseUI();
contents_wrapper_ = nullptr;
caret_bounds_ = absl::nullopt;
}
}

Expand All @@ -188,4 +176,10 @@ bool MakoBubbleCoordinator::IsShowingUI() const {
contents_wrapper_->GetHost() != nullptr;
}

void MakoBubbleCoordinator::CacheContextCaretBounds() {
if (const auto* text_input_client = GetTextInputClient()) {
context_caret_bounds_ = text_input_client->GetCaretBounds();
}
}

} // namespace ash
15 changes: 11 additions & 4 deletions chrome/browser/ui/webui/ash/mako/mako_bubble_coordinator.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,18 @@ class MakoBubbleCoordinator {

bool IsShowingUI() const;

// Caches the caret bounds of the current text input client (if one is
// active). This should be called before showing the mako UI, while the text
// context for the mako UI is focused.
void CacheContextCaretBounds();

private:
// Cached caret bounds to use as the mako UI anchor when there is no text
// input client (e.g. if focus is not regained after switching from the
// consent UI to the rewrite UI).
absl::optional<gfx::Rect> caret_bounds_;
// Cached context caret bounds at which to anchor the mako UI. This might not
// correspond to the most recent active text input client's caret bounds, e.g.
// if the mako UI was triggered from a freeform text input, the cached caret
// bounds should correspond to the original text context rather than the
// freeform input text bounds.
gfx::Rect context_caret_bounds_;

// TODO(b/300554470): This doesn't seem like the right class to own the
// contents wrapper and probably won't handle the bubble widget lifetimes
Expand Down

0 comments on commit 4bf37cc

Please sign in to comment.