Fix broken ARIA reference on SVG icons - #4369
Conversation
The icon resolver emitted aria-labelledby="title", which pointed at an id
that was never set on the title element — and _strip_id removes every id
from the tree in any case, so the reference could never resolve.
Every icons.tag() call site renders the icon next to its own text label, so
the icons carry no information of their own and are decorative in the ARIA
sense. They are now marked aria-hidden="true" and skipped by screen readers,
per the approach jensens set out on the issue.
The <title> tag is still emitted when an alt text is given, for the browser
hover tooltip. It is now created in the SVG namespace: a bare
etree.Element("title") landed in no namespace, so the lookup on the line
above could never find a title this code had created and a second pass
appended a duplicate. The serialised output is unchanged.
Closes plone#3394
|
@kunalKumar-13 thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
There was a problem hiding this comment.
🟡 Changes recommended
_add_aria_title currently derives the SVG namespace from the default namespace mapping, which can mis-handle SVGs using a prefixed namespace and undermine the idempotency/correctness fix.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes an invalid ARIA relationship emitted by the SVG icon resolver and adjusts SVG title handling to be idempotent and consistent with the intended accessibility approach (icons are decorative and should be hidden from assistive technology).
Changes:
- Set
aria-hidden="true"on all SVG icons and remove anyaria-labelledbyattribute. - Keep emitting an SVG
<title>only for hover tooltips whentag_alt/titleis provided, and avoid duplicating it on repeated modification passes. - Add focused unit tests for
_add_aria_titlebehavior (no Plone layer required) and a changelog entry for #3394.
File summaries
| File | Description |
|---|---|
src/Products/CMFPlone/browser/icons.py |
Removes broken aria-labelledby, adds aria-hidden="true", and makes <title> creation/lookup idempotent. |
src/Products/CMFPlone/tests/test_icons.py |
Adds unit tests covering ARIA/title behavior and serialization expectations. |
news/3394.bugfix.md |
Documents the accessibility fix and the idempotency/namespace correction. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ns = root.nsmap.get(None, "") | ||
| # set title tag | ||
| # A title tag is still useful: browsers show it as a hover tooltip. It is | ||
| # not used for screen reader labelling, which aria-hidden now suppresses. | ||
| title = root.find(f"{{{ns}}}title") | ||
| if title is None: | ||
| title = etree.Element("title") | ||
| root.append(title) | ||
| # Build it in the SVG namespace. A bare etree.Element("title") lands in | ||
| # no namespace, so the lookup above could never find a title this code | ||
| # had created and a second pass would append a duplicate one. | ||
| title = etree.SubElement(root, f"{{{ns}}}title" if ns else "title") |
An SVG declaring its namespace with a prefix (<svg:svg xmlns:svg="...">) has no default nsmap entry, so nsmap.get(None) fell back to no namespace and the title would again be created outside the SVG namespace — the exact problem this change set out to fix. etree.QName(root).namespace reads the namespace off the root element itself and handles both forms. Test added for the prefixed-namespace case.
|
Good catch from the Copilot review, and it was a real gap — pushed in c472ef9.
All seven unit tests pass locally on Python 3.13 and |
|
@jenkins-plone-org please run jobs |
1 similar comment
|
@jenkins-plone-org please run jobs |
|
@jenkins-plone-org please run jobs |
|
Gentle nudge on this one — everything it was waiting on has come back green:
@jensens it is approved and No rush at all — flagging it only in case it slipped off the list rather than because it is blocked on a decision. |
Closes #3394.
Implements the approach @jensens set out on the issue in March. Quoting it so the reasoning is visible here:
Every
icons.tag()call site renders the icon next to its own text label, so the icons carry no information of their own and are decorative in the ARIA sense.The bug
_add_aria_titlesetaria-labelledby="title", referencingid="title". That id was never set on the<title>element, and_strip_idremoves everyidfrom the tree afterwards anyway — so the reference could never resolve. This is what the WAVE checker flagged in the original report.One extra thing this surfaced
Writing the tests turned up a second, latent problem in the same function. The title was built with
etree.Element("title"), which lands in no namespace, while the lookup on the line above searches for{svg-ns}title. So a title created by this code could never be found again, and a second pass over the same tree appended a duplicate<title>.It is now created in the SVG namespace. The serialised output is unchanged — lxml emits
<title>Bug</title>either way, which is why this was invisible in practice — so this is not a rendering change, only a correctness one. There is a test pinning both the idempotency and the serialised bytes.Happy to split that into its own PR if you would rather keep this one to the ARIA change alone.
Tests
Six new cases in
SVGAriaTest, exercising_add_aria_titledirectly so they need no Plone layer:aria-hidden="true", with and without an alt textaria-labelledbyis not emitted, and an existing one is removed<title>kept when an alt is given, absent when it is notaria-hidden, noaria-labelledby, and<title>Bug</title>I ran those six locally against this branch (Python 3.13) and they pass;
black --checkis clean on both files. I have not run the full integration suite locally.Note on the earlier discussion
@agitator and @thomasmassmann discussed
role="presentation"and unique ids back in 2022. This follows @jensens' later proposal instead, which supersedes that — the icons are decorative, so no id-based labelling is needed at all. Happy to revisit if that is not the intent.