-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
Fix FXIOS-11101 Bugfix close button #24366
base: main
Are you sure you want to change the base?
Conversation
Fix: Display action sheet with "Close All Tabs" and "Cancel" options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you @nathan9513 for being interested in working on our codebase! I left a couple of comments. While the Close all tabs/cancel pop up is displayed again, tapping on close all tabs does not perform the action expected.
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2025-01-28.at.08.51.03.mp4
I noticed that this ticket is also not tagged with the Contributor OK
label. This means that it hasn't been vetted to be open to contributors.
If you are interested in contributing, we have designated tickets with the Contributor OK
label. Or if you are interested in a specific ticket, then we can discuss prior. Here are our contributing guidelines with the details: https://github.com/mozilla-mobile/firefox-ios/blob/main/CONTRIBUTING.md#guidelines-for-contributing
Happy to continue working on this issue with you (will need to confirm with my team on this ticket) or if you would like, feel free to grab one of the other Contributor OK
tickets.
windowUUID: windowUUID, | ||
actionType: TabPanelViewActionType.confirmCloseAllTabs) | ||
let alertController = UIAlertController(title: nil, message: nil, preferredStyle: .actionSheet) | ||
let closeAction = UIAlertAction(title: "Close All Tabs", style: .destructive) { _ in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For localization purposes, we want to use the existing string from the previous code (.LegacyAppMenu.AppMenuCloseAllTabsTitleString
). Same applies to "Cancel", where we want to use (.TabTrayCloseAllTabsPromptCancel
)
firefox-ios/Client/Frontend/Browser/Tabs/Views/TabTrayViewController.swift
Show resolved
Hide resolved
firefox-ios/Client/Frontend/Browser/Tabs/Views/TabTrayViewController.swift
Outdated
Show resolved
Hide resolved
firefox-ios/Client/Frontend/Browser/Tabs/Views/TabTrayViewController.swift
Outdated
Show resolved
Hide resolved
I reviewed several points. I restarted from the code base and found another way to perform a bug fix. @cyndichin @adudenamedruby let me know what you think now. I compiled the code and tested it and it works. |
let action = TabPanelViewAction(panelType: tabTrayState.selectedPanel, | ||
windowUUID: windowUUID, | ||
actionType: TabPanelViewActionType.closeAllTabs) | ||
store.dispatch(action) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nathan9513 I don't think that we want to remove this. While traditionally we would want to just show the alert controller when the button is taped, with redux we want to go through the closeAllTabs
action. Our tabTrayState
should contain all the information we need to rebuild the view in its current state. I like to think about it as all the information we would need to restore the view exactly as it was including how the user interacted with it.
The logic right now is that the closeAllTabs
action updates tabTrayState.showCloseConfirmation
which should then show the confirmation when the view controller gets the state change based on that action firing. I think that we want to fix this issue through the way we are updating our state and not by going around the state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need a new action to reset the tabTrayState.showCloseConfirmation
to false when cancel is tapped. Something like this:
private func showCloseAllConfirmation() {
let controller = AlertController(title: nil, message: nil, preferredStyle: .actionSheet)
controller.addAction(UIAlertAction(title: .LegacyAppMenu.AppMenuCloseAllTabsTitleString,
style: .default,
handler: { _ in self.confirmCloseAll() }),
accessibilityIdentifier: AccessibilityIdentifiers.TabTray.deleteCloseAllButton)
controller.addAction(UIAlertAction(title: .TabTrayCloseAllTabsPromptCancel,
style: .cancel,
handler: { _ in self.cancelCloseAll() }),
accessibilityIdentifier: AccessibilityIdentifiers.TabTray.deleteCancelButton)
controller.popoverPresentationController?.barButtonItem = deleteButton
present(controller, animated: true, completion: nil)
}
private func cancelCloseAll() {
let action = TabPanelViewAction(panelType: tabTrayState.selectedPanel,
windowUUID: windowUUID,
actionType: TabPanelViewActionType.cancelCloseAllTabs)
store.dispatch(action)
}
And in the middleware we would want to reset showCloseConfirmation
to false:
static func reduceTabPanelViewAction(action: TabPanelViewAction, state: TabTrayState) -> TabTrayState {
switch action.actionType {
case TabPanelViewActionType.closeAllTabs:
return TabTrayState(windowUUID: state.windowUUID,
isPrivateMode: state.isPrivateMode,
selectedPanel: state.selectedPanel,
normalTabsCount: state.normalTabsCount,
hasSyncableAccount: state.hasSyncableAccount,
showCloseConfirmation: true)
case TabPanelViewActionType.cancelCloseAllTabs:
return TabTrayState(windowUUID: state.windowUUID,
isPrivateMode: state.isPrivateMode,
selectedPanel: state.selectedPanel,
normalTabsCount: state.normalTabsCount,
hasSyncableAccount: state.hasSyncableAccount,
showCloseConfirmation: false)
default:
return defaultState(from: state)
}
}
📜 Tickets
Jira ticket
Github issue
💡 Description
Resolved an issue where the "Close All" pop-up would not reappear after being dismissed.
📝 Checklist
You have to check all boxes before merging