Skip to content

Fix - Responsive menu shows enabled but never applies until saved once - #138

Open
deepench wants to merge 5 commits into
developfrom
fix/responsive-menu-default-mismatch
Open

deepench wants to merge 5 commits into
developfrom
fix/responsive-menu-default-mismatch

Conversation

@deepench

Copy link
Copy Markdown
Contributor

Changes proposed in this Pull Request:

Fixes #135. On a fresh install, Customize > Header > Primary Menu > "Switch to new responsive menu" showed as checked (on), but the website was actually still using the old mobile menu. The new menu only started working after the setting was turned off, saved, turned back on, and saved again — even though it was already showing as "on" the whole time. This was caused by the website reading a different default than the Customizer was showing. This fix makes them agree, so the new responsive menu works immediately, without needing to save the setting once first.

How to test the changes in this Pull Request:

  1. On a fresh site, create a menu with at least one item that has a submenu, and assign it to the Primary Menu location.
  2. Go to Customize > Header > Primary Menu. "Switch to new responsive menu" should show as checked — do not touch it or publish anything.
  3. Close the Customizer without saving. View the site on a narrow/mobile screen width (around 390px) and open the mobile menu.
  4. The submenu should now be collapsed with a toggle/caret to expand it (the new responsive menu behavior), not already expanded (the old behavior).

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (modification of the currently available functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you successfully ran tests with your changes locally?
  • Have you updated the documentation accordingly?

Changelog entry

Fix - New responsive menu now works immediately without saving the setting first.

@tg-autopilot
tg-autopilot requested a lite review from Copilot September 24, 2026 09:12
@github-actions

Copy link
Copy Markdown

QA suite — could not run

no parseable result

A harness failure, not a test failure.

Automated check — no AI involved. It runs the tests in this branch.

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.

Copilot review overview

🟡 Changes recommended

Multiple critical and moderate issues remain in test coverage, cleanup, fixture safety, and reporting configuration.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 High severity · 2 Medium severity

Open (6)
What changed in this PR

Fixes the responsive-menu default mismatch so the new menu works immediately on fresh installs, and adds broad Playwright E2E and QA infrastructure.

Changes:

  • Aligns the frontend responsive-menu fallback with the Customizer default.
  • Adds E2E specs, fixtures, setup, teardown, and documentation.
  • Adds QA metadata, reporting configuration, dependencies, and CI workflow.
File Reviewed change
tests/​e2e/​tsconfig.json E2E TypeScript configuration
tests/​e2e/​specs/​woocommerce/​customizer-options-absent-without-plugin.spec.ts WooCommerce option gating coverage
tests/​e2e/​specs/​single-post/​related-posts-toggle.spec.ts Related-posts behavior coverage
tests/​e2e/​specs/​single-post/​author-bio-round-trip.spec.ts Author-bio persistence coverage
tests/​e2e/​specs/​rtl/​stylesheet-swap-on-rtl-locale.spec.ts RTL stylesheet coverage
tests/​e2e/​specs/​roles/​subscriber-cannot-reach-admin-surfaces.spec.ts Subscriber access coverage
tests/​e2e/​specs/​rendering/​clean-render-across-templates.spec.ts Template rendering coverage
tests/​e2e/​specs/​front-page/​slider-activation-toggle.spec.ts Slider activation coverage
tests/​e2e/​specs/​demo-importer/​migration-notice-flag.spec.ts Migration notice coverage
tests/​e2e/​specs/​customizer/​site-layout-body-class-round-trip.spec.ts Site-layout persistence coverage
tests/​e2e/​specs/​customizer/​global-primary-color-round-trip.spec.ts Primary-color persistence coverage
tests/​e2e/​specs/​blog-layout/​archive-display-type-full-content-vs-excerpt.spec.ts Archive content rendering coverage
tests/​e2e/​specs/​blog-layout/​archive-display-type-body-class-round-trip.spec.ts Archive layout persistence coverage
tests/​e2e/​specs/​accessibility/​mobile-menu-toggle.spec.ts Mobile navigation coverage
tests/​e2e/​README.md E2E usage and tier documentation
tests/​e2e/​playwright.config.ts Playwright projects and reporters
tests/​e2e/​global-teardown.ts Post-run restoration
tests/​e2e/​global-setup.ts Database preparation and cleanup
tests/​e2e/​fixtures/​wp-options.ts WordPress option helpers
tests/​e2e/​fixtures/​wp-admin.ts Admin navigation helpers
tests/​e2e/​fixtures/​theme-mods-snapshot.ts Theme-mod snapshot and restoration
tests/​e2e/​fixtures/​geometry.ts Layout geometry helpers
tests/​e2e/​fixtures/​customizer.ts Customizer helpers
tests/​e2e/​fixtures/​content.ts Test content seeding and cleanup
tests/​e2e/​env.ts E2E environment configuration
tests/​e2e/​auth.setup.ts Reusable admin authentication
tests/​e2e/​.gitignore E2E artifact exclusions
tests/​e2e/​.env.example E2E environment documentation
pnpm-workspace.yaml Dependency build configuration
package.json E2E tooling and scripts
inc/​functions.php Responsive-menu fallback alignment
.themegrill-qa/​suite.json QA areas, tiers, and path metadata
.themegrill-qa/​knowledge.md QA knowledge documentation
.themegrill-qa/​docs/​woocommerce.md WooCommerce documentation
.themegrill-qa/​docs/​uncategorised.md Uncategorized documentation
.themegrill-qa/​docs/​typography.md Typography documentation
.themegrill-qa/​docs/​social-icons.md Social-icons documentation
.themegrill-qa/​docs/​slider.md Slider documentation
.themegrill-qa/​docs/​layout.md Layout documentation
.themegrill-qa/​docs/​how-to.md Usage documentation
.themegrill-qa/​docs/​header.md Header documentation
.themegrill-qa/​docs/​global.md Global-settings documentation
.themegrill-qa/​docs/​getting-started.md Setup documentation
.themegrill-qa/​docs/​footer.md Footer documentation
.themegrill-qa/​docs/​faqs.md FAQ documentation
.themegrill-qa/​docs/​demos.md Demo documentation
.themegrill-qa/​docs/​content.md Content documentation
.themegrill-qa/​docs/​color.md Color documentation
.themegrill-qa/​docs-index.json QA documentation index
.gitignore Credential and artifact exclusions
.github/​workflows/​qa-suite.yml Pull-request QA workflow

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

// a customizer spec that fails after publish() but before its own revert
// otherwise leaves the live site mutated for whatever spec happens to run
// next. See theme-mods-snapshot.ts's docblock for the mechanism.
await restoreThemeMods();
Comment on lines +150 to +155
if (!base64) {
throw new Error(
`theme-mods-snapshot: '${option}' not found in ${config.tablePrefix}options — nothing to snapshot. ` +
'That means the active theme has never had a customizer setting saved; open the Customizer ' +
'and publish once, then re-run.',
);
Comment thread tests/e2e/global-setup.ts Outdated
Comment on lines +73 to +75
const sql =
`UPDATE ${config.tablePrefix}posts SET post_status='trash' ` +
`WHERE post_type='customize_changeset' AND post_status='auto-draft';`;
Comment on lines +71 to +76
await toggle.click();

// Open: navigation.js's onclick replaced the class, exposing the menu.
await expect(nav).toHaveClass(/main-small-navigation/);
await expect(nav).not.toHaveClass(/(^|\s)main-navigation(\s|$)/);
await expect(childLink).toBeVisible();
Comment thread .themegrill-qa/suite.json
Comment on lines +98 to +102
"accessibility": [
"js/navigation.js",
"assets/scss/_header.scss",
"assets/scss/_responsive.scss"
],
Comment thread tests/e2e/fixtures/content.ts Outdated
for (const menu of menus) {
// An unassigned menu is never rendered, so reusing it would hand the
// mobile specs labels that do not appear in the header.
if (!menu.locations || 0 === menu.locations.length) continue;
subin-shk

This comment was marked as outdated.

@subin-shk
subin-shk self-requested a review September 24, 2026 10:45

@subin-shk subin-shk 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.

LGTM 👍🏼

@deepench
deepench changed the base branch from master to develop September 25, 2026 04:21
Scopes the changeset cleanup to the suite's own admin user, represents an
absent theme_mods row instead of throwing on a fresh install, restricts menu
reuse to the primary location, maps inc/functions.php into the header area,
fixes the mobile-menu-toggle spec to open the submenu's own caret toggle
before asserting its content is visible, and publishes reverted Customizer
values in cleanup (not just setting them) so Playground runs actually revert.

Also fixes: a missing ignoreHTTPSErrors option that blocked the suite from
ever running against a local DDEV site, and several specs calling
setControl() on a page that had already navigated away from the Customizer.
Waits for both login fields to be visible and confirms the password value
actually landed before submitting, instead of filling immediately after
goto(). A cold Playground boot can serve wp-login.php before it has fully
hydrated, silently losing a fill() with no error.
@deepench

Copy link
Copy Markdown
Contributor Author

Follow-up on the dropdown icon comment above: verified live and found the actual bug - the submenu itself opens/closes correctly, but the caret icon's own direction never flips because its click handler was looking for a child selector that could never match (this inside the handler already is the .sub-toggle element, not a container of one). Fixed in both themes:

  • spacious#149
  • spacious-pro#200

Also checked and ruled out a duplicate id="site-navigation" theory I'd suspected earlier: header.php's two spacious_main_nav() call sites are wrapped in genuinely mutually exclusive conditions, so only one nav ever renders per page - confirmed live with zero duplicates under both header display type settings. Nothing to fix there.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants