Clean up testIdProps usage

Sparowhawk commented 6 months ago

while working on auth slice migration it was noticed that testIdProps existed in several places that are not useful and in some places that are deprecated with our own code changes.

Most notable in the biometrics screen but I am sure there are others

Sparowhawk commented 3 months ago

Waiting on appts work and some of the claims works that is being currently worked on for this one as some of the cleanup is happening there

TKDickson commented 2 months ago

Over my WIP limit; marking as blocked.

TKDickson commented 1 month ago

Copying table from PR into ticket. To be SUPER clear, the "tested on X" signoffs are from Dylan. No QA testing done on this, yet.

Screen or Component	What Changed	Tested on iOS	Tested on Android
AccordionCollapsible	updated to testId since a11yLabel wasn't used	[x]	[x]
BackButton	updated to a11yLabel	[x]	[x]
BaseListItem	updated to testId and a11yLabel	[x]	[x]
CarouselTabBar	updated to testId since a11yLabel wasn't used	[x]	[x]
CollapsibleView	updated to a11yLabel since testID wasn't used	[x]	[x]
BasicError	updated to a11ylabel since testID wasn't used	[x]	[x]
VASelector	Updated to testID and a11yLabel for icon, testID for everything else	[x]	[x]
List	updated to testID and a11yLabel since both were used with same text	[x]	[x]
NavigationTabBar	updated to testID and a11yLabel since both were used with same text	[x]	[x]
Pagination	updated to testID and a11yLabel since both were used with same text	[x]	[x]
VABulletList	Updated to a11yLabel for TextView wrapper as testID wasn't used	[x]	[x]
VAImage	Updated to supply testID, was already supplying a11yLabel separetely	[x]	[x]
AppealIssues	Removed as it wasn't used for either testID or a11yLabel	[x]	[x]
TakePhotos	updated to a11yLabel	[x]	[x]
ClaimStatus	Removed as it wasn't used for either testID or a11yLabel, textID was supplied separately	[x]	[x]
ClaimPhase	change to a11yLabel	[x]	[x]
EstimatedDecisionDate	Changed to a11yLabel to combine the 2 text views to one a11yLabel	[x]	[x]
ClaimsAndAppealsListView	Removed as it wasn't used for either testID or a11yLabel	[x]	[x]
DEPRECATED_ClaimsAndAppealsListView	Removed as it wasn't used for either testID or a11yLabel	[x]	[x]
NeedHelpData	Removed all together from one textview, the call va was changed to a11yLabel	[x]	[x]
NoClaimsAndAppeals	Removed as it wasn't used for either testID or a11yLabel	[x]	[x]
NoClaimsAndAppealsAccess	Removed as it wasn't used for either testID or a11yLabel	[x]	[x]
NoDisabilityRatings	Removed as it wasn't used for either testID or a11yLabel	[x]	[x]
GenericLetter	Removed from template, updated textview to use a11yLabel	[x]	[x]
LettersListScreen	Removed as it wasn't used for either testID or a11yLabel	[x]	[x]
LettersOverviewScreen	Removed as it wasn't used for either testID or a11yLabel, testID was already supplied separately	[x]	[x]
NoLettersScreen	Removed as it wasn't used for either testID or a11yLabel	[x]	[x]
BiometricsPreferenceScreen	Removed from scrollview as it wasn't used and applied a11yLabel to text view since testID wasn't used	[x]	[x]
NoAppointments	Removed testID for both usages as it wasn't used, subText had an a11yLabel and applied that to the correct prop	[x]	[x]
NoMatchInRecords	Removed as it wasn't used for either testID or a11yLabel	[x]	[x]
Folders	Removed as it wasn't used for either testID or a11yLabel	[x]	[x]
NoInboxMessages	Removed as it wasn't used for either testID or a11yLabel	[x]	[x]
NotEnrolledSM	Removed as it wasn't used for either testID or a11yLabel	[x]	[x]
TermsAndConditions	removed one unused testID/a11yLabel and replaced one with a11yLabel for the textA11y	[x]	[x]
NoMilitaryInformationAccess	replaced with a11yLabel for the textA11y	[x]	[x]
GenericOnboarding	replaced with a11yLabel for the textA11y	[x]	[x]
SplashScreen	Replaced testIdProps with testID for e2e tests on scrollview that didn't use a11yLabel	[x]	[x]
SyncScreen	Removed all together, wasn't used for testId or A11yLabel	[x]	[x]
WebviewControlButton	Added a separate a11yLabel to the component props and supplied them both accordingly. Used by next component	[x]	[x]
WebviewControls	Replaced testIdProps with both a11yLabel and testID as it was needed for both	[x]	[x]
WebviewScreen	Replaced testIdProps with testID for e2e tests/unit tests on the wrapper box and content. Neither used the a11yLabel	[x]	[x]
WebviewTitle	updated title to use a11yLabel for the box title wrapper with icon since testId wasn't used	[x]	[x]
LoginScreen	Replaced testIdProps with testID for e2e tests on scrollview that didn't use a11yLabel, updated pressable with a11yLabel since testId wasn't used	[x]	[x]

bischoffa commented 1 month ago

Going to chat with @DJUltraTom about chipping away at this over time.

TKDickson commented 1 month ago

Background explanation of ticket: We had some code from initial implementation, that used to assign both testIDs (for unit tests, and then later detox tests), and also accessibility labels. The way it was written ended up causing a lot of issues, usually along the lines of "The correct thing happens with assistive tech X on (one platform), but with assistive tech Y on (that same platform) or assistive tech X on the other platform, it's not right"

So the FE team has been slowly removing it over time, and not using it for new features, etc. This is a ticket to remove the remaining places, all in one go -- which were a lot of components and some screens.

The three things that need checking are:

Unit tests (obviously not us)
Detox tests (run the full scripts, because these are broad changes), and
Screen reader full pass through the app (plus potentially some targeted testing based on the specific screens listed in the big table a couple of comments up)

Sparowhawk commented 1 month ago

didn't see any comments on why this was moved back so assuming it was due to conflicts in the PR that I just fixed, moving back to with QA

DJUltraTom commented 3 weeks ago

Tagging @rbontrager in on this ticket as I will be OOO

rbontrager commented 3 weeks ago

@Sparowhawk Can you fix the conflicts in the PR please

rbontrager commented 2 weeks ago

@Sparowhawk Couple things:

The webview refresh button is no longer saying "refresh screen" button and now either says button refresh (ios) or just button (android).
Looking at the code (because I can't seem to find any examples in the app) it looks like the following might actually cause issues I'm not seeing. Feel free to correct me if I'm wrong though.
- BaseListItem a11ylabel has been changed from testID || '' to just testID
- ListItem a11ylabel has been changed from titleA11yLabel ? titleA11yLabel : title to just titleA11yLabel

Sparowhawk commented 2 weeks ago

added refresh back to the button for the a11yLabel

Sparowhawk commented 2 weeks ago

both of those are correct

with testIdProps a '' test ID just wouldn't get assigned to the a11yLabel. If I assigned that to the a11yLabel that component would just not be read. So it will now correctly fall back on whatever text is supplied with it. This is very limitedly used and I don't think any had this use case however.

Similar for ListItem is that it will fall back on the title if the a11yLabel doesn't exist anyways. We don't need to specify an a11yLabel that is the exact same as the text.

rbontrager commented 2 weeks ago

Verified the following:

Detox tests pass: Android, Android Contact Info, IOS
Visual check that unit tests passed on the PR
Targeted testing of the screenreader for both Android and iOS in the app off the things changed above/in the code. What I tested can be found here
Cursory testing of the rest of the app using screenreader for both Android/iOS using the All screens in the app test cases as a guide.

Approved by QA

department-of-veterans-affairs / va-mobile-app

Clean up testIdProps usage #8216