views::Label: Implement context menu, keyboard shortcuts for copy/select all.

ui/views/controls/label_unittest.cc

Index: ui/views/controls/label_unittest.cc
diff --git a/ui/views/controls/label_unittest.cc b/ui/views/controls/label_unittest.cc
index 9b15776ff293460cbaeabda9725b715f14a501c8..98d06c772c4d71c0657142a444b4b376e7658571 100644
--- a/ui/views/controls/label_unittest.cc
+++ b/ui/views/controls/label_unittest.cc
@@ -8,6 +8,7 @@
 
 #include "base/command_line.h"
 #include "base/i18n/rtl.h"
+#include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -16,9 +17,11 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/compositor/canvas_painter.h"
 #include "ui/events/base_event_utils.h"
+#include "ui/events/test/event_generator.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/render_text.h"
 #include "ui/gfx/switches.h"
+#include "ui/strings/grit/ui_strings.h"
 #include "ui/views/border.h"
 #include "ui/views/test/focus_manager_test.h"
 #include "ui/views/test/views_test_base.h"
@@ -30,6 +33,63 @@ using base::ASCIIToUTF16;
 
 namespace views {
 
+namespace {
+
+#if defined(OS_MACOSX)
+const int kControlCommandModifier = ui::EF_COMMAND_DOWN;
+#else
+const int kControlCommandModifier = ui::EF_CONTROL_DOWN;
+#endif
+
+// All text sizing measurements (width and height) should be greater than this.
+const int kMinTextDimension = 4;
+
+class TestLabel : public Label {
+ public:
+  TestLabel() : Label(ASCIIToUTF16("TestLabel")) { SizeToPreferredSize(); }
+
+  int schedule_paint_count() const { return schedule_paint_count_; }
+
+  void SimulatePaint() {
+    gfx::Canvas canvas(bounds().size(), 1.0, false /* is_opaque */);
+    Paint(ui::CanvasPainter(&canvas, 1.f).context());
+  }
+
+  // View:
+  void SchedulePaintInRect(const gfx::Rect& r) override {
+    ++schedule_paint_count_;
+    Label::SchedulePaintInRect(r);
+  }
+
+ private:
+  int schedule_paint_count_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(TestLabel);
+};
+
+// A test utility function to set the application default text direction.
+void SetRTL(bool rtl) {
+  // Override the current locale/direction.
+  base::i18n::SetICUDefaultLocale(rtl ? "he" : "en");
+  EXPECT_EQ(rtl, base::i18n::IsRTL());
+}
+
+// Returns true if |current| is bigger than |last|. Sets |last| to |current|.
+bool Increased(int current, int* last) {
+  bool increased = current > *last;
+  *last = current;
+  return increased;
+}
+
+base::string16 GetClipboardText(ui::ClipboardType clipboard_type) {
+  base::string16 clipboard_text;
+  ui::Clipboard::GetForCurrentThread()->ReadText(clipboard_type,
+                                                 &clipboard_text);
+  return clipboard_text;
+}
+
+}  // namespace
+
 class LabelTest : public ViewsTestBase {
  public:
   LabelTest() {}
@@ -85,6 +145,8 @@ class LabelSelectionTest : public LabelTest {
         switches::kEnableHarfBuzzRenderText);
 #endif
     LabelTest::SetUp();
+    event_generator_ =

[karandeepb, 2016/11/15 10:54:32]

Since we are now using EventGenerator anyway, if it's not too cumbersome, I'll
try and use it for mouse events as well in a follow-up, as you had recommended
earlier.

[msw, 2016/11/15 20:06:45]

Acknowledged.

+        base::MakeUnique<ui::test::EventGenerator>(widget()->GetNativeWindow());
   }
 
  protected:
@@ -125,67 +187,18 @@ class LabelSelectionTest : public LabelTest {
         .origin();
   }
 
-  base::string16 GetSelectedText() {
-    const gfx::RenderText* render_text =
-        label()->GetRenderTextForSelectionController();
-    return render_text->GetTextFromRange(render_text->selection());
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(LabelSelectionTest);
-};
-
-namespace {
-
-// All text sizing measurements (width and height) should be greater than this.
-const int kMinTextDimension = 4;
-
-class TestLabel : public Label {
- public:
-  TestLabel() : Label(ASCIIToUTF16("TestLabel")) { SizeToPreferredSize(); }
-
-  int schedule_paint_count() const { return schedule_paint_count_; }
-
-  void SimulatePaint() {
-    gfx::Canvas canvas(bounds().size(), 1.0, false /* is_opaque */);
-    Paint(ui::CanvasPainter(&canvas, 1.f).context());
-  }
+  base::string16 GetSelectedText() { return label()->GetSelectedText(); }
 
-  // View:
-  void SchedulePaintInRect(const gfx::Rect& r) override {
-    ++schedule_paint_count_;
-    Label::SchedulePaintInRect(r);
-  }
+  ui::test::EventGenerator* event_generator() {
+    return event_generator_.get();
+  };

[msw, 2016/11/15 20:06:45]

nit: no trailing semi

[karandeepb, 2016/11/16 07:48:29]

Done.

 
  private:
-  int schedule_paint_count_ = 0;
+  std::unique_ptr<ui::test::EventGenerator> event_generator_;
 
-  DISALLOW_COPY_AND_ASSIGN(TestLabel);
+  DISALLOW_COPY_AND_ASSIGN(LabelSelectionTest);
 };
 
-// A test utility function to set the application default text direction.
-void SetRTL(bool rtl) {
-  // Override the current locale/direction.
-  base::i18n::SetICUDefaultLocale(rtl ? "he" : "en");
-  EXPECT_EQ(rtl, base::i18n::IsRTL());
-}
-
-// Returns true if |current| is bigger than |last|. Sets |last| to |current|.
-bool Increased(int current, int* last) {
-  bool increased = current > *last;
-  *last = current;
-  return increased;
-}
-
-base::string16 GetSelectionClipboardText() {
-  base::string16 selection_clipboard_text;
-  ui::Clipboard::GetForCurrentThread()->ReadText(ui::CLIPBOARD_TYPE_SELECTION,
-                                                 &selection_clipboard_text);
-  return selection_clipboard_text;
-}
-
-}  // namespace
-
 // Crashes on Linux only. http://crbug.com/612406
 #if defined(OS_LINUX)
 #define MAYBE_FontPropertySymbol DISABLED_FontPropertySymbol
@@ -900,6 +913,9 @@ TEST_F(LabelSelectionTest, MouseDrag) {
   PerformMouseDragTo(gfx::Point(200, 0));
   PerformMouseRelease(gfx::Point(200, 0));
   EXPECT_STR_EQ(" mouse drag", GetSelectedText());
+
+  event_generator()->PressKey(ui::VKEY_C, kControlCommandModifier);
+  EXPECT_STR_EQ(" mouse drag", GetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE));

[msw, 2016/11/15 20:06:45]

Avoid testing the system clipboard in unit tests. We can only reliably test the
contents of an actual clipboard in interactive_ui_tests (otherwise tests running
in parallel may modify the clipboard during this test execution). You might be
able to side-step this with something like TestClipboard... I'm not sure.

[tapted, 2016/11/16 02:45:19]

Yah - there is http://crbug.com/623442 about this . There are current flakes
that are able to reliably recover on retries, but adding more could trip the
balance.

[karandeepb, 2016/11/16 07:48:29]

Have created https://codereview.chromium.org/2499303003/ to fix the flaky
clipboard behavior in views_unittests using TestClipboard and rebased this on
top of it.

 }
 
 // Verify the initially selected word on a double click, remains selected on
