Skip to content

Fix accessible names for sidebar icon buttons - #922

Closed
DevvoLazza wants to merge 1 commit into
libredb:mainfrom
DevvoLazza:fix/sidebar-accessibility-labels
Closed

DevvoLazza wants to merge 1 commit into
libredb:mainfrom
DevvoLazza:fix/sidebar-accessibility-labels

Conversation

@DevvoLazza

Copy link
Copy Markdown
Contributor

Description

Give icon-only controls accessible names so screen readers identify their actions, and make connection actions visible when focused with the keyboard.

Type of Change

  • Bug fix
  • Test addition or update

Related Issue

Closes #919

Changes Made

  • Name new/edit/delete connection controls, table action menus, and the search-clear button; include connection/table names where appropriate.
  • Show edit, duplicate, and delete icons on keyboard focus.
  • Name mobile card/table view switches and expose their selected state with aria-pressed.
  • Strengthen tests to locate controls by accessible name and verify callbacks, renamed/active/managed connections, and the already-labelled apply/discard actions.

Testing

  • Targeted regression tests: 125 passed. The revised tests failed before the fix.
  • Full Windows run: 544 files passed; 17,554 tests passed, 71 skipped. Twelve Helm test files were not run because Helm is unavailable.
  • Passed formatting, lint, TypeScript, knip, README/chart/channel/security guards, application build, and library build. Existing lint/build warnings remain.
  • Browser checks: zero unnamed sidebar buttons, including expanded table menus; Tab/Enter activation and mobile view switching verified.
  • Modified components: 100% line coverage. The global coverage gate reports 99.99% because the existing POSIX-only instrumentation failure-path test is skipped on Windows (src/instrumentation.ts:32–36).

Checklist

  • Self-reviewed; only accessibility changes and regression tests included.
  • No generated files, local configuration, secrets, or lockfile changes included.
  • Required CI checks, including the 100% global coverage gate.

@cevheri cevheri added the enhancement New feature or request label Sep 16, 2026

@cevheri cevheri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This closes #919 properly. Every control the issue enumerates now has a name, and I went through every button in the sidebar tree on main looking for a sixth unnamed one and did not find it. The focus-visible:opacity-100 on the connection actions was not asked for and is the right call: those buttons were focusable while invisible.

One change before I merge. Each button carries the same string in aria-label and title, and getByRole resolves a name from title when the aria-label is gone. I deleted only the aria-label from all five components and every test file still passed: 11, 32, 41, 18, 23, zero failures. So nothing holds the aria-label, and a later cleanup that drops the "duplicate" attribute would leave the suite green while quietly downgrading all seventeen buttons to a title-only name. Add one assertion per component, in the style you already used for title:

expect(editButton.getAttribute("aria-label")).toBe("Edit connection Test PostgreSQL");

The parameterisation is already pinned properly. TableItem and ConnectionItem both rerender with a second fixture and assert the old name is gone, so a hardcoded literal would not satisfy them. That is the part most people skip.

One thing that is ours and not yours: jsx-a11y/control-has-associated-label is an error in our oxlint config and stayed silent on all seventeen, because it counts an icon element child as content. I will file that separately.

@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cevheri cevheri added the loop:needs-info Maintainer-loop task blocked on human-reviewed clarification label Sep 17, 2026
@DevvoLazza

Copy link
Copy Markdown
Contributor Author

Hi, I won’t be able to work on this today or tomorrow as I’m away from home. As soon as I’m back, I’ll continue and finish the remaining fixes.

@cevheri

cevheri commented Sep 17, 2026

Copy link
Copy Markdown
Member

Hi, I won’t be able to work on this today or tomorrow as I’m away from home. As soon as I’m back, I’ll continue and finish the remaining fixes.

thanks for replay

@cevheri

cevheri commented Sep 19, 2026

Copy link
Copy Markdown
Member

Sorry, the ground moved under this while you were away, and that is on us and not on your work. Yusuf closed #919 on 17 September and landed the same labelling directly on main in 60e8a77 the next day, and #886 had already covered TableItem. So the branch no longer merges: ConnectionItem.tsx conflicts, and the files that do auto-merge come out with aria-label and title twice on one element, which tsc rejects as TS17001. The green checks above ran against the 16 September main.

Three parts of your diff are still not on main and I want them:

  • aria-pressed on the two StatsBar view switches. main names both buttons but marks neither as selected.
  • The connection name inside the edit and delete labels. main has a flat "Edit connection" on every row, so four connections read as four identical buttons, which is what Seventeen icon-only buttons in the sidebar have no accessible name #919 was about.
  • focus-visible:opacity-100 on edit, duplicate and delete. Today only the favourite star un-hides on keyboard focus.

Keep your SchemaExplorer test change too: main still finds that button by CSS class rather than by name.

Rebase onto main with only those and I will merge it. If you would rather not, after we duplicated your work, say so and I will close it.

@cevheri

cevheri commented Sep 20, 2026

Copy link
Copy Markdown
Member

I am closing this because it is causing confusion
if you have worked on the issue I mentioned earlier, please submit it in a new PR

@cevheri cevheri closed this Sep 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request loop:needs-info Maintainer-loop task blocked on human-reviewed clarification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Seventeen icon-only buttons in the sidebar have no accessible name

2 participants