0

Reland "System Nudge: Add a close button to SystemNudgeView"

This is a reland of commit 0c6050e11a

There was a dangling ptr check failure due to assigning the results of
`widget->SetContentsView()` in the unit test to a raw pointer multiple
times. After re-assigning the results of `SetContentsView` all child
views are removed [1] which caused the dangling ptr.

The need to cache the contents view of the widget was removed by
accessing the required child view through its view ID directly.
A bug was filed (b/306466133) to do the same for other SystemNudgeView
child views, but a fix for this will be done in a separate CL.

[1] https://source.chromium.org/chromium/chromium/src/+/main:ui/views/widget/widget.cc;l=630;drc=430c7ea2e0a2e29142869f6efb112a7979ab605a

Original change's description:
> System Nudge: Add a close button to SystemNudgeView
>
> Add a close button that is visible on hover for nudges that are not
> text-only and do not have an anchor view.
>
> Whenever there is a close button, a new container view with a FillLayout
> is used to hold the `image_and_text_container` and the close button, to
> allow a bit of overlap which is required by the spec.
>
> specs: http://go/cros-educationalnudge-spec
> screenshot: https://screenshot.googleplex.com/5Ai64d4wMahttsH
>
> Bug: b:303684220
> Change-Id: I67a1f9301d45c6e5551cfb33f9933d77cc0d362e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4948990
> Reviewed-by: Jiaming Cheng <jiamingc@chromium.org>
> Reviewed-by: James Cook <jamescook@chromium.org>
> Commit-Queue: Kevin Radtke <kradtke@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1211833}

Bug: b:303684220
Change-Id: I7537c7d26e389b3926994b29e76c0848cb3372f5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4954824
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Jiaming Cheng <jiamingc@chromium.org>
Commit-Queue: Kevin Radtke <kradtke@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1212508}
This commit is contained in:
Kevin Radtke
2023-10-20 01:06:35 +00:00
committed by Chromium LUCI CQ
parent 3031e06e9c
commit 3d683af3ae
10 changed files with 189 additions and 31 deletions

@ -5149,6 +5149,11 @@ Some features are limited to increase battery life.
Undo
</message>
<!-- Nudges -->
<message name="IDS_ASH_SYSTEM_NUDGE_CLOSE_BUTTON_TOOLTIP" desc="The close button tooltip for system nudges.">
Close nudge
</message>
<message name="IDS_ASH_DIALOG_DONT_SHOW_AGAIN" desc="Label for checkbox dismissing show of a dialog.">
Don't show again
</message>

@ -0,0 +1 @@
32e79672f2fa54f8f6b89b62083f8387f8d2a005

