Commit dd9167fe authored by stkhapugin@chromium.org's avatar stkhapugin@chromium.org Committed by Commit Bot

[iOS] Set focus source of omnibox edit model directly.

Exposes a getter and setter for focus_source_ on OmniboxEditModel and
uses them directly to track the focus source (omnibox or fakebox),
instead of relying on a workaround for Linux and using caret visibility
as a proxy.

Bug: 807600
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I6e23c77ca7c8cac39e07cc45c351b076a9183d1c
Reviewed-on: https://chromium-review.googlesource.com/894325
Commit-Queue: Stepan Khapugin <stkhapugin@chromium.org>
Reviewed-by: default avatarGauthier Ambard <gambard@chromium.org>
Reviewed-by: default avatarJustin Cohen <justincohen@chromium.org>
Reviewed-by: default avatarJustin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537350}
parent 8c66c243
...@@ -234,6 +234,11 @@ class OmniboxEditModel { ...@@ -234,6 +234,11 @@ class OmniboxEditModel {
return focus_state_ == OMNIBOX_FOCUS_VISIBLE; return focus_state_ == OMNIBOX_FOCUS_VISIBLE;
} }
FocusSource focus_source() const { return focus_source_; }
void set_focus_source(FocusSource focus_source) {
focus_source_ = focus_source;
}
// Accessors for keyword-related state (see comments on keyword_ and // Accessors for keyword-related state (see comments on keyword_ and
// is_keyword_hint_). // is_keyword_hint_).
const base::string16& keyword() const { return keyword_; } const base::string16& keyword() const { return keyword_; }
......
...@@ -238,8 +238,10 @@ source_set("eg_tests") { ...@@ -238,8 +238,10 @@ source_set("eg_tests") {
"//ios/chrome/browser/reading_list", "//ios/chrome/browser/reading_list",
"//ios/chrome/browser/ui", "//ios/chrome/browser/ui",
"//ios/chrome/browser/ui/content_suggestions/cells:cells_ui", "//ios/chrome/browser/ui/content_suggestions/cells:cells_ui",
"//ios/chrome/browser/ui/location_bar:location_bar",
"//ios/chrome/browser/ui/ntp:ntp_controller", "//ios/chrome/browser/ui/ntp:ntp_controller",
"//ios/chrome/test/app:test_support", "//ios/chrome/test/app:test_support",
"//ios/chrome/test/base:base",
"//ios/chrome/test/earl_grey:test_support", "//ios/chrome/test/earl_grey:test_support",
"//ios/testing:ios_test_support", "//ios/testing:ios_test_support",
"//ios/testing/earl_grey:earl_grey_support", "//ios/testing/earl_grey:earl_grey_support",
......
...@@ -24,6 +24,7 @@ ...@@ -24,6 +24,7 @@
#import "ios/chrome/browser/ui/content_suggestions/ntp_home_constant.h" #import "ios/chrome/browser/ui/content_suggestions/ntp_home_constant.h"
#import "ios/chrome/browser/ui/content_suggestions/ntp_home_provider_test_singleton.h" #import "ios/chrome/browser/ui/content_suggestions/ntp_home_provider_test_singleton.h"
#import "ios/chrome/browser/ui/content_suggestions/ntp_home_test_utils.h" #import "ios/chrome/browser/ui/content_suggestions/ntp_home_test_utils.h"
#import "ios/chrome/browser/ui/location_bar/location_bar_coordinator.h"
#import "ios/chrome/browser/ui/ntp/new_tab_page_controller.h" #import "ios/chrome/browser/ui/ntp/new_tab_page_controller.h"
#include "ios/chrome/browser/ui/ui_util.h" #include "ios/chrome/browser/ui/ui_util.h"
#import "ios/chrome/browser/ui/uikit_ui_util.h" #import "ios/chrome/browser/ui/uikit_ui_util.h"
...@@ -31,6 +32,7 @@ ...@@ -31,6 +32,7 @@
#import "ios/chrome/test/app/chrome_test_util.h" #import "ios/chrome/test/app/chrome_test_util.h"
#import "ios/chrome/test/app/history_test_util.h" #import "ios/chrome/test/app/history_test_util.h"
#import "ios/chrome/test/app/tab_test_util.h" #import "ios/chrome/test/app/tab_test_util.h"
#include "ios/chrome/test/base/scoped_block_swizzler.h"
#include "ios/chrome/test/earl_grey/accessibility_util.h" #include "ios/chrome/test/earl_grey/accessibility_util.h"
#import "ios/chrome/test/earl_grey/chrome_earl_grey.h" #import "ios/chrome/test/earl_grey/chrome_earl_grey.h"
#import "ios/chrome/test/earl_grey/chrome_earl_grey_ui.h" #import "ios/chrome/test/earl_grey/chrome_earl_grey_ui.h"
...@@ -77,7 +79,7 @@ std::unique_ptr<net::test_server::HttpResponse> StandardResponse( ...@@ -77,7 +79,7 @@ std::unique_ptr<net::test_server::HttpResponse> StandardResponse(
// Mock provider from the singleton. // Mock provider from the singleton.
@property(nonatomic, assign, readonly) MockContentSuggestionsProvider* provider; @property(nonatomic, assign, readonly) MockContentSuggestionsProvider* provider;
// Article category, used by the singleton. // Article category, used by the singleton.
@property(nonatomic, assign, readonly) Category category; @property(nonatomic, assign, readonly) ntp_snippets::Category category;
@end @end
...@@ -150,8 +152,8 @@ std::unique_ptr<net::test_server::HttpResponse> StandardResponse( ...@@ -150,8 +152,8 @@ std::unique_ptr<net::test_server::HttpResponse> StandardResponse(
return [[ContentSuggestionsTestSingleton sharedInstance] provider]; return [[ContentSuggestionsTestSingleton sharedInstance] provider];
} }
- (Category)category { - (ntp_snippets::Category)category {
return Category::FromKnownCategory(KnownCategories::ARTICLES); return ntp_snippets::Category::FromKnownCategory(KnownCategories::ARTICLES);
} }
#pragma mark - Tests #pragma mark - Tests
...@@ -400,6 +402,32 @@ std::unique_ptr<net::test_server::HttpResponse> StandardResponse( ...@@ -400,6 +402,32 @@ std::unique_ptr<net::test_server::HttpResponse> StandardResponse(
[ChromeEarlGrey waitForWebViewContainingText:kPageLoadedString]; [ChromeEarlGrey waitForWebViewContainingText:kPageLoadedString];
} }
// Tests that tapping the fake omnibox logs correctly.
// It is important for ranking algorithm of omnibox that requests from fakebox
// and real omnibox are marked appropriately.
- (void)testTapFakeOmniboxLogsCorrectly {
if (!IsIPadIdiom()) {
// This logging only happens on iPad, since on iPhone there is no real
// omnibox on NTP, only fakebox.
return;
}
// Swizzle the method that needs to be called for correct logging.
__block BOOL tapped = NO;
ScopedBlockSwizzler swizzler([LocationBarCoordinator class],
@selector(focusOmniboxFromFakebox), ^() {
tapped = YES;
});
// Tap the fake omnibox.
[[EarlGrey selectElementWithMatcher:grey_accessibilityID(
FakeOmniboxAccessibilityID())]
performAction:grey_tap()];
// Check that the page is loaded.
GREYAssertTrue(tapped, @"The tap on the fakebox was not correctly logged.");
}
// Tests that tapping the fake omnibox moves the collection. // Tests that tapping the fake omnibox moves the collection.
- (void)testTapFakeOmniboxScroll { - (void)testTapFakeOmniboxScroll {
// Get the collection and its layout. // Get the collection and its layout.
......
...@@ -55,8 +55,7 @@ class WebStateList; ...@@ -55,8 +55,7 @@ class WebStateList;
// Indicates if the omnibox currently displays a popup with suggestions. // Indicates if the omnibox currently displays a popup with suggestions.
- (BOOL)showingOmniboxPopup; - (BOOL)showingOmniboxPopup;
// Focuses the omnibox and sets the caret visibility as if it was called from // Focuses omnibox with fakebox as the focus event source.
// the fakebox on NTP.
- (void)focusOmniboxFromFakebox; - (void)focusOmniboxFromFakebox;
// Indicates when the omnibox is the first responder. // Indicates when the omnibox is the first responder.
......
...@@ -153,13 +153,8 @@ ...@@ -153,13 +153,8 @@
- (void)focusOmniboxFromFakebox { - (void)focusOmniboxFromFakebox {
OmniboxEditModel* model = _locationBarController->GetLocationEntry()->model(); OmniboxEditModel* model = _locationBarController->GetLocationEntry()->model();
// Setting the caret visibility to false causes OmniboxEditModel to indicate model->set_focus_source(OmniboxEditModel::FocusSource::FAKEBOX);
// that omnibox interaction was initiated from the fakebox. Note that [self focusOmnibox];
// SetCaretVisibility is a no-op unless OnSetFocus is called first. Only
// set fakebox on iPad, where there is a distinction between the omnibox
// and the fakebox on the NTP.
model->OnSetFocus(false);
model->SetCaretVisibility(false);
} }
- (BOOL)isOmniboxFirstResponder { - (BOOL)isOmniboxFirstResponder {
......
...@@ -367,16 +367,13 @@ void OmniboxViewIOS::OnDidBeginEditing() { ...@@ -367,16 +367,13 @@ void OmniboxViewIOS::OnDidBeginEditing() {
[field_ setText:[field_ text]]; [field_ setText:[field_ text]];
OnBeforePossibleChange(); OnBeforePossibleChange();
// In the case where the user taps the fakebox on the Google landing page, // In the case where the user taps the fakebox on the Google landing page,
// the WebToolbarController invokes OnSetFocus before calling // the focus source is already set to FAKEBOX. Otherwise, set it to OMNIBOX.
// becomeFirstResponder on OmniboxTextFieldIOS (which leads to this method if (model()->focus_source() != OmniboxEditModel::FocusSource::FAKEBOX) {
// beting invoked) so there is no need to call OnSetFocus again. In fact, model()->set_focus_source(OmniboxEditModel::FocusSource::OMNIBOX);
// calling OnSetFocus again here would reset the caret visibility to true and
// it would be impossible to tell that the omnibox was focused by a tap in the
// fakebox instead of the omnibox.
if (!model()->has_focus()) {
model()->OnSetFocus(false);
} }
model()->OnSetFocus(false);
// If the omnibox is displaying a URL and the popup is not showing, set the // 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, // field into pre-editing state. If the omnibox is displaying search terms,
// leave the default behavior of positioning the cursor at the end of the // leave the default behavior of positioning the cursor at the end of the
......
...@@ -123,9 +123,9 @@ ...@@ -123,9 +123,9 @@
// On iPhone there is no visible omnibox, so there's no need to indicate // On iPhone there is no visible omnibox, so there's no need to indicate
// interaction was initiated from the fakebox. // interaction was initiated from the fakebox.
[self.locationBarCoordinator focusOmniboxFromFakebox]; [self.locationBarCoordinator focusOmniboxFromFakebox];
} } else {
[self.locationBarCoordinator focusOmnibox]; [self.locationBarCoordinator focusOmnibox];
}
} }
- (void)onFakeboxBlur { - (void)onFakeboxBlur {
......
...@@ -288,9 +288,8 @@ ...@@ -288,9 +288,8 @@
[self.locationBarCoordinator focusOmniboxFromFakebox]; [self.locationBarCoordinator focusOmniboxFromFakebox];
} else { } else {
[self expandOmniboxAnimated:NO]; [self expandOmniboxAnimated:NO];
}
[self.locationBarCoordinator focusOmnibox]; [self.locationBarCoordinator focusOmnibox];
}
if ([self.locationBarCoordinator omniboxPopupHasAutocompleteResults]) { if ([self.locationBarCoordinator omniboxPopupHasAutocompleteResults]) {
[self onFakeboxAnimationComplete]; [self onFakeboxAnimationComplete];
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment