Skip to content

fix(nav): strip toggle aria-label tags without kses - #4626

Open
Alexia-Soare wants to merge 7 commits into
developmentfrom
fix/4623
Open

Alexia-Soare wants to merge 7 commits into
developmentfrom
fix/4623

Conversation

@Alexia-Soare

@Alexia-Soare Alexia-Soare commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Summary

The mobile menu's dropdown toggle announced raw markup, Toggle <span class="menu-item-title-wrap dd-title">Shop</span>, instead of Toggle Shop when a plugin widened wp_kses_allowed_html for every context. The label is now built with wp_strip_all_tags(), a plain string strip that does not consult that filter.

Note

The kses call was added in 4a37a0e (version 3.6.6). The bug shows only with a plugin that hooks wp_kses_allowed_html without checking for the reserved strip context. Booster for WooCommerce does this.

Reported in #4623 from HelpScout conversation 3451611756.

What changed

  • Nav walker — builds the sidebar toggle aria-label with wp_strip_all_tags() instead of wp_filter_nohtml_kses(). The output was already escaped with esc_attr(), so this is an accessibility fix, not a security one.

  • E2E sample-data environment — installs a must-use plugin that widens wp_kses_allowed_html for every context, the way Booster does. A Playwright spec in specs/accessibility opens the mobile menu on a phone profile and checks every dropdown toggle label is plain text. The customizer, admin and editor specs share this environment; the filter is inert for them.

Note

wp_filter_nohtml_kses() expects slashed form input and runs wp_kses() in the strip context. Neve used it to remove its own <span> wrapper from HTML it built itself, which made the label depend on every plugin behaving correctly in that filter.

Will affect visual aspect of the product

NO

Test instructions

  1. Go to WP Admin → Plugins → Add New Plugin, install and activate Booster for WooCommerce.

    Expect: the plugin is listed as active.

  2. Go to WP Admin → Appearance → Menus. Create a menu with a Shop item that has at least one child item. Under Menu Settings, tick Primary Menu and save.

    Expect: the menu shows in the header.

  3. Open the homepage. Open the browser console and run:

    document.querySelectorAll('#nv-primary-navigation-mobile-sidebar button.caret-wrap')
      .forEach(b => console.log(b.getAttribute('aria-label')));

    Expect: Toggle Shop. Before the fix this printed Toggle <span class=\"menu-item-title-wrap dd-title\">Shop</span>.

  4. Narrow the window to phone width, open the hamburger menu, select the arrow button next to Shop with the element picker and open the Accessibility pane.

    Expect: the computed name is Toggle Shop.

  5. Deactivate Booster for WooCommerce and repeat step 3.

    Expect: Toggle Shop.

Check before Pull Request is ready:

🤖 Generated with Claude Code

The mobile dropdown toggle built its aria-label with
wp_filter_nohtml_kses(), which runs through the pluggable
wp_kses_allowed_html filter. A plugin that widens that filter for every
context, such as Booster for WooCommerce, keeps Neve's own <span> wrapper
in the label, so screen readers announce raw markup instead of the menu
title. Use wp_strip_all_tags(), which does a plain string strip.

Refs: #4623

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@Alexia-Soare
Alexia-Soare requested a balanced review from Copilot September 18, 2026 08:37
@Alexia-Soare Alexia-Soare self-assigned this Sep 18, 2026
@pirate-bot

pirate-bot commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

Plugin build for c9f8091 is ready 🛎️!

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Adds a regression safeguard to ensure Neve’s mobile menu dropdown toggle aria-label remains plain text even when plugins loosen KSES rules.

Changes:

  • Added a WP unit test covering permissive wp_kses_allowed_html behavior for menu toggle labels.
  • Updated Nav_Walker to use wp_strip_all_tags() instead of wp_filter_nohtml_kses() when building the toggle aria-label.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
