Skip to content

Commit 9bb8581

Browse files
David BlackXiaohui Chen
David Black
authored and
Xiaohui Chen
committed
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. (cherry picked from commit 531da0e) Bug: b:120682808 Change-Id: Ib571b3a1a6e9ccbc48d44fe29c67c6aa62aa85b0 Reviewed-on: https://chromium-review.googlesource.com/c/1407172 Commit-Queue: David Black <[email protected]> Reviewed-by: Scott Violet <[email protected]> Reviewed-by: Daniel Cheng <[email protected]> Reviewed-by: Ken Rockot <[email protected]> Reviewed-by: Xiaohui Chen <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#626885} Reviewed-on: https://chromium-review.googlesource.com/c/1444233 Cr-Commit-Position: refs/branch-heads/3683@{#50} Cr-Branched-From: e510299-refs/heads/master@{#625896}
1 parent bedcd38 commit 9bb8581

7 files changed

+27
-0
lines changed

content/browser/content_service_delegate_impl.cc

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,11 @@ class NavigableContentsDelegateImpl : public content::NavigableContentsDelegate,
5858
WebContentsObserver::Observe(nullptr);
5959
}
6060

61+
bool TakeFocus(WebContents* source, bool reverse) override {
62+
client_->ClearViewFocus();
63+
return true;
64+
}
65+
6166
private:
6267
void NotifyAXTreeChange() {
6368
auto* rfh = web_contents_->GetMainFrame();

services/content/public/cpp/navigable_contents.cc

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,11 @@ void NavigableContents::FocusThroughTabTraversal(bool reverse) {
6464
contents_->FocusThroughTabTraversal(reverse);
6565
}
6666

67+
void NavigableContents::ClearViewFocus() {
68+
if (view_)
69+
view_->ClearNativeFocus();
70+
}
71+
6772
void NavigableContents::DidFinishNavigation(
6873
const GURL& url,
6974
bool is_main_frame,

services/content/public/cpp/navigable_contents.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ class COMPONENT_EXPORT(CONTENT_SERVICE_CPP) NavigableContents
7171

7272
private:
7373
// mojom::NavigableContentsClient:
74+
void ClearViewFocus() override;
7475
void DidFinishNavigation(
7576
const GURL& url,
7677
bool is_main_frame,

services/content/public/cpp/navigable_contents_view.cc

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include "ui/accessibility/ax_node_data.h"
1717

1818
#if defined(TOOLKIT_VIEWS)
19+
#include "ui/views/focus/focus_manager.h" // nogncheck
1920
#include "ui/views/layout/fill_layout.h" // nogncheck
2021
#include "ui/views/view.h" // nogncheck
2122

@@ -144,6 +145,14 @@ bool NavigableContentsView::IsClientRunningInServiceProcess() {
144145
return GetInServiceProcessFlag().IsSet();
145146
}
146147

148+
void NavigableContentsView::ClearNativeFocus() {
149+
#if defined(TOOLKIT_VIEWS) && defined(USE_AURA)
150+
auto* focus_manager = view_->GetFocusManager();
151+
if (focus_manager)
152+
focus_manager->ClearNativeFocus();
153+
#endif // defined(TOOLKIT_VIEWS) && defined(USE_AURA)
154+
}
155+
147156
void NavigableContentsView::NotifyAccessibilityTreeChange() {
148157
#if defined(TOOLKIT_VIEWS) && defined(USE_AURA)
149158
view_->NotifyAccessibilityEvent(ax::mojom::Event::kChildrenChanged, false);

services/content/public/cpp/navigable_contents_view.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,9 @@ class COMPONENT_EXPORT(CONTENT_SERVICE_CPP) NavigableContentsView {
7878
gfx::NativeView native_view() const { return view_->native_view(); }
7979
#endif // defined(TOOLKIT_VIEWS) && defined(USE_AURA)
8080

81+
// Clears the native view having focus. See FocusManager::ClearNativeFocus.
82+
void ClearNativeFocus();
83+
8184
// Has this view notify the UI subsystem of an accessibility tree change.
8285
void NotifyAccessibilityTreeChange();
8386

services/content/public/mojom/navigable_contents.mojom

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,9 @@ interface NavigableContents {
5959
// A client interface used by the Content Service to push contents-scoped events
6060
// back to the application.
6161
interface NavigableContentsClient {
62+
// Requests that the client relinquish focus from the content area's view.
63+
ClearViewFocus();
64+
6265
// Notifies the client that a navigation has finished.
6366
DidFinishNavigation(url.mojom.Url url,
6467
bool is_main_frame,

services/content/service_unittest.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ class TestNavigableContentsClient : public mojom::NavigableContentsClient {
3030

3131
private:
3232
// mojom::NavigableContentsClient:
33+
void ClearViewFocus() override {}
3334
void DidFinishNavigation(const GURL& url,
3435
bool is_main_frame,
3536
bool is_error_page,

0 commit comments

Comments
 (0)