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

ui/views/cocoa/bridged_content_view.mm

Index: ui/views/cocoa/bridged_content_view.mm
diff --git a/ui/views/cocoa/bridged_content_view.mm b/ui/views/cocoa/bridged_content_view.mm
index 041e9c572acb77fc5c1c8ea8d5293eb0f20383fa..75ec2eb9be32a5ad5d3e7ddbe35230abee28fdb4 100644
--- a/ui/views/cocoa/bridged_content_view.mm
+++ b/ui/views/cocoa/bridged_content_view.mm
@@ -28,8 +28,10 @@
 #include "ui/gfx/path.h"
 #import "ui/gfx/path_mac.h"
 #include "ui/gfx/scoped_ns_graphics_context_save_gstate_mac.h"
+#include "ui/strings/grit/ui_strings.h"
 #import "ui/views/cocoa/bridged_native_widget.h"
 #import "ui/views/cocoa/drag_drop_client_mac.h"
+#include "ui/views/controls/label.h"
 #include "ui/views/controls/menu/menu_config.h"
 #include "ui/views/controls/menu/menu_controller.h"
 #include "ui/views/view.h"
@@ -41,6 +43,8 @@
 
 namespace {
 
+const int kInvalidTextMenuCommand = 0;

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

0 is valid according to SimpleMenuModel::ValidateItem (and -1 is separator). 
Rather than something arbitrary, I think we can change the signature of
GetContextMenuCommandForMenuAction to return `bool`, or switch on
ui::TextEditCommand after checking that GetTextEditCommandForMenuAction didn't
return INVALID_COMMAND, or just remove GetContextMenuCommandForMenuAction
completely (see later comments)

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

Removed GetContextMenuCommandForMenuAction.

+
 NSString* const kFullKeyboardAccessChangedNotification =
     @"com.apple.KeyboardUIModeDidChange";
 
@@ -247,6 +251,36 @@ bool IsTextRTL(const ui::TextInputClient* client) {
   return str.autorelease();
 }
 
+ui::TextEditCommand GetTextEditCommandForMenuAction(SEL action) {
+  if (action == @selector(undo:))
+    return ui::TextEditCommand::UNDO;
+  if (action == @selector(redo:))
+    return ui::TextEditCommand::REDO;
+  if (action == @selector(cut:))
+    return ui::TextEditCommand::CUT;
+  if (action == @selector(copy:))
+    return ui::TextEditCommand::COPY;
+  if (action == @selector(paste:))
+    return ui::TextEditCommand::PASTE;
+  if (action == @selector(selectAll:))
+    return ui::TextEditCommand::SELECT_ALL;
+  return ui::TextEditCommand::INVALID_COMMAND;
+}
+
+int GetContextMenuCommandForMenuAction(SEL action) {
+  if (action == @selector(undo:))
+    return IDS_APP_UNDO;

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

I don't really like seeing IDS_ translated string ids here instead of
ui::TextEditCommand -- it's too much coupling with things views::Label is
responsible for.

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

Removed this.

+  if (action == @selector(cut:))
+    return IDS_APP_CUT;
+  if (action == @selector(copy:))
+    return IDS_APP_COPY;
+  if (action == @selector(paste:))
+    return IDS_APP_PASTE;
+  if (action == @selector(selectAll:))
+    return IDS_APP_SELECT_ALL;
+  return kInvalidTextMenuCommand;
+}
+
 }  // namespace
 
 @interface BridgedContentView ()
@@ -520,12 +554,6 @@ - (void)insertTextInternal:(id)text {
 }
 
 - (void)undo:(id)sender {
-  // This DCHECK is more strict than a similar check in handleAction:. It can be
-  // done here because the actors sending these actions should be calling
-  // validateUserInterfaceItem: before enabling UI that allows these messages to
-  // be sent. Checking it here would be too late to provide correct UI feedback
-  // (e.g. there will be no "beep").
-  DCHECK(textInputClient_->IsTextEditCommandEnabled(ui::TextEditCommand::UNDO));
   [self handleAction:ui::TextEditCommand::UNDO
              keyCode:ui::VKEY_Z
              domCode:ui::DomCode::US_Z
@@ -533,7 +561,6 @@ - (void)undo:(id)sender {
 }
 
 - (void)redo:(id)sender {
-  DCHECK(textInputClient_->IsTextEditCommandEnabled(ui::TextEditCommand::REDO));
   [self handleAction:ui::TextEditCommand::REDO
              keyCode:ui::VKEY_Z
              domCode:ui::DomCode::US_Z
@@ -541,7 +568,6 @@ - (void)redo:(id)sender {
 }
 
 - (void)cut:(id)sender {
-  DCHECK(textInputClient_->IsTextEditCommandEnabled(ui::TextEditCommand::CUT));
   [self handleAction:ui::TextEditCommand::CUT
              keyCode:ui::VKEY_X
              domCode:ui::DomCode::US_X
@@ -549,7 +575,6 @@ - (void)cut:(id)sender {
 }
 
 - (void)copy:(id)sender {
-  DCHECK(textInputClient_->IsTextEditCommandEnabled(ui::TextEditCommand::COPY));
   [self handleAction:ui::TextEditCommand::COPY
              keyCode:ui::VKEY_C
              domCode:ui::DomCode::US_C
@@ -557,8 +582,6 @@ - (void)copy:(id)sender {
 }
 
 - (void)paste:(id)sender {
-  DCHECK(
-      textInputClient_->IsTextEditCommandEnabled(ui::TextEditCommand::PASTE));
   [self handleAction:ui::TextEditCommand::PASTE
              keyCode:ui::VKEY_V
              domCode:ui::DomCode::US_V
@@ -566,8 +589,6 @@ - (void)paste:(id)sender {
 }
 
 - (void)selectAll:(id)sender {
-  DCHECK(textInputClient_->IsTextEditCommandEnabled(
-      ui::TextEditCommand::SELECT_ALL));
   [self handleAction:ui::TextEditCommand::SELECT_ALL
              keyCode:ui::VKEY_A
              domCode:ui::DomCode::US_A
@@ -1378,30 +1399,27 @@ - (NSArray*)validAttributesForMarkedText {
 // NSUserInterfaceValidations protocol implementation.
 
 - (BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item {
-  if (!textInputClient_)
-    return NO;
+  ui::TextEditCommand command = GetTextEditCommandForMenuAction([item action]);
 
-  SEL action = [item action];
+  if (command == ui::TextEditCommand::INVALID_COMMAND)
+    return NO;
 
-  if (action == @selector(undo:))
-    return textInputClient_->IsTextEditCommandEnabled(
-        ui::TextEditCommand::UNDO);
-  if (action == @selector(redo:))
-    return textInputClient_->IsTextEditCommandEnabled(
-        ui::TextEditCommand::REDO);
-  if (action == @selector(cut:))
-    return textInputClient_->IsTextEditCommandEnabled(ui::TextEditCommand::CUT);
-  if (action == @selector(copy:))
-    return textInputClient_->IsTextEditCommandEnabled(
-        ui::TextEditCommand::COPY);
-  if (action == @selector(paste:))
-    return textInputClient_->IsTextEditCommandEnabled(
-        ui::TextEditCommand::PASTE);
-  if (action == @selector(selectAll:))
-    return textInputClient_->IsTextEditCommandEnabled(
-        ui::TextEditCommand::SELECT_ALL);
+  if (textInputClient_)
+    return textInputClient_->IsTextEditCommandEnabled(command);
 
-  return NO;
+  // views::Label does not implement the TextInputClient interface but still
+  // needs to intercept these menu actions.
+  views::FocusManager* focus_manager =
+      hostedView_->GetWidget()->GetFocusManager();
+  if (!focus_manager || !focus_manager->GetFocusedView() ||
+      focus_manager->GetFocusedView()->GetClassName() !=
+          views::Label::kViewClassName)
+    return false;

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

nit: false -> NO

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

Done.

+
+  views::Label* label =

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

I think this could safely just `return YES`. Right-clicking text on Mac is meant
to highlight the current word (we don't currently do that, but in theory there
should always be a selection when the menu is shown). So there should always be
something to copy. And I guess the static casts scare me more than an enabled
but non-functional menu item for obscured labels. Also we could get rid of
GetContextMenuCommandForMenuAction()


I think it's gross enough just checking views::Label::kViewClassName, but this
goes a big step beyond that.

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

Not really, left clicking a label will give focus to it. Hence on showing the
menu, there's nothing to copy.
Yeah I wasn't also too happy with this. Removed.

+      static_cast<views::Label*>(focus_manager->GetFocusedView());
+  return static_cast<ui::SimpleMenuModel::Delegate*>(label)->IsCommandIdEnabled(

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

you could get rid of this static_cast by assigning to a

ui::SimpleMenuModel::Delegate* 

on the line above (still static_cast to Label* though). But I think we should
remove this, unless there's something more compelling that I've missed.


(there was also base::implicit_cast<>, but it was taken away. I also don't
believe in making overrides of public methods private, but others do :) ).

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

Yeah I remember seeing a thread about this earlier. Maybe it's just an artifact
of not allowing private inheritance.

+      GetContextMenuCommandForMenuAction([item action]));
 }
 
 // NSDraggingSource protocol implementation.