tests/test-neve-nav-walker-aria-label.php Adds regression test reproducing the permissive-KSES scenario and asserting the resulting aria-label.
inc/views/nav_walker.php Switches sanitization approach for the toggle aria-label to prevent tag leakage when KSES rules are widened.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread inc/views/nav_walker.php Outdated
Comment thread tests/test-neve-nav-walker-aria-label.php
The sample-data environment now installs a must-use plugin that widens
wp_kses_allowed_html for every context, the way Booster for WooCommerce
does. A Playwright spec opens the mobile menu on a phone profile and
checks that every dropdown toggle aria-label is plain text.

Refs: #4623

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.

Comment thread tests/test-neve-nav-walker-aria-label.php Outdated
Comment thread tests/test-neve-nav-walker-aria-label.php
alexiasoare and others added 2 commits September 18, 2026 11:59
CI installs Chromium only. The iPhone 13 profile defaults to WebKit, so the
spec could not launch a browser there. Use the Pixel 5 profile, which the
existing accessibility spec also ends up running on.

Refs: #4623

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Address review: array_merge_recursive() turns an existing boolean attribute
allowance into an array, which is not the shape wp_kses expects. Set the
span/class allowance directly in both the unit test and the e2e must-use
plugin.

Refs: #4623

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Copilot reviewed 3 out of 5 changed files in this pull request and generated 4 comments.

Comment thread e2e-tests/specs/accessibility/dropdown-aria-label.spec.ts
Comment thread inc/views/nav_walker.php Outdated
Comment thread tests/test-neve-nav-walker-aria-label.php Outdated
Comment thread tests/test-neve-nav-walker-aria-label.php

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Copilot reviewed 3 out of 5 changed files in this pull request and generated 3 comments.

Comment thread e2e-tests/specs/accessibility/dropdown-aria-label.spec.ts
Comment thread inc/views/nav_walker.php Outdated
Comment thread tests/test-neve-nav-walker-aria-label.php
wp_strip_all_tags() keeps line breaks by default. A menu title with a
newline puts that newline in the attribute, where a screen reader reads
it as a pause. Remove the breaks and trim the result.

Refs: #4623

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.

Comment thread e2e-tests/specs/accessibility/dropdown-aria-label.spec.ts
Comment thread tests/test-neve-nav-walker-aria-label.php

@pirate-bot pirate-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 Changes requested

The new wp_strip_all_tags() call drops the item name from the sidebar toggle label when a menu title contains < followed by a non-space character.

Validation details
  • Files reviewed: 5/5 changed files.
  • Unit test: TestNeveNavWalkerAriaLabel run with PHPUnit 9.6.36, PHP 8.3.33, WordPress 7.1.1 test library. It passes on HEAD and fails on pr-base with the leaked <span> label. The test discriminates the fix.
  • Label comparison: old and new expressions run with wp eval on WordPress 7.1.1 for eleven titles. Apostrophes, &, entities, newlines and icon markup produce the same or better output. Titles with < plus a non-space character lose text.
  • Producer: an administrator with unfiltered_html stores Price <5 items unchanged. the_title filters keep it. This is source and sandbox evidence, not a browser check.
  • Security: the esc_attr() sink is unchanged. bin/ is excluded from the release zip by .distignore.
  • E2E environment: the mu-plugin path and the sample-data menu with children were confirmed from source only.
Untested areas
  • The Playwright spec dropdown-aria-label.spec.ts was not run. It needs the Docker sample-data environment and built theme assets.
  • Booster for WooCommerce was not installed. The unit test reproduces its filter.

🤖 Automated review · run code-review-agent_6aad0e92028285.98308151.


🤖 Review agent — review posted ✅ on 24ff7530 · changes requested · 1 finding · 25 min

Run code-review-agent_6aad0e92028285.98308151 · trail

Comment thread inc/views/nav_walker.php Outdated
strip_tags() reads a lone "<" as the start of a tag and drops the text up
to the next ">". A menu title such as "Kids <12" lost its name from the
aria-label. Encode the lone "<" first with wp_pre_kses_less_than(), which
is a plain regex and does not consult wp_kses_allowed_html.

Refs: #4623

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.

Comment thread tests/test-neve-nav-walker-aria-label.php
Comment thread inc/views/nav_walker.php Outdated
wp_strip_all_tags() ends with trim(), so the outer call did nothing.

Refs: #4623

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

4 participants