Skip to content

[A11y] Fix id / aria-labelledby combinations#5363

Open
rominail wants to merge 2 commits into
vufind-org:devfrom
MSU-Libraries:a11y/aria-labelledby
Open

[A11y] Fix id / aria-labelledby combinations#5363
rominail wants to merge 2 commits into
vufind-org:devfrom
MSU-Libraries:a11y/aria-labelledby

Conversation

@rominail

@rominail rominail commented Jun 9, 2026

Copy link
Copy Markdown
Member

Some aria-labelledby are displayed while the associated id is not

@demiankatz demiankatz added this to the 11.1 milestone Jun 9, 2026

@demiankatz demiankatz 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.

Thanks, @rominail -- see below for a stylistic suggestion.

Beyond that, a couple of other thoughts:

1.) It might be worth considering using the htmlAttributes view helper with an array to build these divs -- could further improve readability. I leave it to your preference, though!

2.) I'm a little confused about the intent of the label in facet-list.phtml -- I wonder if that markup needs overall improvement beyond just this small fix. I think @mtrojan-ub built that control originally, so if there are questions, maybe his input will be helpful.

Comment thread themes/bootstrap5/templates/search/facet-list.phtml Outdated
@rominail

rominail commented Jun 10, 2026

Copy link
Copy Markdown
Member Author

1.) It might be worth considering using the htmlAttributes view helper with an array to build these divs -- could further improve readability. I leave it to your preference, though!

Done

2.) I'm a little confused about the intent of the label in facet-list.phtml -- I wonder if that markup needs overall improvement beyond just this small fix. I think @mtrojan-ub built that control originally, so if there are questions, maybe his input will be helpful.

I was too but I chose to only fix the aria-labelledby mismatch, instead of risking breaking anything.
I'm open to anyone improving the code (which was bigger than the PR intended scope)

PS : One test is failing with an authentication problem if you don't mind taking a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants