Skip to content

FFS-4819 Alternate text for images - #2050

Open
ccvetter wants to merge 9 commits into
mainfrom
ffs-4819-alt-text-for-images
Open

ccvetter wants to merge 9 commits into
mainfrom
ffs-4819-alt-text-for-images

Conversation

@ccvetter

@ccvetter ccvetter commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

FFS-4819

Changes

Added aria-labels to employer buttons so a screen reader can read them. The text reads "Select employer.name" so a user knows that they can take an action with the button rather than just reading the employers name. Buttons do not recognize alt text so an aria-label must be used.

Context for reviewers

These are the buttons:
Screenshot 2026-09-14 at 11 56 04 AM

Acceptance testing

Tag product and design in Slack for acceptance: @emmy-acceptance-testers

  • No acceptance testing needed
    • This change will not affect the user experience (bugfix, dependency updates, etc.)
  • Acceptance testing prior to merge
    • This change can be verified visually via screenshots attached below or by sending a link to a local development environment to the acceptance tester
    • Acceptance testing should be done by design for visual changes, product for behavior/logic changes, or both for changes that impact both.
  • Acceptance testing in PR Environment
    • This change can be verified in a PR environment. Run this Github Action with the existing PR and most recent git sha.
  • Acceptance testing after merge
    • This change is hard to test locally, so we'll test it in the demo environment (deployed automatically after merge.)
    • Make sure to notify the team once this PR is merged so we don't inadvertently deploy the unaccepted change to production. (e.g. :alert: Deploy block! @ffs-eng I just merged PR [#123] and will be doing acceptance testing in demo - please don't deploy until I'm finished!)

AI Usage

  • USED_AI: I attest that I have read, understood, and take ownership of all AI-generated code in this PR.
  • NO_AI: I did not use AI.

Preview environment

@ccvetter
ccvetter marked this pull request as ready for review September 9, 2026 18:44
@ccvetter
ccvetter requested a review from a team as a code owner September 9, 2026 18:44
@github-actions
github-actions Bot requested a review from millerti September 9, 2026 18:44
@ccvetter
ccvetter requested review from daphnegold and removed request for millerti September 10, 2026 13:08

@daphnegold daphnegold left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome, but could you add more test coverage?

Search result buttons are tested, but popular providers are not.

Comment thread app/config/locales/es.yml Outdated
Comment thread app/config/locales/es.yml Outdated
Comment thread app/spec/controllers/cbv/employer_searches_controller_spec.rb Outdated
@emmybot-cms

emmybot-cms commented Sep 11, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@github-actions

Copy link
Copy Markdown
Contributor

test-classifier: AI triage of failing tests

AI Test Classifier — triage of failing tests

Observed — these verdicts are grounded in the actual test run output.

Re-resolved the base: the dispatcher's AI_REVIEW_DIFF_RANGE (origin/main HEAD) was stale — origin/main is 3 commits ahead of this branch's merge-base, producing a reversed 43-file diff that was not PR #2050's change. Used the merge-base 4578ae3 instead, giving the real 8-file change (aria-labels on employer-search select buttons + i18n keys). Bootstrapped Ruby 3.4.10/bundler/Postgres 16/npm and ran CI's suites: rspec non-e2e is 2545 examples 0 failures, vitest is 88 tests 0 failures, erb_lint and rubocop clean on changed files, and both newly added aria-label specs pass. One CI gate genuinely fails: bundle exec i18n-tasks health exits 1 because commit ec07419 removed two es.yml select_employer values but added an ignore_missing entry with a non-existent key path (cbv.applicant_informations.employer_searches.employer.select_employer), leaving cbv.employer_searches.show.select_employer uncovered. An initial 26/26 failure with Sprockets AssetNotFound was my own sandbox missing rake assets:precompile; after precompiling those specs pass, so it is not reported as a finding.

Verdict Test Confidence Scope
APPLICATION_BUG i18n-tasks health › Missing translations (es: cbv.employer_searches.show.select_employer) high change
Per-test rationale
  • APPLICATION_BUG · other — i18n-tasks health › Missing translations (es: cbv.employer_searches.show.select_employer) (app/config/i18n-tasks.yml:157)
    The change deleted two es.yml select_employer values but added an ignore_missing entry for a key path that does not exist (cbv.applicant_informations.employer_searches.employer.select_employeremployer_searches sits directly under cbv: in en.yml), so cbv.employer_searches.show.select_employer has no es value and no ignore, and bundle exec i18n-tasks health exits 1. The check is correctly enforcing locale-completeness on the change's own config; fix the typo'd ignore key (or restore the es values) rather than relaxing the check.

React 👍 if right / 👎 if wrong, and on a 👎 please reply to this comment with a one-line reason — that reply is the most useful tuning signal we get. Advisory, non-blocking.

@ccvetter
ccvetter requested a review from daphnegold September 11, 2026 19:07

@daphnegold daphnegold left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like the CI is still failing because the i18n ignore path isn't correct and some keys are still reported as missing. Could you take another look?

@github-actions

Copy link
Copy Markdown
Contributor

test-classifier: AI triage of failing tests

AI Test Classifier — no action required

Observed — these verdicts are grounded in the actual test run output.

Re-resolved the base: AI_REVIEW_DIFF_RANGE was the two-dot range 'origin/main HEAD' against an origin/main that is 9 commits ahead of the merge-base, so it reported 43 unrelated files. Used 'origin/main...HEAD' (merge-base 4578ae3) instead, giving PR #2050's real 8-file change: aria-labels on employer-select buttons switched to i18n keys t('.select_employer', name:), 4 new en.yml keys, 3 i18n-tasks ignore_missing entries, and 2 new controller-spec examples. Reproduced the rspec.yml CI job locally (Postgres 16, bundle install, npm install, assets:precompile, db:schema:load). The two changed specs pass (26 examples, 0 failures) and the full non-e2e suite passes (2545 examples, 0 failures); i18n-tasks health and erb_lint on the 4 changed views also pass. Nothing to classify. Two bootstrap notes: an initial run showed 26 Sprockets AssetNotFound errors caused by my own omission of assets:precompile (resolved by precompiling, not a real failure, so not classified), and the Ruby 3.4.9 Gemfile pin was temporarily relaxed to 3.4.10 to run locally, then restored. Not run: spec/e2e (excluded by the CI rspec job, runs in a separate browser workflow) and the vitest JS suite (no JS/CSS in this change).

React 👍 if this is right (nothing needed triage) / 👎 if a real failure was missed, and on a 👎 please reply with a one-line reason. Advisory, non-blocking.

@github-actions

Copy link
Copy Markdown
Contributor

test-classifier: AI triage of failing tests

AI Test Classifier — triage of failing tests

Observed — these verdicts are grounded in the actual test run output.

Re-resolved the base: AI_REVIEW_DIFF_RANGE (origin/main HEAD) was wrong because origin/main is 9 commits ahead of the PR head, so the two-dot range reverse-applied unrelated main commits; used the merge base 4578ae3..HEAD instead, which matches PR #2050 (aria-labels on employer-search buttons + new i18n keys + two new controller specs). Ran the real suites: CI-configured rspec (parallel_test, e2e excluded) passed 2547/2547, both changed controller specs passed 28/28, Vitest passed 88/88, and erb_lint was clean. One CI check fails in scope: bundle exec i18n-tasks health exits 1 because en.yml requires normalization -- the new shared.header.menu key was added out of alphabetical order.

Verdict Test Confidence Scope
APPLICATION_BUG i18n-tasks health (normalization check) high change
Per-test rationale
  • APPLICATION_BUG · other — i18n-tasks health (normalization check) (app/config/locales/en.yml:1285)
    The PR added shared.header.menu: Menu after primary: instead of in alphabetical position, so bundle exec i18n-tasks health (the 'Check i18n-tasks health' workflow) exits 1 with 'The following data requires normalization'; running i18n-tasks normalize reproduces exactly that one reordering. The checker is correct and the committed locale data is wrong, so the fix belongs in en.yml, not in the check or its ignore lists.

React 👍 if right / 👎 if wrong, and on a 👎 please reply to this comment with a one-line reason — that reply is the most useful tuning signal we get. Advisory, non-blocking.

@github-actions

Copy link
Copy Markdown
Contributor

test-classifier: AI triage of failing tests

AI Test Classifier — no action required

Observed — these verdicts are grounded in the actual test run output.

Re-resolved the base: AI_REVIEW_DIFF_RANGE was the two-dot range 'origin/main HEAD', which included main-only commits as reversions (43 files). Used the three-dot range origin/main...HEAD (merge-base 4578ae3) instead, giving the real PR #2050 change: 8 files adding aria-labels to employer/provider buttons plus en.yml keys, i18n-tasks ignores, and two new controller specs. Ran the suite OBSERVED against Dockerized Postgres 16 with Ruby 3.4.10 and precompiled assets (per .github/actions/setup-project): full non-e2e RSpec = 2547 examples / 0 failures; the two changed spec files = 28 examples / 0 failures; i18n-tasks health = pass; vitest = 88 tests pass; erb_lint and rubocop on changed files = clean. An initial all-red run was my own bootstrap gap (missing npm install + assets:precompile, which CI performs) and resolved on re-run, so it is not classified. Nothing failed.

React 👍 if this is right (nothing needed triage) / 👎 if a real failure was missed, and on a 👎 please reply with a one-line reason. Advisory, non-blocking.

@daphnegold
daphnegold self-requested a review September 11, 2026 20:34

@daphnegold daphnegold left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you move both the popular provider tests outside the when there are search results context?

Comment thread app/config/i18n-tasks.yml Outdated
Comment thread app/config/locales/en.yml
Comment thread app/spec/controllers/activities/income/employer_searches_controller_spec.rb Outdated
@github-actions

Copy link
Copy Markdown
Contributor

test-classifier: AI triage of failing tests

AI Test Classifier — no action required

Observed — these verdicts are grounded in the actual test run output.

Re-resolved the base: AI_REVIEW_DIFF_RANGE was the two-dot 'origin/main HEAD', but origin/main has advanced 3 commits past the merge-base (4578ae3), so it pulled in ~40 unrelated files. Used the three-dot range origin/main...HEAD (8 files) as the true change under test for PR #2050 — aria-labels on employer-search buttons plus supporting en.yml/i18n-tasks.yml keys and two new controller specs. Ran the repo's real suite after reproducing CI setup (Postgres, bundle install, npm install, assets:precompile, db:schema:load): the two changed spec files pass 28/28, the wider spec/controllers plus activity_flow_navigator_spec pass 969/969, and 'i18n-tasks health' (gated by the locale edits) passes clean. An initial run showed 28 failures all caused by a missing precompiled USWDS sprite asset — a defect in my own bootstrap, resolved by running assets:precompile as CI does, so it is not reported as a classification. Ruby 3.4.9 is pinned but only 3.4.10 is in the runner toolcache; the pin was temporarily bumped and reverted after the run. No failing tests to classify.

React 👍 if this is right (nothing needed triage) / 👎 if a real failure was missed, and on a 👎 please reply with a one-line reason. Advisory, non-blocking.

@ccvetter
ccvetter requested a review from daphnegold September 14, 2026 12:13

@daphnegold daphnegold left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

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.

3 participants