From f976562b01899547de9d288c1cb32b31ce30a629 Mon Sep 17 00:00:00 2001 From: kenny lopez Date: Sun, 16 Aug 2026 14:22:57 +0100 Subject: [PATCH] Fix message action accessibility lifecycle Signed-off-by: kenny lopez --- .../Runner/NativeMessageActionSurface.swift | 76 ++++++++++++++++--- mobile/ios/RunnerTests/RunnerTests.swift | 53 ++++++++++++- .../message_action_popover.dart | 59 ++++++++------ .../channels/message_actions_test.dart | 46 ++++++++++- 4 files changed, 196 insertions(+), 38 deletions(-) diff --git a/mobile/ios/Runner/NativeMessageActionSurface.swift b/mobile/ios/Runner/NativeMessageActionSurface.swift index b348292a0..4b0d637ce 100644 --- a/mobile/ios/Runner/NativeMessageActionSurface.swift +++ b/mobile/ios/Runner/NativeMessageActionSurface.swift @@ -34,7 +34,8 @@ struct NativeMessageActionDefinition { } enum NativeMessageActionSurfaceLayout { - static let rowHeight: CGFloat = 48 + static let minimumRowHeight: CGFloat = 48 + static let rowVerticalPadding: CGFloat = 4 static let separatorHeight: CGFloat = 0.5 static let verticalInset: CGFloat = 4 static let horizontalInset: CGFloat = 16 @@ -62,11 +63,31 @@ enum NativeMessageActionSurfaceLayout { max(0, populatedGroups(actions: actions).count - 1) } - static func preferredHeight( - actions: [NativeMessageActionDefinition] + static func rowHeight( + minimumHeight: CGFloat = minimumRowHeight, + compatibleWith traitCollection: UITraitCollection? = nil ) -> CGFloat { - (verticalInset * 2) - + (CGFloat(actions.count) * rowHeight) + let labelHeight = UIFont.preferredFont( + forTextStyle: .body, + compatibleWith: traitCollection + ).lineHeight + return max( + minimumHeight, + ceil(labelHeight + (rowVerticalPadding * 2)) + ) + } + + static func preferredHeight( + actions: [NativeMessageActionDefinition], + minimumRowHeight: CGFloat = minimumRowHeight, + compatibleWith traitCollection: UITraitCollection? = nil + ) -> CGFloat { + let resolvedRowHeight = rowHeight( + minimumHeight: minimumRowHeight, + compatibleWith: traitCollection + ) + return (verticalInset * 2) + + (CGFloat(actions.count) * resolvedRowHeight) + (CGFloat(separatorCount(actions: actions)) * separatorHeight) } } @@ -110,6 +131,8 @@ final class NativeMessageActionRowControl: UIControl { definition: NativeMessageActionDefinition, foregroundColor: UIColor, destructiveColor: UIColor, + minimumHeight: CGFloat = NativeMessageActionSurfaceLayout.minimumRowHeight, + compatibleWith traitCollection: UITraitCollection? = nil, onSelected: @escaping () -> Void ) { actionImageView = UIImageView(image: UIImage(systemName: definition.symbol)) @@ -121,11 +144,17 @@ final class NativeMessageActionRowControl: UIControl { actionTitleLabel.text = definition.title actionTitleLabel.textColor = color - actionTitleLabel.font = UIFont.preferredFont(forTextStyle: .body) + actionTitleLabel.font = UIFont.preferredFont( + forTextStyle: .body, + compatibleWith: traitCollection + ) actionTitleLabel.adjustsFontForContentSizeCategory = true - actionTitleLabel.numberOfLines = 1 - actionTitleLabel.adjustsFontSizeToFitWidth = true - actionTitleLabel.minimumScaleFactor = 0.8 + actionTitleLabel.numberOfLines = 0 + actionTitleLabel.lineBreakMode = .byWordWrapping + actionTitleLabel.setContentCompressionResistancePriority( + .required, + for: .vertical + ) let iconColumn = UIView() iconColumn.translatesAutoresizingMaskIntoConstraints = false @@ -159,9 +188,20 @@ final class NativeMessageActionRowControl: UIControl { equalTo: trailingAnchor, constant: -NativeMessageActionSurfaceLayout.horizontalInset ), + actionTitleLabel.topAnchor.constraint( + greaterThanOrEqualTo: topAnchor, + constant: NativeMessageActionSurfaceLayout.rowVerticalPadding + ), + actionTitleLabel.bottomAnchor.constraint( + lessThanOrEqualTo: bottomAnchor, + constant: -NativeMessageActionSurfaceLayout.rowVerticalPadding + ), actionTitleLabel.centerYAnchor.constraint(equalTo: centerYAnchor), heightAnchor.constraint( - greaterThanOrEqualToConstant: NativeMessageActionSurfaceLayout.rowHeight + greaterThanOrEqualToConstant: NativeMessageActionSurfaceLayout.rowHeight( + minimumHeight: minimumHeight, + compatibleWith: traitCollection + ) ), ]) @@ -265,6 +305,15 @@ final class NativeMessageActionSurfacePlatformView: NSObject, let interfaceStyle = NativeMessageActionSurfaceAppearance.interfaceStyle( from: arguments?["interfaceStyle"] ) + var minimumRowHeight = NativeMessageActionSurfaceLayout.minimumRowHeight + if let requestedRowHeight = arguments?["rowHeight"] as? NSNumber, + requestedRowHeight.doubleValue.isFinite + { + minimumRowHeight = max( + minimumRowHeight, + CGFloat(requestedRowHeight.doubleValue) + ) + } let actionArguments = arguments?["actions"] as? [[String: Any]] let actions = actionArguments?.compactMap( @@ -317,7 +366,8 @@ final class NativeMessageActionSurfacePlatformView: NSObject, actions: actions, foregroundColor: foregroundColor, destructiveColor: destructiveColor, - separatorColor: separatorColor + separatorColor: separatorColor, + minimumRowHeight: minimumRowHeight ) } @@ -329,7 +379,8 @@ final class NativeMessageActionSurfacePlatformView: NSObject, actions: [NativeMessageActionDefinition], foregroundColor: UIColor, destructiveColor: UIColor, - separatorColor: UIColor + separatorColor: UIColor, + minimumRowHeight: CGFloat ) { let scrollView = UIScrollView() scrollView.translatesAutoresizingMaskIntoConstraints = false @@ -377,6 +428,7 @@ final class NativeMessageActionSurfacePlatformView: NSObject, definition: definition, foregroundColor: foregroundColor, destructiveColor: destructiveColor, + minimumHeight: minimumRowHeight, onSelected: { [weak self] in self?.select(definition) } ) ) diff --git a/mobile/ios/RunnerTests/RunnerTests.swift b/mobile/ios/RunnerTests/RunnerTests.swift index c05032304..e26aa1152 100644 --- a/mobile/ios/RunnerTests/RunnerTests.swift +++ b/mobile/ios/RunnerTests/RunnerTests.swift @@ -431,7 +431,12 @@ class RunnerTests: XCTestCase { 2 ) XCTAssertEqual( - NativeMessageActionSurfaceLayout.preferredHeight(actions: definitions), + NativeMessageActionSurfaceLayout.preferredHeight( + actions: definitions, + compatibleWith: UITraitCollection( + preferredContentSizeCategory: .large + ) + ), 153 ) } @@ -463,6 +468,52 @@ class RunnerTests: XCTestCase { XCTAssertTrue(selected) } + @MainActor + func testNativeMessageActionRowExpandsForAccessibilityTypography() throws { + let traits = UITraitCollection( + preferredContentSizeCategory: .accessibilityExtraExtraExtraLarge + ) + let definition = try XCTUnwrap( + NativeMessageActionDefinition( + arguments: [ + "id": "followThread", "title": "Follow thread", + "symbol": "bell", "group": "utility", + ] + ) + ) + let row = NativeMessageActionRowControl( + definition: definition, + foregroundColor: .label, + destructiveColor: .systemRed, + compatibleWith: traits, + onSelected: {} + ) + let fittingSize = row.systemLayoutSizeFitting( + CGSize(width: 288, height: UIView.layoutFittingCompressedSize.height), + withHorizontalFittingPriority: .required, + verticalFittingPriority: .fittingSizeLevel + ) + row.frame = CGRect(origin: .zero, size: fittingSize) + row.layoutIfNeeded() + let labelFrame = row.convert( + row.actionTitleLabel.bounds, + from: row.actionTitleLabel + ) + + XCTAssertGreaterThan(fittingSize.height, 48) + XCTAssertGreaterThan(labelFrame.height, 0) + XCTAssertGreaterThanOrEqual( + labelFrame.minY, + NativeMessageActionSurfaceLayout.rowVerticalPadding + ) + XCTAssertLessThanOrEqual( + labelFrame.maxY, + fittingSize.height - NativeMessageActionSurfaceLayout.rowVerticalPadding + ) + XCTAssertEqual(row.actionTitleLabel.numberOfLines, 0) + XCTAssertFalse(row.actionTitleLabel.adjustsFontSizeToFitWidth) + } + @MainActor func testNativeMessageActionSurfaceUsesSystemMaterial() { let effect = NativeMessageActionSurfaceAppearance.backdropEffect( diff --git a/mobile/lib/features/channels/message_actions/message_action_popover.dart b/mobile/lib/features/channels/message_actions/message_action_popover.dart index 104f55788..1b07bfb59 100644 --- a/mobile/lib/features/channels/message_actions/message_action_popover.dart +++ b/mobile/lib/features/channels/message_actions/message_action_popover.dart @@ -155,32 +155,39 @@ Future _showMessageActionsPopover({ if (shouldRestoreComposerFocus) composerFocusNode!.unfocus(); String? selectedActionId; + final dialogRoute = RawDialogRoute( + barrierDismissible: true, + barrierLabel: 'Dismiss message actions', + barrierColor: Colors.transparent, + transitionDuration: reduceMotion + ? Duration.zero + : isIos + ? _iosMessageActionTransitionDuration + : _messageActionTransitionDuration, + transitionBuilder: (context, animation, secondaryAnimation, child) => + child, + pageBuilder: (dialogContext, animation, secondaryAnimation) => + _MessageActionsPopover( + anchorRect: anchorRect, + anchorSnapshot: snapshot, + animation: animation, + message: message, + pageContext: context, + pageRef: ref, + actions: actions, + useIosNativeActionSurface: useIosNativeActionSurface, + ), + ); + var routePushed = false; try { - selectedActionId = await showGeneralDialog( - context: context, - barrierDismissible: true, - barrierLabel: 'Dismiss message actions', - barrierColor: Colors.transparent, - transitionDuration: reduceMotion - ? Duration.zero - : isIos - ? _iosMessageActionTransitionDuration - : _messageActionTransitionDuration, - transitionBuilder: (context, animation, secondaryAnimation, child) => - child, - pageBuilder: (dialogContext, animation, secondaryAnimation) => - _MessageActionsPopover( - anchorRect: anchorRect, - anchorSnapshot: snapshot, - animation: animation, - message: message, - pageContext: context, - pageRef: ref, - actions: actions, - useIosNativeActionSurface: useIosNativeActionSurface, - ), - ); + final popResult = Navigator.of( + context, + rootNavigator: true, + ).push(dialogRoute); + routePushed = true; + selectedActionId = await popResult; } finally { + if (routePushed) await dialogRoute.completed; snapshot.dispose(); if (context.mounted) onPopoverDismissed?.call(); } @@ -447,10 +454,12 @@ class _PopoverMessageAction { class _IosNativeMessageActionSurface extends HookWidget { final List<_PopoverMessageAction> actions; + final double rowHeight; final ValueChanged onSelected; const _IosNativeMessageActionSurface({ required this.actions, + required this.rowHeight, required this.onSelected, }); @@ -479,6 +488,7 @@ class _IosNativeMessageActionSurface extends HookWidget { 'separatorColor': context.colors.outlineVariant.toARGB32(), 'errorColor': context.colors.error.toARGB32(), 'interfaceStyle': context.colors.brightness.name, + 'rowHeight': rowHeight, }, creationParamsCodec: const StandardMessageCodec(), onPlatformViewCreated: (id) => viewId.value = id, @@ -690,6 +700,7 @@ class _MessageActionsPopover extends StatelessWidget { child: useIosNativeActionSurface ? _IosNativeMessageActionSurface( actions: actions, + rowHeight: menuLayout.rowHeight, onSelected: (actionId) => Navigator.of(context).pop(actionId), ) diff --git a/mobile/test/features/channels/message_actions_test.dart b/mobile/test/features/channels/message_actions_test.dart index 187c92504..259ca8b96 100644 --- a/mobile/test/features/channels/message_actions_test.dart +++ b/mobile/test/features/channels/message_actions_test.dart @@ -215,6 +215,7 @@ Future<_MessageActionsPopoverHarness> _pumpMessageActionsPopover( bool disableAnimations = false, EdgeInsets viewInsets = EdgeInsets.zero, TextScaler textScaler = TextScaler.noScaling, + Future Function()? captureAnchorSnapshot, FocusNode? composerFocusNode, bool composerInitiallyFocused = false, Rect anchorRect = const Rect.fromLTWH(32, 260, 300, 72), @@ -262,7 +263,8 @@ Future<_MessageActionsPopoverHarness> _pumpMessageActionsPopover( currentPubkey: 'self', isMember: true, anchorRect: anchorRect, - captureAnchorSnapshot: _testMessageSnapshot, + captureAnchorSnapshot: + captureAnchorSnapshot ?? _testMessageSnapshot, onPopoverPresented: () => sourceHidden.value = true, onPopoverDismissed: () => sourceHidden.value = false, composerFocusNode: composerFocusNode, @@ -882,6 +884,48 @@ void main() { await _dismissMessageActionsPopover(tester); }); + testWidgets('keeps the snapshot alive through the reverse transition', ( + tester, + ) async { + debugDefaultTargetPlatformOverride = TargetPlatform.iOS; + ui.Image? snapshot; + try { + final prefs = await _mockPrefs(); + await _pumpMessageActionsPopover( + tester, + message: _message(), + prefs: prefs, + captureAnchorSnapshot: () async { + snapshot = await _testMessageSnapshot(); + return snapshot!; + }, + ); + + final image = snapshot!; + expect(image.debugDisposed, isFalse); + Navigator.of( + tester.element(find.byKey(const ValueKey('message-action-surface'))), + ).pop(); + await tester.pump(); + + expect( + find.byKey(const ValueKey('message-action-preview')), + findsOneWidget, + ); + expect(image.debugDisposed, isFalse); + await tester.pump(const Duration(milliseconds: 110)); + expect(image.debugDisposed, isFalse); + + await tester.pumpAndSettle(); + expect(image.debugDisposed, isTrue); + expect(tester.takeException(), isNull); + } finally { + final image = snapshot; + if (image != null && !image.debugDisposed) image.dispose(); + debugDefaultTargetPlatformOverride = null; + } + }); + testWidgets('shows parity actions for a regular message', (tester) async { final prefs = await _mockPrefs(); await _pumpSheet(tester, message: _message(), prefs: prefs);