Menus: Add pagination to nav menu quick search - #12763
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
Pull request overview
This PR adds paginated “quick search” results in the nav menu metaboxes (post types and taxonomies), enabling a “Load more results” flow (and retry on failure) instead of being limited to the first page of matches.
Changes:
- Add server-side pagination support to
menu-quick-search, including aX-WP-Menu-Quick-Search-Has-Moreresponse header. - Add a “Load more results” button in the nav menu metabox search panels and implement client-side paging/retry/focus/a11y messaging behavior.
- Add automated coverage across PHP unit tests, QUnit, and Playwright E2E.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/wp-admin/includes/nav-menu.php | Adds paged/offset-based pagination and emits the X-WP-Menu-Quick-Search-Has-More header; adds “Load more results” button markup for post type/taxonomy search panels. |
| src/js/_enqueues/lib/nav-menu.js | Implements paginated quick search requests, appends results, manages retry states, focus repair, and a11y announcements. |
| tests/qunit/wp-admin/js/nav-menu.js | Adds QUnit coverage for paging/append/retry, abort behavior, and focus/a11y messaging. |
| tests/phpunit/tests/menu/wpAjaxMenuQuickSearch.php | Adds PHPUnit coverage for paginated post and taxonomy quick searches. |
| tests/e2e/specs/nav-menu-quick-search.test.js | Adds Playwright E2E coverage verifying pagination metadata via response headers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/phpunit/tests/menu/wpAjaxMenuQuickSearch.php:67
- The test request array includes a
posts_per_pageparameter, but_wp_ajax_menu_quick_search()does not readposts_per_pagefrom the request. Keeping this key suggests it affects pagination when it currently doesn’t, which can confuse future maintenance of the test.
$request = array(
'type' => 'quick-search-posttype-page',
'q' => 'Ticket 38224 post result',
'posts_per_page' => 1,
'response-format' => 'json',
);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
src/js/_enqueues/lib/nav-menu.js:1474
- When resetting the quick-search state, the "Load more" button is re-enabled by clearing
aria-disabledand CSS classes, but the nativedisabledproperty is not cleared. If a request was aborted (so the.always()handler doesn't run for the old state), the button could remain disabled.
loadMore
.removeAttr( 'aria-disabled' )
.removeClass( 'disabled' )
.prop( 'hidden', true )
.text( loadMore.data( 'load-more-text' ) );
src/js/_enqueues/lib/nav-menu.js:1513
- While a quick-search request is in flight, the "Load more" button is marked disabled via ARIA/class only. Set the native
disabledproperty too to prevent activation during loading.
loadMore
.attr( 'aria-disabled', 'true' )
.addClass( 'disabled' )
.text( loadMore.data( 'load-more-text' ) );
src/js/_enqueues/lib/nav-menu.js:1548
- After the request completes, the "Load more" button is visually/ARIA re-enabled but the native
disabledproperty should also be cleared so the button becomes interactive again.
if ( panel.data( 'quick-search-state' ) === state && state.request === request ) {
state.loading = false;
state.request = null;
checklist.removeAttr( 'aria-busy' );
$( '.spinner', panel ).removeClass( 'is-active' );
loadMore.removeAttr( 'aria-disabled' ).removeClass( 'disabled' );
}
tests/qunit/wp-admin/js/nav-menu.js:104
this.clockis used without explicitly enabling Sinon fake timers, soclock.tick()can throw if the test wrapper didn't set up a clock. Initialize fake timers viathis.useFakeTimers()and then readthis.clock.
messages = [],
fixture = $( '#qunit-fixture' ),
clock = this.clock,
panel, input, checklist, loadMore, state;
src/js/_enqueues/lib/nav-menu.js:1425
- The quick-search "Load more" control is a
<button>, but it is only marked witharia-disabled/.disabledduring loading. For native buttons, thedisabledproperty should be set as well so the control is actually non-interactive while reported disabled.
This issue also appears in the following locations of the same file:
- line 1470
- line 1510
- line 1542
loadMore
.removeAttr( 'aria-disabled' )
.removeClass( 'disabled' )
.prop( 'hidden', true )
.text( loadMore.data( 'load-more-text' ) );
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
src/js/_enqueues/lib/nav-menu.js:1551
- After moving
state.requestassignment earlier (to avoid stale-callback races), the trailingstate.request = request;can re-set the request reference even if a mocked request resolves synchronously and.always()has already cleared it. Guard this assignment (or remove it) to avoid leavingstate.requestpointing at a completed request.
state.request = request;
src/js/_enqueues/lib/nav-menu.js:1515
state.requestis only assigned after the.done/.fail/.alwayshandlers are registered. If a mocked/pre-resolved Deferred resolves synchronously, the callbacks can run beforestate.requestis set and will be treated as stale (state.request !== request). Assignstate.requestimmediately after creating the request, before attaching handlers.
This issue also appears on line 1551 of the same file.
request = $.post( ajaxurl, params ).done( function( menuMarkup, textStatus, response ) {
tests/e2e/specs/nav-menu-quick-search.test.js:12
- The
valueparameter in theArray.fromcallbacks is unused; this is unnecessary noise and can trip lint rules (no-unused-vars) in the e2e test suite. Rename it to an intentionally-ignored param.
...Array.from( { length: 10 }, ( value, index ) => `Ticket 38224 exact ten ${ index }` ),
...Array.from( { length: 11 }, ( value, index ) => `Ticket 38224 eleven ${ index }` ),
Trac ticket: https://core.trac.wordpress.org/ticket/38224
Use of AI Tools
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.