Skip to content

Give the sign-in field an accessible name and run the E2E suite that covers it - #180

Merged
aaronstevenwhite merged 1 commit into
stagingfrom
fix/e2e-enablement
Aug 31, 2026
Merged

Give the sign-in field an accessible name and run the E2E suite that covers it#180
aaronstevenwhite merged 1 commit into
stagingfrom
fix/e2e-enablement

Conversation

@aaronstevenwhite

Copy link
Copy Markdown
Contributor

Summary

E2E has been commented out of CI since before this backlog was written. Turning it on found a real accessibility bug in the sign-in form, which is the argument for turning it on.

The sign-in field had no accessible name. shadcn's FormControl clones its child to pass id, aria-describedby and aria-invalid; HandleInput accepted none of them and forwarded none to its <input>. So the <label for> never matched, and a screen reader announced an unlabelled text box on the first field of the login page. The E2E page object had been working around it by matching the placeholder — which then changed from bsky.social to yourhandle.example.com, breaking eight tests and leaving the underlying bug in place. The component now accepts and forwards those props, and the test binds to the label, which is the name a screen reader actually announces.

The landing-page tests described a page that no longer exists. They looked for a "Sign in with Bluesky" button and a Bluesky link; the button says "Sign in with ATProto" and the third link is ATProto. Naming one PDS operator was wrong about the product as well as the markup — Chive signs in any ATProto identity. Added coverage for the handle field itself, which nothing tested and which is the first thing a user touches.

Seventeen assertions of the form expect(true).toBe(true) across four spec files now assert something. Each computed a visibility check and discarded the result, so the test passed whether the element rendered or not. Where the seed makes the answer deterministic they assert it — isCorrespondingAuthor is set on every seeded eprint, ORCID is seeded, isHighlighted and contributions and paperDid are not, so those assert absence, which catches a page inventing data it was not given. Where the outcome genuinely depends on state the test does not control, they assert the state machine instead: after clicking vote the page must show either a confirmation or a sign-in prompt, and the approve/reject buttons must appear together or not at all. One case that cannot assert anything on /governance now calls test.skip with a reason, so the report says it did not run rather than that it passed.

The five test.skip calls in home.spec.ts stay skipped. Their reasons are correct — the browse button, submit CTA and features section genuinely are not on the alpha landing page. They were skipped via a comment above a silently-skipped test; they now call test.skip(true, '<reason>') so the reason reaches the Playwright report instead of living in a comment nobody reads.

CI runs the unauthenticated project. That is 24 tests over sign-in and the landing page, about two minutes, and it is green. The authenticated project is 487 tests taking hours on one worker; it is not enabled here and needs its own pass — a sharding decision and a triage of what currently fails. Running the part that is known good beats running nothing, which is what the commented-out job achieved.

Also here, since it is the same "config points somewhere nobody goes" problem:

  • Dependabot targeted main. Work reaches production through staging in this repository, so nine dependency PRs had accumulated against a branch the normal flow cannot merge them into. All four ecosystems now target staging.
  • The README advertised the four plugins that are not loaded. GitHub, ORCID, DOI registration and Wikidata are all written and none is constructed by any service. It now names the eleven that actually register, and says plainly that src/plugins/builtin/ holds more than run.
  • A clean clone could not typecheck. pnpm lexicons:generate must run before pnpm typecheck, because the generated directories are gitignored. Verified by moving src/lexicons/generated aside and watching tsc fail with Cannot find module, then restoring it.

Related Issues

CI-15 (partial), CI-16, CI-20 in the 0.8.0 backlog, plus the Dependabot set (#125-#133) and CFG-16.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

How Has This Been Tested?

Everything here was run, not reasoned about:

  • The accessibility fix was verified in both directions. With it, all 16 auth tests pass; removing the id forwarding again makes login form is accessible fail. The test previously used getByText('Handle or DID'), which matched the label and the description both and would have passed while the input had no accessible name at all — it now uses getByLabel, which only matches a real association.
  • Unauthenticated project: 24 passed, 5 skipped, 0 failed, from 8 passed / 2 failed at the start.
  • The four rewritten spec files plus their neighbours: 43 passed, 1 skipped, 0 failed against the running app and seeded data.
  • Frontend suite green (2647 tests), backend typecheck clean, format clean.
  • The CI workflow parses and registers e2e-tests as a job.

Checklist

General

  • I have performed a self-review of my code
  • Code follows style guide (npm run lint passes)
  • Tests added/updated for changes
  • All new and existing tests pass (npm test)
  • Documentation updated (if applicable)

ATProto Compliance (required for data flow changes)

  • Compliance tests pass (npm run test:compliance — 100% required)
  • No writes to user PDSes
  • BlobRef storage only (never blob data)
  • Indexes can be rebuilt from firehose
  • PDS source is tracked for staleness detection

One component prop change and test/CI configuration; no data flow touched.

Breaking Changes

  • N/A — no breaking changes

Still open

CI-15 is only half done. E2E still authenticates through ENABLE_E2E_AUTH_BYPASS and hand-written localStorage, so no test exercises real OAuth. That needs a test PDS and credentials in CI, which is a separate piece of work, and I have not pretended otherwise by enabling something that only looks like it covers sign-in.

expect(true).toBe(true);
// The seed gives this author an ORCID, so the link is not optional here.
await expect(orcidLink.first()).toBeVisible();
await expect(orcidLink.first()).toHaveAttribute('href', /orcid\.org/);
@aaronstevenwhite
aaronstevenwhite merged commit aa958fe into staging Aug 31, 2026
15 of 16 checks passed
@aaronstevenwhite
aaronstevenwhite deleted the fix/e2e-enablement branch August 31, 2026 10:26
@aaronstevenwhite aaronstevenwhite mentioned this pull request Aug 31, 2026
11 tasks
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.

2 participants