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

LG-12723: Add alt text to the selfie checkmark for screen readability #10401

Merged
merged 6 commits into from
Apr 11, 2024

Conversation

amirbey
Copy link
Contributor

@amirbey amirbey commented Apr 10, 2024

🎫 Ticket

Link to the relevant ticket:
LG-12723

🛠 Summary of changes

Add "submit" in an alt tag to the selfie capture's green checkmark so that it can be read out by the screenreader.

📜 Testing Plan

  • Capture a selfie image via SDK
  • Enable screenreader on mobile device
  • Scroll to the green checkmark and here "use this photo [image]" read out

Note: the current default SDK version is 11.9.2. This can also be tested locally in 11.9.3 by updating application.yml as follows:
idv_acuant_sdk_version_default: '11.9.3'

@amirbey amirbey self-assigned this Apr 10, 2024
@amirbey amirbey marked this pull request as ready for review April 10, 2024 18:48
@amirbey amirbey requested review from a team, dawei-nava, night-jellyfish and kellular and removed request for a team and kellular April 10, 2024 18:49
@aduth
Copy link
Member

aduth commented Apr 10, 2024

Is there any way we can customize this without modifying the Acuant artifacts? This doesn't feel like a sustainable approach for future releases, unless this is something that Acuant plans to address in an upcoming release.

@zachmargolis
Copy link
Contributor

Is there any way we can customize this without modifying the Acuant artifacts? This doesn't feel like a sustainable approach for future releases, unless this is something that Acuant plans to address in an upcoming release.

Mostly just so I could see the diff, I used prettier to format the minified code and then diff it:

> diff <(npx prettier --stdin-filepath AcuantPassiveLiveness.min.js < public/acuant/11.9.2/AcuantPassiveLiveness.min.js) <(npx prettier --stdin-filepath AcuantPassiveLiveness.min.js < public/acuant/11.9.2/AcuantPassiveLiveness.min.js.back)
61186c61186
<                                   alt: 'submit',
---
>                                   alt: 'use this photo',

I've seen similar projects where we patch upstream source that we can't modify at runtime, so if we had the right build step maybe we could check this in as say, add-label.diff and then have a process like:

npx prettier --stdin-filepath AcuantPassiveLiveness.min.js < public/acuant/11.9.2/AcuantPassiveLiveness.min.js > expanded.js
patch -o patched.js expanded.js add-label.diff
npx terser --keep-classnames --keep-fnames --no-rename < expanded.js > FINAL_PATH_TBD.js

@amirbey
Copy link
Contributor Author

amirbey commented Apr 10, 2024

@zachmargolis @aduth - this is supposed to be a one time workaround. We are expecting the next SDK v11.9.4 to come with alt tags

@amirbey
Copy link
Contributor Author

amirbey commented Apr 10, 2024

Is there any way we can customize this without modifying the Acuant artifacts? This doesn't feel like a sustainable approach for future releases, unless this is something that Acuant plans to address in an upcoming release.

Mostly just so I could see the diff, I used prettier to format the minified code and then diff it:

> diff <(npx prettier --stdin-filepath AcuantPassiveLiveness.min.js < public/acuant/11.9.2/AcuantPassiveLiveness.min.js) <(npx prettier --stdin-filepath AcuantPassiveLiveness.min.js < public/acuant/11.9.2/AcuantPassiveLiveness.min.js.back)
61186c61186
<                                   alt: 'submit',
---
>                                   alt: 'use this photo',

I've seen similar projects where we patch upstream source that we can't modify at runtime, so if we had the right build step maybe we could check this in as say, add-label.diff and then have a process like:

npx prettier --stdin-filepath AcuantPassiveLiveness.min.js < public/acuant/11.9.2/AcuantPassiveLiveness.min.js > expanded.js
patch -o patched.js expanded.js add-label.diff
npx terser --keep-classnames --keep-fnames --no-rename < expanded.js > FINAL_PATH_TBD.js

@zachmargolis - sorry for any confusion here ... the alt tag does not currently exist in main. I initially added the alt tag with the value of submit in commits in this branch (c37b46b and .b2c1c70) . and then in a later commit updated the values to use this photo. Since this will be a squash merge I'm not expecting a trail of this outside of this PR.

@zachmargolis
Copy link
Contributor

@zachmargolis - sorry for any confusion here ... the alt tag does not currently exist in main. I initially added the alt tag with the value of submit in commits in this branch (c37b46b and .b2c1c70) . and then in a later commit updated the values to use this photo. Since this will be a squash merge I'm not expecting a trail of this outside of this PR.

got it! I diffed the wrong file version

@zachmargolis
Copy link
Contributor

diff from origin/main

> diff -C 1 <(npx prettier --stdin-filepath AcuantPassiveLiveness.min.js < public/acuant/11.9.2/AcuantPassiveLiveness.min.js) <(npx prettier --stdin-filepath AcuantPassiveLiveness.min.js < public/acuant/11.9.2/AcuantPassiveLiveness.min.js.back)

*** /dev/fd/11	Wed Apr 10 14:27:25 2024
--- /dev/fd/12	Wed Apr 10 14:27:25 2024
***************
*** 61185,61186 ****
--- 61185,61187 ----
                                    onClick: this.takePhoto,
+                                   alt: 'use this photo',
                                    class: 'shoot takePhoto',

@aduth
Copy link
Member

aduth commented Apr 11, 2024

Do we have any consideration for translations here?

@amirbey
Copy link
Contributor Author

amirbey commented Apr 11, 2024

@aduth - i don't think so, this code all within the SDK and the the Acuant SDK code does not perform translations

@amirbey amirbey merged commit 46a1e81 into main Apr 11, 2024
2 checks passed
@amirbey amirbey deleted the amirbey/LG-12723-checkmark-screenreadable branch April 11, 2024 14:17
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.

None yet

3 participants