Skip to content

fix: render cart toggle hidden title label as plain text - #960

Merged
kdaviduik merged 1 commit into
mainfrom
kd-escape-toggle-readable-label
Oct 16, 2026
Merged

kdaviduik merged 1 commit into
mainfrom
kd-escape-toggle-readable-label

Conversation

@kdaviduik

@kdaviduik kdaviduik commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Closes https://github.com/shop/issues/issues/84843.

Context

When options.toggle.contents.title is false (the default), the cart toggle renders a visually hidden label containing options.toggle.text.title. That label rendered the title as HTML instead of plain text. The visible title path already renders the same value as text.

Root Cause

ToggleView#readableLabel interpolated the title directly into an HTML string. That string is appended in wrapTemplate after Mustache has rendered the main template, so it never went through Mustache's escaping before being parsed via innerHTML.

Solution

Escape the title with Mustache.escape, the same escaper used for {{data.text.title}} in the visible title template. A regression test asserts that markup in the title is not parsed and that the label's text content matches the input exactly.

This does not change what screen readers announce. In manual testing, the announced name came from the toggle wrapper's aria-label, which is set via setAttribute and was already treated as plain text.

Risk Assessment

Low. The only behaviour change is that titles containing HTML or entities (e.g. &) are no longer parsed as markup in the hidden label, which matches the visible title and the aria-label.

@kdaviduik
kdaviduik marked this pull request as ready for review October 13, 2026 17:42
@kdaviduik
kdaviduik requested a review from a team as a code owner October 13, 2026 17:42
@kdaviduik
kdaviduik marked this pull request as draft October 13, 2026 19:03
@kdaviduik
kdaviduik force-pushed the kd-escape-toggle-readable-label branch from be6df04 to 6046ef9 Compare October 13, 2026 19:03
@kdaviduik kdaviduik changed the title fix: render cart toggle screen-reader title as plain text fix: render cart toggle hidden title label as plain text Oct 13, 2026
@kdaviduik
kdaviduik marked this pull request as ready for review October 15, 2026 17:37
Comment thread src/views/toggle.js Outdated
Comment on lines +32 to +33
// This label is appended after Mustache has rendered the main template, so it
// must be escaped here to keep the title a plain-text option.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we can remove that comment here

The toggle's visually hidden label interpolated `options.toggle.text.title`
directly into an HTML string. Because that string is appended after Mustache
renders the main template, it bypassed Mustache's escaping, so the title was
parsed as markup instead of text. The visible title path (`contents.title`
enabled) already renders the same value through escaped `{{data.text.title}}`.

Escape the value with `Mustache.escape` before interpolation. This reuses the
escaper the rest of the templates already rely on rather than introducing a
second one, and keeps `readableLabel` returning a string so `wrapTemplate` is
unchanged.

This does not change what screen readers announce. In manual testing, the
announced name came from the toggle wrapper's `aria-label`, which is set via
`setAttribute` and was already treated as plain text.
@kdaviduik
kdaviduik marked this pull request as draft October 16, 2026 02:40
@kdaviduik
kdaviduik force-pushed the kd-escape-toggle-readable-label branch from 6046ef9 to d086bdb Compare October 16, 2026 02:40
@kdaviduik
kdaviduik marked this pull request as ready for review October 16, 2026 02:41
@kdaviduik
kdaviduik merged commit fc11e77 into main Oct 16, 2026
5 checks passed
@kdaviduik
kdaviduik deleted the kd-escape-toggle-readable-label branch October 16, 2026 02:42
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.

3 participants