Fix accessible names for sidebar icon buttons - #922
DevvoLazza wants to merge 1 commit into
Conversation
cevheri
left a comment
There was a problem hiding this comment.
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 Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
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 |
|
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: Three parts of your diff are still not on main and I want them:
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. |
|
I am closing this because it is causing confusion |
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
Related Issue
Closes #919
Changes Made
aria-pressed.Testing
src/instrumentation.ts:32–36).Checklist