@@ -933,15 +949,81 @@ TEST_F(LabelSelectionTest, DISABLED_SelectionClipboard) {
   // selection clipboard.
   label()->SelectRange(gfx::Range(2, 5));
   EXPECT_STR_EQ("bel", GetSelectedText());
-  EXPECT_TRUE(GetSelectionClipboardText().empty());
+  EXPECT_TRUE(GetClipboardText(ui::CLIPBOARD_TYPE_SELECTION).empty());
 
   // Verify text selection using the mouse updates the selection clipboard.
   PerformMousePress(GetCursorPoint(5));
   PerformMouseDragTo(gfx::Point());
   PerformMouseRelease(gfx::Point());
   EXPECT_STR_EQ("Label", GetSelectedText());
-  EXPECT_STR_EQ("Label", GetSelectionClipboardText());
+  EXPECT_STR_EQ("Label", GetClipboardText(ui::CLIPBOARD_TYPE_SELECTION));
 }
 #endif
 
+// Verify that keyboard shortcuts for Copy and Select All work when a selectable
+// label is focused.
+TEST_F(LabelSelectionTest, KeyboardActions) {
+  const base::string16 initial_text = ASCIIToUTF16("Label keyboard actions");
+  label()->SetText(initial_text);
+  label()->SizeToPreferredSize();
+  ASSERT_TRUE(label()->SetSelectable(true));
+
+  PerformClick(gfx::Point());
+  EXPECT_EQ(label(), GetFocusedView());
+
+  event_generator()->PressKey(ui::VKEY_A, kControlCommandModifier);
+  EXPECT_EQ(initial_text, GetSelectedText());
+
+  event_generator()->PressKey(ui::VKEY_C, kControlCommandModifier);
+  EXPECT_EQ(initial_text, GetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE));

