From b152d985b0cb3a01ea08c0848d2044973e38ea65 Mon Sep 17 00:00:00 2001 From: Michelle Linington Date: Mon, 4 Jan 2021 23:36:19 -0800 Subject: [PATCH] PR Feedback - More robust handling of large button labels in the collision view controller - Fixes for VoiceOver labels and navigation - Fixes for layout issues when rotating - Update copy for collision banner - Remove banner shadow for all banners --- .../ConversationViewController+Banners.swift | 7 +- .../ConversationViewController.m | 12 --- ...geRequestNameCollisionViewController.swift | 10 +++ .../src/views/NameCollisionReviewCell.swift | 74 +++++++++++++------ .../translations/en.lproj/Localizable.strings | 5 +- 5 files changed, 72 insertions(+), 36 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController+Banners.swift b/Signal/src/ViewControllers/ConversationView/ConversationViewController+Banners.swift index 1acb5d269d..0134d8a6be 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController+Banners.swift +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController+Banners.swift @@ -357,6 +357,8 @@ private class MessageRequestNameCollisionBanner: UIView { let button = OWSButton( imageName: "x-circle-16", tintColor: Theme.secondaryTextAndIconColor) + button.accessibilityLabel = NSLocalizedString("BANNER_CLOSE_ACCESSIBILITY_LABEL", + comment: "Accessibility label for banner close button") button.translatesAutoresizingMaskIntoConstraints = false button.setCompressionResistanceHigh() button.setContentHuggingHigh() @@ -364,8 +366,7 @@ private class MessageRequestNameCollisionBanner: UIView { }() private let reviewButton: OWSButton = { - let buttonText = NSLocalizedString( - "MESSAGE_REQUEST_REVIEW_NAME_COLLISION", + let buttonText = NSLocalizedString("MESSAGE_REQUEST_REVIEW_NAME_COLLISION", comment: "Button to allow user to review known name collisions with an incoming message request") let button = OWSButton(title: buttonText) @@ -401,6 +402,8 @@ private class MessageRequestNameCollisionBanner: UIView { closeButton.imageView?.autoPinLeading(toTrailingEdgeOf: label, offset: 16) closeButton.imageView?.autoPinTrailing(toEdgeOf: self, offset: -16) reviewButton.titleLabel?.autoPinLeading(toEdgeOf: label) + + accessibilityElements = [label, reviewButton, closeButton] } required init?(coder: NSCoder) { diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index 12c6712b05..952487d7cd 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -782,18 +782,6 @@ typedef enum : NSUInteger { [bannerView autoPinToTopLayoutGuideOfViewController:self withInset:0]; [bannerView autoPinEdgeToSuperviewEdge:ALEdgeLeading]; [bannerView autoPinEdgeToSuperviewEdge:ALEdgeTrailing]; - - UIView *bannerShadow = [UIView new]; - bannerShadow.backgroundColor = Theme.backgroundColor; - // Use a shadow to "pop" the indicator above the other views. - bannerShadow.layer.shadowColor = [UIColor blackColor].CGColor; - bannerShadow.layer.shadowOffset = CGSizeMake(0, 4); - bannerShadow.layer.shadowRadius = 4.f; - bannerShadow.layer.shadowOpacity = 0.15f; - [bannerView addSubview:bannerShadow]; - [bannerShadow autoPinEdgesToSuperviewEdges]; - [bannerView sendSubviewToBack:bannerShadow]; - [self.view layoutSubviews]; self.bannerView = bannerView; diff --git a/Signal/src/ViewControllers/ThreadSettings/MessageRequestNameCollisionViewController.swift b/Signal/src/ViewControllers/ThreadSettings/MessageRequestNameCollisionViewController.swift index 68f351660d..ff47961592 100644 --- a/Signal/src/ViewControllers/ThreadSettings/MessageRequestNameCollisionViewController.swift +++ b/Signal/src/ViewControllers/ThreadSettings/MessageRequestNameCollisionViewController.swift @@ -33,6 +33,7 @@ class MessageRequestNameCollisionViewController: OWSTableViewController { override func viewDidLoad() { super.viewDidLoad() updateModel() + tableView.separatorStyle = .none } override func viewWillAppear(_ animated: Bool) { @@ -45,6 +46,15 @@ class MessageRequestNameCollisionViewController: OWSTableViewController { action: #selector(donePressed)) } + override func viewWillTransition(to size: CGSize, with coordinator: UIViewControllerTransitionCoordinator) { + super.viewWillTransition(to: size, with: coordinator) + coordinator.animate { _ in + // Force tableview to recalculate self-sized cell height + self.tableView.beginUpdates() + self.tableView.endUpdates() + } + } + func updateModel() { databaseStorage.uiRead { readTx in self.requesterModel = NameCollisionModel.buildFromAddress( diff --git a/Signal/src/views/NameCollisionReviewCell.swift b/Signal/src/views/NameCollisionReviewCell.swift index b14cc6c3d9..5c614fd0e6 100644 --- a/Signal/src/views/NameCollisionReviewCell.swift +++ b/Signal/src/views/NameCollisionReviewCell.swift @@ -68,6 +68,16 @@ class NameCollisionReviewContactCell: UITableViewCell { return label }() + // Rolling our own cell separator. It should be aligned with the name/actions (which is pinned to the safe area) + // The separator UITableView provides does not respect safe area. By handling this ourselves it can now respect + // safe area. Additionally it makes the alignment a bit more explicit. + let separatorHairline: UIView = { + let separator = UIView() + separator.backgroundColor = Theme.cellSeparatorColor + separator.autoSetDimension(.height, toSize: CGHairlineWidth()) + return separator + }() + required override init(style: UITableViewCell.CellStyle, reuseIdentifier: String?) { super.init(style: style, reuseIdentifier: reuseIdentifier) @@ -85,14 +95,15 @@ class NameCollisionReviewContactCell: UITableViewCell { horizontalStack.alignment = .top contentView.addSubview(horizontalStack) - horizontalStack.autoPinEdge(toSuperviewSafeArea: .leading, withInset: 16) - horizontalStack.autoPinEdge(toSuperviewSafeArea: .trailing, withInset: 16) - horizontalStack.autoPinEdge(toSuperviewEdge: .top, withInset: 16) - horizontalStack.autoPinEdge(toSuperviewEdge: .bottom) - + horizontalStack.autoPinEdgesToSuperviewEdges(with: UIEdgeInsets(top: 16, leading: 16, bottom: 0, trailing: 16)) verticalStack.autoPinEdge(.bottom, to: .bottom, of: contentView, withOffset: -12, relation: .lessThanOrEqual) avatarView.autoSetDimensions(to: CGSize(square: 64)) + contentView.addSubview(separatorHairline) + separatorHairline.autoPinLeading(toEdgeOf: verticalStack) + separatorHairline.autoPinTrailing(toEdgeOf: contentView) + separatorHairline.autoPinEdge(.bottom, to: .bottom, of: contentView) + isPairedWithActions = false } @@ -127,7 +138,7 @@ class NameCollisionReviewContactCell: UITableViewCell { commonGroupsLabel.text = model.commonGroupsString - if let oldName = model.oldName { + if let _ = model.oldName { nameChangeSpacer.isHidden = false recentNameChangeLabel.isHidden = false recentNameChangeLabel.text = "" // TODO @@ -141,11 +152,12 @@ class NameCollisionReviewContactCell: UITableViewCell { avatarView.autoPinEdge(.bottom, to: .bottom, of: contentView, withOffset: -16, relation: .lessThanOrEqual) }() - // If this cell is paired with actions, we don't need to pad the avatar view + // If the cell is paired with actions, we don't need to pad the avatar view + // If the cell is not paired with actions, we can hide our separator var isPairedWithActions: Bool = false { didSet { avatarBottomEdgeConstraint.isActive = !isPairedWithActions - separatorInset = UIEdgeInsets(top: 0, leading: isPairedWithActions ? 96 : 0, bottom: 0, trailing: 0) + separatorHairline.isHidden = !isPairedWithActions } } } @@ -154,29 +166,49 @@ class NameCollisionActionCell: UITableViewCell { typealias Action = (title: String, action: () -> Void) init(actions: [Action]) { - owsAssertDebug(actions.count < 3, "Untested above two actions. This is likely to truncate button text") + owsAssertDebug(actions.count < 3, "Only supports two actions. Feel free to update this for more.") + super.init(style: .default, reuseIdentifier: nil) selectionStyle = .none - - let buttons = actions.map { (action: Action) -> UIButton in - let button = OWSButton(title: action.title, block: action.action) - button.setTitleColor(Theme.accentBlueColor, for: .normal) - button.setTitleColor(Theme.accentBlueColor.withAlphaComponent(0.7), for: .highlighted) - button.titleLabel?.font = UIFont.ows_dynamicTypeSubheadlineClamped.ows_semibold - button.titleLabel?.adjustsFontForContentSizeCategory = true - return button - } + let buttons = actions.map { createButton(for: $0) } let horizontalStack = UIStackView(arrangedSubviews: buttons + [UIView()]) horizontalStack.axis = .horizontal - horizontalStack.distribution = .equalCentering + horizontalStack.distribution = .equalSpacing + horizontalStack.alignment = .center + horizontalStack.spacing = 8 + + // If one button grows super tall, its larger intrinsic content size could result in the other button + // being compressed very thin and tall in response. It's unlikely, since this would only be hit by a very + // edge case localization. But, if it does happen, things will look reasonably okay. + if let button1 = buttons[safe: 0], let button2 = buttons[safe: 1] { + button1.autoSetDimension(.width, toSize: min(100, button1.intrinsicContentSize.width), relation: .greaterThanOrEqual) + button2.autoSetDimension(.width, toSize: min(100, button2.intrinsicContentSize.width), relation: .greaterThanOrEqual) + } contentView.addSubview(horizontalStack) - horizontalStack.autoPinEdgesToSuperviewSafeArea(with: UIEdgeInsets(top: 8, leading: 96, bottom: 8, trailing: 0)) - + horizontalStack.autoPinEdge(toSuperviewEdge: .top, withInset: 8) + horizontalStack.autoPinEdge(toSuperviewEdge: .bottom, withInset: 8) + horizontalStack.autoPinEdge(toSuperviewEdge: .leading, withInset: 96) + horizontalStack.autoPinEdge(toSuperviewEdge: .trailing) } required init?(coder: NSCoder) { fatalError("init(coder:) has not been implemented") } + + private func createButton(for action: Action) -> UIButton { + let button = OWSButton(title: action.title, block: action.action) + button.setTitleColor(Theme.accentBlueColor, for: .normal) + button.setTitleColor(Theme.accentBlueColor.withAlphaComponent(0.7), for: .highlighted) + button.titleLabel?.font = UIFont.ows_dynamicTypeSubheadlineClamped.ows_semibold + button.titleLabel?.adjustsFontForContentSizeCategory = true + button.titleLabel?.numberOfLines = 0 + button.contentHorizontalAlignment = .leading + + // By default, a button's label will grow outside of the buttons bounds + button.titleLabel?.autoMatch(.height, to: .height, of: button, withMultiplier: 1, relation: .lessThanOrEqual) + button.setContentHuggingHorizontalHigh() + return button + } } diff --git a/Signal/translations/en.lproj/Localizable.strings b/Signal/translations/en.lproj/Localizable.strings index aaa942f68c..f9f974e11e 100644 --- a/Signal/translations/en.lproj/Localizable.strings +++ b/Signal/translations/en.lproj/Localizable.strings @@ -358,6 +358,9 @@ /* Error shown when backup fails due to an unexpected error. */ "BACKUP_UNEXPECTED_ERROR" = "Unexpected Backup Error"; +/* Accessibility label for banner close button */ +"BANNER_CLOSE_ACCESSIBILITY_LABEL" = "Close banner"; + /* An explanation of the consequences of blocking a group. */ "BLOCK_GROUP_BEHAVIOR_EXPLANATION" = "You will no longer receive messages or updates from this group."; @@ -2780,7 +2783,7 @@ "MESSAGE_REQUEST_LEAVE_AND_DELETE_GROUP_TITLE" = "Delete and Leave Group?"; /* Banner label notifying user that a new message is from a user with the same name as an existing contact */ -"MESSAGE_REQUEST_NAME_COLLISON_BANNER_LABEL" = "Review requests carefuly Signal found another contact with the same name."; +"MESSAGE_REQUEST_NAME_COLLISON_BANNER_LABEL" = "Review requests carefully. Signal found another contact with the same name."; /* A header string above a known contact's contact info */ "MESSAGE_REQUEST_NAME_COLLISON_CONTACT_HEADER" = "Your contact";