Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix FXIOS-8364 [v123] Add confirmation dialog #18581

Merged
merged 2 commits into from
Feb 7, 2024
Merged

Bugfix FXIOS-8364 [v123] Add confirmation dialog #18581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea7522f
Add confirmation dialog
OrlaM Feb 5, 2024
ee27544
Fix swiftlint
OrlaM Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions firefox-ios/Client/Frontend/Browser/Tabs/Action/TabPanelAction.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ enum TabPanelAction: Action {
case closeTab(TabUUIDContext)
case undoClose(ActionContext)
case closeAllTabs(ActionContext)
case confirmCloseAllTabs(ActionContext)
case undoCloseAllTabs(ActionContext)
case moveTab(MoveTabContext)
case toggleInactiveTabs(ActionContext)
Expand All @@ -122,6 +123,7 @@ enum TabPanelAction: Action {
.closeTab(let context as ActionContext),
.undoClose(let context),
.closeAllTabs(let context),
.confirmCloseAllTabs(let context),
.undoCloseAllTabs(let context),
.moveTab(let context as ActionContext),
.toggleInactiveTabs(let context),
Expand Down
2 changes: 1 addition & 1 deletion firefox-ios/Client/Frontend/Browser/Tabs/Middleware/TabManagerMiddleware.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class TabManagerMiddleware {
case TabPanelAction.undoClose:
self.undoCloseTab(state: state, uuid: uuid)

case TabPanelAction.closeAllTabs:
case TabPanelAction.confirmCloseAllTabs:
self.closeAllTabs(state: state, uuid: uuid)

case TabPanelAction.undoCloseAllTabs:
Expand Down
23 changes: 19 additions & 4 deletions firefox-ios/Client/Frontend/Browser/Tabs/State/TabTrayState.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ struct TabTrayState: ScreenState, Equatable {
var shouldDismiss: Bool
var shareURL: URL?
var windowUUID: WindowUUID
var showCloseConfirmation: Bool

var navigationTitle: String {
return selectedPanel.navTitle
Expand All @@ -42,7 +43,8 @@ struct TabTrayState: ScreenState, Equatable {
normalTabsCount: panelState.normalTabsCount,
hasSyncableAccount: panelState.hasSyncableAccount,
shouldDismiss: panelState.shouldDismiss,
shareURL: panelState.shareURL)
shareURL: panelState.shareURL,
showCloseConfirmation: panelState.showCloseConfirmation)
}

init(windowUUID: WindowUUID) {
Expand All @@ -68,14 +70,16 @@ struct TabTrayState: ScreenState, Equatable {
normalTabsCount: String,
hasSyncableAccount: Bool,
shouldDismiss: Bool = false,
shareURL: URL? = nil) {
shareURL: URL? = nil,
showCloseConfirmation: Bool = false) {
self.windowUUID = windowUUID
self.isPrivateMode = isPrivateMode
self.selectedPanel = selectedPanel
self.normalTabsCount = normalTabsCount
self.hasSyncableAccount = hasSyncableAccount
self.shouldDismiss = shouldDismiss
self.shareURL = shareURL
self.showCloseConfirmation = showCloseConfirmation
}

static let reducer: Reducer<Self> = { state, action in
Expand Down Expand Up @@ -134,14 +138,25 @@ struct TabTrayState: ScreenState, Equatable {
// Only update the nomal tab count if the tabs being refreshed are not private
let tabModel = context.tabDisplayModel
let isPrivate = tabModel.tabs.first?.isPrivate ?? false
let tabCount = tabModel.normalTabsCount
let tabCount = isPrivate ? state.normalTabsCount : tabModel.normalTabsCount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the difference between state.normalTabsCount and tabModel.normalTabsCount?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible tabModel.normalTabsCount might not be calculated in certain cases if it's updating the private window so just safer to keep the value the same as it's current value since the private screen cannot impact this number for the normal screen.

return TabTrayState(windowUUID: state.windowUUID,
isPrivateMode: state.isPrivateMode,
selectedPanel: state.selectedPanel,
normalTabsCount: tabCount,
hasSyncableAccount: state.hasSyncableAccount)
case TabPanelAction.closeAllTabs(let context):
return TabTrayState(windowUUID: state.windowUUID,
isPrivateMode: state.isPrivateMode,
selectedPanel: state.selectedPanel,
normalTabsCount: state.normalTabsCount,
hasSyncableAccount: state.hasSyncableAccount,
showCloseConfirmation: true)
default:
return state
return TabTrayState(windowUUID: state.windowUUID,
isPrivateMode: state.isPrivateMode,
selectedPanel: state.selectedPanel,
normalTabsCount: state.normalTabsCount,
hasSyncableAccount: state.hasSyncableAccount)
}
}
}
11 changes: 6 additions & 5 deletions firefox-ios/Client/Frontend/Browser/Tabs/State/TabsPanelState.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,12 @@ struct TabsPanelState: ScreenState, Equatable {
tabs: state.tabs,
inactiveTabs: state.inactiveTabs,
isInactiveTabsExpanded: state.isInactiveTabsExpanded)
default: return TabsPanelState(windowUUID: state.windowUUID,
isPrivateMode: state.isPrivateMode,
tabs: state.tabs,
inactiveTabs: state.inactiveTabs,
isInactiveTabsExpanded: state.isInactiveTabsExpanded)
default:
return TabsPanelState(windowUUID: state.windowUUID,
isPrivateMode: state.isPrivateMode,
tabs: state.tabs,
inactiveTabs: state.inactiveTabs,
isInactiveTabsExpanded: state.isInactiveTabsExpanded)
}
}
}
21 changes: 21 additions & 0 deletions firefox-ios/Client/Frontend/Browser/Tabs/Views/TabTrayViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,9 @@ class TabTrayViewController: UIViewController,
if let url = tabTrayState.shareURL {
navigationHandler?.shareTab(url: url, sourceView: self.view)
}
if tabTrayState.showCloseConfirmation {
showCloseAllConfirmation()
}
}

