Repository navigation
Fixed caret button radius and shadow - #4641
Conversation
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Adds guardrails to ensure desktop primary-nav submenu toggles remain visually “unstyled” (don’t inherit global button theming), backed by an e2e test and explicit CSS resets.
Changes:
- Add Playwright e2e coverage that injects global button decoration variables and asserts submenu toggles ignore them.
- Update submenu toggle button styling to explicitly reset
border-radiusandbox-shadow.
| File | Description |
|---|---|
| e2e-tests/specs/a11y-ready/submenu-keyboard.spec.ts | Adds an e2e regression test ensuring submenu toggles don’t inherit global button radius/shadow. |
| assets/scss/components/elements/navigation/_nav-menu.scss | Explicitly resets border-radius and box-shadow on submenu toggle buttons to prevent global styling bleed-through. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .nav-ul button.caret-wrap { | ||
| background: none; | ||
| border: 0; | ||
| border-radius: 0; | ||
| box-shadow: none; |
There was a problem hiding this comment.
Not applied. Neve has no button:focus or button:focus-visible rule that uses box-shadow, in the theme's SCSS, its PHP-generated CSS, or Neve Pro. Keyboard focus relies on the browser's default ring, which is an outline, and this change doesn't touch outlines. Commit eac74f6 says so in _a11y.scss, and its e2e checks don't count shadow glows as a focus indicator. focus-visibility.spec.ts, including its Tab sweep over every interactive element, passed with the fix in place.
| await page.addStyleTag({ | ||
| content: | ||
| 'body { --primarybtnborderradius: 50px; --primarybtnshadow: 0 0 0 4px rgb(255, 0, 0); }', | ||
| }); |
There was a problem hiding this comment.
Not applied. Tabs are the repo's style: .editorconfig sets indent_style = tab. The original spec file has 200 tab-indented lines, and none indented with spaces. Switching to spaces would create exactly the formatter churn the comment is worried about.
| async function expectUndecoratedToggles(toggles: Locator): Promise<void> { | ||
| const count = await toggles.count(); | ||
| expect(count, 'fixture menu must render dropdown toggles').toBeGreaterThan( | ||
| 0 | ||
| ); | ||
| for (let i = 0; i < count; i++) { | ||
| expect( | ||
| await buttonDecoration(toggles.nth(i)), | ||
| 'submenu toggle must not inherit global button radius/shadow' | ||
| ).toEqual({ borderRadius: '0px', boxShadow: 'none' }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Not applied. The fixture menu renders only two desktop arrows, so the saving is negligible. The spec's existing tests already loop over toggles with a sequential await in the same way, and running the checks one by one keeps each failure tied to the arrow that caused it.
There was a problem hiding this comment.
🟢 Approved
The caret reset removes the global button radius and shadow, and the new test fails without the fix.
Validation details
- Files reviewed: 2/2 changed files.
- The compiled CSS diff (sass 1.43.2) adds only
border-radius: 0andbox-shadow: noneto.nav-ul button.caret-wrap. - The new test passed on HEAD. It failed on base with
50pxradius and a red shadow. The run used WordPress, PHP 8.3, Chromium and the a11y-ready fixtures. - A computed-style probe showed that sidebar carets were already undecorated on base. They stay undecorated on HEAD.
Untested areas
- The frontend JavaScript bundles were not built. Placeholder files satisfied the theme's source check.
- Hover, focus and RTL screenshots were not compared.
🤖 Automated review · run code-review-agent_6abb95c6541793.94149552.
🤖 Review agent — review posted ✅ on 300e7892 · approved · 0 findings · 12 min
Run code-review-agent_6abb95c6541793.94149552 · trail
|
🎉 This PR is included in version 4.2.15 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
pirate-bot
added
the
released
Summary
Improves the visual consistency and test coverage of submenu toggle buttons in the navigation menu. The main focus is ensuring these buttons don't inherit global button styles (such as border-radius and box-shadow) and verifying this behavior with new end-to-end tests.
Check before Pull Request is ready:
Closes #4636