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

ui/views/controls/label.cc

Index: ui/views/controls/label.cc
diff --git a/ui/views/controls/label.cc b/ui/views/controls/label.cc
index 3533ea6f267d18a52a57a5c26999d270c3fd088b..19484ace6cde19ad0ba15ef3af19a2a845c5ce93 100644
--- a/ui/views/controls/label.cc
+++ b/ui/views/controls/label.cc
@@ -29,6 +29,8 @@
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/text_elider.h"
 #include "ui/native_theme/native_theme.h"
+#include "ui/strings/grit/ui_strings.h"
+#include "ui/views/controls/menu/menu_runner.h"
 #include "ui/views/focus/focus_manager.h"
 #include "ui/views/native_cursor.h"
 #include "ui/views/selection_controller.h"
@@ -544,6 +546,58 @@ void Label::OnMouseCaptureLost() {
   selection_controller_->OnMouseCaptureLost();
 }
 
+bool Label::AcceleratorPressed(const ui::Accelerator& accelerator) {
+  // The only accelerator a Label needs to handle is the Copy command from the

[msw, 2016/11/09 18:32:52]

Not CTRL+A for select all?

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

My understanding of accelerators is that they are to be used when a view wants
to perform some action on a key event even if it's not focused (hence generally
to be used for keyboard shortcuts). 

The only reason for handling Ctrl+C as an accelerator in the Label class is to
get Copy from the main Chrome menu (I think it has a specific name but not sure,
it's the one invoked from the button with three dots) working. See also the
Textfield class, where we add accelerators for Cut/Copy/Paste, again so that
these actions from the Chrome menu are handled. See
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi....
 for more details. Since SelectAll is not an action in that menu, it does not
need to be handled via an accelerator.

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

Acknowledged, thanks for the explanation. 'Chrome menu' is what i say.

+  // Chrome menu.
+  if(accelerator.key_code() == ui::VKEY_C && accelerator.IsCtrlDown()) {

[msw, 2016/11/09 18:32:52]

nit: space after if, git cl format

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

Done.

+    CopyToClipboard();
+    return true;
+  }
+  return false;
+}
+
+bool Label::CanHandleAccelerators() const {
+  // See related comment in BrowserView::CutCopyPaste.

[msw, 2016/11/09 18:32:53]

nit: please explain a bit more here; I'm not sure what I'm supposed to be
getting from that function's comment. (eg. should that explain why the returned
value depends on focus, or something?)

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

Done. I was referring to the following comment in the function- 
  // Any Views which want to handle the clipboard commands in the Chrome menu
  // should:
  //   (a) Register ctrl-X/C/V as accelerators
  //   (b) Implement CanHandleAccelerators() to not return true unless they're
  //       focused, as the FocusManager will try all registered accelerator
  //       handlers, not just the focused one.
  // Currently, Textfield (which covers the omnibox and find bar, and likely any
  // other native UI in the future that wants to deal with clipboard commands)
  // does the above.

