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..d770eb91bc607276197edd178242ce3a87a5a72f 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"
@@ -44,7 +46,8 @@ Label::Label() : Label(base::string16()) {
 Label::Label(const base::string16& text) : Label(text, GetDefaultFontList()) {
 }
 
-Label::Label(const base::string16& text, const gfx::FontList& font_list) {
+Label::Label(const base::string16& text, const gfx::FontList& font_list)
+    : context_menu_contents_(this) {
   Init(text, font_list);
 }
 
@@ -69,6 +72,9 @@ void Label::SetText(const base::string16& new_text) {
   is_first_paint_text_ = true;
   render_text_->SetText(new_text);
   ResetLayout();
+
+  // Invalidate |stored_selection_range_|.

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

nit: this comment seems unnecessary (blank line above too)

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

Done.

+  stored_selection_range_ = gfx::Range::InvalidRange();

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

nit: maybe do this before ResetLayout?

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

This needs to be done after ResetLayout. ResetLayout calls ClearRenderTextLines
which is what persists the selection range.

 }
 
 void Label::SetAutoColorReadabilityEnabled(bool enabled) {
@@ -253,10 +259,12 @@ bool Label::SetSelectable(bool value) {
 
   if (!value) {
     ClearSelection();
+    stored_selection_range_ = gfx::Range::InvalidRange();
     selection_controller_.reset();
     return true;
   }
 
+  DCHECK(!stored_selection_range_.IsValid());
   if (!IsSelectionSupported())
     return false;
 
@@ -544,6 +552,60 @@ void Label::OnMouseCaptureLost() {
   selection_controller_->OnMouseCaptureLost();
 }
 
+bool Label::OnKeyPressed(const ui::KeyEvent& event) {
+  if (!GetRenderTextForSelectionController())
+    return false;
+
+  const bool shift = event.IsShiftDown();

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

This is probably orthogonal, but I feel like there should be a way to do
something like

  int command =
      context_menu_contents_.CommandForAccelerator(ui::Accelerator(event));
  if (!IsCommandIdEnabled(command))
    return false;

  ExecuteCommand(command, event.flags());
  return true;


But I guess that would require [Simple]MenuModel to support multiple
accelerators per command. Alternatively, just add an additional menu item with
MenuModel::IsVisibleAt returning false and IsEnabledAt returning true (although
that's a bit tricky -- a duplicate command works for MenuModel since it is
index-based, but SimpleMenuModel is command_id-based)

That code fragment above could even move into SimpleMenuModel so that this
method becomes just

  return context_menu_contents_.HandleAccelerator(ui::Accelerator(event));

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

So if I am understanding you correctly, the SimpleMenuModel should maintain a
mapping of Accelerators<->Menu commands. Also, as you said, multiple
accelerators could map to the same command. 

While this will help to reduce duplication of logic and code to some extent, I
am not too sure whether this is the right thing to do. It seems weird for
SimpleMenuModel to assist in handling keyboard events. Also I think, the use
case here is not generic enough to modify SimpleMenuModel. 

I think if any consolidation of logic needs to happen, (say sharing some code in
OnKeyPressed, GetAcceleratorForCommandId and AcceleratorPressed), I think it
should happen in the Label class itself.

[tapted, 2016/11/16 08:39:07]

It's pretty much exactly how Cocoa makes you do it (so, yes, definitely weird
:p). i.e. Most accelerator handling in Cocoa UI is done by doing [[NSApp
mainMenu] performKeyEquivalent:SomeKeyEvent()]. 

Custom accelerator remapping is all done by searching through command strings in
the menuBar. The reason why a user-remapped "Copy" command works on MacViews is
that the menuBar receives the event and sends it to [BridgedContentView copy:]
then that maps it back to VKEY_C

[karandeepb, 2016/11/16 23:47:13]

Oh ok. Din't know this. As talked offline, this can be handled subsequently if a
similar recurring pattern emerges elsewhere as well. 

+  const bool control = event.IsControlDown();
+  const bool alt = event.IsAltDown() || event.IsAltGrDown();
+
+  switch (event.key_code()) {
+    case ui::VKEY_C:
+      if (control && !alt && HasSelection()) {
+        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();
+        DCHECK(HasSelection());
+        UpdateSelectionClipboard();
+        return true;
+      }
+      break;
+    default:
+      break;
+  }
+
+  return false;
+}
+
+bool Label::AcceleratorPressed(const ui::Accelerator& accelerator) {
+  // The only accelerator a Label needs to handle is the Copy command from the
+  // Chrome menu.

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

In the comment, say why? I had the same immediate reaction - "why not select
all". Maybe something like

  // Allow the "Copy" action from the Chrome menu to be invoked. E.g., if a user
  // selects a Label on a web modal dialog. "Select All" doesn't appear in the
  // Chrome menu so isn't handled here.

[and is that example right? - is there another way it might be activated?]

(but, also, with my suggestion above, this could just be

  return context_menu_contents_.HandleAccelerator(accelerator);

and we wouldn't have to worry about clarifications :)

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

Done.
So yeah it should be applicable for any persistent dialog. [Any dialog which
stays visible on opening the chrome menu.]

+  if (accelerator.key_code() == ui::VKEY_C && accelerator.IsCtrlDown()) {
+    CopyToClipboard();
+    return true;
+  }
+  return false;
+}
+
+bool Label::CanHandleAccelerators() const {
+  // Focus needs to be checked since the accelerator for the Copy command from
+  // the Chrome menu should only be handled when the current view has focus. See
+  // related comment in BrowserView::CutCopyPaste.
+  return HasFocus() && GetRenderTextForSelectionController() &&
+         View::CanHandleAccelerators();
+}
+
 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 +619,21 @@ void Label::VisibilityChanged(View* starting_from, bool is_visible) {
     ClearRenderTextLines();
 }
 
+void Label::ShowContextMenuForView(View* source,
+                                   const gfx::Point& point,
+                                   ui::MenuSourceType source_type) {
+  if (!GetRenderTextForSelectionController())
+    return;
+
+  context_menu_runner_.reset(
+      new MenuRunner(&context_menu_contents_, MenuRunner::HAS_MNEMONICS |
+                                                  MenuRunner::CONTEXT_MENU |
+                                                  MenuRunner::ASYNC));
+  ignore_result(context_menu_runner_->RunMenuAt(
+      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,16 +688,57 @@ 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
 }
 
+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();
+      DCHECK(HasSelection());
+      UpdateSelectionClipboard();
+      break;
+    default:
+      NOTREACHED();
+  }
+}
+
+bool Label::GetAcceleratorForCommandId(int command_id,
+                                       ui::Accelerator* accelerator) const {
+  switch (command_id) {
+    case IDS_APP_COPY:
+      *accelerator = ui::Accelerator(ui::VKEY_C, 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;
+  }
+}
+
 const gfx::RenderText* Label::GetRenderTextForSelectionController() const {
   if (!selectable())
     return nullptr;
@@ -659,6 +777,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.

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

nit: CanProcessEventsWithinSubtree().

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

Done.

+  BuildContextMenuContents();
+  set_context_menu_controller(this);
+
+  // This allows the BrowserView to pass the copy command from the Chrome menu
+  // to the Label.
+  AddAccelerator(ui::Accelerator(ui::VKEY_C, ui::EF_CONTROL_DOWN));
 }
 
 void Label::ResetLayout() {
@@ -862,6 +989,9 @@ bool Label::ShouldShowDefaultTooltip() const {
 }
 
 void Label::ClearRenderTextLines() const {
+  if(lines_.empty())

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

This is because we follow this codepath ClearRenderTextLines -> HasSelection->
GetRenderTextForSelectionController -> MaybeBuildRenderTextLines. 

Hence calling ClearRenderTextLines repeatedly could have lead to unnecessary
computation.

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

Add a quick explanatory comment; and run git cl format (space after if).

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

Done.

+    return;
+
   // Persist the selection range if there is an active selection.
   if (HasSelection()) {
     stored_selection_range_ =
@@ -870,4 +1000,23 @@ 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() {
+  context_menu_contents_.AddItemWithStringId(IDS_APP_COPY, IDS_APP_COPY);
+  context_menu_contents_.AddItemWithStringId(IDS_APP_SELECT_ALL,
+                                             IDS_APP_SELECT_ALL);
+}
+
 }  // namespace views