Commit 531da0ed authored by David Black's avatar David Black Committed by Commit Bot

Fix tab traversal order in Assistant.

Summary of issue: Tab traversal within Assistant was very irregular,
the Assistant cards (backed by NavigableContents) would steal focus
when attempting to traverse the native view hierarchy. See video in
bug comments for example of issue.

Solution: Handle WebContentsDelegate::TakeFocus event to trigger a
clearing of native view focus in NavigableContentsView.

Bug: b:120682808
Change-Id: Ib571b3a1a6e9ccbc48d44fe29c67c6aa62aa85b0
Reviewed-on: https://chromium-review.googlesource.com/c/1407172
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: default avatarScott Violet <sky@chromium.org>
Reviewed-by: default avatarDaniel Cheng <dcheng@chromium.org>
Reviewed-by: default avatarKen Rockot <rockot@google.com>
Reviewed-by: default avatarXiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626885}
parent 9bc55a53
......@@ -58,6 +58,11 @@ class NavigableContentsDelegateImpl : public content::NavigableContentsDelegate,
WebContentsObserver::Observe(nullptr);
}
bool TakeFocus(WebContents* source, bool reverse) override {
client_->ClearViewFocus();
return true;
}
private:
void NotifyAXTreeChange() {
auto* rfh = web_contents_->GetMainFrame();
......
......@@ -65,6 +65,11 @@ void NavigableContents::FocusThroughTabTraversal(bool reverse) {
contents_->FocusThroughTabTraversal(reverse);
}
void NavigableContents::ClearViewFocus() {
if (view_)
view_->ClearNativeFocus();
}
void NavigableContents::DidFinishNavigation(
const GURL& url,
bool is_main_frame,
......
......@@ -71,6 +71,7 @@ class COMPONENT_EXPORT(CONTENT_SERVICE_CPP) NavigableContents
private:
// mojom::NavigableContentsClient:
void ClearViewFocus() override;
void DidFinishNavigation(
const GURL& url,
bool is_main_frame,
......
......@@ -17,6 +17,7 @@
#include "ui/accessibility/ax_node_data.h"
#if defined(TOOLKIT_VIEWS)
#include "ui/views/focus/focus_manager.h" // nogncheck
#include "ui/views/layout/fill_layout.h" // nogncheck
#include "ui/views/view.h" // nogncheck
......@@ -145,6 +146,14 @@ bool NavigableContentsView::IsClientRunningInServiceProcess() {
return GetInServiceProcessFlag().IsSet();
}
void NavigableContentsView::ClearNativeFocus() {
#if defined(TOOLKIT_VIEWS) && defined(USE_AURA)
auto* focus_manager = view_->GetFocusManager();
if (focus_manager)
focus_manager->ClearNativeFocus();
#endif // defined(TOOLKIT_VIEWS) && defined(USE_AURA)
}
void NavigableContentsView::NotifyAccessibilityTreeChange() {
#if defined(TOOLKIT_VIEWS) && defined(USE_AURA)
view_->NotifyAccessibilityEvent(ax::mojom::Event::kChildrenChanged, false);
......
......@@ -78,6 +78,9 @@ class COMPONENT_EXPORT(CONTENT_SERVICE_CPP) NavigableContentsView {
gfx::NativeView native_view() const { return view_->native_view(); }
#endif // defined(TOOLKIT_VIEWS) && defined(USE_AURA)
// Clears the native view having focus. See FocusManager::ClearNativeFocus.
void ClearNativeFocus();
// Has this view notify the UI subsystem of an accessibility tree change.
void NotifyAccessibilityTreeChange();
......
......@@ -59,6 +59,9 @@ interface NavigableContents {
// A client interface used by the Content Service to push contents-scoped events
// back to the application.
interface NavigableContentsClient {
// Requests that the client relinquish focus from the content area's view.
ClearViewFocus();
// Notifies the client that a navigation has finished.
DidFinishNavigation(url.mojom.Url url,
bool is_main_frame,
......
......@@ -30,6 +30,7 @@ class TestNavigableContentsClient : public mojom::NavigableContentsClient {
private:
// mojom::NavigableContentsClient:
void ClearViewFocus() override {}
void DidFinishNavigation(const GURL& url,
bool is_main_frame,
bool is_error_page,
......
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