Mac: Clicking an omnibox decoration should not highlight the omnibox.

chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm

 // Copyright (c) 2012 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 <ApplicationServices/ApplicationServices.h>
 #import <Cocoa/Cocoa.h>
 
 #include "base/mac/foundation_util.h"
 #include "base/mac/scoped_nsobject.h"
 #import "chrome/browser/ui/cocoa/cocoa_test_helper.h"
@@ skipping 771 matching lines @@
                             ofView:field_];
   EXPECT_FALSE([[field_ cell] showsFirstResponder]);
   EXPECT_FALSE([FieldEditor() shouldDrawInsertionPoint]);
 
   [[field_ cell] setHideFocusState:NO
                             ofView:field_];
   EXPECT_TRUE([[field_ cell] showsFirstResponder]);
   EXPECT_TRUE([FieldEditor() shouldDrawInsertionPoint]);
 }
 
-// Verify that OnSetFocus for button decorations is only sent after the
-// decoration is picked as the target for the subsequent -mouseDown:. Otherwise
-// hiding a ButtonDecoration in OnSetFocus will prevent a call to
-// OnMousePressed, since it is already hidden at the time of mouseDown.
+// Verify that clicking on a button decoration does not focus the omnibox.
 TEST_F(AutocompleteTextFieldObserverTest, ButtonDecorationFocus) {

[erikchen, 2015/05/07 19:33:51]

This test was written for Omnitheatre, and is no longer relevant. I repurposed
the test into a regression test for this CL. 

Note that the previous CL checked that clicking a decoration (which auto-hides)
causes the omnibox to become focused. The auto-hide behavior is no longer
relevant, and of course the omnibox should not become focused.

[Scott Hess - ex-Googler, 2015/05/07 20:07:42]

Would it make sense to further revise this to not be tied to button decorations?
 The change seems to apply to any decoration.

[erikchen, 2015/05/08 01:51:46]

Good suggestion. I updated the test, and discovered a bug in my original CL.
I've made 3 changes:

1. Moved logic from becomeFirstResponder -> acceptsFirstResponder. This has the
same behavior, but makes more sense semantically.
2. I added a check for AcceptsMousePress(). This was a bug in the logic of the
original CL.
3. I added some logic to make the text field resign first responder when a
decoration is selected. This is technically not a mistake in the previous CL,
but it is a bug fix in the same vein.

   // Add the mock button.
   MockButtonDecoration mock_button;
   mock_button.SetVisible(true);
   AutocompleteTextFieldCell* cell = [field_ cell];
   [cell addLeftDecoration:&mock_button];
 
-  // Ensure button is hidden when OnSetFocus() is called.
-  EXPECT_CALL(field_observer_, OnSetFocus(false)).WillOnce(
-      testing::InvokeWithoutArgs(&mock_button, &MockButtonDecoration::Hide));
-
   // Ignore incidental calls.
   EXPECT_CALL(field_observer_, SelectionRangeForProposedRange(_))
       .WillRepeatedly(testing::Return(NSMakeRange(0, 0)));
   EXPECT_CALL(field_observer_, OnMouseDown(_));
 
-  // Still expect an OnMousePressed on the button.
+  // Expect an OnMousePressed on the button.
   EXPECT_CALL(mock_button, OnMousePressed(_, _))
       .WillOnce(testing::Return(true));
 
   // Get click point for button decoration.
   NSRect button_rect =
       [cell frameForDecoration:&mock_button inFrame:[field_ bounds]];
   EXPECT_FALSE(NSIsEmptyRect(button_rect));
   NSPoint click_location =
       NSMakePoint(NSMidX(button_rect), NSMidY(button_rect));
 
   // Ensure the field is currently not first responder.
   [test_window() makePretendKeyWindowAndSetFirstResponder:nil];
-  EXPECT_NSNE([[field_ window] firstResponder], field_);
+  EXPECT_FALSE([base::mac::ObjCCast<NSView>(
+      [[field_ window] firstResponder]) isDescendantOf:field_]);

[Scott Hess - ex-Googler, 2015/05/07 20:07:42]

At this point is there any first responder at all for the window?  As-is this
feels like it could be FALSE for many reasons, the most specific one might be
preferable.

[erikchen, 2015/05/08 01:51:46]

The first responder is CocoaTestHelperWindow. I'd prefer to not gain specificity
at the cost of adding a dependency between this class and the default first
responder of the test window.

 
   // Execute button click event sequence.
   NSEvent* downEvent = Event(field_, click_location, NSLeftMouseDown);
   NSEvent* upEvent = Event(field_, click_location, NSLeftMouseUp);
 
   // Can't just use -sendEvent:, since that doesn't populate -currentEvent.
   [NSApp postEvent:downEvent atStart:YES];
   [NSApp postEvent:upEvent atStart:NO];
   NSEvent* next_event = [NSApp nextEventMatchingMask:NSAnyEventMask
                                            untilDate:nil
                                               inMode:NSDefaultRunLoopMode
                                              dequeue:YES];
   [NSApp sendEvent:next_event];
 
-  // Expectations check that both OnSetFocus and OnMouseDown were called.
-  // Additionally, ensure button is hidden and field is firstResponder.
-  EXPECT_FALSE(mock_button.IsVisible());
-  EXPECT_TRUE(NSIsEmptyRect([cell frameForDecoration:&mock_left_decoration_
+  // Expectations check that OnMouseDown was called. Additionally, ensure
+  // button is visible and field is not firstResponder.
+  EXPECT_TRUE(mock_button.IsVisible());
+  EXPECT_FALSE(NSIsEmptyRect([cell frameForDecoration:&mock_button
                                              inFrame:[field_ bounds]]));

[Scott Hess - ex-Googler, 2015/05/07 20:07:42]

Alignment.

[erikchen, 2015/05/08 01:51:46]

I clang-format-ed the entire CL.

[Scott Hess - ex-Googler, 2015/05/08 19:38:28]

And the offending code is no longer present anyhow... if the previous version
was run through clang-format, then there's a bug somewhere.

[erikchen, 2015/05/08 20:02:20]

The previous code was not. I should have been more clear - I clang-format-ed the
updated CL.

-  EXPECT_TRUE([base::mac::ObjCCastStrict<NSView>(
+  EXPECT_FALSE([base::mac::ObjCCast<NSView>(
       [[field_ window] firstResponder]) isDescendantOf:field_]);

[Scott Hess - ex-Googler, 2015/05/07 20:07:42]

Here also.

[erikchen, 2015/05/08 01:51:46]

ditto.

 }
 
 TEST_F(AutocompleteTextFieldObserverTest, SendsEditingMessages) {
   // Many of these methods try to change the selection.
   EXPECT_CALL(field_observer_, SelectionRangeForProposedRange(A<NSRange>()))
       .WillRepeatedly(ReturnArg<0>());
 
   EXPECT_CALL(field_observer_, OnSetFocus(false));
   // Becoming first responder doesn't begin editing.
   [test_window() makePretendKeyWindowAndSetFirstResponder:field_];
@@ skipping 38 matching lines @@
   base::scoped_nsobject<AutocompleteTextField> pin([field_ retain]);
   [field_ removeFromSuperview];
   [test_window() resignKeyWindow];
 
   [[test_window() contentView] addSubview:field_];
   EXPECT_CALL(field_observer_, ClosePopup());
   [test_window() resignKeyWindow];
 }
 
 }  // namespace