Skip to content

Commit

Permalink
Refactor FXIOS-10669, Bugfix FXIOS-10826, FXIOS-10749, FXIOS-10825, F…
Browse files Browse the repository at this point in the history 
…XIOS-10827, FXIOS-10830 [Sent from Firefox] Refactor the ShareManager to use an explicit interface for sharing different types of content (mozilla-mobile#23620)

* Refactor the implementation of ShareManager to separate out logic it should not be handling and to use static methods instead of instantiation. Update DownloadsCoordinator, LibraryCoordinator, DownloadsPanel, and ShareSheetCoordinator usage.

* Correctly implement the main menu and deeplink route paths to sharing.

* Update all the share paths via the BrowserNavigationHandler protocol share method. Concert tryDownloadingTabFileToShare to an async method.

* Add ShareManager unit tests. Updated share manager / share coordinator interface in other unit tests.

* Add special BrowserCoordinator unit test to check for TemporaryDocument downloads for tab shares getting promoted to file shares. Added MockTemporaryDocument and TemporaryDocument protocol types. Updated usage in Client. Added documentation.

* Rename MainMenuCoordinatorDelegate's showShareSheet method to showShareSheetForCurrentlySelectedTab for clarity amongst different ways to share content.

* Add FIXME note for FXIOS-10334 regarding unclear handling of JS alert. Add ShareType.wrappedURL helper getter.

* For Send to Device functionality, only send the selectedTab info if it's a .tab type share.

* Make the new toolbar share path explicit about sharing the currently selected tab. Add documentation.

* Add TODO note for email subtitles from deep link sharesheet route.

* Add telemetry to track cases where the ShareSheetCoordinator might not be deallocated properly in the previous session. Add fallback for existing ShareSheetCoordinators in BrowserCoordinator, LibraryCoordinator, and DownloadsCoordinator.

* Update reader mode URL handling in the URLActivityItemProvider for sharing. Add documentation for handling reader mode URLs using a tab's displayURL.
  • Loading branch information
@ih-codes
ih-codes authored Dec 10, 2024
1 parent 25be1aa commit aee4b83
Show file tree
Hide file tree
Showing 27 changed files with 743 additions and 315 deletions.
3 changes: 3 additions & 0 deletions BrowserKit/Sources/Common/Logger/LoggerCategory.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ public enum LoggerCategory: String {
/// Related to the setup of services on app launch.
case setup

/// Related to showing the share sheet from multiple places in the app.
case shareSheet

/// Related to storage (keychain, SQL database, store of different types, etc).
case storage

Expand Down
8 changes: 8 additions & 0 deletions firefox-ios/Client.xcodeproj/project.pbxproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1844,9 +1844,11 @@
ED371A682CB881EF0099F3C8 /* SearchEngineSelectionCoordinator.swift in Sources */ = {isa = PBXBuildFile; fileRef = ED371A672CB881EF0099F3C8 /* SearchEngineSelectionCoordinator.swift */; };
ED390D202CF4DE2E00A775F9 /* URLActivityItemProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = ED390D1F2CF4DE2E00A775F9 /* URLActivityItemProvider.swift */; };
ED390D222CF4DEDB00A775F9 /* TitleSubtitleActivityItemProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = ED390D212CF4DEDB00A775F9 /* TitleSubtitleActivityItemProvider.swift */; };
ED4294852CF1286E0077D7CB /* ShareManagerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = ED4294842CF1286E0077D7CB /* ShareManagerTests.swift */; };
ED45893E2CC800D9006F2C0B /* SearchEngineSelectionViewControllerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = ED45893D2CC800D9006F2C0B /* SearchEngineSelectionViewControllerTests.swift */; };
ED4589402CC8220A006F2C0B /* MockSearchEngineSelectionCoordinator.swift in Sources */ = {isa = PBXBuildFile; fileRef = ED45893F2CC8220A006F2C0B /* MockSearchEngineSelectionCoordinator.swift */; };
ED55DC8C2CC2D7DA00E3FE3A /* SearchEngineSelectionCoordinatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = ED55DC8B2CC2D7DA00E3FE3A /* SearchEngineSelectionCoordinatorTests.swift */; };
ED575BD32D00F9D00056BCDA /* MockTemporaryDocument.swift in Sources */ = {isa = PBXBuildFile; fileRef = ED575BD22D00F9D00056BCDA /* MockTemporaryDocument.swift */; };
ED6C8DAC2CE6A4BB00D7F7F3 /* Sentry-Dynamic in Frameworks */ = {isa = PBXBuildFile; productRef = ED6C8DAB2CE6A4BB00D7F7F3 /* Sentry-Dynamic */; };
ED70CD812CE3BD2C0018761B /* MockStoreForMiddleware.swift in Sources */ = {isa = PBXBuildFile; fileRef = ED70CD802CE3BD2C0018761B /* MockStoreForMiddleware.swift */; };
ED7A08DB2CF674730035EC8F /* ShareMessage.swift in Sources */ = {isa = PBXBuildFile; fileRef = ED7A08DA2CF674730035EC8F /* ShareMessage.swift */; };
Expand Down Expand Up @@ -9377,10 +9379,12 @@
ED371A672CB881EF0099F3C8 /* SearchEngineSelectionCoordinator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SearchEngineSelectionCoordinator.swift; sourceTree = "<group>"; };
ED390D1F2CF4DE2E00A775F9 /* URLActivityItemProvider.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = URLActivityItemProvider.swift; sourceTree = "<group>"; };
ED390D212CF4DEDB00A775F9 /* TitleSubtitleActivityItemProvider.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TitleSubtitleActivityItemProvider.swift; sourceTree = "<group>"; };
ED4294842CF1286E0077D7CB /* ShareManagerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShareManagerTests.swift; sourceTree = "<group>"; };
ED45893D2CC800D9006F2C0B /* SearchEngineSelectionViewControllerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SearchEngineSelectionViewControllerTests.swift; sourceTree = "<group>"; };
ED45893F2CC8220A006F2C0B /* MockSearchEngineSelectionCoordinator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockSearchEngineSelectionCoordinator.swift; sourceTree = "<group>"; };
ED5144A0BE3A8C7B15047C3F /* anp */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = anp; path = anp.lproj/3DTouchActions.strings; sourceTree = "<group>"; };
ED55DC8B2CC2D7DA00E3FE3A /* SearchEngineSelectionCoordinatorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SearchEngineSelectionCoordinatorTests.swift; sourceTree = "<group>"; };
ED575BD22D00F9D00056BCDA /* MockTemporaryDocument.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockTemporaryDocument.swift; sourceTree = "<group>"; };
ED70CD802CE3BD2C0018761B /* MockStoreForMiddleware.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockStoreForMiddleware.swift; sourceTree = "<group>"; };
ED7A08DA2CF674730035EC8F /* ShareMessage.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShareMessage.swift; sourceTree = "<group>"; };
ED7A08DC2CF6749B0035EC8F /* ShareType.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShareType.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -12771,6 +12775,7 @@
8A7653C328A2E68B00924ABF /* MockPocketAPI.swift */,
281B2BE91ADF4D90002917DC /* MockProfile.swift */,
ED7A08E02CF679CE0035EC8F /* MockShareTab.swift */,
ED575BD22D00F9D00056BCDA /* MockTemporaryDocument.swift */,
8A7A26E029D4785900EA76F1 /* MockRouter.swift */,
C8C3FE9C29F907B30038E3BA /* MockSearchHandlerRouteCoordinator.swift */,
21D884402A79628E00AF144C /* MockSettingsDelegate.swift */,
Expand Down Expand Up @@ -14135,6 +14140,7 @@
D3BA41671BD82F2200DA5457 /* XCTestCaseExtensions.swift */,
8A96C4BA28F9E7B300B75884 /* XCTestCaseRootViewController.swift */,
0ECB6B672CCFE663006A7C82 /* DateGroupedTableDataTests.swift */,
ED4294842CF1286E0077D7CB /* ShareManagerTests.swift */,
);
path = ClientTests;
sourceTree = "<group>";
Expand Down Expand Up @@ -17046,6 +17052,7 @@
8AA7347B28AEDB3100443D24 /* PocketViewModelTests.swift in Sources */,
C2446B312A856D13000C527D /* MockLibraryCoordinatorDelegate.swift in Sources */,
C869915728917809007ACC5C /* NetworkingMock.swift in Sources */,
ED575BD32D00F9D00056BCDA /* MockTemporaryDocument.swift in Sources */,
8A4190D22A6B0848001E8401 /* StatusBarOverlayTests.swift in Sources */,
C8EDDBF029DD83FC003A4C07 /* RouteTests.swift in Sources */,
0A7693612C7DD19600103A6D /* CertificatesViewModelTests.swift in Sources */,
Expand Down Expand Up @@ -17145,6 +17152,7 @@
5A9A09D428B01D8700B6F51E /* MockTelemetryWrapper.swift in Sources */,
C80C11F028B3C9150062922A /* MockUserDefaults.swift in Sources */,
D8BA1790206D47830023AC00 /* DeferredTestUtils.swift in Sources */,
ED4294852CF1286E0077D7CB /* ShareManagerTests.swift in Sources */,
5AA75A652A46274A00533F8D /* MockThemeManager.swift in Sources */,
8A5038142A5DFCE000A1B02A /* MockBrowserProfile.swift in Sources */,
C23889E52A50329200429673 /* MockParentCoordinator.swift in Sources */,
Expand Down
187 changes: 121 additions & 66 deletions firefox-ios/Client/Coordinators/Browser/BrowserCoordinator.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,12 +301,8 @@ class BrowserCoordinator: BaseCoordinator,
case let .searchURL(url, tabId):
handle(searchURL: url, tabId: tabId)

