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

Base implementations for SemanticEffect and SemanticOrderView #1240

Merged
merged 28 commits into from May 7, 2021

Conversation

jamesmontemagno
Copy link
Contributor

@jamesmontemagno jamesmontemagno commented Apr 26, 2021

Description of Change

Implements SemanticEffect and SemanticOrderView based on @PureWeen's repo

Bugs Fixed

API Changes

Added:

  • SemanticEffect
  • SemanticEffectRouter
  • SemanticOrderView

Behavioral Changes

PR Checklist

  • Has a linked Issue, and the Issue has been approved
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard

@jfversluis
Copy link
Member

Hey friends! How are we looking with this one? Just checking to see if you didn't accidentally forgot to flip it to "Ready for review"

@rachelkang rachelkang self-requested a review May 5, 2021 16:57
rachelkang
rachelkang previously approved these changes May 6, 2021
Copy link
Collaborator

@rachelkang rachelkang left a comment

Choose a reason for hiding this comment

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

it's READYYY! :D

@pictos pictos marked this pull request as ready for review May 7, 2021 01:49
Copy link
Contributor

@pictos pictos left a comment

Choose a reason for hiding this comment

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

Hey friends, thanks for this really awesome! I have some comments and a question related to the video below.
When I access the effect page and turn on the ScreenReader everything works as expected, but when I go to the SemanticPage the SecreenReader breaks, as you can see there is no blue rectangle in the selected element and to make it return I need to block the screen and wait for a while. I believe that is a bug.

a11y_broken.mp4

@rachelkang
Copy link
Collaborator

thanks for the review @pictos! I incorporated your feedback and fixed the code :)

As for the potential bug you described - were you able to repro it on a physical device? I tried but wasn't able to repro it / think it might potentially be an emulator issue and therefore not a bug

@PureWeen
Copy link
Contributor

PureWeen commented May 7, 2021

Another thing I'd like to highlight is that in general Forms is buggy with screen readers

That's one reason we had to add this workaround
https://github.com/xamarin/XamarinCommunityToolkit/pull/1240/files#diff-77077346fa32e599b9e0c8f6bbb51a6ac2e18b4182109177f4cf48ef94c80c94R13

Once we can merge this PR into 5.0
xamarin/Xamarin.Forms#14089

A lot of behavior should smooth out once that flag is enabled

@jfversluis jfversluis merged commit 54e3e94 into main May 7, 2021
@jfversluis jfversluis deleted the accessibility-views-effects branch May 7, 2021 15:11
@danielmonettelli
Copy link

Hi everyone, thank you so much for adding these great accessibility APIs, I wish they were available in the documentation including how to use IA11yService.

@jamesmontemagno
Copy link
Contributor Author

I will be working on the docs soon. Be sure to checkout https://channel9.msdn.com/Shows/XamarinShow/Improving-Accessibility-with-Xamarin-Community-Toolkit

@danielmonettelli
Copy link

danielmonettelli commented Jun 2, 2021

Thank you very much @rachelkang @PureWeen @jamesmontemagno @jfversluis, I love that they take accessibility as a priority.

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

Successfully merging this pull request may close these issues.

[Spec] SemanticOrderView
7 participants