Skip to content

Add "Copy to clipboard" button to screenshot preview #39431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
sangwoo108 wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add "Copy to clipboard" button to screenshot preview #39431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions app/brave_generated_resources.grd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,9 @@ Or change later at <ph name="SETTINGS_EXTENIONS_LINK">$2<ex>brave://settings/ext
<message name="IDS_BRAVE_SCREENSHOT_PREVIEW_DIALOG_DOWNLOAD_BUTTON" desc="Label of the button in the screenshot preview dialog that proceeds to save the screenshot">
Download
</message>
<message name="IDS_BRAVE_SCREENSHOT_PREVIEW_DIALOG_COPY_BUTTON" desc="Label of the button in the screenshot preview dialog that copies the screenshot to clipboard">
Copy to clipboard
</message>

<!-- Extensions page strings -->
<message name="IDS_EXTENSIONS_BRAVE_ITEM_SOURCE_WEBSTORE" desc="The text to indicate that an extension is from the Web Extensions Store.">
Expand Down
23 changes: 20 additions & 3 deletions browser/ui/screenshot/screenshot_controller.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "third_party/skia/include/core/SkColor.h"
#include "third_party/skia/include/core/SkImage.h"
#include "third_party/skia/include/core/SkRect.h"
#include "ui/base/clipboard/scoped_clipboard_writer.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/gfx/codec/png_codec.h"
#include "ui/gfx/image/image.h"
Expand Down Expand Up @@ -339,17 +340,33 @@ void ScreenshotController::OnEncoded(std::optional<std::vector<uint8_t>> png) {

void ScreenshotController::ShowPreviewDialog(std::vector<uint8_t> png) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
// The dialog takes ownership of `png` for display and hands the same bytes
// back to ShowSaveDialog() via `on_download` if the user confirms, so
// there's never a need for a second copy here.
// The dialog takes ownership of `png` for display.
// `on_download`: user clicked Download -> proceed to ShowSaveDialog()
// `on_copy`: user clicked Copy -> copy to clipboard and finish
// `on_cancel`: user closed the dialog -> finish with error
preview_dialog_shower_.Run(
parent_window_getter_.Run(), std::move(png),
base::BindOnce(&ScreenshotController::ShowSaveDialog,
weak_factory_.GetWeakPtr()),
base::BindOnce(&ScreenshotController::CopyToClipboard,
weak_factory_.GetWeakPtr()),
base::BindOnce(&ScreenshotController::FinishWithError,
weak_factory_.GetWeakPtr(), Error::kUserCancelled));
}

void ScreenshotController::CopyToClipboard(std::vector<uint8_t> png) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
SkBitmap bitmap = gfx::PNGCodec::Decode(png);
ui::ScopedClipboardWriter clipboard_writer(ui::ClipboardBuffer::kCopyPaste);
clipboard_writer.WriteImage(bitmap);
auto cb = std::move(pending_callback_);
Reset();
if (cb) {
std::move(cb).Run(base::FilePath()); // No path to return for clipboard
// copy, but still signal success.
}
}