case .sharesheet:
// FIXME: FXIOS-10669 Implement with real data, (shareType, shareMessage)
// let tempURL = URL(string: "https://www.google.ca")
// let tempTitle = "Test Title"
// showShareSheet(with: tempURL, title: tempTitle)
break
case let .sharesheet(shareType, shareMessage):
handleShareRoute(shareType: shareType, shareMessage: shareMessage)

case let .glean(url):
glean.handleDeeplinkUrl(url: url)
Expand Down Expand Up @@ -393,6 +389,22 @@ class BrowserCoordinator: BaseCoordinator,
browserViewController.presentSignInViewController(fxaParams)
}

/// Starts the share sheet coordinator for the deep link `.sharesheet` route (share content via Nimbus Messaging).
/// - Parameters:
/// - shareType: The content to share.
/// - shareMessage: An optional textual message to accompany the shared content. May contain a Mail subject line.
private func handleShareRoute(shareType: ShareType, shareMessage: ShareMessage?) {
// FIXME: FXIOS-10829 Deep link shares should set a reasonable sourceView for iPad... or sourceView could be optional
startShareSheetCoordinator(
shareType: shareType,
shareMessage: shareMessage,
sourceView: browserViewController.addressToolbarContainer,
sourceRect: nil,
toastContainer: browserViewController.contentContainer,
popoverArrowDirection: .any
)
}

