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

ui/views/cocoa/bridged_content_view.mm

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ui/views/cocoa/bridged_content_view.h"
 
 #include "base/logging.h"
 #import "base/mac/mac_util.h"
 #import "base/mac/scoped_nsobject.h"
 #import "base/mac/sdk_forward_declarations.h"
@@ skipping 10 matching lines @@
 #include "ui/events/event_utils.h"
 #include "ui/events/keycodes/dom/dom_code.h"
 #import "ui/events/keycodes/keyboard_code_conversion_mac.h"
 #include "ui/gfx/canvas_paint_mac.h"
 #include "ui/gfx/decorated_text.h"
 #include "ui/gfx/geometry/rect.h"
 #import "ui/gfx/mac/coordinate_conversion.h"
 #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"
 #include "ui/views/widget/native_widget_mac.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/word_lookup_client.h"
 
 using views::MenuController;
 
 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";
 
 // Returns true if all four corners of |rect| are contained inside |path|.
 bool IsRectInsidePath(NSRect rect, NSBezierPath* path) {
   return [path containsPoint:rect.origin] &&
          [path containsPoint:NSMakePoint(rect.origin.x + rect.size.width,
                                          rect.origin.y)] &&
          [path containsPoint:NSMakePoint(rect.origin.x,
                                          rect.origin.y + rect.size.height)] &&
@@ skipping 186 matching lines @@
     if (attribute.strike)
       attrs[NSStrikethroughStyleAttributeName] = line_style;
 
     [str setAttributes:attrs range:range];
   }
 
   [str endEditing];
   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 ()
 
 // Returns the active menu controller corresponding to |hostedView_|,
 // nil otherwise.
 - (MenuController*)activeMenuController;
 
 // Passes |event| to the InputMethod for dispatch.
 - (void)handleKeyEvent:(ui::KeyEvent*)event;
@@ skipping 253 matching lines @@
   }
 }
 
 - (views::DragDropClientMac*)dragDropClient {
   views::BridgedNativeWidget* bridge =
       views::NativeWidgetMac::GetBridgeForNativeWindow([self window]);
   return bridge ? bridge->drag_drop_client() : nullptr;
 }
 
 - (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
           eventFlags:ui::EF_CONTROL_DOWN];
 }
 
 - (void)redo:(id)sender {
-  DCHECK(textInputClient_->IsTextEditCommandEnabled(ui::TextEditCommand::REDO));
   [self handleAction:ui::TextEditCommand::REDO
              keyCode:ui::VKEY_Z
              domCode:ui::DomCode::US_Z
           eventFlags:ui::EF_CONTROL_DOWN | ui::EF_SHIFT_DOWN];
 }
 
 - (void)cut:(id)sender {
-  DCHECK(textInputClient_->IsTextEditCommandEnabled(ui::TextEditCommand::CUT));
   [self handleAction:ui::TextEditCommand::CUT
              keyCode:ui::VKEY_X
              domCode:ui::DomCode::US_X
           eventFlags:ui::EF_CONTROL_DOWN];
 }
 
 - (void)copy:(id)sender {
-  DCHECK(textInputClient_->IsTextEditCommandEnabled(ui::TextEditCommand::COPY));
   [self handleAction:ui::TextEditCommand::COPY
              keyCode:ui::VKEY_C
              domCode:ui::DomCode::US_C
           eventFlags:ui::EF_CONTROL_DOWN];
 }
 
 - (void)paste:(id)sender {
-  DCHECK(
-      textInputClient_->IsTextEditCommandEnabled(ui::TextEditCommand::PASTE));
   [self handleAction:ui::TextEditCommand::PASTE
              keyCode:ui::VKEY_V
              domCode:ui::DomCode::US_V
           eventFlags:ui::EF_CONTROL_DOWN];
 }
 
 - (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
           eventFlags:ui::EF_CONTROL_DOWN];
 }
 
 // BaseView implementation.
 
 // Don't use tracking areas from BaseView. BridgedContentView's tracks
 // NSTrackingCursorUpdate and Apple's documentation suggests it's incompatible.
@@ skipping 790 matching lines @@
     textInputClient_->ConfirmCompositionText();
 }
 
 - (NSArray*)validAttributesForMarkedText {
   return @[];
 }
 
 // NSUserInterfaceValidations protocol implementation.
 
 - (BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item {
-  if (!textInputClient_)
+  ui::TextEditCommand command = GetTextEditCommandForMenuAction([item action]);
+
+  if (command == ui::TextEditCommand::INVALID_COMMAND)
     return NO;
 
-  SEL action = [item action];
+  if (textInputClient_)
+    return textInputClient_->IsTextEditCommandEnabled(command);
 
-  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);
+  // 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.

 
-  return NO;
+  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.
 
 - (NSDragOperation)draggingSession:(NSDraggingSession*)session
     sourceOperationMaskForDraggingContext:(NSDraggingContext)context {
   return NSDragOperationEvery;
 }
 
 - (void)draggingSession:(NSDraggingSession*)session
@@ skipping 18 matching lines @@
   return [hostedView_->GetNativeViewAccessible() accessibilityHitTest:point];
 }
 
 - (id)accessibilityFocusedUIElement {
   if (!hostedView_)
     return nil;
   return [hostedView_->GetNativeViewAccessible() accessibilityFocusedUIElement];
 }
 
 @end