func updateTabCountImage(count: String) {
Expand Down Expand Up @@ -460,6 +463,24 @@ class TabTrayViewController: UIViewController,
store.dispatch(TabPanelAction.closeAllTabs(windowUUID.context))
}

private func showCloseAllConfirmation() {
let controller = AlertController(title: nil, message: nil, preferredStyle: .actionSheet)
controller.addAction(UIAlertAction(title: .AppMenu.AppMenuCloseAllTabsTitleString,
style: .default,
handler: { _ in self.confirmCloseAll() }),
accessibilityIdentifier: AccessibilityIdentifiers.TabTray.deleteCloseAllButton)
controller.addAction(UIAlertAction(title: .TabTrayCloseAllTabsPromptCancel,
style: .cancel,
handler: nil),
accessibilityIdentifier: AccessibilityIdentifiers.TabTray.deleteCancelButton)
controller.popoverPresentationController?.barButtonItem = deleteButton
present(controller, animated: true, completion: nil)
}
Copy link
Contributor

@dataports dataports Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, we don't have any existing Alert Controller designs for destructive actions? The UX of this to me doesn't emphasize the fact that this is to delete a bunch of data (for example, using red in the text of the confirmation)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improving the UX is mostly out of scope for this project so I think it's fine to keep it the same for now but this is a very good point, would you mind creating a bug for it and we can tackle it through our regular triage process since it' needs UX input.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a bug here https://mozilla-hub.atlassian.net/browse/FXIOS-8399


private func confirmCloseAll() {
store.dispatch(TabPanelAction.confirmCloseAllTabs(windowUUID.context))
}

@objc
private func newTabButtonTapped() {
let context = AddNewTabContext(urlRequest: nil, isPrivate: tabTrayState.isPrivateMode, windowUUID: windowUUID)
Expand Down
Loading