Commit 49c53bee authored by David Black's avatar David Black Committed by Commit Bot

Migrate Assistant Settings host off of Content Service.

Previously the host for Assistant Settings used the Content Service
for embedding WebContents, but this introduced a number for focus and
A11Y related bugs.

Now, we are migrating back to a WebView based implementation that
doesn't exhibit the same buggy behavior.

Manual tests run:
- Tab traverse in the forward direction
- Tab traverse in the backward direction
- ChromeVox traverse in the forward direction
- ChromeVox traverse in the backwards direction
- Changing between windows and then tab traversing
- Changing between windows and then ChromeVox traversing

Bug: b:146351046
Change-Id: I02601da8c8d726faaad527dda5785b4ccc60102b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1999042Reviewed-by: default avatarTao Wu <wutao@chromium.org>
Reviewed-by: default avatarXiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#731770}
parent 10d71a2f
......@@ -2239,6 +2239,10 @@ static_library("test_support") {
"assistant/test/assistant_ash_test_base.h",
"assistant/test/test_assistant_service.cc",
"assistant/test/test_assistant_service.h",
"assistant/test/test_assistant_web_view.cc",
"assistant/test/test_assistant_web_view.h",
"assistant/test/test_assistant_web_view_factory.cc",
"assistant/test/test_assistant_web_view_factory.h",
"display/display_configuration_controller_test_api.cc",
"display/display_configuration_controller_test_api.h",
"display/mirror_window_test_api.cc",
......
......@@ -11,6 +11,7 @@
#include "ash/app_list/views/assistant/assistant_main_view.h"
#include "ash/app_list/views/assistant/assistant_page_view.h"
#include "ash/assistant/assistant_controller.h"
#include "ash/assistant/test/test_assistant_web_view_factory.h"
#include "ash/keyboard/ui/keyboard_ui_controller.h"
#include "ash/keyboard/ui/test/keyboard_test_util.h"
#include "ash/public/cpp/app_list/app_list_features.h"
......@@ -62,7 +63,8 @@ void PressHomeButton() {
} // namespace
AssistantAshTestBase::AssistantAshTestBase()
: test_api_(AssistantTestApi::Create()) {}
: test_api_(AssistantTestApi::Create()),
test_web_view_factory_(std::make_unique<TestAssistantWebViewFactory>()) {}
AssistantAshTestBase::~AssistantAshTestBase() = default;
......
......@@ -23,8 +23,9 @@ namespace ash {
class AssistantController;
class AssistantInteractionController;
class TestAssistantService;
class AssistantTestApi;
class TestAssistantService;
class TestAssistantWebViewFactory;
// Helper class to make testing the Assistant Ash UI easier.
class AssistantAshTestBase : public AshTestBase {
......@@ -145,6 +146,7 @@ class AssistantAshTestBase : public AshTestBase {
TestAssistantService* assistant_service();
std::unique_ptr<AssistantTestApi> test_api_;
std::unique_ptr<TestAssistantWebViewFactory> test_web_view_factory_;
base::test::ScopedFeatureList scoped_feature_list_;
AssistantController* controller_ = nullptr;
......
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "ash/assistant/test/test_assistant_web_view.h"
#include "base/threading/sequenced_task_runner_handle.h"
namespace ash {
TestAssistantWebView::TestAssistantWebView() = default;
TestAssistantWebView::~TestAssistantWebView() = default;
void TestAssistantWebView::AddObserver(Observer* observer) {
observers_.AddObserver(observer);
}
void TestAssistantWebView::RemoveObserver(Observer* observer) {
observers_.RemoveObserver(observer);
}
gfx::NativeView TestAssistantWebView::GetNativeView() {
// Not yet implemented for unittests.
return nullptr;
}
bool TestAssistantWebView::GoBack() {
// Not yet implemented for unittests.
return false;
}
void TestAssistantWebView::Navigate(const GURL& url) {
// Simulate navigation by notifying |observers_| of the expected event that
// would normally signal navigation completion. We do this asynchronously to
// more accurately simulate real-world conditions.
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(
[](const base::WeakPtr<TestAssistantWebView>& self) {
if (self) {
for (auto& observer : self->observers_)
observer.DidStopLoading();
}
},
weak_factory_.GetWeakPtr()));
}
} // namespace ash
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef ASH_ASSISTANT_TEST_TEST_ASSISTANT_WEB_VIEW_H_
#define ASH_ASSISTANT_TEST_TEST_ASSISTANT_WEB_VIEW_H_
#include "ash/public/cpp/assistant/assistant_web_view_2.h"
#include "base/observer_list.h"
namespace ash {
// An implementation of AssistantWebView2 for use in unittests.
class TestAssistantWebView : public AssistantWebView2 {
public:
TestAssistantWebView();
TestAssistantWebView(const TestAssistantWebView& copy) = delete;
TestAssistantWebView& operator=(const TestAssistantWebView& assign) = delete;
~TestAssistantWebView() override;
// AssistantWebView2:
void AddObserver(Observer* observer) override;
void RemoveObserver(Observer* observer) override;
gfx::NativeView GetNativeView() override;
bool GoBack() override;
void Navigate(const GURL& url) override;
private:
base::ObserverList<Observer> observers_;
base::WeakPtrFactory<TestAssistantWebView> weak_factory_{this};
};
} // namespace ash
#endif // ASH_ASSISTANT_TEST_TEST_ASSISTANT_WEB_VIEW_H_
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "ash/assistant/test/test_assistant_web_view_factory.h"
#include "ash/assistant/test/test_assistant_web_view.h"
namespace ash {
TestAssistantWebViewFactory::TestAssistantWebViewFactory() = default;
TestAssistantWebViewFactory::~TestAssistantWebViewFactory() = default;
std::unique_ptr<AssistantWebView2> TestAssistantWebViewFactory::Create(
const AssistantWebView2::InitParams& params) {
return std::make_unique<TestAssistantWebView>();
}
} // namespace ash
\ No newline at end of file
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef ASH_ASSISTANT_TEST_TEST_ASSISTANT_WEB_VIEW_FACTORY_H_
#define ASH_ASSISTANT_TEST_TEST_ASSISTANT_WEB_VIEW_FACTORY_H_
#include <memory>
#include "ash/public/cpp/assistant/assistant_web_view_factory.h"
namespace ash {
// An implementation of AssistantWebViewFactory for use in unittests.
class TestAssistantWebViewFactory : public AssistantWebViewFactory {
public:
TestAssistantWebViewFactory();
TestAssistantWebViewFactory(const AssistantWebViewFactory& copy) = delete;
TestAssistantWebViewFactory& operator=(
const AssistantWebViewFactory& assign) = delete;
~TestAssistantWebViewFactory() override;
// AssistantWebViewFactory:
std::unique_ptr<AssistantWebView2> Create(
const AssistantWebView2::InitParams& params) override;
};
} // namespace ash
#endif // ASH_ASSISTANT_TEST_TEST_ASSISTANT_WEB_VIEW_FACTORY_H_
......@@ -85,12 +85,6 @@ void AssistantWebContainerView::InitLayout() {
views::Widget* widget = new views::Widget;
widget->Init(std::move(params));
// TODO(b/146351046): Temporary workaround for an a11y bug b/144765770.
// Should be removed once we have moved off of the Content Service
// (tracked in b/146351046).
widget->client_view()->SetFocusBehavior(FocusBehavior::ALWAYS);
widget->client_view()->RequestFocus();
SetLayoutManager(std::make_unique<views::FillLayout>());
SetBackground(views::CreateSolidBackground(SK_ColorWHITE));
......
......@@ -13,10 +13,10 @@
#include "ash/assistant/ui/assistant_web_view_delegate.h"
#include "ash/assistant/util/deep_link_util.h"
#include "ash/public/cpp/app_list/app_list_features.h"
#include "ash/public/cpp/assistant/assistant_web_view_factory.h"
#include "base/bind.h"
#include "base/callback.h"
#include "chromeos/services/assistant/public/features.h"
#include "services/content/public/cpp/navigable_contents_view.h"
#include "ui/aura/window.h"
#include "ui/compositor/layer.h"
#include "ui/display/display.h"
......@@ -80,27 +80,6 @@ void AssistantWebView::ChildPreferredSizeChanged(views::View* child) {
SchedulePaint();
}
void AssistantWebView::OnFocus() {
if (contents_)
contents_->Focus();
}
void AssistantWebView::AboutToRequestFocusFromTabTraversal(bool reverse) {
if (contents_) {
// TODO(b/146351046): Temporary workaround for b/145213680. Should be
// removed once we have moved off of the Content Service (tracked in
// b/146351046).
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(
[](base::WeakPtr<AssistantWebView> view) {
if (view)
view->GetFocusManager()->ClearFocus();
},
weak_factory_.GetWeakPtr()));
contents_->FocusThroughTabTraversal(reverse);
}
}
void AssistantWebView::InitLayout() {
SetLayoutManager(std::make_unique<views::BoxLayout>(
views::BoxLayout::Orientation::kVertical));
......@@ -120,20 +99,10 @@ void AssistantWebView::InitLayout() {
bool AssistantWebView::OnCaptionButtonPressed(AssistantButtonId id) {
// We need special handling of the back button. When possible, the back button
// should navigate backwards in the web contents' history stack.
if (id == AssistantButtonId::kBack && contents_) {
contents_->GoBack(base::BindOnce(
[](const base::WeakPtr<AssistantWebView>& assistant_web_view,
bool success) {
// If we can't navigate back in the web contents' history stack we
// defer back to our primary caption button delegate.
if (!success && assistant_web_view) {
assistant_web_view->assistant_view_delegate_
->GetCaptionBarDelegate()
->OnCaptionButtonPressed(AssistantButtonId::kBack);
}
},
weak_factory_.GetWeakPtr()));
// should navigate backwards in the WebContents' history stack. If we can't go
// back, control is returned to the primary caption button delegate.
if (id == AssistantButtonId::kBack && contents_view_ &&
contents_view_->GoBack()) {
return true;
}
......@@ -144,20 +113,20 @@ bool AssistantWebView::OnCaptionButtonPressed(AssistantButtonId id) {
void AssistantWebView::DidStopLoading() {
// We should only respond to the |DidStopLoading| event the first time, to add
// the view for the navigable contents to our view hierarchy and perform other
// one-time view initializations.
// the view for contents to our view hierarchy and perform other one-time view
// initializations.
if (contents_view_initialized_)
return;
contents_view_initialized_ = true;
UpdateContentSize();
AddChildView(contents_->GetView()->view());
AddChildView(contents_view_.get());
SetFocusBehavior(FocusBehavior::ALWAYS);
// We need to clip the corners of our web contents to match our container.
// We need to clip the corners of our WebContents to match our container.
if (!chromeos::assistant::features::IsAssistantWebContainerEnabled()) {
contents_->GetView()->native_view()->layer()->SetRoundedCornerRadius(
contents_view_->GetNativeView()->layer()->SetRoundedCornerRadius(
{/*top_left=*/0, /*top_right=*/0, /*bottom_right=*/kCornerRadiusDip,
/*bottom_left=*/kCornerRadiusDip});
}
......@@ -179,11 +148,11 @@ void AssistantWebView::DidSuppressNavigation(const GURL& url,
return;
}
// Otherwise we'll allow our web contents to navigate freely.
contents_->Navigate(url);
// Otherwise we'll allow our WebContents to navigate freely.
contents_view_->Navigate(url);
}
void AssistantWebView::UpdateCanGoBack(bool can_go_back) {
void AssistantWebView::DidChangeCanGoBack(bool can_go_back) {
if (!chromeos::assistant::features::IsAssistantWebContainerEnabled())
return;
......@@ -210,23 +179,22 @@ void AssistantWebView::OnUiVisibilityChanged(
void AssistantWebView::OpenUrl(const GURL& url) {
RemoveContents();
if (!contents_factory_.is_bound()) {
assistant_view_delegate_->GetNavigableContentsFactoryForView(
contents_factory_.BindNewPipeAndPassReceiver());
}
AssistantWebView2::InitParams contents_params;
contents_params.suppress_navigation = true;
auto contents_params = content::mojom::NavigableContentsParams::New();
contents_params->suppress_navigations = true;
contents_view_ = AssistantWebViewFactory::Get()->Create(contents_params);
contents_ = std::make_unique<content::NavigableContents>(
contents_factory_.get(), std::move(contents_params));
// We retain ownership of |contents_view_| as it is only added to the view
// hierarchy once loading stops and we want to ensure that it is cleaned up in
// the rare chance that that never occurs.
contents_view_->set_owned_by_client();
// We observe |contents_| so that we can handle events from the underlying
// web contents.
contents_->AddObserver(this);
// We observe |contents_view_| so that we can handle events from the
// underlying WebContents.
contents_view_->AddObserver(this);
// Navigate to the specified |url|.
contents_->Navigate(url);
contents_view_->Navigate(url);
}
void AssistantWebView::OnUsableWorkAreaChanged(
......@@ -237,32 +205,32 @@ void AssistantWebView::OnUsableWorkAreaChanged(
}
void AssistantWebView::RemoveContents() {
if (!contents_)
if (!contents_view_)
return;
views::View* view = contents_->GetView()->view();
if (view)
RemoveChildView(view);
RemoveChildView(contents_view_.get());
SetFocusBehavior(FocusBehavior::NEVER);
contents_->RemoveObserver(this);
contents_.reset();
contents_view_->RemoveObserver(this);
contents_view_.reset();
contents_view_initialized_ = false;
}
void AssistantWebView::UpdateContentSize() {
if (!contents_ || !contents_view_initialized_)
if (!contents_view_ || !contents_view_initialized_)
return;
if (chromeos::assistant::features::IsAssistantWebContainerEnabled()) {
contents_->GetView()->view()->SetPreferredSize(GetPreferredSize());
contents_view_->SetPreferredSize(GetPreferredSize());
return;
}
const gfx::Size preferred_size = gfx::Size(
kPreferredWidthDip, GetHeightForWidth(kPreferredWidthDip) -
caption_bar_->GetPreferredSize().height());
contents_->GetView()->view()->SetPreferredSize(preferred_size);
contents_view_->SetPreferredSize(preferred_size);
}
} // namespace ash
......@@ -12,11 +12,10 @@
#include "ash/assistant/model/assistant_ui_model_observer.h"
#include "ash/assistant/ui/assistant_view_delegate.h"
#include "ash/assistant/ui/caption_bar.h"
#include "ash/public/cpp/assistant/assistant_web_view_2.h"
#include "base/component_export.h"
#include "base/macros.h"
#include "base/optional.h"
#include "mojo/public/cpp/bindings/remote.h"
#include "services/content/public/cpp/navigable_contents.h"
#include "ui/views/view.h"
namespace ash {
......@@ -28,12 +27,11 @@ class AssistantWebViewDelegate;
// standalone Assistant UI.
// AssistantWebView is a child of AssistantContainerView which allows Assistant
// UI to render remotely hosted content within its bubble. It provides a
// CaptionBar for window level controls and embeds web contents with help from
// the Content Service.
// CaptionBar for window level controls and embeds WebContents.
class COMPONENT_EXPORT(ASSISTANT_UI) AssistantWebView
: public views::View,
public CaptionBarDelegate,
public content::NavigableContentsObserver,
public AssistantWebView2::Observer,
public AssistantUiModelObserver {
public:
AssistantWebView(AssistantViewDelegate* assistant_view_delegate,
......@@ -45,18 +43,16 @@ class COMPONENT_EXPORT(ASSISTANT_UI) AssistantWebView
gfx::Size CalculatePreferredSize() const override;
int GetHeightForWidth(int width) const override;
void ChildPreferredSizeChanged(views::View* child) override;
void OnFocus() override;
void AboutToRequestFocusFromTabTraversal(bool reverse) override;
// CaptionBarDelegate:
bool OnCaptionButtonPressed(AssistantButtonId id) override;
// content::NavigableContentsObserver:
// AssistantWebView2::Observer:
void DidStopLoading() override;
void DidSuppressNavigation(const GURL& url,
WindowOpenDisposition disposition,
bool from_user_gesture) override;
void UpdateCanGoBack(bool can_go_back) override;
void DidChangeCanGoBack(bool can_go_back) override;
// AssistantUiModelObserver:
void OnUiVisibilityChanged(
......@@ -86,14 +82,10 @@ class COMPONENT_EXPORT(ASSISTANT_UI) AssistantWebView
AssistantWebViewDelegate* const web_container_view_delegate_;
CaptionBar* caption_bar_ = nullptr; // Owned by view hierarchy.
mojo::Remote<content::mojom::NavigableContentsFactory> contents_factory_;
std::unique_ptr<content::NavigableContents> contents_;
std::unique_ptr<AssistantWebView2> contents_view_;
bool contents_view_initialized_ = false;
base::WeakPtrFactory<AssistantWebView> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(AssistantWebView);
};
......
......@@ -17,8 +17,8 @@ namespace ash {
// TODO(b/146520500): Rename to AssistantWebView after freeing up name which is
// currently in use.
// A view which wraps a views::WebView (and associated content::WebContents) to
// work around dependency restrictions in Ash.
// A view which wraps a views::WebView (and associated WebContents) to work
// around dependency restrictions in Ash.
class ASH_PUBLIC_EXPORT AssistantWebView2 : public views::View {
public:
// Initialization parameters which dictate how an instance of
......@@ -29,14 +29,14 @@ class ASH_PUBLIC_EXPORT AssistantWebView2 : public views::View {
~InitParams();
// If enabled, AssistantWebView2 will automatically resize to the size
// desired by its embedded content::WebContents. Note that, if specified,
// the content::WebContents will be bounded by |min_size| and |max_size|.
// desired by its embedded WebContents. Note that, if specified, the
// WebContents will be bounded by |min_size| and |max_size|.
bool enable_auto_resize = false;
base::Optional<gfx::Size> min_size = base::nullopt;
base::Optional<gfx::Size> max_size = base::nullopt;
// If enabled, AssistantWebView2 will suppress navigation attempts of its
// embedded content::WebContents. When navigation suppression occurs,
// embedded WebContents. When navigation suppression occurs,
// Observer::DidSuppressNavigation() will be invoked.
bool suppress_navigation = false;
};
......@@ -44,16 +44,19 @@ class ASH_PUBLIC_EXPORT AssistantWebView2 : public views::View {
// An observer which receives AssistantWebView2 events.
class Observer : public base::CheckedObserver {
public:
// Invoked when the embedded content::WebContents has stopped loading.
// Invoked when the embedded WebContents has stopped loading.
virtual void DidStopLoading() {}
// Invoked when the embedded content::WebContents has suppressed navigation.
// Invoked when the embedded WebContents has suppressed navigation.
virtual void DidSuppressNavigation(const GURL& url,
WindowOpenDisposition disposition,
bool from_user_gesture) {}
// Invoked when the focused node within the embedded content::WebContents
// has changed.
// Invoked when the embedded WebContents' ability to go back has changed.
virtual void DidChangeCanGoBack(bool can_go_back) {}
// Invoked when the focused node within the embedded WebContents has
// changed.
virtual void DidChangeFocusedNode(const gfx::Rect& node_bounds_in_screen) {}
};
......@@ -63,12 +66,15 @@ class ASH_PUBLIC_EXPORT AssistantWebView2 : public views::View {
virtual void AddObserver(Observer* observer) = 0;
virtual void RemoveObserver(Observer* observer) = 0;
// Invoke to navigate back in the embedded content::WebContents' navigation
// stack. If backwards navigation is not possible, return |false|. Otherwise
// returns |true| to indicate success.
// Returns the native view associated w/ the underlying WebContents.
virtual gfx::NativeView GetNativeView() = 0;
// Invoke to navigate back in the embedded WebContents' navigation stack. If
// backwards navigation is not possible, return |false|. Otherwise returns
// |true| to indicate success.
virtual bool GoBack() = 0;
// Invoke to navigate the embedded content::WebContents' to |url|.
// Invoke to navigate the embedded WebContents' to |url|.
virtual void Navigate(const GURL& url) = 0;
protected:
......
......@@ -31,6 +31,10 @@ const char* AssistantWebViewImpl::GetClassName() const {
return "AssistantWebViewImpl";
}
gfx::NativeView AssistantWebViewImpl::GetNativeView() {
return web_contents_->GetNativeView();
}
void AssistantWebViewImpl::ChildPreferredSizeChanged(views::View* child) {
DCHECK_EQ(web_view_.get(), child);
SetPreferredSize(web_view_->GetPreferredSize());
......@@ -109,6 +113,13 @@ bool AssistantWebViewImpl::TakeFocus(content::WebContents* web_contents,
return false;
}
void AssistantWebViewImpl::NavigationStateChanged(
content::WebContents* web_contents,
content::InvalidateTypes changed_flags) {
DCHECK_EQ(web_contents_.get(), web_contents);
UpdateCanGoBack();
}
void AssistantWebViewImpl::DidStopLoading() {
for (auto& observer : observers_)
observer.DidStopLoading();
......@@ -141,6 +152,18 @@ void AssistantWebViewImpl::RenderViewHostChanged(
max_size);
}
void AssistantWebViewImpl::NavigationEntriesDeleted() {
UpdateCanGoBack();
}
void AssistantWebViewImpl::DidAttachInterstitialPage() {
UpdateCanGoBack();
}
void AssistantWebViewImpl::DidDetachInterstitialPage() {
UpdateCanGoBack();
}
void AssistantWebViewImpl::InitWebContents(Profile* profile) {
web_contents_ =
content::WebContents::Create(content::WebContents::CreateParams(
......@@ -168,3 +191,14 @@ void AssistantWebViewImpl::InitLayout(Profile* profile) {
web_view_->SetWebContents(web_contents_.get());
AddChildView(web_view_.get());
}
void AssistantWebViewImpl::UpdateCanGoBack() {
const bool can_go_back = web_contents_->GetController().CanGoBack();
if (can_go_back_ == can_go_back)
return;
can_go_back_ = can_go_back;
for (auto& observer : observers_)
observer.DidChangeCanGoBack(can_go_back_);
}
......@@ -33,6 +33,7 @@ class AssistantWebViewImpl : public ash::AssistantWebView2,
// ash::AssistantWebView2:
const char* GetClassName() const override;
gfx::NativeView GetNativeView() override;
void ChildPreferredSizeChanged(views::View* child) override;
void Layout() override;
void AddObserver(Observer* observer) override;
......@@ -53,22 +54,32 @@ class AssistantWebViewImpl : public ash::AssistantWebView2,
void ResizeDueToAutoResize(content::WebContents* web_contents,
const gfx::Size& new_size) override;
bool TakeFocus(content::WebContents* web_contents, bool reverse) override;
void NavigationStateChanged(content::WebContents* web_contents,
content::InvalidateTypes changed_flags) override;
// content::WebContentsObserver:
void DidStopLoading() override;
void OnFocusChangedInPage(content::FocusedNodeDetails* details) override;
void RenderViewHostChanged(content::RenderViewHost* old_host,
content::RenderViewHost* new_host) override;
void NavigationEntriesDeleted() override;
void DidAttachInterstitialPage() override;
void DidDetachInterstitialPage() override;
private:
void InitWebContents(Profile* profile);
void InitLayout(Profile* profile);
void UpdateCanGoBack();
const InitParams params_;
std::unique_ptr<content::WebContents> web_contents_;
std::unique_ptr<views::WebView> web_view_;
// Whether or not the embedded |web_contents_| can go back.
bool can_go_back_ = false;
base::ObserverList<Observer> observers_;
};
......
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