Implement new accessibility voiceover labels ios#9867
Conversation
540ba86 to
45375fc
Compare
|
🚨 End to end tests failed. Please check the failed workflow run. |
8bcad91 to
d734daa
Compare
rablador
left a comment
There was a problem hiding this comment.
@rablador reviewed 28 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on pinkisemils).
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 36 at r1 (raw file):
let currentState = tunnelManager.tunnelStatus.state pendingAnnouncementTask = Task { [weak self] in try? await Task.sleep(for: .seconds(4))
It doesn't feel great using arbitrary delays to achieve what we want here (and in handleTunnelStateChange below). I tried using the accessibilitySpeechQueueAnnouncement attribute on attributed string, but it didn't help very much. The main problem is that the user might start interacting in different ways and is then suddenly interrupted by the announcement once the timer runs out.
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 46 at r1 (raw file):
private func handleTunnelStateChange(_ state: TunnelState) { guard UIAccessibility.isVoiceOverRunning else { return }
I don't think we need to check for this, neither here nor below. If voiceover is not running, nothing is announced anyway. This attribute is mostly useful for adapting the UI for some accessibility specific thing.
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 54 at r1 (raw file):
// Cancel any pending announcement to avoid stale reads. pendingAnnouncementTask?.cancel()
I think this is not necessary. Any unread announcements should be canceled whenever a new one is posted.
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 69 at r1 (raw file):
/// Returns a localized announcement string, or `nil` for transient states that should be silent. private nonisolated func announcementString(for state: TunnelState) -> String? {
We should prefix a few of this with "Tunnel" or "VPN" or something, eg. "(Tunnel) Disconnected". Otherwise it's a little hard to know the context, especially when there are button labels with the exact same text.
pinkisemils
left a comment
There was a problem hiding this comment.
@pinkisemils made 4 comments.
Reviewable status: 24 of 29 files reviewed, 4 unresolved discussions (waiting on rablador).
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 36 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
It doesn't feel great using arbitrary delays to achieve what we want here (and in
handleTunnelStateChangebelow). I tried using theaccessibilitySpeechQueueAnnouncementattribute on attributed string, but it didn't help very much. The main problem is that the user might start interacting in different ways and is then suddenly interrupted by the announcement once the timer runs out.
Since the UI changes after a user initiated connection/disconnection/reconnection, voiceover will read out the currently selected label which will stop the announcement early. Do you have a good idea as to how to work around that? I agree this is not an ideal solution. I feel like we either accept the bad one here or remove it from this PR and work hard on fixing that separately. I think there's enough interesting work here as is.
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 46 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
I don't think we need to check for this, neither here nor below. If voiceover is not running, nothing is announced anyway. This attribute is mostly useful for adapting the UI for some accessibility specific thing.
Done.
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 54 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
I think this is not necessary. Any unread announcements should be canceled whenever a new one is posted.
I think it is feasible that multiple announcements might be delivered in series where the user should only care to know what the current tunnel state is. Thus, cancelling the previous invocation works best IMO.
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 69 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
We should prefix a few of this with "Tunnel" or "VPN" or something, eg. "(Tunnel) Disconnected". Otherwise it's a little hard to know the context, especially when there are button labels with the exact same text.
Done.
|
The fruit of my frivolous labour - https://github.com/mullvad/mullvadvpn-app/actions/runs/22191083075 |
rablador
left a comment
There was a problem hiding this comment.
@rablador reviewed 5 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pinkisemils).
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 36 at r1 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
Since the UI changes after a user initiated connection/disconnection/reconnection, voiceover will read out the currently selected label which will stop the announcement early. Do you have a good idea as to how to work around that? I agree this is not an ideal solution. I feel like we either accept the bad one here or remove it from this PR and work hard on fixing that separately. I think there's enough interesting work here as is.
Yes, as discussed IRL, let's keep it as-is.
mojganii
left a comment
There was a problem hiding this comment.
Accessibility hint is not available for the footer of sections in ´SelectocationView´. don't we have corresponding action for the long press?
@mojganii made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pinkisemils and rablador).
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 36 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
Yes, as discussed IRL, let's keep it as-is.
there is a slightly delay when several action happen. is it bad if we let OS dequeue them?
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 60 at r3 (raw file):
try? await Task.sleep(for: .seconds(1.5)) guard !Task.isCancelled else { return } guard let self else { return }
unused, self != ni?
pinkisemils
left a comment
There was a problem hiding this comment.
Currently we do not. The location view changes are mostly drive-by changes - more improvements can be had there in general.
@pinkisemils made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on rablador).
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 36 at r1 (raw file):
Previously, mojganii wrote…
there is a slightly delay when several action happen. is it bad if we let OS dequeue them?
The issue is that moving to a different UI control will override our global announcement. To delay the global announcement is not an ideal solution, in fact, it is a rather bad one, but we do not know if/how to improve it. This is still better than not announcing it at all.
rablador
left a comment
There was a problem hiding this comment.
@rablador resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pinkisemils).
pinkisemils
left a comment
There was a problem hiding this comment.
@pinkisemils made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on mojganii).
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 60 at r3 (raw file):
Previously, mojganii wrote…
unused, self != ni?
If the announcer no longer exists, we shouldn't send any notification.
mojganii
left a comment
There was a problem hiding this comment.
@mojganii made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pinkisemils).
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 60 at r3 (raw file):
Previously, pinkisemils (Emīls Piņķis) wrote…
If the announcer no longer exists, we shouldn't send any notification.
I know but linter complains it's unused. self != nil can silence it.
df4facd to
1fea9ce
Compare
pinkisemils
left a comment
There was a problem hiding this comment.
@pinkisemils made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on mojganii).
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 60 at r3 (raw file):
Previously, mojganii wrote…
I know but linter complains it's unused. self != nil can silence it.
Done.
mojganii
left a comment
There was a problem hiding this comment.
@mojganii made 1 comment and resolved 1 discussion.
Reviewable status: 21 of 29 files reviewed, all discussions resolved (waiting on rablador).
rablador
left a comment
There was a problem hiding this comment.
@rablador reviewed 8 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pinkisemils).
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 60 at r4 (raw file):
try? await Task.sleep(for: .seconds(1.5)) guard !Task.isCancelled else { return } if self == nil { return }
Do we even need this check?
pinkisemils
left a comment
There was a problem hiding this comment.
@pinkisemils reviewed 1 file and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on rablador).
ios/MullvadVPN/Accessibility/TunnelStateAccessibilityAnnouncer.swift line 60 at r4 (raw file):
Previously, rablador (Jon Petersson) wrote…
Do we even need this check?
Yes, for we don't want to announce anything when we are shutting down.
1fea9ce to
0840049
Compare
rablador
left a comment
There was a problem hiding this comment.
@rablador resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved.
The task only concerns the connect view, but I've gone and slightly improved accessibility also in the location view.
The PR introduces a lot of small fixes for accessibility in the main view:
This change is