Fixes #3712: Add accessibility support for ImageRegionSelectionInteraction [Removed Todo followed by #4497]

[vrajdesai78]

Explanation

Fixes #3712: Removed TODO followed by PR #4497

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).

Video

image-region.mp4

[BenHenning]

Thanks @vrajdesai78. I had one comment, but otherwise isn't this duplicating work already checked into develop from #4497? Can those deltas be removed since they're no longer needed?

[BenHenning]

FWIW I suspect that you need to merge in the latest develop to remove the duplicated deltas.

[vrajdesai78]

Thanks @vrajdesai78. I had one comment, but otherwise isn't this duplicating work already checked into develop from #4497? Can those deltas be removed since they're no longer needed?

You are correct this changes are already merged with previous PR and for this PR I have already merged with latest develop still it shows changes. So, I think I need to create a new branch instead of using older branch to remove this duplicate deltas? WDYT @BenHenning

[rt4914]

Approved for code-owners and based on previous PR

[BenHenning]

Thanks @vrajdesai78. I had one comment, but otherwise isn't this duplicating work already checked into develop from #4497? Can those deltas be removed since they're no longer needed?

You are correct this changes are already merged with previous PR and for this PR I have already merged with latest develop still it shows changes. So, I think I need to create a new branch instead of using older branch to remove this duplicate deltas? WDYT @BenHenning

@vrajdesai78 are you sure that your fork's develop is up-to-date with Oppia Android's develop branch? I just want to make sure since the situation seems a bit odd.

We shouldn't need to recreate the branch. If the above doesn't help, I suggest reaching out to me on chat and we can work through steps to try and address it.

[vrajdesai78]

Thanks @vrajdesai78. I had one comment, but otherwise isn't this duplicating work already checked into develop from #4497? Can those deltas be removed since they're no longer needed?

You are correct this changes are already merged with previous PR and for this PR I have already merged with latest develop still it shows changes. So, I think I need to create a new branch instead of using older branch to remove this duplicate deltas? WDYT @BenHenning

@vrajdesai78 are you sure that your fork's develop is up-to-date with Oppia Android's develop branch? I just want to make sure since the situation seems a bit odd.

We shouldn't need to recreate the branch. If the above doesn't help, I suggest reaching out to me on chat and we can work through steps to try and address it.

I hope now develop branch is merged properly. @BenHenning

[BenHenning]

Great to hear that the merging is addressed now @vrajdesai78. I've followed up to your other comment (apologies, I think I missed it earlier).

[oppiabot]

Hi @vrajdesai78, 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!

[vrajdesai78]

Keep this PR open

[oppiabot]

Hi @vrajdesai78, 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!

[vrajdesai78]

Keep this PR open

[oppiabot]

Hi @vrajdesai78, 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!

[vrajdesai78]

Keep this PR open

[oppiabot]

Hi @vrajdesai78, 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!

[vrajdesai78]

Keep this PR open

[vrajdesai78]

I have filed a new issue #4639 to fetch image region from it's content description in end-to-end tests. So, in this PR I have have updated issue number in ToDo. Can we merge this PR and close issue #3712 now as seperate issue for end-to-end tests is already filed.

[BenHenning]

I have filed a new issue #4639 to fetch image region from it's content description in end-to-end tests. So, in this PR I have have updated issue number in ToDo. Can we merge this PR and close issue #3712 now as seperate issue for end-to-end tests is already filed.

@vrajdesai78 please add a video to this PR manually demonstrating what the end-to-end test would be verifying. That part is important before we consider #3712 is done without updating the end-to-end tests.

[BenHenning]

Thanks @vrajdesai78. PTAL at my latest comment. That needs to be addressed before I can approve & merge this.

[vrajdesai78]

I have filed a new issue #4639 to fetch image region from it's content description in end-to-end tests. So, in this PR I have have updated issue number in ToDo. Can we merge this PR and close issue #3712 now as seperate issue for end-to-end tests is already filed.

@vrajdesai78 please add a video to this PR manually demonstrating what the end-to-end test would be verifying. That part is important before we consider #3712 is done without updating the end-to-end tests.

Thanks @BenHenning, I have added video in this PR description as well as in #4639 also.

[BenHenning]

Thanks @vrajdesai78. LGTM.

Also for posterity, we're fine with removing this TODO because end-to-end tests aren't quite 100% stable/ready to run, and getting them working has taken longer than expected. We'll need to update & verify the test passes.