[msw, 2016/11/15 20:06:45]

You'll need to modify/disable this test for the same clipboard concerns above.
I'd encourage you to (1) fix and re-enable these tests in place using something
like TestClipboard, (2) move these tests to interactive_ui_tests and re-enable
them, or (3) split out the clipboard-testing to separate fixtures and disable
those for now, citing a bug; leave the non-clipboard stuff enabled.

[karandeepb, 2016/11/16 07:48:29]

Have created https://codereview.chromium.org/2499303003/ to fix the flaky
clipboard behavior in views_unittests using TestClipboard and rebased this on
top of it.

+
+  // The selection should get cleared on changing the text, but focus should not
+  // be affected.
+  const base::string16 new_text = ASCIIToUTF16("Label obscured text");
+  label()->SetText(new_text);
+  EXPECT_FALSE(label()->HasSelection());
+  EXPECT_EQ(label(), GetFocusedView());
+
+  label()->SetObscured(true);
+  event_generator()->PressKey(ui::VKEY_A, kControlCommandModifier);
+  EXPECT_EQ(new_text, GetSelectedText());
+
+  // Since the text is obscured, clipboard text should not change.
+  event_generator()->PressKey(ui::VKEY_C, kControlCommandModifier);
+  EXPECT_EQ(initial_text, GetClipboardText(ui::CLIPBOARD_TYPE_COPY_PASTE));
+}
+
+// Verify the context menu options are enabled and disabled appropriately.
+TEST_F(LabelSelectionTest, ContextMenuContents) {
+  label()->SetText(ASCIIToUTF16("Label context menu"));
+  label()->SizeToPreferredSize();
+
+  // A non-selectable label would not show a context menu and both COPY and
+  // SELECT_ALL context menu items should be disabled for it.
+  EXPECT_FALSE(label()->IsCommandIdEnabled(IDS_APP_COPY));
+  EXPECT_FALSE(label()->IsCommandIdEnabled(IDS_APP_SELECT_ALL));
+
+  // For a selectable label with no selection, only SELECT_ALL should be
+  // enabled.
+  ASSERT_TRUE(label()->SetSelectable(true));
+  EXPECT_FALSE(label()->IsCommandIdEnabled(IDS_APP_COPY));
+  EXPECT_TRUE(label()->IsCommandIdEnabled(IDS_APP_SELECT_ALL));
+
+  // For a selectable label with a selection, both COPY and SELECT_ALL should be

[msw, 2016/11/15 20:06:45]

nit: also check that some unhandled command (eg. paste) is still not enabled.

[karandeepb, 2016/11/16 07:48:29]

Done.

+  // enabled.
+  label()->SelectRange(gfx::Range(0, 4));
+  EXPECT_TRUE(label()->IsCommandIdEnabled(IDS_APP_COPY));
+  EXPECT_TRUE(label()->IsCommandIdEnabled(IDS_APP_SELECT_ALL));
+
+  // For an obscured selectable label, only SELECT_ALL should be enabled.

[msw, 2016/11/15 20:06:45]

I wonder if we should disable select-all (or even more? make the context menu
not appear? disabling selectable?) for obscured labels. What's the point of
selecting the obscured text if you can't do anything with it?

[karandeepb, 2016/11/16 07:48:29]

Yeah I agree making obscured labels selectable does not make much sense. Have
disabled text selection for obscured labels.

+  label()->SetObscured(true);
+  EXPECT_FALSE(label()->IsCommandIdEnabled(IDS_APP_COPY));
+  EXPECT_TRUE(label()->IsCommandIdEnabled(IDS_APP_SELECT_ALL));
+
+  // For an empty label, both COPY and SELECT_ALL should be disabled.
+  label()->SetText(base::string16());
+  EXPECT_FALSE(label()->IsCommandIdEnabled(IDS_APP_COPY));
+  EXPECT_FALSE(label()->IsCommandIdEnabled(IDS_APP_SELECT_ALL));
+}
+
 }  // namespace views