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 #4790 Talkback will read toolbar title clickable #5027

Merged
merged 34 commits into from
Sep 15, 2023

Conversation

XichengSpencer
Copy link
Collaborator

@XichengSpencer XichengSpencer commented Jun 8, 2023

Explanation

Fix #4790 For toolbars that have marquee effects, add additional checks so that the effect will only be enabled when the accessibility service is not on.
Also updated the existing test for the marquee effect. The test now checks if the effect is set and activated correctly depending on the accessibility service status.
Update the test setting to ensure that test behavior is correctly simulated.

Whenever users use vocal assisting tools such as talkback, the accessibility service we inject in the field will be enabled. We will choose either to use a marquee effect for visual display, or just pure vocal display when the user enables the service.

For a toolbar that doesn't have an individual view for the toolbar title, setting 'onNavgation..listener' will make the whole toolbar clickable, and talkback will read "double click.."-
While we want to keep the navigation function, we can use getChildAt(0) to set the listener to just the nav icon.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
    Exploration Activity:

  • Normal:

20230616131620.mp4
  • Talkback On:
20230616131602.mp4

Topic Fragment:

  • Talkback On:
e5a4934e65656ff54bc48909d73bfad6.mp4

Issue with Espresso test:

when readerOff, toolbar.title.click() behaves different from manual test.

  • Video record for manual test

    Listener function is set and is called when click is performed. Regardless of title length, is selected will always be modified.

oppia-android.TopicFragmentPresenter.kt.oppia-android.app.2023-07-26.13-28-58.mp4
  • Video record for espresso test
    Listener function is set but not called when click is performed.
    The isSelected attribute does not been modified after click
oppia-android.TopicFragmentTest.kt.oppia-android.app.2023-07-26.15-03-52.mp4

Solution to Solve Error in Espresso Tests.

  • Setting the accessibility service before starting the activity solves accessibility service not correctly set except in TopicFragmentTest.
    Reason The set listener code in the app executes before setting the accessibility service in the test.

  • Add markSpotlightSeen before the start of the activity.
    Reason The spotlight might cause the toolbar unable to reach to simulate a click.

@XichengSpencer XichengSpencer requested a review from a team as a code owner June 8, 2023 01:50
@XichengSpencer XichengSpencer marked this pull request as draft June 8, 2023 01:50
@XichengSpencer XichengSpencer marked this pull request as ready for review June 13, 2023 14:24
Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

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

Thanks @XichengSpencer, this approach looks good, and you can look into applying this fix for other toolbars with the marquee effect.

I have left a suggestion, and don't forget to add tests for your changes.

@oppiabot
Copy link

oppiabot bot commented Jun 15, 2023

Hi @XichengSpencer, it looks like some changes were requested on this pull request by @adhiamboperes. PTAL. Thanks!

@XichengSpencer XichengSpencer removed their assignment Jun 30, 2023
@adhiamboperes adhiamboperes self-assigned this Jul 3, 2023
Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

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

Hi @XichengSpencer, really great job on this; your PR description is well detailed and good job on setting up the tests!

I have left some revision comments, and in addition:

  • Do use the ktlint formatter comand to order your imports before committing.
  • Please replicate these tests to the StoryFragmentTest and the corresponding test files of the modified activities.
  • Reply (single comment) to the comments that you have addressed.
  • Re-assign me with my @username PTAL.

@XichengSpencer
Copy link
Collaborator Author

@adhiamboperes
Hi Adhiambo,
After I made changes to add the Accessibility Service to the constructor of different presenters, it seems to mess up many tests and gives errors like this
image
They all complain about somewhere in the tests have a different constructor setting than our modified constructors. Is there any suggestion for this error?

Thanks!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 2, 2023
@adhiamboperes adhiamboperes removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 4, 2023
@oppiabot
Copy link

oppiabot bot commented Aug 11, 2023

Hi @XichengSpencer, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 11, 2023
@adhiamboperes adhiamboperes removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 11, 2023
@oppiabot
Copy link

oppiabot bot commented Aug 18, 2023

Hi @XichengSpencer, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 18, 2023
@adhiamboperes adhiamboperes removed PR: LGTM stale Corresponds to items that haven't seen a recent update and may be automatically closed. labels Aug 18, 2023
@oppiabot
Copy link

oppiabot bot commented Sep 5, 2023

Hi @XichengSpencer, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added stale Corresponds to items that haven't seen a recent update and may be automatically closed. and removed stale Corresponds to items that haven't seen a recent update and may be automatically closed. labels Sep 5, 2023
Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

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

Thanks @XichengSpencer! I only have a few minor corrections. See in-line.

I'd also like to point out that its good practice to avoid committing style only code changes in the future, as they can inflate the size of the PR. this happens when you format a file using Ctrl+Alt+L, but you can always be able to identify and revert unneeded formatting changes. Let me know if you need to learn how to do this in the next meeting.

@XichengSpencer
Copy link
Collaborator Author

Thanks @XichengSpencer! I only have a few minor corrections. See in-line.

I'd also like to point out that its good practice to avoid committing style only code changes in the future, as they can inflate the size of the PR. this happens when you format a file using Ctrl+Alt+L, but you can always be able to identify and revert unneeded formatting changes. Let me know if you need to learn how to do this in the next meeting.

Got it. I totally understand it will be great to address the code and style issues in the same commit. But I am not sure what needed formatting changes mean. Would you mind clarify a bit?

@adhiamboperes
Copy link
Collaborator

This LGTM. Thanks @XichengSpencer

@adhiamboperes adhiamboperes merged commit 89e8032 into oppia:develop Sep 15, 2023
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.

Talkback shouldn't read double tap to activate for the labels that are not clickable
2 participants