From 9df3139ac7ac165c4b9e468e3d44161f115c8b8f Mon Sep 17 00:00:00 2001 From: Doug <6060466+pixlwave@users.noreply.github.com> Date: Tue, 5 Dec 2023 11:14:16 +0000 Subject: [PATCH] Make the attachment menu an actual menu. (#2199) There was a bug with the sheet/popover when overriding the colour scheme where the button renders in light mode when the app is in dark mode. --- .../ComposerToolbarModels.swift | 28 +++++--------- .../ComposerToolbarViewModel.swift | 13 ++----- .../View/RoomAttachmentPicker.swift | 37 +++++-------------- .../RoomScreen/RoomScreenViewModel.swift | 27 +++++++++----- IntegrationTests/Sources/UserFlowTests.swift | 10 ++++- UITests/Sources/UserSessionScreenTests.swift | 4 +- ...Pad-9th-generation.userSessionScreen-3.png | 4 +- ...-9th-generation.userSessionScreenRTE-1.png | 4 +- .../en-GB-iPhone-14.userSessionScreen-3.png | 4 +- ...en-GB-iPhone-14.userSessionScreenRTE-1.png | 4 +- ...Pad-9th-generation.userSessionScreen-3.png | 4 +- ...-9th-generation.userSessionScreenRTE-1.png | 4 +- .../pseudo-iPhone-14.userSessionScreen-3.png | 4 +- ...seudo-iPhone-14.userSessionScreenRTE-1.png | 4 +- 14 files changed, 65 insertions(+), 86 deletions(-) diff --git a/ElementX/Sources/Screens/ComposerToolbar/ComposerToolbarModels.swift b/ElementX/Sources/Screens/ComposerToolbar/ComposerToolbarModels.swift index 41468e945..372d80149 100644 --- a/ElementX/Sources/Screens/ComposerToolbar/ComposerToolbarModels.swift +++ b/ElementX/Sources/Screens/ComposerToolbar/ComposerToolbarModels.swift @@ -32,11 +32,7 @@ enum ComposerToolbarVoiceMessageAction { enum ComposerToolbarViewModelAction { case sendMessage(plain: String, html: String?, mode: RoomScreenComposerMode, intentionalMentions: IntentionalMentions) - case displayCameraPicker - case displayMediaPicker - case displayDocumentPicker - case displayLocationPicker - case displayNewPollForm + case attach(ComposerAttachmentType) case handlePasteOrDrop(provider: NSItemProvider) @@ -51,11 +47,7 @@ enum ComposerToolbarViewAction { case sendMessage case cancelReply case cancelEdit - case displayCameraPicker - case displayMediaPicker - case displayDocumentPicker - case displayLocationPicker - case displayNewPollForm + case attach(ComposerAttachmentType) case handlePasteOrDrop(provider: NSItemProvider) case enableTextFormatting case composerAction(action: ComposerAction) @@ -64,6 +56,14 @@ enum ComposerToolbarViewAction { case voiceMessage(ComposerToolbarVoiceMessageAction) } +enum ComposerAttachmentType { + case camera + case photoLibrary + case file + case location + case poll +} + struct ComposerToolbarViewState: BindableState { var composerMode: RoomScreenComposerMode = .default var composerEmpty = true @@ -116,14 +116,6 @@ struct ComposerToolbarViewStateBindings { var composerExpanded = false var formatItems: [FormatItem] = .init() var alertInfo: AlertInfo? - - var showAttachmentPopover = false { - didSet { - if showAttachmentPopover { - composerFocused = false - } - } - } } /// An item in the toolbar diff --git a/ElementX/Sources/Screens/ComposerToolbar/ComposerToolbarViewModel.swift b/ElementX/Sources/Screens/ComposerToolbar/ComposerToolbarViewModel.swift index 1db108baf..1d273abf4 100644 --- a/ElementX/Sources/Screens/ComposerToolbar/ComposerToolbarViewModel.swift +++ b/ElementX/Sources/Screens/ComposerToolbar/ComposerToolbarViewModel.swift @@ -120,16 +120,9 @@ final class ComposerToolbarViewModel: ComposerToolbarViewModelType, ComposerTool case .cancelEdit: set(mode: .default) set(text: "") - case .displayCameraPicker: - actionsSubject.send(.displayCameraPicker) - case .displayMediaPicker: - actionsSubject.send(.displayMediaPicker) - case .displayDocumentPicker: - actionsSubject.send(.displayDocumentPicker) - case .displayLocationPicker: - actionsSubject.send(.displayLocationPicker) - case .displayNewPollForm: - actionsSubject.send(.displayNewPollForm) + case .attach(let attachment): + state.bindings.composerFocused = false + actionsSubject.send(.attach(attachment)) case .handlePasteOrDrop(let provider): actionsSubject.send(.handlePasteOrDrop(provider: provider)) case .enableTextFormatting: diff --git a/ElementX/Sources/Screens/ComposerToolbar/View/RoomAttachmentPicker.swift b/ElementX/Sources/Screens/ComposerToolbar/View/RoomAttachmentPicker.swift index 8d494dcbc..05a1cc872 100644 --- a/ElementX/Sources/Screens/ComposerToolbar/View/RoomAttachmentPicker.swift +++ b/ElementX/Sources/Screens/ComposerToolbar/View/RoomAttachmentPicker.swift @@ -20,13 +20,12 @@ import WysiwygComposer struct RoomAttachmentPicker: View { @ObservedObject var context: ComposerToolbarViewModel.Context - @Environment(\.isPresented) var isPresented - - @State private var sheetContentFrame: CGRect = .zero var body: some View { - Button { - context.showAttachmentPopover = true + // Use a menu instead of the popover/sheet shown in Figma because overriding the colour scheme + // results in a rendering bug on 17.1: https://github.com/vector-im/element-x-ios/issues/2157 + Menu { + menuContent } label: { CompoundIcon(asset: Asset.Images.composerAttachment, size: .custom(30), relativeTo: .title) .scaledPadding(7, relativeTo: .title) @@ -34,21 +33,12 @@ struct RoomAttachmentPicker: View { .buttonStyle(RoomAttachmentPickerButtonStyle()) .accessibilityLabel(L10n.actionAddToTimeline) .accessibilityIdentifier(A11yIdentifiers.roomScreen.composerToolbar.openComposeOptions) - .popover(isPresented: $context.showAttachmentPopover) { - menuContent - .padding(.top, isPresented ? 20 : 0) - .readFrame($sheetContentFrame) - .presentationDetents([.height(sheetContentFrame.height)]) - .presentationBackground(.compound.bgCanvasDefault) - .presentationDragIndicator(.visible) - } } var menuContent: some View { VStack(alignment: .leading, spacing: 0.0) { Button { - context.showAttachmentPopover = false - context.send(viewAction: .displayMediaPicker) + context.send(viewAction: .attach(.photoLibrary)) } label: { Label(L10n.screenRoomAttachmentSourceGallery, icon: \.image) .labelStyle(.menuSheet) @@ -56,8 +46,7 @@ struct RoomAttachmentPicker: View { .accessibilityIdentifier(A11yIdentifiers.roomScreen.attachmentPickerPhotoLibrary) Button { - context.showAttachmentPopover = false - context.send(viewAction: .displayDocumentPicker) + context.send(viewAction: .attach(.file)) } label: { Label(L10n.screenRoomAttachmentSourceFiles, iconAsset: Asset.Images.attachment) .labelStyle(.menuSheet) @@ -65,8 +54,7 @@ struct RoomAttachmentPicker: View { .accessibilityIdentifier(A11yIdentifiers.roomScreen.attachmentPickerDocuments) Button { - context.showAttachmentPopover = false - context.send(viewAction: .displayCameraPicker) + context.send(viewAction: .attach(.camera)) } label: { Label(L10n.screenRoomAttachmentSourceCamera, iconAsset: Asset.Images.takePhoto) .labelStyle(.menuSheet) @@ -74,8 +62,7 @@ struct RoomAttachmentPicker: View { .accessibilityIdentifier(A11yIdentifiers.roomScreen.attachmentPickerCamera) Button { - context.showAttachmentPopover = false - context.send(viewAction: .displayLocationPicker) + context.send(viewAction: .attach(.location)) } label: { Label(L10n.screenRoomAttachmentSourceLocation, iconAsset: Asset.Images.addLocation) .labelStyle(.menuSheet) @@ -83,8 +70,7 @@ struct RoomAttachmentPicker: View { .accessibilityIdentifier(A11yIdentifiers.roomScreen.attachmentPickerLocation) Button { - context.showAttachmentPopover = false - context.send(viewAction: .displayNewPollForm) + context.send(viewAction: .attach(.poll)) } label: { Label(L10n.screenRoomAttachmentSourcePoll, iconAsset: Asset.Images.polls) .labelStyle(.menuSheet) @@ -93,7 +79,6 @@ struct RoomAttachmentPicker: View { if ServiceLocator.shared.settings.richTextEditorEnabled { Button { - context.showAttachmentPopover = false context.send(viewAction: .enableTextFormatting) } label: { Label(L10n.screenRoomAttachmentTextFormatting, iconAsset: Asset.Images.textFormat) @@ -109,10 +94,6 @@ private struct RoomAttachmentPickerButtonStyle: ButtonStyle { func makeBody(configuration: Configuration) -> some View { configuration.label .foregroundStyle(configuration.isPressed ? .compound.bgActionPrimaryPressed : .compound.bgActionPrimaryRest) - // Disable animations to fix a bug when the system is in Light mode but the app in Dark mode. For some - // reason the animation causes a glitch with sheet's colour scheme when there are presentation detents. - // https://github.com/vector-im/element-x-ios/issues/2157 - .animation(.noAnimation, value: configuration.isPressed) } } diff --git a/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift index 4acdaa96c..afdb51e97 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift +++ b/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift @@ -180,16 +180,8 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol mode: mode, intentionalMentions: intentionalMentions) } - case .displayCameraPicker: - actionsSubject.send(.displayCameraPicker) - case .displayMediaPicker: - actionsSubject.send(.displayMediaPicker) - case .displayDocumentPicker: - actionsSubject.send(.displayDocumentPicker) - case .displayLocationPicker: - actionsSubject.send(.displayLocationPicker) - case .displayNewPollForm: - actionsSubject.send(.displayPollForm(mode: .new)) + case .attach(let attachment): + attach(attachment) case .handlePasteOrDrop(let provider): roomScreenInteractionHandler.handlePasteOrDrop(provider) case .composerModeChanged(mode: let mode): @@ -203,6 +195,21 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol // MARK: - Private + private func attach(_ attachment: ComposerAttachmentType) { + switch attachment { + case .camera: + actionsSubject.send(.displayCameraPicker) + case .photoLibrary: + actionsSubject.send(.displayMediaPicker) + case .file: + actionsSubject.send(.displayDocumentPicker) + case .location: + actionsSubject.send(.displayLocationPicker) + case .poll: + actionsSubject.send(.displayPollForm(mode: .new)) + } + } + private func processPollAction(_ action: RoomScreenViewPollAction) { switch action { case let .selectOption(pollStartID, optionID): diff --git a/IntegrationTests/Sources/UserFlowTests.swift b/IntegrationTests/Sources/UserFlowTests.swift index 8525dbe9f..28191c3e0 100644 --- a/IntegrationTests/Sources/UserFlowTests.swift +++ b/IntegrationTests/Sources/UserFlowTests.swift @@ -59,13 +59,13 @@ class UserFlowTests: XCTestCase { for identifier in [A11yIdentifiers.roomScreen.attachmentPickerPhotoLibrary, A11yIdentifiers.roomScreen.attachmentPickerDocuments, A11yIdentifiers.roomScreen.attachmentPickerLocation] { - tapOnButton(A11yIdentifiers.roomScreen.composerToolbar.openComposeOptions) + tapOnMenu(A11yIdentifiers.roomScreen.composerToolbar.openComposeOptions) tapOnButton(identifier) tapOnButton("Cancel") } // Open attachments picker - tapOnButton(A11yIdentifiers.roomScreen.composerToolbar.openComposeOptions) + tapOnMenu(A11yIdentifiers.roomScreen.composerToolbar.openComposeOptions) // Open photo library picker tapOnButton(A11yIdentifiers.roomScreen.attachmentPickerPhotoLibrary) @@ -169,6 +169,12 @@ class UserFlowTests: XCTestCase { button.tap() } + private func tapOnMenu(_ identifier: String) { + let button = app.buttons[identifier] + XCTAssertTrue(button.waitForExistence(timeout: 10.0)) + button.forceTap() + } + /// Taps on a back button that the system configured with a label but no identifier. /// /// When there are multiple buttons with the same label in the hierarchy, all the buttons we created diff --git a/UITests/Sources/UserSessionScreenTests.swift b/UITests/Sources/UserSessionScreenTests.swift index 9ccd4a450..3ec63733c 100644 --- a/UITests/Sources/UserSessionScreenTests.swift +++ b/UITests/Sources/UserSessionScreenTests.swift @@ -29,7 +29,7 @@ class UserSessionScreenTests: XCTestCase { try await Task.sleep(for: .seconds(1)) try await app.assertScreenshot(.userSessionScreen, step: 2) - app.buttons[A11yIdentifiers.roomScreen.composerToolbar.openComposeOptions].tap() + app.buttons[A11yIdentifiers.roomScreen.composerToolbar.openComposeOptions].forceTap() try await app.assertScreenshot(.userSessionScreen, step: 3) } @@ -52,7 +52,7 @@ class UserSessionScreenTests: XCTestCase { XCTAssert(app.staticTexts[firstRoomName].waitForExistence(timeout: 5.0)) try await Task.sleep(for: .seconds(1)) - app.buttons[A11yIdentifiers.roomScreen.composerToolbar.openComposeOptions].tap() + app.buttons[A11yIdentifiers.roomScreen.composerToolbar.openComposeOptions].forceTap() try await app.assertScreenshot(.userSessionScreenRTE, step: 1) app.buttons[A11yIdentifiers.roomScreen.attachmentPickerTextFormatting].tap() diff --git a/UITests/Sources/__Snapshots__/Application/en-GB-iPad-9th-generation.userSessionScreen-3.png b/UITests/Sources/__Snapshots__/Application/en-GB-iPad-9th-generation.userSessionScreen-3.png index b1c6d626d..8a0e6bb3b 100644 --- a/UITests/Sources/__Snapshots__/Application/en-GB-iPad-9th-generation.userSessionScreen-3.png +++ b/UITests/Sources/__Snapshots__/Application/en-GB-iPad-9th-generation.userSessionScreen-3.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:51572a32caa534f7da3120f69a8ab046e7f84c911a38bbfd714128364319a085 -size 512517 +oid sha256:aaefc3ed4e24c6efb98965c23b931566ad3e214df66beae06980d627078989a8 +size 676247 diff --git a/UITests/Sources/__Snapshots__/Application/en-GB-iPad-9th-generation.userSessionScreenRTE-1.png b/UITests/Sources/__Snapshots__/Application/en-GB-iPad-9th-generation.userSessionScreenRTE-1.png index cafc51c00..7e64f9dd3 100644 --- a/UITests/Sources/__Snapshots__/Application/en-GB-iPad-9th-generation.userSessionScreenRTE-1.png +++ b/UITests/Sources/__Snapshots__/Application/en-GB-iPad-9th-generation.userSessionScreenRTE-1.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:c1218fb8e4cfab37f2013282aedc84081c48b067d64d06f5533e8d6f6f76b9a9 -size 520169 +oid sha256:6dd40677ed718698138d9ef21feae07c4474f6e77d179a69549004933c502f0f +size 723856 diff --git a/UITests/Sources/__Snapshots__/Application/en-GB-iPhone-14.userSessionScreen-3.png b/UITests/Sources/__Snapshots__/Application/en-GB-iPhone-14.userSessionScreen-3.png index 48318c4b5..16adabb55 100644 --- a/UITests/Sources/__Snapshots__/Application/en-GB-iPhone-14.userSessionScreen-3.png +++ b/UITests/Sources/__Snapshots__/Application/en-GB-iPhone-14.userSessionScreen-3.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:0fe47246cf49b033e4c47a3aa3aca3637d4023ed16affaf1338d3154b1cb2cbd -size 283237 +oid sha256:8d81e5b309e74f8f5f37ea7d9929fae7ff54b3fd502eac1e5a5b322cc024cc98 +size 768968 diff --git a/UITests/Sources/__Snapshots__/Application/en-GB-iPhone-14.userSessionScreenRTE-1.png b/UITests/Sources/__Snapshots__/Application/en-GB-iPhone-14.userSessionScreenRTE-1.png index 9c2039692..9aedc1492 100644 --- a/UITests/Sources/__Snapshots__/Application/en-GB-iPhone-14.userSessionScreenRTE-1.png +++ b/UITests/Sources/__Snapshots__/Application/en-GB-iPhone-14.userSessionScreenRTE-1.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:b74b75ca3a65c7e17a188e7bb06730fe408518679f6f2a1d99026025305d3166 -size 276022 +oid sha256:5a796e2a7131226e3d1a6eca09f1f3c144f605bb2f85d863fd5483b2acd76d9c +size 851474 diff --git a/UITests/Sources/__Snapshots__/Application/pseudo-iPad-9th-generation.userSessionScreen-3.png b/UITests/Sources/__Snapshots__/Application/pseudo-iPad-9th-generation.userSessionScreen-3.png index 270381da0..2ed436153 100644 --- a/UITests/Sources/__Snapshots__/Application/pseudo-iPad-9th-generation.userSessionScreen-3.png +++ b/UITests/Sources/__Snapshots__/Application/pseudo-iPad-9th-generation.userSessionScreen-3.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:e1994a582d2cfe62b69d4665bb847ac3c4f796b4cc9f8bd83f6bb231f84566db -size 505029 +oid sha256:d2a14abaf3a46bfd09368bea8e5f21c5f98d541fc9ddf77eb603237928a2f44a +size 711656 diff --git a/UITests/Sources/__Snapshots__/Application/pseudo-iPad-9th-generation.userSessionScreenRTE-1.png b/UITests/Sources/__Snapshots__/Application/pseudo-iPad-9th-generation.userSessionScreenRTE-1.png index e7280f13c..b7347722b 100644 --- a/UITests/Sources/__Snapshots__/Application/pseudo-iPad-9th-generation.userSessionScreenRTE-1.png +++ b/UITests/Sources/__Snapshots__/Application/pseudo-iPad-9th-generation.userSessionScreenRTE-1.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:4b0e3e0f5368ac7e5c5563d5967f1dc224aedebe9b04d10141989ec8883922b6 -size 508387 +oid sha256:499cdf4bdbcef9f952c97cc519894100d5cf964254c31bfc7d9fceb67150eb18 +size 773160 diff --git a/UITests/Sources/__Snapshots__/Application/pseudo-iPhone-14.userSessionScreen-3.png b/UITests/Sources/__Snapshots__/Application/pseudo-iPhone-14.userSessionScreen-3.png index fd38d08a6..b5b0a5e7c 100644 --- a/UITests/Sources/__Snapshots__/Application/pseudo-iPhone-14.userSessionScreen-3.png +++ b/UITests/Sources/__Snapshots__/Application/pseudo-iPhone-14.userSessionScreen-3.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:e55c79a65f37a3340c716e6f79d304f26772aaa9aebbdf6f404d45808fea55c0 -size 281966 +oid sha256:0e0de41f6bd8aa6e09e97fa346590e68c7dc320bed218dac22619e95ece787df +size 829018 diff --git a/UITests/Sources/__Snapshots__/Application/pseudo-iPhone-14.userSessionScreenRTE-1.png b/UITests/Sources/__Snapshots__/Application/pseudo-iPhone-14.userSessionScreenRTE-1.png index 49d11a000..1a2ab2471 100644 --- a/UITests/Sources/__Snapshots__/Application/pseudo-iPhone-14.userSessionScreenRTE-1.png +++ b/UITests/Sources/__Snapshots__/Application/pseudo-iPhone-14.userSessionScreenRTE-1.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:88f1c46520cc79be3b233a617af82b8ff88dbabf8ebd8ba441a1f5fd4d44857b -size 284369 +oid sha256:468fb91896df309b60179b93f567c2bc9971dcb4d81514aa49d7a786f01704ab +size 942085