[Mac] Manually reposition Omnibox field editor rather than using focus machinery.

Previously this code abused the Cocoa focus machinery to move the
field editor around when needed, which required the code to mess
around with delegation to make sure the changes didn't leak out to the
core autocomplete code.  Unfortunately, it also messes with other
features, like undo.  This change does the positioning manually, which
should let everything just work.

TEST=Tab-to-search and keyword hints shouldn't cause the field to break.

Review URL: http://codereview.chromium.org/219031

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@27217 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 90 insertions, 59 deletions

diff --git a/chrome/browser/cocoa/autocomplete_text_field.h b/chrome/browser/cocoa/autocomplete_text_field.h
index 9c8b1c5..f850a27 100644
--- a/chrome/browser/cocoa/autocomplete_text_field.h
+++ b/chrome/browser/cocoa/autocomplete_text_field.h
@@ -62,8 +62,7 @@ class AutocompleteTextFieldObserver {
 - (AutocompleteTextFieldCell*)autocompleteTextFieldCell;
 
 // If the keyword, keyword hint, or search hint changed, then the
-// field needs to be relaidout.  This accomplishes that in a manner
-// which doesn't disrupt the field delegate.
+// field editor may need to be repositioned.
 - (void)resetFieldEditorFrameIfNeeded;
 
 @end
diff --git a/chrome/browser/cocoa/autocomplete_text_field.mm b/chrome/browser/cocoa/autocomplete_text_field.mm
index f48d508..25db457 100644
--- a/chrome/browser/cocoa/autocomplete_text_field.mm
+++ b/chrome/browser/cocoa/autocomplete_text_field.mm
@@ -29,38 +29,33 @@
   return static_cast<AutocompleteTextFieldCell*>([self cell]);
 }
 
-// TODO(shess): An alternate implementation of this would be to pick
-// out the field's subview (which may be a clip view around the field
-// editor) and manually resize it to the textFrame returned from the
-// cell's -splitFrame:*.  That doesn't mess with the editing state of
-// the field editor system, but does make other assumptions about how
-// field editors are placed.
+// Cocoa text fields are edited by placing an NSTextView as subview,
+// positioned by the cell's -editWithFrame:inView:... method.  Using
+// the standard -makeFirstResponder: machinery to reposition the field
+// editor results in reseting the field editor's editing state, which
+// AutocompleteEditViewMac monitors.  This causes problems because
+// editing can require the field editor to be repositioned, which
+// could disrupt editing.  This code repositions the subview directly,
+// which causes no editing-state changes.
 - (void)resetFieldEditorFrameIfNeeded {
   AutocompleteTextFieldCell* cell = [self cell];
   if ([cell fieldEditorNeedsReset]) {
-    NSTextView* editor = (id)[self currentEditor];
+    // No change to bounds necessary if not editing.
+    NSText* editor = [self currentEditor];
     if (editor) {
-      // Clear the delegate so that it doesn't see
-      // -control:textShouldEndEditing: (closes autocomplete popup).
-      id delegate = [self delegate];
-      [self setDelegate:nil];
-
-      // -makeFirstResponder: will select-all, restore selection.
-      NSRange sel = [editor selectedRange];
-      [[self window] makeFirstResponder:self];
-      [editor setSelectedRange:sel];
-
-      [self setDelegate:delegate];
-
-      // Now provoke call to delegate's -controlTextDidBeginEditing:.
-      // This is necessary to make sure that we'll send the
-      // appropriate -control:textShouldEndEditing: call when we lose
-      // focus.
-      // TODO(shess): Would be better to only restore this state if
-      // -controlTextDidBeginEditing: had already been sent.
-      // Unfortunately, that's hard to detect.  Could either check
-      // popup IsOpen() or model has_focus()?
-      [editor shouldChangeTextInRange:sel replacementString:@""];
+      // When editing, we should have exactly one subview, which is a
+      // clipview containing the editor (for purposes of scrolling).
+      NSArray* subviews = [self subviews];
+      DCHECK_EQ([subviews count], 1U);
+      DCHECK([editor isDescendantOf:self]);
+      if ([subviews count] > 0) {
+        const NSRect frame([cell drawingRectForBounds:[self bounds]]);
+        [[subviews objectAtIndex:0] setFrame:frame];
+
+        // Make sure the selection remains visible.
+        // TODO(shess) Could this be janky?
+        [editor scrollRangeToVisible:[editor selectedRange]];
+      }
     }
     [cell setFieldEditorNeedsReset:NO];
   }
diff --git a/chrome/browser/cocoa/autocomplete_text_field_cell.h b/chrome/browser/cocoa/autocomplete_text_field_cell.h
index 9bca85d..17ec714 100644
--- a/chrome/browser/cocoa/autocomplete_text_field_cell.h
+++ b/chrome/browser/cocoa/autocomplete_text_field_cell.h
@@ -64,7 +64,8 @@
 // Return the portion of the cell to show the text cursor over.
 - (NSRect)textCursorFrameForFrame:(NSRect)cellFrame;
 
-// Return the portion of the cell to use for text display.
+// Return the portion of the cell to use for text display.  This
+// corrosponds to the frame with our added decorations sliced off.
 - (NSRect)textFrameForFrame:(NSRect)cellFrame;
 
 @end
diff --git a/chrome/browser/cocoa/autocomplete_text_field_cell.mm b/chrome/browser/cocoa/autocomplete_text_field_cell.mm
index d93e518..c8532bd 100644
--- a/chrome/browser/cocoa/autocomplete_text_field_cell.mm
+++ b/chrome/browser/cocoa/autocomplete_text_field_cell.mm
@@ -168,6 +168,11 @@ const NSInteger kHintIconHorizontalPad = 5;
   }
 }
 
+// TODO(shess): This code is manually drawing the cell's border area,
+// but otherwise the cell assumes -setBordered:YES for purposes of
+// calculating things like the editing area.  This is probably
+// incorrect.  I know that this affects -drawingRectForBounds:.
+
 - (void)drawWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
   DCHECK([controlView isFlipped]);
   [[NSColor colorWithCalibratedWhite:1.0 alpha:0.25] set];
@@ -242,6 +247,13 @@ const NSInteger kHintIconHorizontalPad = 5;
   return textFrame;
 }
 
+// For NSTextFieldCell this is the area within the borders.  For our
+// purposes, we count the info decorations as being part of the
+// border.
+- (NSRect)drawingRectForBounds:(NSRect)theRect {
+  return [super drawingRectForBounds:[self textFrameForFrame:theRect]];
+}
+
 - (void)drawHintWithFrame:(NSRect)cellFrame inView:(NSView*)controlView {
   DCHECK(hintString_);
 
@@ -321,33 +333,6 @@ const NSInteger kHintIconHorizontalPad = 5;
                         inView:controlView];
 }
 
-// Override these methods so that the field editor shows up in the right place
-- (void)editWithFrame:(NSRect)cellFrame
-               inView:(NSView*)controlView
-               editor:(NSText*)textObj
-             delegate:(id)anObject
-                event:(NSEvent*)theEvent {
-  [super editWithFrame:[self textFrameForFrame:cellFrame]
-                inView:controlView
-                editor:textObj
-              delegate:anObject
-                 event:theEvent];
-}
-
-// Override these methods so that the field editor shows up in the right place
-- (void)selectWithFrame:(NSRect)cellFrame
-                 inView:(NSView*)controlView
-                 editor:(NSText*)textObj
-               delegate:(id)anObject
-                  start:(NSInteger)selStart
-                 length:(NSInteger)selLength {
-  [super selectWithFrame:[self textFrameForFrame:cellFrame]
-                  inView:controlView editor:textObj
-                delegate:anObject
-                   start:selStart
-                  length:selLength];
-}
-
 - (void)resetCursorRect:(NSRect)cellFrame inView:(NSView *)controlView {
   [super resetCursorRect:[self textCursorFrameForFrame:cellFrame]
                   inView:controlView];
diff --git a/chrome/browser/cocoa/autocomplete_text_field_cell_unittest.mm b/chrome/browser/cocoa/autocomplete_text_field_cell_unittest.mm
index 8a6b4cd..076789f 100644
--- a/chrome/browser/cocoa/autocomplete_text_field_cell_unittest.mm
+++ b/chrome/browser/cocoa/autocomplete_text_field_cell_unittest.mm
@@ -22,6 +22,7 @@ class AutocompleteTextFieldCellTest : public PlatformTest {
     scoped_nsobject<AutocompleteTextFieldCell> cell(
         [[AutocompleteTextFieldCell alloc] initTextCell:@"Testing"]);
     [cell setEditable:YES];
+    [cell setBordered:YES];
     [view_ setCell:cell.get()];
     [cocoa_helper_.contentView() addSubview:view_.get()];
   }
@@ -304,4 +305,54 @@ TEST_F(AutocompleteTextFieldCellTest, TextFrame) {
   EXPECT_LT(NSWidth(textFrameWithHintText), NSWidth(textFrame));
 }
 
+// The editor frame should be slightly inset from the text frame.
+TEST_F(AutocompleteTextFieldCellTest, DrawingRectForBounds) {
+  AutocompleteTextFieldCell* cell =
+      static_cast<AutocompleteTextFieldCell*>([view_ cell]);
+  const NSRect bounds([view_ bounds]);
+  NSRect textFrame, drawingRect;
+
+  textFrame = [cell textFrameForFrame:bounds];
+  drawingRect = [cell drawingRectForBounds:bounds];
+  EXPECT_FALSE(NSIsEmptyRect(drawingRect));
+  EXPECT_TRUE(NSContainsRect(textFrame, NSInsetRect(drawingRect, 1, 1)));
+
+  // Save the starting frame for after clear.
+  const NSRect originalDrawingRect(drawingRect);
+
+  // TODO(shess): Do we really need to test every combination?
+
+  [cell setSearchHintString:@"Search hint"];
+  textFrame = [cell textFrameForFrame:bounds];
+  drawingRect = [cell drawingRectForBounds:bounds];
+  EXPECT_FALSE(NSIsEmptyRect(drawingRect));
+  EXPECT_TRUE(NSContainsRect(textFrame, NSInsetRect(drawingRect, 1, 1)));
+
+  NSImage* image = [NSImage imageNamed:@"NSApplicationIcon"];
+  [cell setKeywordHintPrefix:@"Keyword " image:image suffix:@" hint"];
+  textFrame = [cell textFrameForFrame:bounds];
+  drawingRect = [cell drawingRectForBounds:bounds];
+  EXPECT_FALSE(NSIsEmptyRect(drawingRect));
+  EXPECT_TRUE(NSContainsRect(NSInsetRect(textFrame, 1, 1), drawingRect));
+
+  [cell setKeywordString:@"Search Engine:"];
+  textFrame = [cell textFrameForFrame:bounds];
+  drawingRect = [cell drawingRectForBounds:bounds];
+  EXPECT_FALSE(NSIsEmptyRect(drawingRect));
+  EXPECT_TRUE(NSContainsRect(NSInsetRect(textFrame, 1, 1), drawingRect));
+
+  [cell clearKeywordAndHint];
+  textFrame = [cell textFrameForFrame:bounds];
+  drawingRect = [cell drawingRectForBounds:bounds];
+  EXPECT_FALSE(NSIsEmptyRect(drawingRect));
+  EXPECT_TRUE(NSContainsRect(NSInsetRect(textFrame, 1, 1), drawingRect));
+  EXPECT_TRUE(NSEqualRects(drawingRect, originalDrawingRect));
+
+  [cell setHintIcon:[NSImage imageNamed:@"NSComputer"]];
+  textFrame = [cell textFrameForFrame:bounds];
+  drawingRect = [cell drawingRectForBounds:bounds];
+  EXPECT_FALSE(NSIsEmptyRect(drawingRect));
+  EXPECT_TRUE(NSContainsRect(NSInsetRect(textFrame, 1, 1), drawingRect));
+}
+
 }  // namespace
diff --git a/chrome/browser/cocoa/autocomplete_text_field_unittest.mm b/chrome/browser/cocoa/autocomplete_text_field_unittest.mm
index 6d4f843..ab50073 100644
--- a/chrome/browser/cocoa/autocomplete_text_field_unittest.mm
+++ b/chrome/browser/cocoa/autocomplete_text_field_unittest.mm
@@ -327,7 +327,7 @@ TEST_F(AutocompleteTextFieldTest, ResetFieldEditorBlocksEndEditing) {
     EXPECT_FALSE([delegate receivedControlTextShouldEndEditing]);
     [field_ resetFieldEditorFrameIfNeeded];
     EXPECT_FALSE([delegate receivedControlTextShouldEndEditing]);
-    EXPECT_TRUE([delegate receivedControlTextDidBeginEditing]);
+    EXPECT_FALSE([delegate receivedControlTextDidBeginEditing]);
     [field_ setDelegate:nil];
   }
 }