Skip to content

Commit

Permalink
VC UI: Change click target for expand/collapse return to app panel
Browse files Browse the repository at this point in the history 
The expand indicator is small and should not be treated as the button to
expand/collapse the return to app panel. This CL changes the click
target to be the whole summary row. Clicking the row will toggle the
panel.

Fixed: b:266630808
Change-Id: I33ccc6e5bb9817c3614fd45cb683e30a796e13e5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4240698
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Commit-Queue: Andre Le <leandre@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1104280}
  • Loading branch information
Andre Le authored and Chromium LUCI CQ committed Feb 12, 2023
1 parent ed6e114 commit 9655a1c
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 58 deletions.
61 changes: 26 additions & 35 deletions ash/system/video_conference/bubble/return_to_app_panel.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

#include "ash/resources/vector_icons/vector_icons.h"
#include "ash/strings/grit/ash_strings.h"
#include "ash/style/ash_color_id.h"
#include "ash/system/video_conference/bubble/bubble_view_ids.h"
#include "ash/system/video_conference/video_conference_tray_controller.h"
#include "base/strings/utf_string_conversions.h"
Expand All @@ -18,11 +17,8 @@
#include "ui/base/models/image_model.h"
#include "ui/chromeos/styles/cros_tokens_color_mappings.h"
#include "ui/gfx/canvas.h"
#include "ui/gfx/geometry/point_f.h"
#include "ui/gfx/scoped_canvas.h"
#include "ui/views/background.h"
#include "ui/views/border.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/layout/flex_layout.h"
Expand Down Expand Up @@ -90,23 +86,13 @@ std::u16string GetMediaAppDisplayText(
: media_app->title;
}

void ReturnToApp(const base::UnguessableToken& id) {
// Returns early for the summary row, which has empty `id`.
if (id.is_empty()) {
return;
}
ash::VideoConferenceTrayController::Get()->ReturnToApp(id);
}

// A customized toggle button for the return to app panel, which rotates
// depending on the expand state.
class ReturnToAppExpandButton : public views::ImageButton,
class ReturnToAppExpandButton : public views::ImageView,
ReturnToAppButton::Observer {
public:
ReturnToAppExpandButton(PressedCallback callback,
ReturnToAppButton* return_to_app_button)
: views::ImageButton(std::move(callback)),
return_to_app_button_(return_to_app_button) {
explicit ReturnToAppExpandButton(ReturnToAppButton* return_to_app_button)
: return_to_app_button_(return_to_app_button) {
return_to_app_button_->AddObserver(this);
}

Expand All @@ -117,16 +103,16 @@ class ReturnToAppExpandButton : public views::ImageButton,
return_to_app_button_->RemoveObserver(this);
}

// views::ImageButton:
void PaintButtonContents(gfx::Canvas* canvas) override {
// views::ImageView:
void OnPaint(gfx::Canvas* canvas) override {
// Rotate the canvas to rotate the button depending on the panel's expanded
// state.
gfx::ScopedCanvas scoped(canvas);
canvas->Translate(gfx::Vector2d(size().width() / 2, size().height() / 2));
if (!expanded_) {
canvas->sk_canvas()->rotate(180.);
}
gfx::ImageSkia image = GetImageToPaint();
gfx::ImageSkia image = GetImage();
canvas->DrawImageInt(image, -image.width() / 2, -image.height() / 2);
}

Expand Down Expand Up @@ -163,11 +149,13 @@ ReturnToAppButton::ReturnToAppButton(ReturnToAppPanel* panel,
bool is_capturing_microphone,
bool is_capturing_screen,
const std::u16string& display_text)
: views::Button(base::BindRepeating(&ReturnToApp, id)),
is_capturing_camera_(is_capturing_camera),
: is_capturing_camera_(is_capturing_camera),
is_capturing_microphone_(is_capturing_microphone),
is_capturing_screen_(is_capturing_screen),
panel_(panel) {
SetCallback(base::BindRepeating(&ReturnToAppButton::OnButtonClicked,
weak_ptr_factory_.GetWeakPtr(), id));

auto spacing = is_top_row ? kReturnToAppButtonTopRowSpacing / 2
: kReturnToAppButtonSpacing / 2;
SetLayoutManager(std::make_unique<views::FlexLayout>())
Expand All @@ -189,17 +177,12 @@ ReturnToAppButton::ReturnToAppButton(ReturnToAppPanel* panel,
label_ = AddChildView(std::make_unique<views::Label>(display_text));

if (is_top_row) {
auto expand_button = std::make_unique<ReturnToAppExpandButton>(
base::BindRepeating(&ReturnToAppButton::OnExpandButtonToggled,
weak_ptr_factory_.GetWeakPtr()),
this);
expand_button->SetImageModel(
views::Button::ButtonState::STATE_NORMAL,
ui::ImageModel::FromVectorIcon(kUnifiedMenuExpandIcon,
cros_tokens::kCrosSysSecondary, 16));
expand_button->SetTooltipText(l10n_util::GetStringUTF16(
auto expand_indicator = std::make_unique<ReturnToAppExpandButton>(this);
expand_indicator->SetImage(ui::ImageModel::FromVectorIcon(
kUnifiedMenuExpandIcon, cros_tokens::kCrosSysSecondary, 16));
expand_indicator->SetTooltipText(l10n_util::GetStringUTF16(
IDS_ASH_VIDEO_CONFERENCE_RETURN_TO_APP_SHOW_TOOLTIP));
expand_button_ = AddChildView(std::move(expand_button));
expand_indicator_ = AddChildView(std::move(expand_indicator));
}

// TODO(b/253646076): Double check accessible name for this button.
Expand All @@ -216,7 +199,15 @@ void ReturnToAppButton::RemoveObserver(Observer* observer) {
observer_list_.RemoveObserver(observer);
}

void ReturnToAppButton::OnExpandButtonToggled(const ui::Event& event) {
void ReturnToAppButton::OnButtonClicked(const base::UnguessableToken& id) {
// For rows that are not the summary row (which has non-empty `id`), perform
// return to app.
if (!id.is_empty()) {
ash::VideoConferenceTrayController::Get()->ReturnToApp(id);
return;
}

// For summary row, toggle the expand state.
expanded_ = !expanded_;

for (auto& observer : observer_list_) {
Expand All @@ -227,7 +218,7 @@ void ReturnToAppButton::OnExpandButtonToggled(const ui::Event& event) {
auto tooltip_text_id =
expanded_ ? IDS_ASH_VIDEO_CONFERENCE_RETURN_TO_APP_HIDE_TOOLTIP
: IDS_ASH_VIDEO_CONFERENCE_RETURN_TO_APP_SHOW_TOOLTIP;
expand_button_->SetTooltipText(l10n_util::GetStringUTF16(tooltip_text_id));
expand_indicator_->SetTooltipText(l10n_util::GetStringUTF16(tooltip_text_id));
}

// -----------------------------------------------------------------------------
Expand Down Expand Up @@ -292,7 +283,7 @@ void ReturnToAppPanel::AddButtonsToPanel(MediaApps apps) {
/*is_top_row=*/true, app->id, app->is_capturing_camera,
app->is_capturing_microphone, app->is_capturing_screen,
GetMediaAppDisplayText(app));
app_button->expand_button()->SetVisible(false);
app_button->expand_indicator()->SetVisible(false);
container_view_->AddChildView(std::move(app_button));

return;
Expand Down
18 changes: 7 additions & 11 deletions ash/system/video_conference/bubble/return_to_app_panel.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,8 @@ namespace base {
class UnguessableToken;
} // namespace base

namespace ui {
class Event;
} // namespace ui

namespace views {
class ImageButton;
class ImageView;
class Label;
class View;
} // namespace views
Expand Down Expand Up @@ -75,13 +71,13 @@ class ASH_EXPORT ReturnToAppButton : public views::Button {

views::Label* label() { return label_; }
views::View* icons_container() { return icons_container_; }
views::ImageButton* expand_button() { return expand_button_; }
views::ImageView* expand_indicator() { return expand_indicator_; }

private:
FRIEND_TEST_ALL_PREFIXES(ReturnToAppPanelTest, ExpandCollapse);

// Callback for `expand_button_`.
void OnExpandButtonToggled(const ui::Event& event);
// Callback for the button.
void OnButtonClicked(const base::UnguessableToken& id);

// Indicates if the running app is using camera, microphone, or screen
// sharing.
Expand Down Expand Up @@ -109,9 +105,9 @@ class ASH_EXPORT ReturnToAppButton : public views::Button {
// capturing of the media app.
views::View* icons_container_ = nullptr;

// The button to toggle expand/collapse the panel. Only available if the
// button is in the top row.
views::ImageButton* expand_button_ = nullptr;
// The indicator showing if the panel is in expanded or collapsed state. Only
// available if the button is in the top row.
views::ImageView* expand_indicator_ = nullptr;

base::WeakPtrFactory<ReturnToAppButton> weak_ptr_factory_{this};
};
Expand Down
23 changes: 11 additions & 12 deletions ash/system/video_conference/bubble/return_to_app_panel_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@
#include "base/unguessable_token.h"
#include "chromeos/crosapi/mojom/video_conference.mojom.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/events/test/test_event.h"
#include "ui/views/controls/button/image_button.h"
#include "ui/views/controls/image_view.h"
#include "ui/views/controls/label.h"

namespace {
Expand Down Expand Up @@ -149,7 +148,7 @@ TEST_F(ReturnToAppPanelTest, OneApp) {

auto* app_button = static_cast<ReturnToAppButton*>(
return_to_app_container->children().front());
EXPECT_FALSE(app_button->expand_button()->GetVisible());
EXPECT_FALSE(app_button->expand_indicator()->GetVisible());
VerifyReturnToAppButtonInfo(app_button, is_capturing_camera,
is_capturing_microphone, is_capturing_screen,
kExpectedGoogleMeetDisplayedUrl);
Expand Down Expand Up @@ -214,7 +213,7 @@ TEST_F(ReturnToAppPanelTest, ExpandCollapse) {
auto* return_to_app_container = GetReturnToAppContainer(panel.get());
auto* summary_row = static_cast<ReturnToAppButton*>(
return_to_app_container->children().front());
EXPECT_TRUE(summary_row->expand_button()->GetVisible());
EXPECT_TRUE(summary_row->expand_indicator()->GetVisible());

auto* first_app_row =
static_cast<ReturnToAppButton*>(return_to_app_container->children()[1]);
Expand All @@ -228,24 +227,24 @@ TEST_F(ReturnToAppPanelTest, ExpandCollapse) {
EXPECT_TRUE(summary_row->icons_container()->GetVisible());
EXPECT_EQ(l10n_util::GetStringUTF16(
IDS_ASH_VIDEO_CONFERENCE_RETURN_TO_APP_SHOW_TOOLTIP),
summary_row->expand_button()->GetTooltipText());
summary_row->expand_indicator()->GetTooltipText());
EXPECT_FALSE(first_app_row->GetVisible());
EXPECT_FALSE(second_app_row->GetVisible());

// Clicking the expand button should expand the panel.
summary_row->OnExpandButtonToggled(ui::test::TestEvent());
// Clicking the summary row should expand the panel.
summary_row->OnButtonClicked(/*id=*/base::UnguessableToken::Null());
EXPECT_TRUE(summary_row->expanded());

// Verify the views in expanded state:
EXPECT_FALSE(summary_row->icons_container()->GetVisible());
EXPECT_EQ(l10n_util::GetStringUTF16(
IDS_ASH_VIDEO_CONFERENCE_RETURN_TO_APP_HIDE_TOOLTIP),
summary_row->expand_button()->GetTooltipText());
summary_row->expand_indicator()->GetTooltipText());
EXPECT_TRUE(first_app_row->GetVisible());
EXPECT_TRUE(second_app_row->GetVisible());

// Click again. Should be in collapsed state.
summary_row->OnExpandButtonToggled(ui::test::TestEvent());
summary_row->OnButtonClicked(/*id=*/base::UnguessableToken::Null());
EXPECT_FALSE(summary_row->expanded());
}

Expand Down Expand Up @@ -314,13 +313,13 @@ TEST_F(ReturnToAppPanelTest, ReturnToApp) {
auto* second_app_row =
static_cast<ReturnToAppButton*>(return_to_app_container->children()[2]);

// Clicking on the summary row should not launch any apps.
// Clicking on the summary row should not launch any apps (it switched the
// panel to expanded state).
LeftClickOn(summary_row);
ASSERT_TRUE(summary_row->expanded());
EXPECT_FALSE(controller()->app_to_launch_state_[app_id1]);
EXPECT_FALSE(controller()->app_to_launch_state_[app_id2]);

LeftClickOn(summary_row->expand_button());

// Clicking each row should open the corresponding app.
LeftClickOn(first_app_row);
EXPECT_TRUE(controller()->app_to_launch_state_[app_id1]);
Expand Down

0 comments on commit 9655a1c

Please sign in to comment.