private func canHandleSettings(with section: Route.SettingsSection) -> Bool {
guard !childCoordinators.contains(where: { $0 is SettingsCoordinator }) else {
return false // route is handled with existing child coordinator
Expand Down Expand Up @@ -551,8 +563,24 @@ class BrowserCoordinator: BaseCoordinator,
browserViewController.updateZoomPageBarVisibility(visible: true)
}

func showShareSheet(with url: URL?) {
showShareSheet(with: url, title: nil)
/// Share the currently selected tab using the share sheet.
///
/// Part of the MainMenuCoordinatorDelegate implementation, called from the New Menu > Tools > Share.
func showShareSheetForCurrentlySelectedTab() {
// We share the tab's displayURL to make sure we don't share reader mode localhost URLs
guard let selectedTab = tabManager.selectedTab,
let url = selectedTab.canonicalURL?.displayURL else {
return
}

startShareSheetCoordinator(
shareType: .tab(url: url, tab: selectedTab),
shareMessage: nil,
sourceView: self.browserViewController.addressToolbarContainer,
sourceRect: nil,
toastContainer: self.browserViewController.contentContainer,
popoverArrowDirection: .any
)
}

private func makeMenuNavViewController() -> DismissableNavigationViewController? {
Expand Down Expand Up @@ -642,15 +670,17 @@ class BrowserCoordinator: BaseCoordinator,

// MARK: - BrowserNavigationHandler

func showShareSheet(url: URL,
title: String?,
sourceView: UIView,
sourceRect: CGRect?,
toastContainer: UIView,
popoverArrowDirection: UIPopoverArrowDirection) {
func showShareSheet(
shareType: ShareType,
shareMessage: ShareMessage?,
sourceView: UIView,
sourceRect: CGRect?,
toastContainer: UIView,
popoverArrowDirection: UIPopoverArrowDirection
) {
startShareSheetCoordinator(
url: url,
title: title,
shareType: shareType,
shareMessage: shareMessage,
sourceView: sourceView,
sourceRect: sourceRect,
toastContainer: toastContainer,
Expand Down Expand Up @@ -741,35 +771,78 @@ class BrowserCoordinator: BaseCoordinator,
return coordinator
}

/// Starts the ShareSheetCoordinator, which initiates opening the iOS share sheet using an `UIActivityViewController`.
///
/// For shared tabs where the user is currently on a non-HTML page (e.g. viewing a PDF), this method will initiate a
/// file download, and then share the file instead. This currently blocks without any UI indication, which is less
/// than ideal but has been the existing behaviour for a long time (task to address this: FXIOS-10823).
///
/// - Parameters:
/// - shareType: The type of content to share.
/// - shareMessage: An optional accompanying message to share (with optional email subject line).
/// - sourceView: The view tapped to initiate share. iPad share sheet popovers will point to this element.
/// - sourceRect: The source rect for the view tapped to initiate share. iPad share sheet popovers will point to this
/// element.
/// - toastContainer: The container for displaying toast information.
/// - popoverArrowDirection: The arrow direction for iPad share sheet popovers.
///
/// There are many ways to share many types of content from various areas of the app. Code paths that go through this
/// method include:
/// * Sharing content from a long press on Home screen tiles (e.g. long press Jump Back In context menu)
/// * From the old Menu > Share and the new Menu > Tools > Share
/// * From the new toolbar share button beside the address bar
/// * From long pressing a link in the WKWebView and sharing from the context menu (via ActionProviderBuilder > addShare)
/// * Via the sharesheet deeplink path in `RouteBuilder` (e.g. tapping home cards that initiate sharing content)
/// * Currently this is the only path that is using the ShareMessage param (Info Card Referral experiment FXE-1090)
func startShareSheetCoordinator(
url: URL,
title: String?,
shareType: ShareType,
shareMessage: ShareMessage?,
sourceView: UIView,
sourceRect: CGRect?,
toastContainer: UIView,
popoverArrowDirection: UIPopoverArrowDirection
) {
guard childCoordinators.first(where: { $0 is ShareSheetCoordinator }) as? ShareSheetCoordinator == nil
else {
// If this case is hitted it means the share extension coordinator wasn't removed
// correctly in the previous session.
return
if let coordinator = childCoordinators.first(where: { $0 is ShareSheetCoordinator }) as? ShareSheetCoordinator {
// The share sheet extension coordinator wasn't correctly removed in the last share session. Attempt to recover.
logger.log(
"ShareSheetCoordinator already exists when it shouldn't. Removing and recreating it to access share sheet",
level: .info,
category: .shareSheet,
extra: ["existing ShareSheetCoordinator UUID": "\(coordinator.windowUUID)",
"BrowserCoordinator windowUUID": "\(windowUUID)"]
)

coordinator.dismiss()
}

Task {
// FXIOS-10824 It's strange if the user has to wait a long time to download files that are literally already
// being shown in the webview.
var overrideShareType = shareType
if case ShareType.tab = shareType {
overrideShareType = await tryDownloadingTabFileToShare(shareType: shareType)
}

await MainActor.run { [weak self, overrideShareType] in
guard let self else { return }

let shareSheetCoordinator = ShareSheetCoordinator(
alertContainer: toastContainer,
router: router,
profile: profile,
parentCoordinator: self,
tabManager: tabManager
)
add(child: shareSheetCoordinator)
shareSheetCoordinator.start(
shareType: overrideShareType,
shareMessage: shareMessage,
sourceView: sourceView,
sourceRect: sourceRect,
popoverArrowDirection: popoverArrowDirection
)
}
}
let shareSheetCoordinator = ShareSheetCoordinator(
alertContainer: toastContainer,
router: router,
profile: profile,
parentCoordinator: self,
tabManager: tabManager
)
add(child: shareSheetCoordinator)
shareSheetCoordinator.start(
url: url,
title: title,
sourceView: sourceView,
sourceRect: sourceRect,
popoverArrowDirection: popoverArrowDirection
)
}

func showCreditCardAutofill(creditCard: CreditCard?,
Expand Down Expand Up @@ -1057,38 +1130,20 @@ class BrowserCoordinator: BaseCoordinator,

// MARK: - Private helpers

private func showShareSheet(with url: URL?, title: String?) {
guard let url else { return }

let showShareSheet = { url in
self.startShareSheetCoordinator(
url: url,
title: title,
sourceView: self.browserViewController.addressToolbarContainer,
sourceRect: nil,
toastContainer: self.browserViewController.contentContainer,
popoverArrowDirection: .any
)
private func tryDownloadingTabFileToShare(shareType: ShareType) async -> ShareType {
// We can only try to download files for `.tab` type shares that have a TemporaryDocument
guard case let ShareType.tab(_, tab) = shareType,
let temporaryDocument = tab.temporaryDocument else {
return shareType
}

// We only care about the temp document for .tab shares right?
// BUT!! A tab might be displaying a downloaded PDF! Then it's file:// url
guard let temporaryDocument = browserViewController.tabManager.selectedTab?.temporaryDocument else {
showShareSheet(url)
return
guard let fileURL = await temporaryDocument.getDownloadedURL() else {
// If no file was downloaded, simply share the tab as usual with a web URL
return shareType
}

temporaryDocument.getURL { tempDocURL in
DispatchQueue.main.async {
// If we successfully got a temp file URL, share it like a downloaded file,
// otherwise present the ordinary share menu for the web URL.
if let tempDocURL = tempDocURL, tempDocURL.isFileURL {
showShareSheet(tempDocURL)
} else {
showShareSheet(url)
}
}
}
// If we successfully got a temp file URL, share it like a downloaded file
return .file(url: fileURL)
}

/// Utility. Performs the supplied action if a coordinator of the indicated type
Expand Down
7 changes: 4 additions & 3 deletions firefox-ios/Client/Coordinators/Browser/BrowserNavigationHandler.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,14 @@ protocol BrowserNavigationHandler: AnyObject, QRCodeNavigationHandler {

/// Shows the share sheet.
///
/// - Parameter url: The url to be shared.
/// - Parameter shareType: The content to be shared.
/// - Parameter shareMessage: An optional plain text message to be shared.
/// - Parameter sourceView: The reference view to show the popoverViewController.
/// - Parameter sourceRect: An optional rect to use for ipad popover presentation.
/// - Parameter toastContainer: The view in which is displayed the toast results from actions in the share extension.
/// - Parameter popoverArrowDirection: The arrow direction for the view controller presented as popover.
func showShareSheet(url: URL,
title: String?,
func showShareSheet(shareType: ShareType,
shareMessage: ShareMessage?,
sourceView: UIView,
sourceRect: CGRect?,
toastContainer: UIView,
Expand Down
Loading

0 comments on commit aee4b83

Please sign in to comment.