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

TabBar - Better accessibility roles and instructions #3501

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

nitzanyiz
Copy link
Collaborator

@nitzanyiz nitzanyiz commented Feb 12, 2025

Description

  • Added tab accessibility role to the tab items and tablist role to the tab controller.
  • Added selected accessibility state - It was reading that everything was not selected, even when the tab was selected.
  • Remove the state and test the instructions it gives on a real device. When it is not passed it doesn't give feedback that the tab was selected and doesn't state if the tab is selected when you focus it.

We can give each tab item an isSelected state and change it accordingly so a state change will only effect the specific TabItem instead of handling it in the context scope and passing it down. wdyt?

Changelog

TabBar - Added better accessibility roles.

Additional info

MADS-4590

@nitzanyiz nitzanyiz requested a review from M-i-k-e-l February 12, 2025 12:31
@nitzanyiz nitzanyiz changed the title Fix/tab bar accessibility role TabBar - Better accessibility roles and instructions Feb 12, 2025
@nitzanyiz nitzanyiz marked this pull request as ready for review February 12, 2025 12:31
Copy link
Contributor

@M-i-k-e-l M-i-k-e-l left a comment

Choose a reason for hiding this comment

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

Not sure why but it's not working for me

@@ -226,6 +226,9 @@ export default function TabBarItem({
style={_style}
onLayout={onLayout}
testID={testID}
accessible
accessibilityRole="tab"
accessibilityState={{selected: selectedIndex === index}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use currentPage.value to remove most of the code in the PR

Suggested change
accessibilityState={{selected: selectedIndex === index}}
accessibilityState={{selected: currentPage.value === index}}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately this doesn't work because the value doesn't change with the javascript :/
Did it work for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's the re-selection, I'll re-test

Copy link
Contributor

Choose a reason for hiding this comment

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

@nitzanyiz lets try to do what you've suggested (use useAnimatedReaction) instead of the context

Copy link
Collaborator Author

@nitzanyiz nitzanyiz Feb 20, 2025

Choose a reason for hiding this comment

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

Changed. I added useAnimatedReaction in the TabBarItem that will change its state only when it becomes selected or unselected.

@M-i-k-e-l M-i-k-e-l assigned nitzanyiz and unassigned M-i-k-e-l Feb 18, 2025
@nitzanyiz
Copy link
Collaborator Author

Did you try on a real device? It not working in the inspector but does on a real device.

@nitzanyiz nitzanyiz assigned M-i-k-e-l and unassigned nitzanyiz Feb 18, 2025
@M-i-k-e-l
Copy link
Contributor

Yes, I've tested on a real device withe the public demo

@nitzanyiz nitzanyiz assigned M-i-k-e-l and unassigned nitzanyiz Feb 19, 2025
@M-i-k-e-l M-i-k-e-l assigned nitzanyiz and unassigned M-i-k-e-l Feb 20, 2025
@nitzanyiz nitzanyiz assigned M-i-k-e-l and unassigned nitzanyiz Feb 20, 2025
@nitzanyiz
Copy link
Collaborator Author

@M-i-k-e-l I changed the state to be controlled in the TabBarItem. Wdyt?

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

Successfully merging this pull request may close these issues.

2 participants