+  return HasFocus() && GetRenderTextForSelectionController() &&
+         View::CanHandleAccelerators();
+}
+
+bool Label::OnKeyPressed(const ui::KeyEvent& event) {

[msw, 2016/11/09 18:32:52]

Should this use Textfield's GetCommandForKeyEvent and then just filter the
supported commands? I worry about forked/duplicated logic drifting that should
probably be consistent.

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

Currently the TextEditCommand enum is only being used by TextInputClient and
subclasses.
If you think it's better, I can move Textfield's GetCommandForKeyEvent into a
separate file as a free function. Maybe a file called text_utils.h in ui/views.

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

Hmm, this is okay for now; worth considering some cleanup later.
(maybe Label and Textfield will share quite a bit more code someday)

+  if (!GetRenderTextForSelectionController())
+    return false;
+
+  const bool shift = event.IsShiftDown();
+  const bool control = event.IsControlDown();
+  const bool alt = event.IsAltDown() || event.IsAltGrDown();
+
+  switch (event.key_code()) {
+    case ui::VKEY_C:
+      if (control && !alt && HasSelection()) {

[msw, 2016/11/09 18:32:53]

nit && !obscured()? here and below?

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

But shouldn't we consume the event (by returning true) when the Label has an
active selection but has obscured text. Otherwise the event would propagate to
the parent view.

FWIW, the textfield class returns false when the textfield does not have an
active selection or when it has obscured text, for Ctrl+C. Hence the event
propagates to the parent view for the active selection but obscured text case,
which is probably incorrect.

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

Okay, that's fair, and the CopyToClipboard's early return works fine.

+        CopyToClipboard();
+        return true;
+      }
+      break;
+    case ui::VKEY_INSERT:
+      if (control && !shift && HasSelection()) {
+        CopyToClipboard();
+        return true;
+      }
+      break;
+    case ui::VKEY_A:
+      if (control && !alt && !text().empty()) {
+        SelectAll();
+        if (HasSelection())

[msw, 2016/11/09 18:32:53]

Should this be a DCHECK given the !text().empty() conditional above?

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

Oh yeah, done. 

+          UpdateSelectionClipboard();

[msw, 2016/11/09 18:32:53]

Should this be a part of SelectAll?

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

Don't think so. SelectAll is a part of the public Label API. We don't want
programmatic changes to the text selection to update the selection clipboard.

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

Acknowledged.

+        return true;
+      }
+      break;
+    default:
+      break;
+  }
+
+  return false;
+}
+
 void Label::OnDeviceScaleFactorChanged(float device_scale_factor) {
   View::OnDeviceScaleFactorChanged(device_scale_factor);
   // When the device scale factor is changed, some font rendering parameters is
@@ -557,6 +611,79 @@ void Label::VisibilityChanged(View* starting_from, bool is_visible) {
     ClearRenderTextLines();
 }
 
+bool Label::IsCommandIdChecked(int command_id) const {
+  return true;
+}
+
+bool Label::IsCommandIdEnabled(int command_id) const {
+  switch (command_id) {
+    case IDS_APP_COPY:
+      return HasSelection() && !obscured();
+    case IDS_APP_SELECT_ALL:
+      return GetRenderTextForSelectionController() && !text().empty();
+  }
+  return false;
+}
+
+void Label::ExecuteCommand(int command_id, int event_flags) {
+  switch (command_id) {
+    case IDS_APP_COPY:
+      CopyToClipboard();
+      break;
+    case IDS_APP_SELECT_ALL:
+      SelectAll();
+      if (HasSelection())
+        UpdateSelectionClipboard();
+      break;
+    default:
+      NOTREACHED();
+  }
+}
+
+bool Label::GetAcceleratorForCommandId(int command_id,

[msw, 2016/11/09 18:32:52]

nit: Share a helper with textfield if feasible.

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

Do you think it's still necessary now that we are handling just Copy/SelectAll?
Because it would lead to more boilerplate without much sharing of logic.

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

Nope, the simpler inlined impl here makes sense for now.

+                                       ui::Accelerator* accelerator) const {
+  switch (command_id) {
+    case IDS_APP_UNDO:
+      *accelerator = ui::Accelerator(ui::VKEY_Z, ui::EF_CONTROL_DOWN);
+      return true;
+
+    case IDS_APP_CUT:
+      *accelerator = ui::Accelerator(ui::VKEY_X, ui::EF_CONTROL_DOWN);
+      return true;
+
+    case IDS_APP_COPY:
+      *accelerator = ui::Accelerator(ui::VKEY_C, ui::EF_CONTROL_DOWN);
+      return true;
+
+    case IDS_APP_PASTE:
+      *accelerator = ui::Accelerator(ui::VKEY_V, ui::EF_CONTROL_DOWN);
+      return true;
+
+    case IDS_APP_SELECT_ALL:
+      *accelerator = ui::Accelerator(ui::VKEY_A, ui::EF_CONTROL_DOWN);
+      return true;
+
+    default:
+      return false;
+  }
+}
+
+void Label::ShowContextMenuForView(View* source,
+                                   const gfx::Point& point,
+                                   ui::MenuSourceType source_type) {
+  DCHECK(context_menu_contents_);
+  if (!GetRenderTextForSelectionController())
+    return;
+
+  context_menu_runner_.reset(new MenuRunner(context_menu_contents_.get(),

[msw, 2016/11/09 18:32:52]

Would it suffice to initialize the runner once in/after
BuildContextMenuContents?

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

Although the MenuRunner documentation does not state this, it seems that (at
least on Linux) the MenuRunner needs to be initialized every time before it is
used, even if the last exit was NORMAL_EXIT. Also, weirdly this is not the case
on MacViews, where the same MenuRunner can be reused.

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

Acknowledged.

+                                            MenuRunner::HAS_MNEMONICS |
+                                                MenuRunner::CONTEXT_MENU |
+                                                MenuRunner::ASYNC));
+  ignore_result(context_menu_runner_->RunMenuAt(

[msw, 2016/11/09 18:32:53]

Perhaps we should check that the result isn't MENU_DELETED?

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

But what to do if it is MENU_DELETED? 

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

Right, it doesn't actually matter now, since the function doesn't use any
instance members after this call. This is fine for now.

+      GetWidget(), nullptr, gfx::Rect(point, gfx::Size()), MENU_ANCHOR_TOPLEFT,
+      source_type));
+}
+
 gfx::RenderText* Label::GetRenderTextForSelectionController() {
   return const_cast<gfx::RenderText*>(
       static_cast<const Label*>(this)->GetRenderTextForSelectionController());
@@ -611,12 +738,8 @@ bool Label::PasteSelectionClipboard() {
 void Label::UpdateSelectionClipboard() {
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   if (!obscured()) {
-    const gfx::RenderText* render_text = GetRenderTextForSelectionController();
-    DCHECK(render_text);
-    const base::string16 selected_text =
-        render_text->GetTextFromRange(render_text->selection());
     ui::ScopedClipboardWriter(ui::CLIPBOARD_TYPE_SELECTION)
-        .WriteText(selected_text);
+        .WriteText(GetSelectedText());
   }
 #endif
 }
@@ -659,6 +782,15 @@ void Label::Init(const base::string16& text, const gfx::FontList& font_list) {
   max_width_ = 0;
   is_first_paint_text_ = true;
   SetText(text);
+
+  // Only selectable labels will get requests to show the context menu, due to
+  // CanProcessEventsWithinSubtree.
+  BuildContextMenuContents();
+  set_context_menu_controller(this);
+
+  // This allows the BrowserView to pass the copy command from the Chrome menu

[msw, 2016/11/09 18:32:53]

Should we also add CTRL+A and Ctrl+Insert? It seems odd to only add one
accelerator, and just for the purpose of supporting some BrowserView behavior.

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

See my comment above for "Not CTRL+A for select all?"

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

Acknowledged.

+  // to the Label.
+  AddAccelerator(ui::Accelerator(ui::VKEY_C, ui::EF_CONTROL_DOWN));
 }
 
 void Label::ResetLayout() {
@@ -870,4 +1002,31 @@ void Label::ClearRenderTextLines() const {
   lines_.clear();
 }
 
+base::string16 Label::GetSelectedText() const {
+  const gfx::RenderText* render_text = GetRenderTextForSelectionController();
+  return render_text ? render_text->GetTextFromRange(render_text->selection())
+                     : base::string16();
+}
+
+void Label::CopyToClipboard() {
+  if (!HasSelection() || obscured())
+    return;
+  ui::ScopedClipboardWriter(ui::CLIPBOARD_TYPE_COPY_PASTE)
+      .WriteText(GetSelectedText());
+}
+
+void Label::BuildContextMenuContents() {
+  DCHECK(!context_menu_contents_);
+  context_menu_contents_.reset(new ui::SimpleMenuModel(this));

[karandeepb, 2016/11/09 10:20:00]

Do we want to show only Copy/SelectAll in the context menu for Labels or is this
ok?

[msw, 2016/11/09 18:32:52]

Yeah, I think we should only show copy and select all.

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

Done.

+  context_menu_contents_->AddItemWithStringId(IDS_APP_UNDO, IDS_APP_UNDO);
+  context_menu_contents_->AddSeparator(ui::NORMAL_SEPARATOR);
+  context_menu_contents_->AddItemWithStringId(IDS_APP_CUT, IDS_APP_CUT);
+  context_menu_contents_->AddItemWithStringId(IDS_APP_COPY, IDS_APP_COPY);
+  context_menu_contents_->AddItemWithStringId(IDS_APP_PASTE, IDS_APP_PASTE);
+  context_menu_contents_->AddItemWithStringId(IDS_APP_DELETE, IDS_APP_DELETE);
+  context_menu_contents_->AddSeparator(ui::NORMAL_SEPARATOR);
+  context_menu_contents_->AddItemWithStringId(IDS_APP_SELECT_ALL,
+                                              IDS_APP_SELECT_ALL);
+}
+
 }  // namespace views