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

Fix:announcements by the screen reader interrupt any existing speech #5644

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

1010nishant
Copy link

TODO comment: Use announceForAccessibilityWithOptions to queue this behind any in-progress announcements
announceForAccessibility() replaced with announceForAccessibilityWithOptions().
By default, announcements will interrupt any existing speech. they can be queued behind existing speech by setting queue to true.

fixes: #5611

@chrisbobbe
Copy link
Contributor

Thanks! I see you created two different PRs, #5642 and #5643, for the same issue. In future please force-push to the same PR instead of making a new one, as I said at #5643 (comment).

Please format your commit message according to our style: https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#git-commits-commit-messages

Were you able to do any manual testing to make sure this new piece of React Native API worked as intended, without breaking anything?

@1010nishant
Copy link
Author

1010nishant commented Jan 23, 2023

Yes, As you told me previously that force-push to the same PR instead of making a new one, I had tried to do that but couldn't figure it out, Open Pull Request Button had been disabled in PR #5643 and I got confused there and opened a new PR.
Can you tell me the correct procedure to do it?

also, the commit message style part
if you explain to me how my commit is not formatted according to the zulip style then it would be really helpful.

I know, I am asking to you very silly questions but it will be very helpful for me in my growth.

Yes, I manually tested by running command tools/test --all

@1010nishant 1010nishant force-pushed the set-queue-on-accessibility-announcement branch from 5c4236d to 964ab78 Compare January 23, 2023 20:29
@chrisbobbe
Copy link
Contributor

chrisbobbe commented Jan 23, 2023

For the commit message:

Fix:announcements by the screen reader interrupt any existing speech

TODO comment:Use announceForAccessibilityWithOptions to queue this behind
any in-progress announcements

By default announcements will interrupt any existing speech, but they can be queued
behind existing speech by setting queue to true in the second parameter of
announceForAccessibilityWithOptions method as it takes an object.

i have replaced announceForAccessibility() with announceForAccessibilityWithOptions().

fixes: #5611

How about:

offline notice: Queue non-urgent a11y announcements behind in-progress ones

Fixes: #5611

(That uses the abbreviation "a11y" for "accessibility" just to keep the summary line at 76 characters or below, according to our requirements.)

I think that communicates the change more efficiently to someone who's interested in this part of the project's Git commit history. It's a great idea to read the history of the project if you're still confused about our commit style after reading the style guide.


For issues with force pushing your changes to the same PR branch, please ask in #git help in the Zulip development community.


tools/test --all runs automated tests, and I don't expect those to confirm that

this new piece of React Native API worked as intended, without breaking anything

What I mean by "manual testing" is, have you run the app with TalkBack (Android) or VoiceOver (iOS) enabled, and confirmed that your commit changes the app's behavior in the way we want it to?

@1010nishant 1010nishant force-pushed the set-queue-on-accessibility-announcement branch from 964ab78 to de56346 Compare January 25, 2023 16:30
@1010nishant
Copy link
Author

I have changed the commit according to zulip commit style and as you suggested to do.
Also, I have tested manually.
it works as we want to.

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Jan 25, 2023

One commit-message nit: please capitalize the "f" in Fixes: #5611, according to our style guide and the suggestion I gave.

Also, I have tested manually.
it works as we want to.

Great, thanks! Could you walk me through your testing procedure in some more detail (what you did, and what you observed)? In particular, if there's anything you didn't get a chance to test, please let us know so that we can test ourselves. If you tested on both iOS and Android, did you notice any differences in behavior, or did the announcement get queued on both platforms? Thanks.

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.

Set queue on accessibility announcements in OfflineNoticeProvider
2 participants