@ -108,6 +108,9 @@ enum ViewID {
VIEW_ID_STYLE_SYSTEM_DIALOG_DELEGATE_ACCEPT_BUTTON,
VIEW_ID_STYLE_SYSTEM_DIALOG_DELEGATE_CANCEL_BUTTON,
// System nudge view:
VIEW_ID_SYSTEM_NUDGE_CLOSE_BUTTON,
// System toast view:
VIEW_ID_TOAST_BUTTON,
VIEW_ID_TOAST_IMAGE_VIEW,

@ -70,6 +70,9 @@ struct ASH_PUBLIC_EXPORT AnchoredNudgeData {
ui::ImageModel image_model;
std::u16string title_text;
// Callback for close button pressed.
base::RepeatingClosure close_button_callback;
// Optional system nudge buttons. If the text is not empty, the respective
// button will be created. Pressing the button will execute its callback, if
// any, followed by the nudge being closed. `second_button_text` should only

@ -86,6 +86,7 @@ aggregate_vector_icons("ash_vector_icons") {
"clipboard_launcher.icon",
"clipboard_launcher_no_assistant.icon",
"clipboard_search.icon",
"close_small.icon",
"color_correction.icon",
"combine_desks.icon",
"copy.icon",

@ -0,0 +1,18 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
CANVAS_DIMENSIONS, 20,
MOVE_TO, 12.94f, 6.01f,
LINE_TO, 6, 12.94f,
LINE_TO, 7.06f, 14,
LINE_TO, 14, 7.07f,
LINE_TO, 12.94f, 6.01f,
CLOSE,
NEW_PATH,
MOVE_TO, 7.06f, 6,
LINE_TO, 6, 7.06f,
LINE_TO, 12.94f, 14,
LINE_TO, 14, 12.93f,
LINE_TO, 7.06f, 6,
CLOSE

@ -344,6 +344,9 @@ void AnchoredNudgeManagerImpl::Show(AnchoredNudgeData& nudge_data) {
/*first_button=*/false);
}
nudge_data.close_button_callback = base::BindRepeating(
&AnchoredNudgeManagerImpl::Cancel, base::Unretained(this), id);
auto anchored_nudge = std::make_unique<AnchoredNudge>(nudge_data);
auto* anchored_nudge_ptr = anchored_nudge.get();
shown_nudges_[id] = anchored_nudge_ptr;

@ -7,24 +7,30 @@
#include <string>
#include "ash/constants/ash_features.h"
#include "ash/public/cpp/ash_view_ids.h"
#include "ash/public/cpp/style/color_provider.h"
#include "ash/public/cpp/system/anchored_nudge_data.h"
#include "ash/resources/vector_icons/vector_icons.h"
#include "ash/shell.h"
#include "ash/strings/grit/ash_strings.h"
#include "ash/style/ash_color_id.h"
#include "ash/style/pill_button.h"
#include "ash/style/system_shadow.h"
#include "ash/style/typography.h"
#include "ash/system/toast/nudge_constants.h"
#include "chromeos/constants/chromeos_features.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/metadata/metadata_impl_macros.h"
#include "ui/chromeos/styles/cros_tokens_color_mappings.h"
#include "ui/compositor/layer.h"
#include "ui/gfx/geometry/insets.h"
#include "ui/gfx/geometry/point.h"
#include "ui/views/background.h"
#include "ui/views/controls/button/image_button.h"
#include "ui/views/controls/image_view.h"
#include "ui/views/controls/label.h"
#include "ui/views/highlight_border.h"
#include "ui/views/layout/fill_layout.h"
#include "ui/views/layout/flex_layout.h"
#include "ui/views/layout/flex_layout_view.h"
@ -35,6 +41,15 @@ namespace {
// Nudge constants
constexpr gfx::Insets kNudgeInteriorMargin = gfx::Insets::VH(20, 20);
constexpr gfx::Insets kTextOnlyNudgeInteriorMargin = gfx::Insets::VH(12, 20);
constexpr gfx::Insets kNudgeWithCloseButton_InteriorMargin =
gfx::Insets::TLBR(8, 20, 20, 8);
constexpr gfx::Insets
kNudgeWithCloseButton_ImageAndTextContainerInteriorMargin =
gfx::Insets::TLBR(12, 0, 0, 12);
constexpr gfx::Insets kNudgeWithCloseButton_ButtonContainerInteriorMargin =
gfx::Insets::TLBR(0, 0, 0, 12);
constexpr float kNudgeCornerRadius = 24.0f;
// Label constants
@ -78,6 +93,7 @@ SystemNudgeView::SystemNudgeView(const AnchoredNudgeData& nudge_data) {
SetBorder(std::make_unique<views::HighlightBorder>(
kNudgeCornerRadius,
views::HighlightBorder::Type::kHighlightBorderOnShadow));
SetNotifyEnterExitOnChild(true);
// Since nudges have a large corner radius, we use the shadow on texture
// layer. Refer to `ash::SystemShadowOnTextureLayer` for more details.
@ -89,11 +105,59 @@ SystemNudgeView::SystemNudgeView(const AnchoredNudgeData& nudge_data) {
SetInteriorMargin(kNudgeInteriorMargin);
SetCrossAxisAlignment(views::LayoutAlignment::kStretch);
auto* image_and_text_container =
AddChildView(views::Builder<views::FlexLayoutView>()
.SetOrientation(views::LayoutOrientation::kHorizontal)
.SetCrossAxisAlignment(views::LayoutAlignment::kStart)
.Build());
const bool nudge_is_text_only = nudge_data.image_model.IsEmpty() &&
nudge_data.title_text.empty() &&
nudge_data.first_button_text.empty();
// Nudges without an anchor view that are not text-only will have a close
// button that is visible on view hovered.
bool has_close_button = !nudge_data.anchor_view && !nudge_is_text_only;
views::View* image_and_text_container;
auto image_and_text_container_unique =
views::Builder<views::FlexLayoutView>()
.SetOrientation(views::LayoutOrientation::kHorizontal)
.SetCrossAxisAlignment(views::LayoutAlignment::kStart)
.SetInteriorMargin(
has_close_button
? kNudgeWithCloseButton_ImageAndTextContainerInteriorMargin
: gfx::Insets())
.Build();
if (has_close_button) {
SetInteriorMargin(kNudgeWithCloseButton_InteriorMargin);
// Set the `image_and_text_container` parent to use a `FillLayout` so it can
// allow for overlap with the close button.
auto* fill_layout_container = AddChildView(std::make_unique<views::View>());
fill_layout_container->SetLayoutManager(
std::make_unique<views::FillLayout>());
image_and_text_container = fill_layout_container->AddChildView(
std::move(image_and_text_container_unique));
auto* close_button_container = fill_layout_container->AddChildView(
views::Builder<views::FlexLayoutView>()
.SetOrientation(views::LayoutOrientation::kHorizontal)
.SetMainAxisAlignment(views::LayoutAlignment::kEnd)
.SetCrossAxisAlignment(views::LayoutAlignment::kStart)
.Build());
close_button_ = close_button_container->AddChildView(
views::Builder<views::ImageButton>()
.SetID(VIEW_ID_SYSTEM_NUDGE_CLOSE_BUTTON)
.SetCallback(std::move(nudge_data.close_button_callback))
.SetImageModel(views::Button::STATE_NORMAL,
ui::ImageModel::FromVectorIcon(
kCloseSmallIcon, cros_tokens::kCrosSysOnSurface))
.SetTooltipText(l10n_util::GetStringUTF16(
IDS_ASH_SYSTEM_NUDGE_CLOSE_BUTTON_TOOLTIP))
.SetVisible(false)
.Build());
} else {
image_and_text_container =
AddChildView(std::move(image_and_text_container_unique));
}
if (!nudge_data.image_model.IsEmpty()) {
image_view_ = image_and_text_container->AddChildView(
@ -153,7 +217,7 @@ SystemNudgeView::SystemNudgeView(const AnchoredNudgeData& nudge_data) {
CHECK(nudge_data.second_button_text.empty());
// Update nudge margins and body label max width if nudge only has text.
if (nudge_data.title_text.empty() && nudge_data.image_model.IsEmpty()) {
if (nudge_is_text_only) {
SetInteriorMargin(kTextOnlyNudgeInteriorMargin);
// `SizeToFit` is reset to zero so a maximum width can be set.
body_label_->SizeToFit(0);
@ -166,12 +230,16 @@ SystemNudgeView::SystemNudgeView(const AnchoredNudgeData& nudge_data) {
AddPaddingView(this, image_and_text_container->width(),
kButtonContainerTopPadding);
auto* buttons_container =
AddChildView(views::Builder<views::FlexLayoutView>()
.SetMainAxisAlignment(views::LayoutAlignment::kEnd)
.SetIgnoreDefaultMainAxisMargins(true)
.SetCollapseMargins(true)
.Build());
auto* buttons_container = AddChildView(
views::Builder<views::FlexLayoutView>()
.SetMainAxisAlignment(views::LayoutAlignment::kEnd)
.SetInteriorMargin(
has_close_button
? kNudgeWithCloseButton_ButtonContainerInteriorMargin
: gfx::Insets())
.SetIgnoreDefaultMainAxisMargins(true)
.SetCollapseMargins(true)
.Build());
buttons_container->SetDefault(views::kMarginsKey, kButtonsMargins);
const bool has_second_button = !nudge_data.second_button_text.empty();
@ -216,6 +284,22 @@ void SystemNudgeView::AddedToWidget() {
widget_layer->StackAtBottom(shadow_layer);
}
void SystemNudgeView::OnMouseEntered(const ui::MouseEvent& event) {
HandleOnMouseHovered(/*mouse_entered=*/true);
}
void SystemNudgeView::OnMouseExited(const ui::MouseEvent& event) {
HandleOnMouseHovered(/*mouse_entered=*/false);
}
void SystemNudgeView::HandleOnMouseHovered(const bool mouse_entered) {
if (!close_button_) {
return;
}
close_button_->SetVisible(mouse_entered);
}
BEGIN_METADATA(SystemNudgeView, views::View)
END_METADATA

@ -14,6 +14,7 @@ namespace views {
class ImageView;
class Label;
class LabelButton;
class ImageButton;
} // namespace views
namespace ash {
@ -34,6 +35,8 @@ class ASH_EXPORT SystemNudgeView : public views::FlexLayoutView {
SystemNudgeView& operator=(const SystemNudgeView&) = delete;
~SystemNudgeView() override;
// TODO(b/306466133): Use `GetViewByID` when applicable in tests instead of
// exposing nudge child views.
views::ImageView* image_view() const { return image_view_; }
views::Label* body_label() const { return body_label_; }
views::Label* title_label() const { return title_label_; }
@ -50,11 +53,17 @@ class ASH_EXPORT SystemNudgeView : public views::FlexLayoutView {
raw_ptr<views::Label> title_label_ = nullptr;
raw_ptr<views::LabelButton> first_button_ = nullptr;
raw_ptr<views::LabelButton> second_button_ = nullptr;
raw_ptr<views::ImageButton> close_button_ = nullptr;
std::unique_ptr<SystemShadow> shadow_;
// views::View:
void AddedToWidget() override;
void OnMouseEntered(const ui::MouseEvent& event) override;
void OnMouseExited(const ui::MouseEvent& event) override;
// Handles mouse enter/exit events to either show or hide `close_button_`.
void HandleOnMouseHovered(const bool mouse_entered);
};
} // namespace ash

@ -5,13 +5,16 @@
#include "ash/system/toast/system_nudge_view.h"
#include "ash/constants/ash_features.h"
#include "ash/public/cpp/ash_view_ids.h"
#include "ash/public/cpp/system/anchored_nudge_data.h"
#include "ash/style/ash_color_id.h"
#include "ash/system/toast/nudge_constants.h"
#include "ash/test/ash_test_base.h"
#include "base/test/scoped_feature_list.h"
#include "components/vector_icons/vector_icons.h"
#include "ui/views/controls/button/image_button.h"
#include "ui/views/controls/label.h"
#include "ui/views/test/views_test_utils.h"
#include "ui/views/widget/widget.h"
namespace ash {
@ -19,15 +22,19 @@ namespace ash {
namespace {
// Creates an `AnchoredNudgeData` object with only the required elements.
AnchoredNudgeData CreateBaseNudgeData(views::View* contents_view) {
// This will create a nudge shown on its default location.
AnchoredNudgeData CreateBaseNudgeData() {
// Set up nudge data contents.
const std::string id = "id";
const std::u16string body_text = u"text";
auto catalog_name = NudgeCatalogName::kTestCatalogName;
auto* anchor_view =
contents_view->AddChildView(std::make_unique<views::View>());
return AnchoredNudgeData(id, catalog_name, body_text, anchor_view);
return AnchoredNudgeData(id, catalog_name, body_text);
}
views::ImageButton* GetCloseButton(views::View* nudge_view) {
return static_cast<views::ImageButton*>(
nudge_view->GetViewByID(VIEW_ID_SYSTEM_NUDGE_CLOSE_BUTTON));
}
} // namespace
@ -44,14 +51,12 @@ class SystemNudgeViewTest : public AshTestBase {
TEST_F(SystemNudgeViewTest, TextOnly) {
std::unique_ptr<views::Widget> widget = CreateFramelessTestWidget();
auto* contents_view =
widget->SetContentsView(std::make_unique<views::View>());
// Set up base nudge data which will create a text-only nudge.
auto nudge_data = CreateBaseNudgeData(contents_view);
auto nudge_data = CreateBaseNudgeData();
auto* system_nudge_view = contents_view->AddChildView(
std::make_unique<SystemNudgeView>(nudge_data));
SystemNudgeView* system_nudge_view =
widget->SetContentsView(std::make_unique<SystemNudgeView>(nudge_data));
// Test that appropriate nudge elements were created.
EXPECT_FALSE(system_nudge_view->image_view());
@ -67,16 +72,14 @@ TEST_F(SystemNudgeViewTest, TextOnly) {
TEST_F(SystemNudgeViewTest, WithButtons) {
std::unique_ptr<views::Widget> widget = CreateFramelessTestWidget();
auto* contents_view =
widget->SetContentsView(std::make_unique<views::View>());
// Set up base nudge data and add two buttons.
auto nudge_data = CreateBaseNudgeData(contents_view);
auto nudge_data = CreateBaseNudgeData();
nudge_data.first_button_text = u"Button";
nudge_data.second_button_text = u"Button";
auto* system_nudge_view = contents_view->AddChildView(
std::make_unique<SystemNudgeView>(nudge_data));
SystemNudgeView* system_nudge_view =
widget->SetContentsView(std::make_unique<SystemNudgeView>(nudge_data));
// Test that appropriate nudge elements were created.
EXPECT_FALSE(system_nudge_view->image_view());
@ -92,17 +95,15 @@ TEST_F(SystemNudgeViewTest, WithButtons) {
TEST_F(SystemNudgeViewTest, TitleAndLeadingImage) {
std::unique_ptr<views::Widget> widget = CreateFramelessTestWidget();
auto* contents_view =
widget->SetContentsView(std::make_unique<views::View>());
// Set up base nudge data and add a title and an image model.
auto nudge_data = CreateBaseNudgeData(contents_view);
auto nudge_data = CreateBaseNudgeData();
nudge_data.title_text = u"Title";
nudge_data.image_model = ui::ImageModel::FromVectorIcon(
vector_icons::kDogfoodIcon, kColorAshIconColorPrimary, /*icon_size=*/60);
auto* system_nudge_view = contents_view->AddChildView(
std::make_unique<SystemNudgeView>(nudge_data));
SystemNudgeView* system_nudge_view =
widget->SetContentsView(std::make_unique<SystemNudgeView>(nudge_data));
// Test that appropriate nudge elements were created.
EXPECT_TRUE(system_nudge_view->image_view());
@ -116,4 +117,34 @@ TEST_F(SystemNudgeViewTest, TitleAndLeadingImage) {
system_nudge_view->body_label()->GetFixedWidth());
}
// Test that the nudge close button is properly created / made visible in
// different circumstances.
TEST_F(SystemNudgeViewTest, CloseButton) {
std::unique_ptr<views::Widget> widget = CreateFramelessTestWidget();
widget->SetFullscreen(true);
// Test that text-only nudges will not have a close button.
auto nudge_data = CreateBaseNudgeData();
widget->SetContentsView(std::make_unique<SystemNudgeView>(nudge_data));
EXPECT_FALSE(GetCloseButton(widget->GetContentsView()));
// Test that a non-text-only nudge will have a close button.
nudge_data.first_button_text = u"Button";
widget->SetContentsView(std::make_unique<SystemNudgeView>(nudge_data));
ASSERT_TRUE(GetCloseButton(widget->GetContentsView()));
EXPECT_FALSE(GetCloseButton(widget->GetContentsView())->GetVisible());
// Simulate mouse hover events to toggle the close button visibility.
GetEventGenerator()->MoveMouseTo(
widget->GetContentsView()->GetBoundsInScreen().CenterPoint());
EXPECT_TRUE(GetCloseButton(widget->GetContentsView())->GetVisible());
GetEventGenerator()->MoveMouseTo(-100, -100);
EXPECT_FALSE(GetCloseButton(widget->GetContentsView())->GetVisible());
// Test that nudges with an anchor view will not have a close button.
nudge_data.anchor_view = std::make_unique<views::View>().release();
widget->SetContentsView(std::make_unique<SystemNudgeView>(nudge_data));
EXPECT_FALSE(GetCloseButton(widget->GetContentsView()));
}
} // namespace ash