Skip to content
This repository has been archived by the owner on Jun 7, 2020. It is now read-only.

[WIP] [NEW] Project 08 - Adding Reactions VoiceOver accessible #2746

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

rudrankriyam
Copy link
Contributor

@rudrankriyam rudrankriyam commented Aug 7, 2019

@RocketChat/ios

  • To open the Add Reactions View, double tap on the name and hold it for a second. (The Voice Over way of long press)
  • Categories.custom / categories.recent and all the other categories need to be assigned an accessibility label, which are defined in the RCEmojiKit.strings. Currently, they are described in VoiceOver as “Tab x of 10” button.
  • The skinTone button needs to be assigned accessibilityLabel = “Change Skin Tone”
  • Added current skin tone for the skinTone button
  • The custom emojis are not tapped as single elements, but as a whole view.
  • Add the labels for each custom emoji.
  • Fixed custom emojis bug.
  • Header accessibility for emoji category.

Copy link
Collaborator

@Sameesunkaria Sameesunkaria left a comment

Choose a reason for hiding this comment

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

On the cells with the Emojione emojis, I have to swipe twice to move to the next one. This is really critical to fix.

The Change skin tone button does not give me any feedback about what skin tone is currently selected. This is also really important to fix.

The emoji category headings need to have the heading trait.

Also, the reactions view on the messages is not really accessible. Rn it just says the name of the emoji, and the number of people who reacted with that emoji, when you swipe right. I'm also curious about how you'll be handling voiceover for the messages view, and especially with the reactions.

@rudrankriyam rudrankriyam changed the title [NEW] Adding Reactions VoiceOver accessible - Project 08 [WIP] [NEW] Adding Reactions VoiceOver accessible - Project 08 Aug 7, 2019
@codecov
Copy link

codecov bot commented Aug 11, 2019

Codecov Report

Merging #2746 into develop will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2746      +/-   ##
===========================================
- Coverage     46.1%   46.09%   -0.02%     
===========================================
  Files          626      626              
  Lines        28938    28946       +8     
===========================================
  Hits         13343    13343              
- Misses       15595    15603       +8
Impacted Files Coverage Δ
...nal/RCEmojiKit/Views/EmojiPicker/EmojiPicker.swift 0% <0%> (ø) ⬆️
...it/Views/EmojiPicker/EmojiCollectionViewCell.swift 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f5ad00...46d200c. Read the comment docs.

Copy link
Collaborator

@Sameesunkaria Sameesunkaria left a comment

Choose a reason for hiding this comment

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

The headers and the custom emojis are working nicely.

There are still a few things that need to be improved.

  • Now when I swipe through the normal emojis, sometimes they are skipped. I've pointed that out in a comment.

  • The skin tones should have a more descriptive name. (Maybe you can use Emojipedia skin tone modifiers as an example. Apple uses the same naming convention.)

  • I noticed the default/yellow skin tone is called "tone 5", which is the same as the darkest skin tone. You should have to handle the case. And maybe you can call it the default skin tone.

  • The change skin tone button should tell us the current skin tone whenever voiceover is reading the button out. Right now the skin tone is only read out when the button is tapped.

@@ -44,8 +44,11 @@ final class EmojiCollectionViewCell: UICollectionViewCell {
guard let url = url else { return }
ImageManager.loadImage(with: url, into: emojiImageView)
emojiImageView.isHidden = false
emojiImageView.isAccessibilityElement = true
emojiLabel.accessibilityElementsHidden = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

You would wanna undo these changes in the .standard case. Because, the cells are reusable and later when the same cell is populated with a standard emoji, it'll be skipped by voiceover.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch. I forgot this. I will fix this.

@rudrankriyam
Copy link
Contributor Author

Thank you for your review @Sameesunkaria !

And thanks for the skin tone names! I will edit the skinTone array from
let skinTones: [(name: String?, color: UIColor)]
to
let skinTones: [(name: String?, color: UIColor, accessibilityName: String?)]

For the default skinTone, I don't know why nil was used for the name. I will edit the accessibilityName to default.

@rudrankriyam rudrankriyam changed the title [WIP] [NEW] Adding Reactions VoiceOver accessible - Project 08 [WIP] [NEW] Project 08 - Adding Reactions VoiceOver accessible Aug 18, 2019
@@ -112,7 +112,7 @@ final class EmojiPicker: UIView, RCEmojiKitLocalizable {
}
}

var currentSkinTone: (name: String?, color: UIColor) {
var currentSkinTone: (name: String?, color: UIColor, accessibilityName: String) {

Choose a reason for hiding this comment

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

Large Tuple Violation: Tuples should have at most 2 members. (large_tuple)

(name: "tone3", color: #colorLiteral(red: 0.7776196599, green: 0.6034522057, blue: 0.4516467452, alpha: 1)),
(name: "tone4", color: #colorLiteral(red: 0.6469842792, green: 0.4368215203, blue: 0.272474587, alpha: 1)),
(name: "tone5", color: #colorLiteral(red: 0.391161263, green: 0.3079459369, blue: 0.2550256848, alpha: 1))
let skinTones: [(name: String?, color: UIColor, accessibilityName: String)] = [

Choose a reason for hiding this comment

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

Large Tuple Violation: Tuples should have at most 2 members. (large_tuple)

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

Successfully merging this pull request may close these issues.

3 participants