Skip to content

Commit

Permalink
Fix copy/paste shortcuts in omnibox.
Browse files Browse the repository at this point in the history
Fixes Cmd+C and Cmd+V in pre-edit. Adds a unit test to prevent
future regressions. Currently an EGTest is not possible, as EGTest
doesn't have actions for sending keyboard events.
Removes a DCHECK for OmniboxTextFieldIOS' delegate to be a more
specific subclass, which seems fine as all of the methods of
OTFIDelegate are optional


Bug: 945024
Change-Id: I216bc8361e01e9874623e2105416e2cfa9ff03aa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1547747
Commit-Queue: Stepan Khapugin <stkhapugin@chromium.org>
Auto-Submit: Stepan Khapugin <stkhapugin@chromium.org>
Reviewed-by: Robbie Gibson <rkgibson@google.com>
Cr-Commit-Position: refs/heads/master@{#647751}
  • Loading branch information
Stepan Khapugin authored and Commit Bot committed Apr 4, 2019
1 parent fcfffe6 commit 1b7e6b8
Show file tree
Hide file tree
Showing 5 changed files with 149 additions and 55 deletions.
4 changes: 4 additions & 0 deletions ios/chrome/browser/ui/omnibox/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -134,15 +134,19 @@ source_set("unit_tests") {
testonly = true
sources = [
"omnibox_text_field_ios_unittest.mm",
"omnibox_view_ios_unittest.mm",
]
deps = [
":omnibox",
":omnibox_internal",
":resources_unit_tests",
"//base",
"//base/test:test_support",
"//ios/chrome/app/strings",
"//ios/chrome/browser",
"//ios/chrome/browser/browser_state:test_support",
"//testing/gtest",
"//third_party/ocmock",
"//ui/base",
]
}
Expand Down
35 changes: 9 additions & 26 deletions ios/chrome/browser/ui/omnibox/omnibox_text_field_ios.mm
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,7 @@ @implementation OmniboxTextFieldIOS {
UIColor* _displayedTintColor;
}

@synthesize preEditText = _preEditText;
@synthesize clearingPreEditText = _clearingPreEditText;
@synthesize placeholderTextColor = _placeholderTextColor;
@synthesize incognito = _incognito;
@synthesize suggestionCommandsEndpoint = _suggestionCommandsEndpoint;
@dynamic delegate;

#pragma mark - Public methods
// Overload to allow for code-based initialization.
Expand Down Expand Up @@ -369,20 +365,6 @@ - (UILabel*)preEditStaticLabel {

#pragma mark - Properties

// Enforces that the delegate is an OmniboxTextFieldDelegate.
- (id<OmniboxTextFieldDelegate>)delegate {
id delegate = [super delegate];
DCHECK(delegate == nil ||
[[delegate class]
conformsToProtocol:@protocol(OmniboxTextFieldDelegate)]);
return delegate;
}

// Overridden to require an OmniboxTextFieldDelegate.
- (void)setDelegate:(id<OmniboxTextFieldDelegate>)delegate {
[super setDelegate:delegate];
}

- (UIFont*)largerFont {
return PreferredFontForTextStyleWithMaxCategory(
UIFontTextStyleBody, self.traitCollection.preferredContentSizeCategory,
Expand Down Expand Up @@ -608,15 +590,16 @@ - (BOOL)canPerformAction:(SEL)action withSender:(id)sender {
return YES;
}

// Handle pre-edit shortcuts.
if ([self isPreEditing]) {
// Allow cut and copy in preedit.
if ((action == @selector(copy:)) || (action == @selector(cut:))) {
return YES;
}
}

// Note that this NO does not keep other elements in the responder chain from
// adding actions they handle to the menu.
// No special handling is necessary for pre-edit and autocomplete states.
// In pre-edit, the text in the textfield is selected even though it is not
// shown. so the behavior is correct. As an aside, the only way to access the
// editing menu without exiting the pre-edit state is via accessibility
// features. For inline autocomplete, any action on the textfield first
// accepts the autocompletion and unselects the text. It is therefore not
// possible to open the editing menu in this state.
return NO;
}

Expand Down
29 changes: 29 additions & 0 deletions ios/chrome/browser/ui/omnibox/omnibox_text_field_ios_unittest.mm
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "ios/chrome/grit/ios_strings.h"
#include "testing/gtest_mac.h"
#include "testing/platform_test.h"
#import "third_party/ocmock/OCMock/OCMock.h"
#include "ui/base/l10n/l10n_util_mac.h"

#if !defined(__has_feature) || !__has_feature(objc_arc)
Expand Down Expand Up @@ -170,4 +171,32 @@ void VerifySelectedNSRanges(NSString* text) {
EXPECT_FALSE([textfield_ isPreEditing]);
}

TEST_F(OmniboxTextFieldTest, CopyInPreedit) {
id delegateMock = OCMProtocolMock(@protocol(OmniboxTextFieldDelegate));
NSString* testString = @"omnibox test string";
[textfield_ setText:testString];
textfield_.delegate = delegateMock;
[textfield_ becomeFirstResponder];
[textfield_ enterPreEditState];
EXPECT_TRUE([textfield_ canPerformAction:@selector(copy:) withSender:nil]);
OCMExpect([delegateMock onCopy]).andReturn(YES);
[textfield_ copy:nil];
EXPECT_TRUE([textfield_.text isEqualToString:testString]);
[delegateMock verify];
}

TEST_F(OmniboxTextFieldTest, CutInPreedit) {
id delegateMock = OCMProtocolMock(@protocol(OmniboxTextFieldDelegate));
NSString* testString = @"omnibox test string";
[textfield_ setText:testString];
textfield_.delegate = delegateMock;
[textfield_ becomeFirstResponder];
[textfield_ enterPreEditState];
EXPECT_TRUE([textfield_ canPerformAction:@selector(cut:) withSender:nil]);
OCMExpect([delegateMock onCopy]).andReturn(YES);
[textfield_ cut:nil];
EXPECT_TRUE([textfield_.text isEqualToString:@""]);
[delegateMock verify];
}

} // namespace
78 changes: 49 additions & 29 deletions ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm
Original file line number Diff line number Diff line change
Expand Up @@ -161,9 +161,11 @@ - (void)onDeleteBackward {
id<OmniboxLeftImageConsumer> left_image_consumer,
ios::ChromeBrowserState* browser_state,
id<OmniboxFocuser> omnibox_focuser)
: OmniboxView(
controller,
std::make_unique<ChromeOmniboxClientIOS>(controller, browser_state)),
: OmniboxView(controller,
controller
? std::make_unique<ChromeOmniboxClientIOS>(controller,
browser_state)
: nullptr),
browser_state_(browser_state),
field_(field),
controller_(controller),
Expand Down Expand Up @@ -232,7 +234,7 @@ - (void)onDeleteBackward {
if (update_popup)
UpdatePopup();

if (notify_text_changed)
if (notify_text_changed && model())
model()->OnChanged();

SetCaretPos(caret_pos);
Expand All @@ -254,18 +256,20 @@ - (void)onDeleteBackward {
}

void OmniboxViewIOS::UpdatePopup() {
model()->SetInputInProgress(true);
if (model())
model()->SetInputInProgress(true);

if (!model()->has_focus())
if (model() && !model()->has_focus())
return;

// Prevent inline-autocomplete if the IME is currently composing or if the
// cursor is not at the end of the text.
bool prevent_inline_autocomplete =
IsImeComposing() ||
NSMaxRange(current_selection_) != [[field_ text] length];
model()->StartAutocomplete(current_selection_.length != 0,
prevent_inline_autocomplete);
if (model())
model()->StartAutocomplete(current_selection_.length != 0,
prevent_inline_autocomplete);
DCHECK(popup_provider_);
popup_provider_->SetTextAlignment([field_ bestTextAlignment]);
}
Expand All @@ -276,7 +280,8 @@ - (void)onDeleteBackward {
bool save_original_selection,
bool notify_text_changed) {
SetWindowTextAndCaretPos(display_text, display_text.size(), false, false);
model()->OnChanged();
if (model())
model()->OnChanged();
}

bool OmniboxViewIOS::OnInlineAutocompleteTextMaybeChanged(
Expand All @@ -287,7 +292,8 @@ - (void)onDeleteBackward {

NSAttributedString* as = ApplyTextAttributes(display_text);
[field_ setText:as userTextLength:user_text_length];
model()->OnChanged();
if (model())
model()->OnChanged();
return true;
}

Expand All @@ -308,9 +314,11 @@ - (void)onDeleteBackward {

// iOS does not supports KeywordProvider, so never allow keyword UI changes.
const bool something_changed =
model() &&
model()->OnAfterPossibleChange(state_changes, allow_keyword_ui_change);

model()->OnChanged();
if (model())
model()->OnChanged();

// TODO(justincohen): Find a different place to call this. Give the omnibox
// a chance to update the alignment for a text direction change.
Expand Down Expand Up @@ -363,15 +371,20 @@ - (void)onDeleteBackward {
// strip them out by calling setText (as opposed to setAttributedText).
[field_ setText:[field_ text]];
OnBeforePossibleChange();
// In the case where the user taps the fakebox on the Google landing page,
// or from the secondary toolbar search button, the focus source is already
// set to FAKEBOX or SEARCH_BUTTON respectively. Otherwise, set it to OMNIBOX.
if (model()->focus_source() != OmniboxEditModel::FocusSource::FAKEBOX &&
model()->focus_source() != OmniboxEditModel::FocusSource::SEARCH_BUTTON) {
model()->set_focus_source(OmniboxEditModel::FocusSource::OMNIBOX);
}

model()->OnSetFocus(false);
if (model()) {
// In the case where the user taps the fakebox on the Google landing page,
// or from the secondary toolbar search button, the focus source is already
// set to FAKEBOX or SEARCH_BUTTON respectively. Otherwise, set it to
// OMNIBOX.
if (model()->focus_source() != OmniboxEditModel::FocusSource::FAKEBOX &&
model()->focus_source() !=
OmniboxEditModel::FocusSource::SEARCH_BUTTON) {
model()->set_focus_source(OmniboxEditModel::FocusSource::OMNIBOX);
}

model()->OnSetFocus(false);
}

// If the omnibox is displaying a URL and the popup is not showing, set the
// field into pre-editing state. If the omnibox is displaying search terms,
Expand Down Expand Up @@ -488,7 +501,7 @@ - (void)onDeleteBackward {
omnibox_interacted_while_focused_ = YES;

// Sanitize pasted text.
if (model()->is_pasting()) {
if (model() && model()->is_pasting()) {
base::string16 pastedText = base::SysNSStringToUTF16([field_ text]);
base::string16 newText = OmniboxView::SanitizeTextForPaste(pastedText);
if (pastedText != newText) {
Expand Down Expand Up @@ -550,7 +563,9 @@ - (void)onDeleteBackward {
base::RecordAction(UserMetricsAction("MobileOmniboxUse"));

WindowOpenDisposition disposition = WindowOpenDisposition::CURRENT_TAB;
model()->AcceptInput(disposition, false);
if (model()) {
model()->AcceptInput(disposition, false);
}
RevertAll();
}

Expand Down Expand Up @@ -583,7 +598,9 @@ - (void)onDeleteBackward {

GURL url;
bool write_url = false;
model()->AdjustTextForCopy(start_location, &text, &url, &write_url);
// Model can be nullptr in tests.
if (model())
model()->AdjustTextForCopy(start_location, &text, &url, &write_url);

// Create the pasteboard item manually because the pasteboard expects a single
// item with multiple representations. This is expressed as a single
Expand All @@ -600,7 +617,8 @@ - (void)onDeleteBackward {
}

void OmniboxViewIOS::WillPaste() {
model()->OnPaste();
if (model())
model()->OnPaste();
}

// static
Expand Down Expand Up @@ -628,18 +646,19 @@ - (void)onDeleteBackward {
DCHECK(attributing_display_string_ == nil);
base::AutoReset<NSMutableAttributedString*> resetter(
&attributing_display_string_, as);
UpdateTextStyle(text, model()->CurrentTextIsURL(),
AutocompleteSchemeClassifierImpl());
if (model())
UpdateTextStyle(text, model()->CurrentTextIsURL(),
AutocompleteSchemeClassifierImpl());
return as;
}

void OmniboxViewIOS::UpdateAppearance() {
// If Siri is thinking, treat that as user input being in progress. It is
// unsafe to modify the text field while voice entry is pending.
if (model()->ResetDisplayTexts()) {
if (model() && model()->ResetDisplayTexts()) {
// Revert everything to the baseline look.
RevertAll();
} else if (!model()->has_focus() &&
} else if (model() && !model()->has_focus() &&
!ShouldIgnoreUserInputDueToPendingVoiceSearch()) {
// Even if the change wasn't "user visible" to the model, it still may be
// necessary to re-color to the URL string. Only do this if the omnibox is
Expand All @@ -666,7 +685,8 @@ - (void)onDeleteBackward {
// set to the empty string by OnWillChange so when OnAfterPossibleChange
// checks if the text has changed it does not see any difference so it
// never sets the input-in-progress flag.
model()->SetInputInProgress(YES);
if (model())
model()->SetInputInProgress(YES);
} else {
RemoveQueryRefinementChip();
}
Expand Down Expand Up @@ -714,7 +734,7 @@ - (void)onDeleteBackward {
}

void OmniboxViewIOS::EndEditing() {
if (model()->has_focus()) {
if (model() && model()->has_focus()) {
CloseOmniboxPopup();

model()->OnWillKillFocus();
Expand Down
58 changes: 58 additions & 0 deletions ios/chrome/browser/ui/omnibox/omnibox_view_ios_unittest.mm
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// Copyright 2019 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 "ios/chrome/browser/ui/omnibox/omnibox_view_ios.h"

#include "base/test/scoped_task_environment.h"
#include "ios/chrome/browser/browser_state/test_chrome_browser_state.h"
#include "testing/gtest_mac.h"
#include "testing/platform_test.h"
#import "third_party/ocmock/OCMock/OCMock.h"

#if !defined(__has_feature) || !__has_feature(objc_arc)
#error "This file requires ARC support."
#endif

namespace {

class OmniboxViewIOSTest : public PlatformTest {
protected:
void SetUp() override {
PlatformTest::SetUp();
TestChromeBrowserState::Builder test_cbs_builder;
browser_state_ = test_cbs_builder.Build();
mockOmniboxTextfield_ = OCMClassMock([OmniboxTextFieldIOS class]);
view_ = std::make_unique<OmniboxViewIOS>(
mockOmniboxTextfield_, /* WebOmniboxEditController*/ nullptr,
/*id<OmniboxLeftImageConsumer> */ nil, browser_state_.get(),
/*id<OmniboxFocuser>*/ nil);
}

// Test broser state.
std::unique_ptr<TestChromeBrowserState> browser_state_;
// The tested object.
std::unique_ptr<OmniboxViewIOS> view_;
// Mock for the OmniboxTextFieldIOS.
id mockOmniboxTextfield_;
// Message loop for the main test thread.
base::test::ScopedTaskEnvironment environment_;
};

TEST_F(OmniboxViewIOSTest, copyAddsTextToPasteboard) {
[[UIPasteboard generalPasteboard] setString:@""];

OCMExpect([mockOmniboxTextfield_ isPreEditing]).andReturn(YES);
OCMExpect([mockOmniboxTextfield_ preEditText]).andReturn(@"foobar");

view_->OnCopy();

EXPECT_TRUE(
[[[UIPasteboard generalPasteboard] string] isEqualToString:@"foobar"]);
[mockOmniboxTextfield_ verify];

// Clear the pasteboard state.
[[UIPasteboard generalPasteboard] setString:@""];
}

} // namespace

0 comments on commit 1b7e6b8

Please sign in to comment.