Skip to content
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

IOS-10448 [Mistica] RowList A11y update #399

Merged
merged 10 commits into from
Sep 12, 2024

Conversation

dhidalgofadrique
Copy link
Contributor

@dhidalgofadrique dhidalgofadrique commented Sep 6, 2024

🎟️ Jira ticket

IOS-10448 [Mistica] RowList A11y update

πŸ₯… What's the goal?

Update Mistica to satisfy A11y for row lists according to Figma specs. Several issues has been detected. These are some of the main ones:

1.- All the cells were behaving the same way (due to the "default" UITableViewCell's accessibility). All the main elements (inside center view) were being read in natural order. As specified in Figma specs the order is custom (title must precede headline)

2.- Mistica is not using native accessory views so they were being focused as any other element (e.g: Navigation arrow or switches)

3.- In custom action cells like cells with a switch (e.g: Cells to enable/disable PIN code to access the app) it's not possible to simulate native behavior that focus the whole cell and toggles the switch when user double taps.

⚠️ NOTE: Due to SwiftUI is not used in iphoneapp and the changes to be done in SwiftUI are complex, it has been created a separate ticket for it: IOS-10555

🚧 How do we do it?

Due to Mistica cells are so configurable that admit generic views in some of their components, it's not possible to deduce the appropiate accessibility type. The proposed solution is letting the client the responsability of determine the cell accessibility type.

For that purpose, a new enum AccessibilityListCellType has been created to set accessibility type on cells. It admits four possibile values:

  • interactive: The default value. The cell is focused as a whole. Typically used in navigation cells. It admits two configurations:
    • label: If provided, it will be the label read when the cell is focused. If not provided, the default label will be read (with the order specified in Figma specs)
    • action: If provided, the custom action to be executed when double tap (Usefull for cells contaning a switch to be able to toggle the switch when double tapping the cell)
  • double interaction: Covered this case specified in spect but shouldn't be used (not a good UX). The cell has two focusable elements: center view (main cell data) and control view (right view). Should be used if the cell has an interactive element in the right side. It admits the same config as interactive cell.
  • informative: Each cell element is focusable individually.
  • customInformative: This case could be assumed by the interactive one but to clearly separate this case from an interactive cell (has en action associated like navigation) it has it's own case. The cell is focused as a whole and the label read is the (mandatory) provided one

With all these possibilities there are some combinations that will have no sense, for example, adding an interactive element in the right side of the cell and set the accessibility type as customInteractive, which will hide the action to the user using accessibility. So it's responsability of the client to use the appropiate accessibility type

πŸ§ͺ How can I verify this?

RPReplay_Final1725957831.MP4

πŸ‘ AppCenter build

https://install.appcenter.ms/orgs/tuenti-organization/apps/mistica-ios/distribution_groups/public/releases/264

Added full cell accessbility config struct
First approach for improving cells accessibility
Update ListCellContent accessibility (via delegate) when headlineView is updated
Fixed some typos
Deleted no longer used FullCellAccessibilityConfig and created accessibility type
Created separate files for list cell accessibility data
Added comments to help understanding this new feature
Comment on lines +81 to +90
private lazy var accessibilityTypeCell: UISegmentedControlTableViewCell = {
let cell = UISegmentedControlTableViewCell(reuseIdentifier: "accessibilityTypeCell")
cell.segmentedControl.insertSegment(withTitle: "Default", at: 0, animated: false)
cell.segmentedControl.insertSegment(withTitle: "Informative", at: 1, animated: false)
cell.segmentedControl.insertSegment(withTitle: "Custom informative", at: 2, animated: false)
cell.segmentedControl.insertSegment(withTitle: "Interactive", at: 3, animated: false)
cell.segmentedControl.insertSegment(withTitle: "Double interaction", at: 4, animated: false)
cell.segmentedControl.selectedSegmentIndex = 0
return cell
}()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New cell to configure accessibility

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only see a small issue and that is that the literals can not be read as a whole.
Screenshot 2024-09-10 at 15 35 11

Perhaps it would be interesting to rotate the segmented control vertically XD
https://stackoverflow.com/questions/3490358/can-i-show-an-uisegmentedcontrol-object-in-vertical

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried rotation with no success xD. It seems also not possible to modify segmented control to allow multiple lines. Maybe the solution is to create a new cell type for "many" options using buttons

Comment on lines 210 to 216
let accessibilityInteractiveData = AccessibilityListCellInteractiveData { [weak self] in
let alertController = UIAlertController(title: nil, message: "Custom action", preferredStyle: .alert)
let alertAction = UIAlertAction(title: "Accept", style: .cancel)
alertController.addAction(alertAction)

self?.present(alertController, animated: true)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Show an alert just to simulate a custom action

var headlineView: UIView? {
var accessibilityActivationAction: (() -> Void)?

var headlineView: AccessibleTextualView? {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to it's possible to use any UIView, used a protocol that exposes an accessible text

didSet {
oldValue?.removeFromSuperview()

if let view = headlineView {
insertArrangedSubview(view, at: 0)
updateSpacing()
}

listCellContentViewDelegate?.accessibilityChanged()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notify delegate that accessibility has changed

Comment on lines +83 to +90
override public func accessibilityActivate() -> Bool {
guard let accessibilityActivationAction else {
return false
}

accessibilityActivationAction()
return true
}
Copy link
Contributor Author

@dhidalgofadrique dhidalgofadrique Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Execute custom accessibility action on double tap if it's defined. This case only occurs in cells with accessibility of type doubleInteraction when double tap the center section (for cells with interactive accesibility type, this action is managed in the parent cell view)

Comment on lines +23 to +29
if case .doubleInteraction(let accessibilityInteractiveData) = accessibilityType {
// If double interaction accessibility, make centerSection accessible to be focusable (isAccessibilityElement = true)
centerSection.isAccessibilityElement = true
// Set center section label to the provided one (or the default one if not provided)
centerSection.accessibilityLabel = accessibilityInteractiveData.label ?? defaultAccessibilityLabel
// Set accessibility activation action to be executed on center section double tap
centerSection.accessibilityActivationAction = accessibilityInteractiveData.action
Copy link
Contributor Author

@dhidalgofadrique dhidalgofadrique Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of double interaction accessibility, center section has to be accessible to be focusable by VoiceOver. The label associated will be the provided one or if no label provided, the default one (defined in Figma specs) and the optional accessibility action is stored

Comment on lines +48 to +53
let accessibilityComponents: [String?] = [
titleText,
headlineText,
subtitleText,
detailText
]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Composed default accessibility label following Figma specs

Comment on lines +453 to +455
// Set accessibility order following Figma spec:
// https://www.figma.com/design/Be8QB9onmHunKCCAkIBAVr/%F0%9F%94%B8-Lists-Specs?node-id=0-1&node-type=CANVAS&t=jgG9X5qKokaMwJjm-0
accessibilityElements = [centerSection.titleLabel, headlineView as Any, centerSection.subtitleLabel, centerSection.detailLabel, controlView as Any].compactMap { $0 }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If accessibility is informative, define elements order according Figma specs

accessibilityElements = [centerSection.titleLabel, headlineView as Any, centerSection.subtitleLabel, centerSection.detailLabel, controlView as Any].compactMap { $0 }
case .doubleInteraction:
// If double interaction, just two elements: center section and right section
accessibilityElements = [centerSection, controlView as Any].compactMap { $0 }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of double interaction accessibility, there will be two elements: center section (which will manage it's own accessibility) and the control view

Comment on lines 166 to 170
extension TagView: AccessibleTextualView {
public var accessibleText: String? {
return text
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most common heading view is a TagView, so implemented AccessibleTextualView protocol by default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data associated to the cell accessibility type. It includes the label (can be null, if so, the default one will be used) and the associated action (that can also be nil)

self.action = action
}

public static var `default`: AccessibilityListCellInteractiveData = .init(action: nil)
Copy link
Contributor Author

@dhidalgofadrique dhidalgofadrique Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default data has no label (it will use the default one) and no action

@dhidalgofadrique dhidalgofadrique marked this pull request as ready for review September 10, 2024 09:46
}
}

lazy var titleLabel = IntrinsictHeightLabel()
lazy var subtitleLabel = IntrinsictHeightLabel()
lazy var detailLabel = IntrinsictHeightLabel()

var listCellContentViewDelegate: ListCellContentViewDelegate?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weak?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor

@DavidMarinCalleja DavidMarinCalleja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job

Comment on lines +81 to +90
private lazy var accessibilityTypeCell: UISegmentedControlTableViewCell = {
let cell = UISegmentedControlTableViewCell(reuseIdentifier: "accessibilityTypeCell")
cell.segmentedControl.insertSegment(withTitle: "Default", at: 0, animated: false)
cell.segmentedControl.insertSegment(withTitle: "Informative", at: 1, animated: false)
cell.segmentedControl.insertSegment(withTitle: "Custom informative", at: 2, animated: false)
cell.segmentedControl.insertSegment(withTitle: "Interactive", at: 3, animated: false)
cell.segmentedControl.insertSegment(withTitle: "Double interaction", at: 4, animated: false)
cell.segmentedControl.selectedSegmentIndex = 0
return cell
}()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only see a small issue and that is that the literals can not be read as a whole.
Screenshot 2024-09-10 at 15 35 11

Perhaps it would be interesting to rotate the segmented control vertically XD
https://stackoverflow.com/questions/3490358/can-i-show-an-uisegmentedcontrol-object-in-vertical

Present interactive cell alert in tabbarcontroller to avoid warning
@dhidalgofadrique dhidalgofadrique merged commit 023b62c into main Sep 12, 2024
2 checks passed
@dhidalgofadrique dhidalgofadrique deleted the IOS-10448-RowList-A11y-update branch September 12, 2024 06:11
tuentisre pushed a commit that referenced this pull request Sep 12, 2024
# [33.1.0](v33.0.0...v33.1.0) (2024-09-12)

### Features

* **Accessibility:** IOS-10448 [Mistica] RowList A11y update ([#399](#399)) ([023b62c](023b62c))
@tuentisre
Copy link
Collaborator

πŸŽ‰ This PR is included in version 33.1.0 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants