Commit e1ffc6da authored by Ewann's avatar Ewann Committed by Commit Bot

[iOS] Adds action sheet to avoid unwanted tab clearing

This CL is developed under a flag.
It adds an action sheet that asks for confirmation when 'Close All' button is tapped on the tab grid to avoid unwanted clearing
Metrics related to the "Close all" button have been moved in the mediator.

Bug: 1119319
Change-Id: Ie1901b7f24faf63745290310efef805d11859940
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2362925
Commit-Queue: Mark Cogan <marq@chromium.org>
Reviewed-by: default avatarMark Cogan <marq@chromium.org>
Reviewed-by: default avatarGauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803414}
parent 93eb2b03
......@@ -1272,6 +1272,11 @@
"owners": [ "rkgibson", "gangwu", "chrome-omnibox-team@google.com" ],
"expiry_milestone": 90
},
{
"name": "enable-close-all-tabs-confirmation",
"owners": [ "ewannpv. gambard" ],
"expiry_milestone": 90
},
{
"name": "enable-close-tab-suggestions",
"owners": [ "memex-team@google.com" ],
......
......@@ -57,6 +57,7 @@ source_set("flags") {
"//ios/chrome/browser/ui/infobars:feature_flags",
"//ios/chrome/browser/ui/page_info:features",
"//ios/chrome/browser/ui/settings/autofill:feature_flags",
"//ios/chrome/browser/ui/tab_grid:features",
"//ios/chrome/browser/ui/table_view:feature_flags",
"//ios/chrome/browser/ui/toolbar/public:feature_flags",
"//ios/chrome/browser/ui/toolbar_container:feature_flags",
......
......@@ -69,6 +69,7 @@
#import "ios/chrome/browser/ui/infobars/infobar_feature.h"
#import "ios/chrome/browser/ui/page_info/features.h"
#include "ios/chrome/browser/ui/settings/autofill/features.h"
#import "ios/chrome/browser/ui/tab_grid/features.h"
#import "ios/chrome/browser/ui/table_view/feature_flags.h"
#import "ios/chrome/browser/ui/toolbar/public/features.h"
#import "ios/chrome/browser/ui/toolbar_container/toolbar_container_features.h"
......@@ -677,6 +678,10 @@ const flags_ui::FeatureEntry kFeatureEntries[] = {
flag_descriptions::kWellKnownChangePasswordName,
flag_descriptions::kWellKnownChangePasswordDescription, flags_ui::kOsIos,
FEATURE_VALUE_TYPE(password_manager::features::kWellKnownChangePassword)},
{"enable-close-all-tabs-confirmation",
flag_descriptions::kEnableCloseAllTabsConfirmationName,
flag_descriptions::kEnableCloseAllTabsConfirmationDescription,
flags_ui::kOsIos, FEATURE_VALUE_TYPE(kEnableCloseAllTabsConfirmation)},
};
bool SkipConditionalFeatureEntry(const flags_ui::FeatureEntry& entry) {
......
......@@ -235,6 +235,12 @@ const char kEnableClipboardProviderImageSuggestionsName[] =
const char kEnableClipboardProviderImageSuggestionsDescription[] =
"Enable suggesting a search for the image copied to the clipboard";
const char kEnableCloseAllTabsConfirmationName[] =
"Enable Close All Tabs confirmation";
const char kEnableCloseAllTabsConfirmationDescription[] =
"Enable showing an action sheet that asks for confirmation when 'Close "
"All' button is tapped on the tab grid to avoid unwanted clearing.";
const char kEnableFullPageScreenshotName[] = "Enable fullpage screenshots";
const char kEnableFullPageScreenshotDescription[] =
"Enables the option of capturing an entire webpage as a PDF when a "
......
......@@ -196,6 +196,11 @@ extern const char kEnableAutofillPasswordReauthIOSDescription[];
extern const char kEnableClipboardProviderImageSuggestionsName[];
extern const char kEnableClipboardProviderImageSuggestionsDescription[];
// Title and description for the flag to enable the confirmational action sheet
// for the tab grid "Close All" action.
extern const char kEnableCloseAllTabsConfirmationName[];
extern const char kEnableCloseAllTabsConfirmationDescription[];
// Title and description for the flag to enable fullpage screenshots.
extern const char kEnableFullPageScreenshotName[];
extern const char kEnableFullPageScreenshotDescription[];
......
......@@ -21,6 +21,8 @@ source_set("tab_grid") {
"//base",
"//components/favicon/ios",
"//components/sessions",
"//components/strings",
"//ios/chrome/app/strings",
"//ios/chrome/browser",
"//ios/chrome/browser/browser_state",
"//ios/chrome/browser/drag_and_drop",
......@@ -32,6 +34,7 @@ source_set("tab_grid") {
"//ios/chrome/browser/tabs",
"//ios/chrome/browser/ui:feature_flags",
"//ios/chrome/browser/ui/activity_services",
"//ios/chrome/browser/ui/alert_coordinator",
"//ios/chrome/browser/ui/commands",
"//ios/chrome/browser/ui/coordinators:chrome_coordinators",
"//ios/chrome/browser/ui/history",
......@@ -85,6 +88,7 @@ source_set("tab_grid_ui") {
configs += [ "//build/config/compiler:enable_arc" ]
deps = [
":features",
":tab_grid_ui_constants",
"grid:grid_ui",
"grid:grid_ui_constants",
......@@ -111,6 +115,15 @@ source_set("tab_grid_ui") {
]
}
source_set("features") {
configs += [ "//build/config/compiler:enable_arc" ]
sources = [
"features.h",
"features.mm",
]
deps = [ "//base" ]
}
source_set("unit_tests") {
configs += [ "//build/config/compiler:enable_arc" ]
testonly = true
......
// Copyright 2020 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.
#ifndef IOS_CHROME_BROWSER_UI_TAB_GRID_FEATURES_H_
#define IOS_CHROME_BROWSER_UI_TAB_GRID_FEATURES_H_
#include "base/feature_list.h"
// Feature to enable Close All Tabs confirmation.
extern const base::Feature kEnableCloseAllTabsConfirmation;
#endif // IOS_CHROME_BROWSER_UI_TAB_GRID_FEATURES_H_
// Copyright 2020 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/tab_grid/features.h"
#if !defined(__has_feature) || !__has_feature(objc_arc)
#error "This file requires ARC support."
#endif
const base::Feature kEnableCloseAllTabsConfirmation{
"EnableCloseAllTabsConfirmation", base::FEATURE_DISABLED_BY_DEFAULT};
......@@ -24,7 +24,7 @@
// Tells the receiver to close the item with identifier |itemID|. If there is
// no item with that identifier, no item is closed.
- (void)closeItemWithID:(NSString*)itemID;
// Tells the receiver to close all items. This operation does not save items.
// Tells the receiver to close all items.
- (void)closeAllItems;
// Tells the receiver to save all items for an undo operation, then close all
// items.
......@@ -35,6 +35,9 @@
// Tells the receiver to discard saved closed items. If the consumer has saved
// closed items, it will discard them. Otherwise, this is a no-op.
- (void)discardSavedClosedItems;
// Shows an action sheet that asks for confirmation when 'Close All' button is
// tapped.
- (void)showCloseAllConfirmationActionSheet;
@end
#endif // IOS_CHROME_BROWSER_UI_TAB_GRID_GRID_GRID_COMMANDS_H_
......@@ -9,11 +9,13 @@
#include "base/metrics/histogram_functions.h"
#include "base/metrics/user_metrics.h"
#include "base/metrics/user_metrics_action.h"
#include "components/strings/grit/components_strings.h"
#include "ios/chrome/browser/browser_state/chrome_browser_state.h"
#include "ios/chrome/browser/chrome_url_constants.h"
#include "ios/chrome/browser/main/browser.h"
#include "ios/chrome/browser/sessions/ios_chrome_tab_restore_service_factory.h"
#import "ios/chrome/browser/ui/activity_services/activity_params.h"
#import "ios/chrome/browser/ui/alert_coordinator/action_sheet_coordinator.h"
#import "ios/chrome/browser/ui/commands/application_commands.h"
#import "ios/chrome/browser/ui/commands/browser_commands.h"
#import "ios/chrome/browser/ui/commands/browsing_data_commands.h"
......@@ -28,6 +30,7 @@
#import "ios/chrome/browser/ui/recent_tabs/recent_tabs_table_view_controller.h"
#include "ios/chrome/browser/ui/recent_tabs/synced_sessions.h"
#import "ios/chrome/browser/ui/sharing/sharing_coordinator.h"
#import "ios/chrome/browser/ui/tab_grid/grid/grid_commands.h"
#import "ios/chrome/browser/ui/tab_grid/tab_grid_coordinator_delegate.h"
#import "ios/chrome/browser/ui/tab_grid/tab_grid_mediator.h"
#import "ios/chrome/browser/ui/tab_grid/tab_grid_paging.h"
......@@ -38,15 +41,18 @@
#import "ios/chrome/browser/url_loading/url_loading_browser_agent.h"
#import "ios/chrome/browser/url_loading/url_loading_params.h"
#import "ios/chrome/browser/web_state_list/web_state_list.h"
#include "ios/chrome/grit/ios_strings.h"
#include "ui/base/l10n/l10n_util.h"
#if !defined(__has_feature) || !__has_feature(objc_arc)
#error "This file requires ARC support."
#endif
@interface TabGridCoordinator () <TabPresentationDelegate,
HistoryPresentationDelegate,
@interface TabGridCoordinator () <HistoryPresentationDelegate,
RecentTabsContextMenuDelegate,
RecentTabsPresentationDelegate> {
RecentTabsPresentationDelegate,
TabGridMediatorDelegate,
TabPresentationDelegate> {
// Use an explicit ivar instead of synthesizing as the setter isn't using the
// ivar.
Browser* _incognitoBrowser;
......@@ -75,6 +81,8 @@
@property(nonatomic, strong) SharingCoordinator* sharingCoordinator;
@property(nonatomic, strong)
RecentTabsContextMenuHelper* recentTabsContextMenuHelper;
// The action sheet coordinator, if one is currently being shown.
@property(nonatomic, strong) ActionSheetCoordinator* actionSheetCoordinator;
@end
......@@ -265,6 +273,7 @@
_regularBrowser ? _regularBrowser->GetWebStateList() : nullptr;
self.regularTabsMediator.browser = _regularBrowser;
self.regularTabsMediator.delegate = self;
if (regularBrowserState) {
self.regularTabsMediator.tabRestoreService =
IOSChromeTabRestoreServiceFactory::GetForBrowserState(
......@@ -273,6 +282,7 @@
self.incognitoTabsMediator = [[TabGridMediator alloc]
initWithConsumer:baseViewController.incognitoTabsConsumer];
self.incognitoTabsMediator.browser = _incognitoBrowser;
self.incognitoTabsMediator.delegate = self;
baseViewController.regularTabsDelegate = self.regularTabsMediator;
baseViewController.incognitoTabsDelegate = self.incognitoTabsMediator;
baseViewController.regularTabsDragDropHandler = self.regularTabsMediator;
......@@ -352,6 +362,8 @@
[self.baseViewController.remoteTabsViewController dismissModals];
[self.remoteTabsMediator disconnect];
self.remoteTabsMediator = nil;
[self.actionSheetCoordinator stop];
self.actionSheetCoordinator = nil;
}
#pragma mark - TabPresentationDelegate
......@@ -381,6 +393,37 @@
focusOmnibox:focusOmnibox];
}
- (void)showCloseAllConfirmationActionSheetWitTabGridMediator:
(TabGridMediator*)tabGridMediator
numberOfTabs:
(NSInteger)numberOfTabs {
self.actionSheetCoordinator = [[ActionSheetCoordinator alloc]
initWithBaseViewController:self.baseViewController
browser:self.browser
title:nil
message:nil
rect:self.baseViewController.view.frame
view:self.baseViewController.view];
self.actionSheetCoordinator.popoverArrowDirection = 0;
self.actionSheetCoordinator.alertStyle =
IsIPadIdiom() ? UIAlertControllerStyleAlert
: UIAlertControllerStyleActionSheet;
// TODO(crbug.com/1119319): Displays the number of tabs to close.
[self.actionSheetCoordinator
addItemWithTitle:l10n_util::GetNSString(IDS_IOS_TAB_GRID_CLOSE_ALL_BUTTON)
action:^{
[tabGridMediator closeAllItems];
}
style:UIAlertActionStyleDestructive];
[self.actionSheetCoordinator
addItemWithTitle:l10n_util::GetNSString(IDS_CANCEL)
action:^{
}
style:UIAlertActionStyleCancel];
[self.actionSheetCoordinator start];
}
#pragma mark - RecentTabsPresentationDelegate
- (void)dismissRecentTabs {
......
......@@ -11,13 +11,27 @@
#import "ios/chrome/browser/ui/tab_grid/grid/grid_drag_drop_handler.h"
#import "ios/chrome/browser/ui/tab_grid/grid/grid_image_data_source.h"
@protocol GridConsumer;
class Browser;
@protocol GridConsumer;
@class TabGridMediator;
namespace sessions {
class TabRestoreService;
} // namespace sessions
// Delegate protocol for an object that can handle the action sheet that asks
// for confirmation from the tab grid.
@protocol TabGridMediatorDelegate <NSObject>
// Shows an action sheet that asks for confirmation when 'Close All' button is
// tapped.
- (void)showCloseAllConfirmationActionSheetWitTabGridMediator:
(TabGridMediator*)tabGridMediator
numberOfTabs:
(NSInteger)numberOfTabs;
@end
// Mediates between model layer and tab grid UI layer.
@interface TabGridMediator
: NSObject <GridCommands, GridDragDropHandler, GridImageDataSource>
......@@ -26,6 +40,8 @@ class TabRestoreService;
@property(nonatomic, assign) Browser* browser;
// TabRestoreService holds the recently closed tabs.
@property(nonatomic, assign) sessions::TabRestoreService* tabRestoreService;
// Delegate to handle presenting the action sheet.
@property(nonatomic, weak) id<TabGridMediatorDelegate> delegate;
// Initializer with |consumer| as the receiver of model layer updates.
- (instancetype)initWithConsumer:(id<GridConsumer>)consumer
......
......@@ -10,6 +10,8 @@
#include "base/bind.h"
#include "base/metrics/histogram_macros.h"
#include "base/metrics/user_metrics.h"
#include "base/metrics/user_metrics_action.h"
#include "base/scoped_observer.h"
#include "components/favicon/ios/web_favicon_driver.h"
#include "components/sessions/core/tab_restore_service.h"
......@@ -315,12 +317,23 @@ web::WebState* GetWebStateWithId(WebStateList* web_state_list,
}
- (void)closeAllItems {
if (!self.browserState->IsOffTheRecord()) {
base::RecordAction(
base::UserMetricsAction("MobileTabGridCloseAllRegularTabs"));
} else {
base::RecordAction(
base::UserMetricsAction("MobileTabGridCloseAllIncognitoTabs"));
}
// This is a no-op if |webStateList| is already empty.
self.webStateList->CloseAllWebStates(WebStateList::CLOSE_USER_ACTION);
SnapshotBrowserAgent::FromBrowser(self.browser)->RemoveAllSnapshots();
}
// TODO(crbug.com/1123536): Merges this method with |closeAllItems| once
// EnableCloseAllTabsConfirmation is landed.
- (void)saveAndCloseAllItems {
base::RecordAction(
base::UserMetricsAction("MobileTabGridCloseAllRegularTabs"));
if (self.webStateList->empty())
return;
self.closedSessionWindow = SerializeWebStateList(self.webStateList);
......@@ -334,6 +347,8 @@ web::WebState* GetWebStateWithId(WebStateList* web_state_list,
}
- (void)undoCloseAllItems {
base::RecordAction(
base::UserMetricsAction("MobileTabGridUndoCloseAllRegularTabs"));
if (!self.closedSessionWindow)
return;
SessionRestorationBrowserAgent::FromBrowser(self.browser)
......@@ -351,6 +366,13 @@ web::WebState* GetWebStateWithId(WebStateList* web_state_list,
SnapshotBrowserAgent::FromBrowser(self.browser)->RemoveAllSnapshots();
}
- (void)showCloseAllConfirmationActionSheet {
[self.delegate
showCloseAllConfirmationActionSheetWitTabGridMediator:self
numberOfTabs:self.webStateList
->count()];
}
#pragma mark GridCommands helpers
- (void)insertNewItemAtIndex:(NSUInteger)index withURL:(const GURL&)newTabURL {
......
......@@ -13,6 +13,7 @@
#include "ios/chrome/browser/crash_report/crash_keys_helper.h"
#import "ios/chrome/browser/ui/commands/application_commands.h"
#import "ios/chrome/browser/ui/recent_tabs/recent_tabs_table_view_controller.h"
#import "ios/chrome/browser/ui/tab_grid/features.h"
#import "ios/chrome/browser/ui/tab_grid/grid/grid_commands.h"
#import "ios/chrome/browser/ui/tab_grid/grid/grid_constants.h"
#import "ios/chrome/browser/ui/tab_grid/grid/grid_consumer.h"
......@@ -742,7 +743,12 @@ NSUInteger GetPageIndexFromPage(TabGridPage page) {
[self.view addSubview:topToolbar];
topToolbar.leadingButton.target = self;
topToolbar.leadingButton.action = @selector(closeAllButtonTapped:);
if (base::FeatureList::IsEnabled(kEnableCloseAllTabsConfirmation)) {
topToolbar.leadingButton.action =
@selector(closeAllButtonTappedShowConfirmation:);
} else {
topToolbar.leadingButton.action = @selector(closeAllButtonTapped:);
}
topToolbar.trailingButton.title =
l10n_util::GetNSString(IDS_IOS_TAB_GRID_DONE_BUTTON);
topToolbar.trailingButton.accessibilityIdentifier =
......@@ -781,7 +787,12 @@ NSUInteger GetPageIndexFromPage(TabGridPage page) {
]];
bottomToolbar.leadingButton.target = self;
bottomToolbar.leadingButton.action = @selector(closeAllButtonTapped:);
if (base::FeatureList::IsEnabled(kEnableCloseAllTabsConfirmation)) {
bottomToolbar.leadingButton.action =
@selector(closeAllButtonTappedShowConfirmation:);
} else {
bottomToolbar.leadingButton.action = @selector(closeAllButtonTapped:);
}
bottomToolbar.trailingButton.title =
l10n_util::GetNSString(IDS_IOS_TAB_GRID_DONE_BUTTON);
bottomToolbar.trailingButton.accessibilityIdentifier =
......@@ -1198,23 +1209,33 @@ NSUInteger GetPageIndexFromPage(TabGridPage page) {
}
}
// Shows an action sheet that asks for confirmation when 'Close All' button is
// tapped.
- (void)closeAllButtonTappedShowConfirmation:(id)sender {
switch (self.currentPage) {
case TabGridPageIncognitoTabs:
[self.incognitoTabsDelegate showCloseAllConfirmationActionSheet];
break;
case TabGridPageRegularTabs:
[self.regularTabsDelegate showCloseAllConfirmationActionSheet];
break;
case TabGridPageRemoteTabs:
NOTREACHED() << "It is invalid to call close all tabs on remote tabs.";
break;
}
}
- (void)closeAllButtonTapped:(id)sender {
switch (self.currentPage) {
case TabGridPageIncognitoTabs:
base::RecordAction(
base::UserMetricsAction("MobileTabGridCloseAllIncognitoTabs"));
[self.incognitoTabsDelegate closeAllItems];
break;
case TabGridPageRegularTabs:
DCHECK_EQ(self.undoCloseAllAvailable,
self.regularTabsViewController.gridEmpty);
if (self.undoCloseAllAvailable) {
base::RecordAction(
base::UserMetricsAction("MobileTabGridUndoCloseAllRegularTabs"));
[self.regularTabsDelegate undoCloseAllItems];
} else {
base::RecordAction(
base::UserMetricsAction("MobileTabGridCloseAllRegularTabs"));
[self.regularTabsDelegate saveAndCloseAllItems];
}
self.undoCloseAllAvailable = !self.undoCloseAllAvailable;
......
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