diff options
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]; } } |