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.

Sign up for GitHub

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

Jump to bottom

ANDROID-15075 ListRowItem Switch & CheckBox a11y compose #377

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

ANDROID-15075 ListRowItem Switch & CheckBox a11y compose #377

wants to merge 15 commits into from

Conversation

jeprubio
Copy link
Contributor

@jeprubio jeprubio commented Aug 28, 2024 •
edited
Loading

🥅 What's the goal?

Align ListRowItem accessibility for Compose according to new Figma definition: https://www.figma.com/design/Be8QB9onmHunKCCAkIBAVr/%F0%9F%94%B8-Lists-Specs?node-id=4977-5442&t=iqE1tr998GXqKVZ4-4

🚧 How do we do it?

  • Creating ListRowItemWithSwitch and ListRowItemWithCheckBox.
  • Letting developers to pass a contentDescription for badges.
  • Adding info to the compose list README file.
  • Fixing a bug in the catalog in compose as it was saying "Clickable Asset in Clickable Row" even if the row was not clickable.
  • Scouting (unrelated task): Adding MisticaTheme to the PasswordInput so the previews in that file work again.
  • Fixing links of README in ANDROID-14884. Improve accessibility for XML list items (Classic) #376 as lint was complaining that links are not found.

☑️ Checks

  • I updated the documentation, including readmes and wikis. If this is a breaking change, tag the PR with "Breaking Change" label and remember to include breaking change migration guide in release notes where this version is released.
  • Tested with dark mode.
  • Tested with API 24. The two new components work well in 24 but I had to use talkback on a device with api level 28, I couldn't manage to run talkback on 24-27 devices.
  • Sync done with iOS team for this feature to ensure alignment, if applies.

🧪 How can I test this?

  • 🖼️ Screenshots/Videos
Change Before After
Ordering
Current.Ordering.mov
New.Ordering.mov
Toggleables -
New.Toggleables.mov

@jeprubio jeprubio marked this pull request as ready for review August 29, 2024 08:49
@jeprubio jeprubio requested review from haynlo, a team, dpastor and yceballost and removed request for a team August 29, 2024 08:53
@Telefonica Telefonica deleted a comment from github-actions bot Aug 29, 2024
@Telefonica Telefonica deleted a comment from github-actions bot Aug 29, 2024
dpastor
dpastor approved these changes Aug 29, 2024
View reviewed changes
Copy link
Contributor

@dpastor dpastor left a comment

Choose a reason for hiding this comment

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

Good work!

@Telefonica Telefonica deleted a comment from github-actions bot Aug 30, 2024
@jeprubio jeprubio requested a review from haynlo August 30, 2024 06:47
haynlo
haynlo approved these changes Aug 30, 2024
View reviewed changes
Copy link
Contributor

@haynlo haynlo left a comment

Choose a reason for hiding this comment

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

Great job! 👏🏼

@yceballost yceballost requested review from aweell and removed request for yceballost September 2, 2024 10:48
@aweell
Copy link

aweell commented Sep 4, 2024 •
edited
Loading

Interactive lists in compose

According to spec, the interactive lists should be like one element to the talkback focus, in classic this is working as expected, but in compose seems that no matter the list is informative or interactive (navigation) the elements are focused individually.

Video of the behaviour:

Record_2024-09-04-11-36-33.mp4

(Some texts are not announced, but this is an issue with my phone not able to handle talkback and screen recording at the same time

The version where I'm seeing this issue is: https://install.appcenter.ms/orgs/tuenti-organization/apps/mistica-catalog/distribution_groups/public/releases/592

(Just in case I'm not testing in the latest version)

@jeprubio
Copy link
Contributor Author

jeprubio commented Sep 4, 2024 •
edited
Loading

Interactive lists in compose

According to spec, the interactive lists should be like one element to the talkback focus, in classic this is working as expected, but in compose seems that no matter the list is informative or interactive (navigation) the elements are focused individually.

Video of the behaviour:

Record_2024-09-04-11-36-33.mp4
(Some texts are not announced, but this is an issue with my phone not able to handle talkback and screen recording at the same time

The version where I'm seeing this issue is: https://install.appcenter.ms/orgs/tuenti-organization/apps/mistica-catalog/distribution_groups/public/releases/592

(Just in case I'm not testing in the latest version)

It was agreed in the Teams channel

This is compose and information cells, so the focus is individual with specific order as it is in iOS (not as in classic views).
Many of the rows in the catalog are not clickable despite having the chevron.

@Telefonica Telefonica deleted a comment from github-actions bot Sep 4, 2024
@aweell
Copy link

aweell commented Sep 4, 2024

#377 (comment)

Resolved offline, I was assuming that all list w/chevron were interactive, but the cases tested didn't have onClick.

The behaviour of the interactive list in compose works as expected

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 4, 2024

📱 New catalog for testing generated: Download

@aweell
Copy link

aweell commented Sep 4, 2024 •
edited
Loading

Badges behaviour in list with onClick

The badge should no have an independent focus of the list, currently it seems that:

  • Classic: the badge is included in the concatenated content (expected behaviour)
  • Compose: the badge is not included and has an independent focus after the list has been focused

Read badges content

I see value in allow to provide a contentDescription for the badge component i would keep that change in the badge for other uses.

In the list component we decided to remove the content of the badges of the concatenated content and make them in all cases invisible to screen readers (so the should not be focusable content even if the list is informative).

Why this?

  • If a content description is not provided, the numeric content of the badge is not giving the user a lot of information about what that number means, that depends of the context and we can't control the context.
  • If a content description is provided, we cannot anticipated in which position the badge should be ordered when concatenating the text.

The option we concluded that was the best experience for users is to delegate that responsibility in the product implementation phase, where the context can be controlled, allowing to provide a custom accesibilityLabel to the whole list where this information can be included in the right way.

Revisiting the specs I detected that this behaviour is not as well explained as should be, so i updated the section.

@jeprubio
Merge branch 'refs/heads/main' into feature/ANDROID-15075-ListRowItem…
023c01c 
…A11yCompose

# Conflicts:
#	library/src/main/java/com/telefonica/mistica/compose/badge/Badge.kt
#	library/src/main/java/com/telefonica/mistica/compose/list/ListRowItem.kt
@github-actions GitHub Actions
Copy link

📱 New catalog for testing generated: Download

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haynlo haynlo haynlo approved these changes

@dpastor dpastor dpastor approved these changes

@aweell aweell Awaiting requested review from aweell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jeprubio @aweell @dpastor @haynlo