zulip-mobile icon indicating copy to clipboard operation
zulip-mobile copied to clipboard

Fix:announcements by the screen reader interrupt any existing speech

Open 1010nishant opened this issue 3 years ago • 5 comments

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

1010nishant avatar Jan 23 '23 19:01 1010nishant

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 https://github.com/zulip/zulip-mobile/pull/5643#issuecomment-1398824903.

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?

chrisbobbe avatar Jan 23 '23 19:01 chrisbobbe

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 avatar Jan 23 '23 20:01 1010nishant

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?

chrisbobbe avatar Jan 23 '23 21:01 chrisbobbe

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.

1010nishant avatar Jan 25 '23 16:01 1010nishant

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.

chrisbobbe avatar Jan 25 '23 21:01 chrisbobbe