Commit f64a4c42 authored by Stepan Khapugin's avatar Stepan Khapugin Committed by Chromium LUCI CQ

[iOS] Fix bug where omnibox doesn't defocus on tab switch.

Fixes the bug which was caused by a mistake in a refactoring.
Add a test to prevent regression.

Fixed: 1151035
Change-Id: I9f3d00cfdd13321dd289a9944e155c76bc8b6524
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2600821
Commit-Queue: Robbie Gibson <rkgibson@google.com>
Reviewed-by: default avatarRobbie Gibson <rkgibson@google.com>
Auto-Submit: Stepan Khapugin <stkhapugin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840112}
parent 68593fb0
...@@ -201,6 +201,7 @@ ...@@ -201,6 +201,7 @@
self.mediator.templateURLService = self.mediator.templateURLService =
ios::TemplateURLServiceFactory::GetForBrowserState(self.browserState); ios::TemplateURLServiceFactory::GetForBrowserState(self.browserState);
self.mediator.consumer = self; self.mediator.consumer = self;
self.mediator.webStateList = self.webStateList;
self.steadyViewMediator = [[LocationBarSteadyViewMediator alloc] self.steadyViewMediator = [[LocationBarSteadyViewMediator alloc]
initWithLocationBarModel:[self locationBarModel]]; initWithLocationBarModel:[self locationBarModel]];
...@@ -235,6 +236,7 @@ ...@@ -235,6 +236,7 @@
_editController.reset(); _editController.reset();
self.viewController = nil; self.viewController = nil;
[self.mediator disconnect];
self.mediator = nil; self.mediator = nil;
[self.steadyViewMediator disconnect]; [self.steadyViewMediator disconnect];
self.steadyViewMediator = nil; self.steadyViewMediator = nil;
......
...@@ -9,6 +9,7 @@ ...@@ -9,6 +9,7 @@
@protocol LocationBarConsumer; @protocol LocationBarConsumer;
class TemplateURLService; class TemplateURLService;
class WebStateList;
// A mediator object that updates location bar state not relating to the // A mediator object that updates location bar state not relating to the
// LocationBarSteadyView. In practice, this is any state not relating to the // LocationBarSteadyView. In practice, this is any state not relating to the
...@@ -25,6 +26,13 @@ class TemplateURLService; ...@@ -25,6 +26,13 @@ class TemplateURLService;
// object and may be nil. // object and may be nil.
@property(nonatomic, weak) id<LocationBarConsumer> consumer; @property(nonatomic, weak) id<LocationBarConsumer> consumer;
// The WebStateList that this mediator listens for any changes on the active web
// state.
@property(nonatomic, assign) WebStateList* webStateList;
// Stops observing all objects.
- (void)disconnect;
@end @end
#endif // IOS_CHROME_BROWSER_UI_LOCATION_BAR_LOCATION_BAR_MEDIATOR_H_ #endif // IOS_CHROME_BROWSER_UI_LOCATION_BAR_LOCATION_BAR_MEDIATOR_H_
...@@ -11,6 +11,8 @@ ...@@ -11,6 +11,8 @@
#import "ios/chrome/browser/ui/ntp/ntp_util.h" #import "ios/chrome/browser/ui/ntp/ntp_util.h"
#import "ios/chrome/browser/ui/omnibox/omnibox_util.h" #import "ios/chrome/browser/ui/omnibox/omnibox_util.h"
#import "ios/chrome/browser/ui/util/uikit_ui_util.h" #import "ios/chrome/browser/ui/util/uikit_ui_util.h"
#import "ios/chrome/browser/web_state_list/web_state_list.h"
#import "ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h"
#include "ios/chrome/grit/ios_theme_resources.h" #include "ios/chrome/grit/ios_theme_resources.h"
#include "ios/web/public/navigation/navigation_item.h" #include "ios/web/public/navigation/navigation_item.h"
#import "ios/web/public/navigation/navigation_manager.h" #import "ios/web/public/navigation/navigation_manager.h"
...@@ -20,7 +22,7 @@ ...@@ -20,7 +22,7 @@
#error "This file requires ARC support." #error "This file requires ARC support."
#endif #endif
@interface LocationBarMediator () <SearchEngineObserving> @interface LocationBarMediator () <SearchEngineObserving, WebStateListObserving>
// Whether the current default search engine supports search by image. // Whether the current default search engine supports search by image.
@property(nonatomic, assign) BOOL searchEngineSupportsSearchByImage; @property(nonatomic, assign) BOOL searchEngineSupportsSearchByImage;
...@@ -29,16 +31,26 @@ ...@@ -29,16 +31,26 @@
@implementation LocationBarMediator { @implementation LocationBarMediator {
std::unique_ptr<SearchEngineObserverBridge> _searchEngineObserver; std::unique_ptr<SearchEngineObserverBridge> _searchEngineObserver;
std::unique_ptr<WebStateListObserverBridge> _webStateListObserver;
} }
- (instancetype)init { - (instancetype)init {
self = [super init]; self = [super init];
if (self) { if (self) {
_searchEngineSupportsSearchByImage = NO; _searchEngineSupportsSearchByImage = NO;
_webStateListObserver = std::make_unique<WebStateListObserverBridge>(self);
} }
return self; return self;
} }
- (void)disconnect {
self.webStateList = nil;
}
- (void)dealloc {
[self disconnect];
}
#pragma mark - SearchEngineObserving #pragma mark - SearchEngineObserving
- (void)searchEngineChanged { - (void)searchEngineChanged {
...@@ -73,4 +85,27 @@ ...@@ -73,4 +85,27 @@
} }
} }
- (void)setWebStateList:(WebStateList*)webStateList {
if (_webStateList) {
_webStateList->RemoveObserver(_webStateListObserver.get());
}
_webStateList = webStateList;
if (_webStateList) {
_webStateList->AddObserver(_webStateListObserver.get());
}
}
#pragma mark - WebStateListObserver
- (void)webStateList:(WebStateList*)webStateList
didChangeActiveWebState:(web::WebState*)newWebState
oldWebState:(web::WebState*)oldWebState
atIndex:(int)atIndex
reason:(ActiveWebStateChangeReason)reason {
DCHECK_EQ(_webStateList, webStateList);
[self.consumer defocusOmnibox];
}
@end @end
...@@ -238,6 +238,7 @@ source_set("eg2_tests") { ...@@ -238,6 +238,7 @@ source_set("eg2_tests") {
":test_support+eg2", ":test_support+eg2",
"//ios/chrome/app/strings:ios_strings_grit", "//ios/chrome/app/strings:ios_strings_grit",
"//ios/chrome/browser/ui/content_suggestions:content_suggestions_constant", "//ios/chrome/browser/ui/content_suggestions:content_suggestions_constant",
"//ios/chrome/browser/ui/omnibox/popup:popup_accessibility_identifier_constants",
"//ios/chrome/test/earl_grey:eg_test_support+eg2", "//ios/chrome/test/earl_grey:eg_test_support+eg2",
"//ios/testing/earl_grey:eg_test_support+eg2", "//ios/testing/earl_grey:eg_test_support+eg2",
"//ios/third_party/earl_grey2:test_lib", "//ios/third_party/earl_grey2:test_lib",
......
...@@ -11,6 +11,7 @@ ...@@ -11,6 +11,7 @@
#import "base/test/ios/wait_util.h" #import "base/test/ios/wait_util.h"
#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/omnibox/omnibox_app_interface.h" #import "ios/chrome/browser/ui/omnibox/omnibox_app_interface.h"
#import "ios/chrome/browser/ui/omnibox/popup/omnibox_popup_accessibility_identifier_constants.h"
#include "ios/chrome/grit/ios_strings.h" #include "ios/chrome/grit/ios_strings.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"
...@@ -421,6 +422,65 @@ id<GREYMatcher> SearchCopiedTextButton() { ...@@ -421,6 +422,65 @@ id<GREYMatcher> SearchCopiedTextButton() {
assertWithMatcher:chrome_test_util::OmniboxContainingText(kPage1URL)]; assertWithMatcher:chrome_test_util::OmniboxContainingText(kPage1URL)];
} }
- (void)testOmniboxDefocusesOnTabSwitch {
[self openPage1];
[ChromeEarlGrey openNewTab];
[ChromeEarlGrey waitForMainTabCount:2];
[self openPage2];
[ChromeEarlGreyUI focusOmnibox];
[[EarlGrey selectElementWithMatcher:chrome_test_util::Omnibox()]
performAction:grey_typeText(@"Obama")];
// The popup should open.
[[EarlGrey
selectElementWithMatcher:
grey_accessibilityID(kOmniboxPopupTableViewAccessibilityIdentifier)]
assertWithMatcher:grey_notNil()];
// Switch to the first tab.
[ChromeEarlGrey selectTabAtIndex:0];
[ChromeEarlGrey waitForWebStateContainingText:kPage1];
// The omnibox shouldn't be focused and the popup should be closed.
[self checkLocationBarSteadyState];
[[EarlGrey
selectElementWithMatcher:
grey_accessibilityID(kOmniboxPopupTableViewAccessibilityIdentifier)]
assertWithMatcher:grey_notVisible()];
}
- (void)testOmniboxDefocusesOnTabSwitchIncognito {
[ChromeEarlGrey openNewIncognitoTab];
[ChromeEarlGrey waitForIncognitoTabCount:1];
[self openPage1];
[ChromeEarlGrey openNewIncognitoTab];
[ChromeEarlGrey waitForIncognitoTabCount:2];
[self openPage2];
[ChromeEarlGreyUI focusOmnibox];
[[EarlGrey selectElementWithMatcher:chrome_test_util::Omnibox()]
performAction:grey_typeText(@"Obama")];
// The popup should open.
[[EarlGrey
selectElementWithMatcher:
grey_accessibilityID(kOmniboxPopupTableViewAccessibilityIdentifier)]
assertWithMatcher:grey_notNil()];
// Switch to the first tab.
[ChromeEarlGrey selectTabAtIndex:0];
[ChromeEarlGrey waitForWebStateContainingText:kPage1];
// The omnibox shouldn't be focused and the popup should be closed.
[self checkLocationBarSteadyState];
[[EarlGrey
selectElementWithMatcher:
grey_accessibilityID(kOmniboxPopupTableViewAccessibilityIdentifier)]
assertWithMatcher:grey_notVisible()];
}
#pragma mark - Helpers #pragma mark - Helpers
// Navigates to Page 1 in a tab and waits for it to load. // Navigates to Page 1 in a tab and waits for it to load.
......
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