void ScreenshotController::ShowSaveDialog(std::vector<uint8_t> png) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
pending_png_ = std::move(png);
Expand Down
12 changes: 8 additions & 4 deletions browser/ui/screenshot/screenshot_controller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,14 @@ class ScreenshotController : public ui::SelectFileDialog::Listener {
using NativeWindowGetter = base::RepeatingCallback<gfx::NativeWindow()>;

// Shows a preview of the captured `png` and asks the user to confirm
// before saving. Exactly one of `on_download` (with `png` handed back) or
// `on_cancel` is run once, depending on whether the user confirms or
// dismisses the dialog.
// before saving. Exactly one of `on_download` (with `png` handed back),
// `on_copy`, or `on_cancel` is run once, depending on whether the user
// confirms (download), copies to clipboard, or dismisses the dialog.
using PreviewDialogShower = base::RepeatingCallback<void(
gfx::NativeWindow parent,
std::vector<uint8_t> png,
base::OnceCallback<void(std::vector<uint8_t>)> on_download,
base::OnceCallback<void(std::vector<uint8_t>)> on_copy,
base::OnceClosure on_cancel)>;

ScreenshotController(content::BrowserContext* profile,
Expand Down Expand Up @@ -121,10 +122,13 @@ class ScreenshotController : public ui::SelectFileDialog::Listener {

void OnEncoded(std::optional<std::vector<uint8_t>> png);
// Shows the preview dialog for `png`; proceeds to ShowSaveDialog() if the
// user clicks Download, or finishes with kUserCancelled otherwise.
// user clicks Download, copies to clipboard via OnCopiedToClipboard() if
// they click Copy, or finishes with kUserCancelled otherwise.
void ShowPreviewDialog(std::vector<uint8_t> png);
void ShowSaveDialog(std::vector<uint8_t> png);
void ShowSaveDialogWithPath(const base::FilePath& default_path);
void CopyToClipboard(std::vector<uint8_t> png);

// Reply callback for WritePngFile posted from FileSelected().
void OnFileWritten(const base::FilePath& path, bool ok);
void FinishWithError(Error error);
Expand Down
42 changes: 40 additions & 2 deletions browser/ui/screenshot/screenshot_controller_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class ScreenshotControllerTest : public ChromeRenderViewHostTestHarness {
gfx::NativeWindow parent,
std::vector<uint8_t> png,
base::OnceCallback<void(std::vector<uint8_t>)> on_download,
base::OnceCallback<void(std::vector<uint8_t>)> on_copy,
base::OnceClosure on_cancel) {
std::move(on_download).Run(std::move(png));
}
Expand Down Expand Up @@ -231,8 +232,9 @@ TEST_F(ScreenshotControllerTest,
base::test::TestFuture<void> preview_shown;
base::OnceClosure captured_cancel;
auto shower = base::BindLambdaForTesting(
[&](gfx::NativeWindow parent, std::vector<uint8_t> png,
base::OnceCallback<void(std::vector<uint8_t>)> on_download,
[&](gfx::NativeWindow, std::vector<uint8_t>,
base::OnceCallback<void(std::vector<uint8_t>)>,
base::OnceCallback<void(std::vector<uint8_t>)>,
base::OnceClosure on_cancel) {
captured_cancel = std::move(on_cancel);
preview_shown.SetValue();
Expand All @@ -254,6 +256,42 @@ TEST_F(ScreenshotControllerTest,
EXPECT_FALSE(dialog_factory_->GetLastDialog());
}

TEST_F(ScreenshotControllerTest,
PreviewDialog_UserCopies_ReturnsSuccessWithoutSaveDialog) {
base::test::TestFuture<void> preview_shown;
std::vector<uint8_t> captured_png;
base::OnceCallback<void(std::vector<uint8_t>)> captured_copy;
auto shower = base::BindLambdaForTesting(
[&](gfx::NativeWindow, std::vector<uint8_t> png,
base::OnceCallback<void(std::vector<uint8_t>)>,
base::OnceCallback<void(std::vector<uint8_t>)> on_copy,
base::OnceClosure) {
captured_png = std::move(png);
captured_copy = std::move(on_copy);
preview_shown.SetValue();
});

auto controller = std::make_unique<ScreenshotController>(
profile(), base::BindRepeating([]() { return gfx::NativeWindow(); }),
shower);
controller->set_download_dir_for_testing(temp_dir_.GetPath());

SkBitmap bitmap = MakeSolidBitmap(64, 64, SK_ColorBLUE);

base::test::TestFuture<Result> future;
InjectBitmapInto(controller.get(), std::move(bitmap), future.GetCallback());

ASSERT_TRUE(preview_shown.Wait());
ASSERT_FALSE(captured_png.empty());
ASSERT_FALSE(captured_copy.is_null());
std::move(captured_copy).Run(std::move(captured_png));

Result result = future.Get();
ASSERT_TRUE(result.has_value());
EXPECT_TRUE(result.value().empty());
EXPECT_FALSE(dialog_factory_->GetLastDialog());
}

TEST_F(ScreenshotControllerTest,
Pipeline_UserCancelsDialog_ReturnsUserCancelled) {
// Signal when the fake dialog is opened so we can cancel it from outside
Expand Down
24 changes: 18 additions & 6 deletions browser/ui/views/toolbar/screenshot_preview_dialog.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "brave/grit/brave_generated_resources.h"
#include "chrome/browser/platform_util.h"
#include "third_party/skia/include/core/SkBitmap.h"
#include "ui/base/clipboard/scoped_clipboard_writer.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/base/models/image_model.h"
#include "ui/base/mojom/ui_base_types.mojom-shared.h"
Expand Down Expand Up @@ -72,13 +73,17 @@ class ScreenshotPreviewDialogDelegate : public views::DialogDelegate {
SetModalType(ui::mojom::ModalType::kWindow);
SetTitle(
l10n_util::GetStringUTF16(IDS_BRAVE_SCREENSHOT_PREVIEW_DIALOG_TITLE));
// Only the Download action is a button; dismissal happens via the frame's
// close (X) control or Esc, both surfaced as a non-accept ClosedReason to
// ScreenshotPreviewDialogHolder::OnClosed() below.
SetButtons(static_cast<int>(ui::mojom::DialogButton::kOk));
// Add both Download and Copy to clipboard buttons.
// Download button is mapped to the OK button, Copy to clipboard is mapped
// to the Cancel button.
SetButtons(static_cast<int>(ui::mojom::DialogButton::kOk) |
static_cast<int>(ui::mojom::DialogButton::kCancel));
SetButtonLabel(ui::mojom::DialogButton::kOk,
l10n_util::GetStringUTF16(
IDS_BRAVE_SCREENSHOT_PREVIEW_DIALOG_DOWNLOAD_BUTTON));
SetButtonLabel(ui::mojom::DialogButton::kCancel,
l10n_util::GetStringUTF16(
IDS_BRAVE_SCREENSHOT_PREVIEW_DIALOG_COPY_BUTTON));
SetShowCloseButton(true);

// Same content margins Chrome's own screenshot-captured bubble uses
Expand All @@ -104,10 +109,12 @@ class ScreenshotPreviewDialogHolder {
gfx::NativeWindow parent,
std::vector<uint8_t> png,
base::OnceCallback<void(std::vector<uint8_t>)> on_download,
base::OnceCallback<void(std::vector<uint8_t>)> on_copy,
base::OnceClosure on_cancel)
: delegate_(
std::make_unique<ScreenshotPreviewDialogDelegate>(std::move(png))),
on_download_(std::move(on_download)),
on_copy_(std::move(on_copy)),
on_cancel_(std::move(on_cancel)) {
widget_.reset(views::DialogDelegate::CreateDialogWidget(
delegate_.get(), gfx::NativeWindow(),
Expand All @@ -125,6 +132,8 @@ class ScreenshotPreviewDialogHolder {
widget_.reset();
if (reason == views::Widget::ClosedReason::kAcceptButtonClicked) {
std::move(on_download_).Run(delegate_->TakePng());
} else if (reason == views::Widget::ClosedReason::kCancelButtonClicked) {
std::move(on_copy_).Run(delegate_->TakePng());
} else {
std::move(on_cancel_).Run();
}
Expand All @@ -134,6 +143,7 @@ class ScreenshotPreviewDialogHolder {
std::unique_ptr<ScreenshotPreviewDialogDelegate> delegate_;
std::unique_ptr<views::Widget> widget_;
base::OnceCallback<void(std::vector<uint8_t>)> on_download_;
base::OnceCallback<void(std::vector<uint8_t>)> on_copy_;
base::OnceClosure on_cancel_;
};

Expand All @@ -143,9 +153,11 @@ void ShowScreenshotPreviewDialog(
gfx::NativeWindow parent,
std::vector<uint8_t> png,
base::OnceCallback<void(std::vector<uint8_t>)> on_download,
base::OnceCallback<void(std::vector<uint8_t>)> on_copy,
base::OnceClosure on_cancel) {
new ScreenshotPreviewDialogHolder(
parent, std::move(png), std::move(on_download), std::move(on_cancel));
new ScreenshotPreviewDialogHolder(parent, std::move(png),
std::move(on_download), std::move(on_copy),
std::move(on_cancel));
}

} // namespace screenshot
6 changes: 4 additions & 2 deletions browser/ui/views/toolbar/screenshot_preview_dialog.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@ namespace screenshot {
// Shows a modal dialog previewing the captured `png` in a scroll view,
// taking ownership of it for the dialog's lifetime. Invokes `on_download`
// with `png` handed back if the user clicks the Download button, or
// `on_cancel` if they dismiss the dialog (Esc or closing the window).
// Exactly one of the two is run, exactly once.
// `on_copy` if they click the Copy to clipboard button, or `on_cancel`
// if they dismiss the dialog (Esc or closing the window).
// Exactly one of the three is run, exactly once.
void ShowScreenshotPreviewDialog(
gfx::NativeWindow parent,
std::vector<uint8_t> png,
base::OnceCallback<void(std::vector<uint8_t>)> on_download,
base::OnceCallback<void(std::vector<uint8_t>)> on_copy,
base::OnceClosure on_cancel);

} // namespace screenshot
Expand Down
12 changes: 9 additions & 3 deletions browser/ui/views/toolbar/screenshot_preview_dialog_browsertest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,12 @@ IN_PROC_BROWSER_TEST_F(ScreenshotPreviewDialogBrowserTest,
DownloadButton_RunsOnDownloadAndClosesWidget) {
PreviewWidgetWaiter widget_waiter;
base::test::TestFuture<std::vector<uint8_t>> on_download;
base::test::TestFuture<std::vector<uint8_t>> on_copy;
base::test::TestFuture<void> on_cancel;

std::vector<uint8_t> png = MakeTestPng();
ShowScreenshotPreviewDialog(browser()->GetWindow()->GetNativeWindow(), png,
on_download.GetCallback(),
on_download.GetCallback(), on_copy.GetCallback(),
on_cancel.GetCallback());

views::Widget* widget = widget_waiter.Wait();
Expand All @@ -120,6 +121,7 @@ IN_PROC_BROWSER_TEST_F(ScreenshotPreviewDialogBrowserTest,
widget->widget_delegate()->AsDialogDelegate()->AcceptDialog();

EXPECT_EQ(on_download.Get(), png);
EXPECT_FALSE(on_copy.IsReady());
EXPECT_FALSE(on_cancel.IsReady());
destruction_waiter.Wait();
}
Expand All @@ -128,11 +130,12 @@ IN_PROC_BROWSER_TEST_F(ScreenshotPreviewDialogBrowserTest,
EscapeKey_RunsOnCancelAndClosesWidget) {
PreviewWidgetWaiter widget_waiter;
base::test::TestFuture<std::vector<uint8_t>> on_download;
base::test::TestFuture<std::vector<uint8_t>> on_copy;
base::test::TestFuture<void> on_cancel;

ShowScreenshotPreviewDialog(browser()->GetWindow()->GetNativeWindow(),
MakeTestPng(), on_download.GetCallback(),
on_cancel.GetCallback());
on_copy.GetCallback(), on_cancel.GetCallback());

views::Widget* widget = widget_waiter.Wait();
ASSERT_TRUE(widget);
Expand All @@ -142,18 +145,20 @@ IN_PROC_BROWSER_TEST_F(ScreenshotPreviewDialogBrowserTest,

EXPECT_TRUE(on_cancel.Wait());
EXPECT_FALSE(on_download.IsReady());
EXPECT_FALSE(on_copy.IsReady());
destruction_waiter.Wait();
}

IN_PROC_BROWSER_TEST_F(ScreenshotPreviewDialogBrowserTest,
CloseButton_RunsOnCancelAndClosesWidget) {
PreviewWidgetWaiter widget_waiter;
base::test::TestFuture<std::vector<uint8_t>> on_download;
base::test::TestFuture<std::vector<uint8_t>> on_copy;
base::test::TestFuture<void> on_cancel;

ShowScreenshotPreviewDialog(browser()->GetWindow()->GetNativeWindow(),
MakeTestPng(), on_download.GetCallback(),
on_cancel.GetCallback());
on_copy.GetCallback(), on_cancel.GetCallback());

views::Widget* widget = widget_waiter.Wait();
ASSERT_TRUE(widget);
Expand All @@ -163,6 +168,7 @@ IN_PROC_BROWSER_TEST_F(ScreenshotPreviewDialogBrowserTest,

EXPECT_TRUE(on_cancel.Wait());
EXPECT_FALSE(on_download.IsReady());
EXPECT_FALSE(on_copy.IsReady());
destruction_waiter.Wait();
}

Expand Down
Loading