Skip to content

Media: Add a settings dialog to the Media Library toolbar. - #12932

Open
t-hamano wants to merge 31 commits into
WordPress:trunkfrom
t-hamano:add/media-library-settings-dialog
Open

Media: Add a settings dialog to the Media Library toolbar.#12932
t-hamano wants to merge 31 commits into
WordPress:trunkfrom
t-hamano:add/media-library-settings-dialog

Conversation

@t-hamano

@t-hamano t-hamano commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Overview

WordPress core does not yet seem to have an API for rendering what are known as popovers. This PR instead attempts to render a settings dialog for the media library using the HTML native dialog element. The only concern is that this PR is the first to introduce a UI using the dialog element.

New APIs

  • PHP:
    • wp_ajax_set_media_library_settings()
  • JavaScript
    • wp.media.view.LibrarySettings
    • wp.media.view.settings.librarySettings

Use of AI Tools

Note

This PR is a prototype. Almost all of the code in this PR was generated by Claude. If we find the approach taken in this PR to be reasonable, I would like to review all of the code.

Screenshots

image image image

The "Infinite Scrolling" personal option could only be reached from the user
profile screen, far from the grid it affects. This adds a toggle next to the
search field in the attachments browser toolbar that opens a modal dialog with
that option, in both the grid and the media modal.

The dialog is a native `dialog` element opened with `showModal()`, so the focus
trap, the Escape handling, the backdrop and the focus restore on close come from
the platform rather than from a hand-rolled implementation. Keyboard events are
kept from reaching the media modal, whose own Escape handler and focus manager
would otherwise fight the dialog.

Toggling the checkbox saves the preference over Ajax and applies it in place:
`wp.media.view.Attachments` can now bind and unbind its scroll handler after
creation, and `wp.media.view.AttachmentsBrowser` swaps the Load more view in and
out, so no page reload is needed. Both views now read the setting at creation
time instead of capturing it when the module loads.

The toggle is not rendered when a `media_library_infinite_scrolling` filter
callback overrides the user preference, since the control would have no effect.

Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

Test using WordPress Playground

The 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

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Allows space for the media library settings toggle.
Improves experience for voice control
@joedolson

Copy link
Copy Markdown
Contributor

I pushed a few minor visual tweaks and changed the label configuration to avoid using an implicit label. Overall, I think this is very workable.

I noticed that the modal is not actually restricting focus, so that will need to be adjusted, but this is going in a direction that I think is viable.

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.

Pull request overview

Adds a Media Library toolbar control that opens a native HTML <dialog> for editing per-user Media Library preferences (starting with infinite scrolling), and wires those preference changes through AJAX to update behavior immediately in the attachments browser.

Changes:

  • Adds a toolbar “settings” toggle and <dialog> templates for Media Library settings.
  • Introduces a new core AJAX action to persist the user’s infinite scrolling preference.
  • Updates media views (Attachments/AttachmentsBrowser) and CSS to support toggling between infinite scrolling and “Load more” without a reload.

Reviewed changes

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

Show a summary per file
File Description
src/wp-includes/media.php Exposes filtered vs user preference values and passes dialog config (nonce, state) to JS.
src/wp-includes/media-template.php Adds Underscore templates for the settings toggle and <dialog> UI.
src/wp-includes/css/media-views.css Styles the settings toggle/dialog and adjusts load-more visibility behavior.
src/wp-admin/includes/ajax-actions.php Adds wp_ajax_set_media_library_settings() handler to save the preference.
src/wp-admin/css/media.css Adjusts grid toolbar layout to accommodate the new toggle in narrow viewports.
src/wp-admin/admin-ajax.php Registers set-media-library-settings as a core POST AJAX action.
src/js/media/views/library-settings.js Implements the toolbar control, dialog open/close behavior, and AJAX save.
src/js/media/views/attachments/browser.js Listens for preference changes and swaps infinite-scroll vs load-more UI live.
src/js/media/views/attachments.js Makes infinite scrolling togglable after initialization; cleans up scroll bindings.
src/js/_enqueues/wp/media/views.js Registers the new LibrarySettings view in the media views bundle.
Gruntfile.js Adds build mapping for the new library-settings.js file.

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

Comment thread src/wp-includes/media.php Outdated
Comment thread src/js/media/views/library-settings.js
Comment thread src/wp-includes/media-template.php
Comment thread src/wp-admin/includes/ajax-actions.php
Comment thread src/wp-admin/css/media.css Outdated
Ensures isFiltered compares like types when a callback returns 0/1.
Covers nonce failure, missing capability, the missing value and the stored preference.
@t-hamano

Copy link
Copy Markdown
Contributor Author

I believe I've addressed all the issues I could think of. We should be ready for review now.

@t-hamano
t-hamano marked this pull request as ready for review August 10, 2026 10:53
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props wildworks, joedolson, tyxla, annezazu, fcoveram, joen, 369work.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@joedolson

Copy link
Copy Markdown
Contributor

Pushed two changes on narrower viewports - the button position below 782px and the button size between 782px and 900px.

@joedolson joedolson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have two items that I think are non-blocking changes, and can be iterated based on feedback.

</label>
</p>
<p class="description" id="{{ data.infiniteScrollingId }}-description"><?php _e( 'Load more media items automatically when scrolling, instead of using a Load more button.' ); ?></p>
<p class="media-library-settings__status" role="status"></p>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The role of 'status' is equivalent to aria-live="polite", and I don't think that's quite right here. Mostly because of the extended aria-description, which means that when you toggle the control, you have to wait for the full label & description to finish reading before the status announcement occurs. For that reason, I'd be more inclined to use role="alert".

@afercia Do you have an opinion here?

/**
* Media Library settings
*/
.wp-core-ui .button.media-library-settings__toggle {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Having the button visually embedded in the search field is something I'm not really sure about. It somewhat conveys the idea that this is specifically search options, rather than general media library options.

@annezazu

Copy link
Copy Markdown

I don’t think this is very discoverable or well connected to the media library. As Joe notes above, it feels more about search options. I’m not comfortable right now proceeding with this, especially with where we are in the release cycle too.

@joedolson

Copy link
Copy Markdown
Contributor

@annezazu Honestly, the positioning of the button is a relatively trivial part of this; relocating that is very doable. The more important part, in my opinion, is:

  • Is a settings toggle a good idea? (In my opinion yes, this is an appropriate place for this.)
  • Is the dialog functional, accessible, and clear? (Also in my opinion, yes.)

But I'm not convinced by the location; I think it was better when it wasn't directly inlined with the search field, however, and think it probably should be even earlier in the modal.

… modal.

Co-Authored-By: Claude <noreply@anthropic.com>
@t-hamano

Copy link
Copy Markdown
Contributor Author

it feels more about search options

We could indicate that the button is an independent option by placing everything except the toggle button on the left side, but I believe this would require a significant CSS change. I believe the structure of the media toolbar needs a fundamental overhaul.

image

@fcoveram

Copy link
Copy Markdown

The button location shown in @t-hamano's message above looks good to me. The layout is similar to that in DataViews (Picker) and the media upload UX.

@tyxla
tyxla requested a review from jasmussen August 11, 2026 08:54
@tyxla

tyxla commented Aug 11, 2026

Copy link
Copy Markdown
Member

@jasmussen I just requested review from you here but saw @fcoveram's feedback late. Thank you both!

@jasmussen

Copy link
Copy Markdown

Happy to ship this as well! Thanks all.

@t-hamano

Copy link
Copy Markdown
Contributor Author

We could indicate that the button is an independent option by placing everything except the toggle button on the left side, but I believe this would require a significant CSS change. I believe the structure of the media toolbar needs a fundamental overhaul.

To be honest, I'm hesitant to make major CSS changes around the media view and media toolbar right now because the CSS is overly complex 😅 While we might refactor the media-related UI with modern CSS in the future, I don't want to do that right before the final RC release. Let's find an acceptable compromise for the 7.1 release.

@tyxla tyxla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for working on this @t-hamano 🙌 The feature is a nice quality-of-life win.

My main hesitation is timing rather than the code. We're at RC2, and this adds a new public JS API, a new AJAX endpoint, and runtime layout changes to one of the most-exercised screens in the admin, one that a lot of plugins hook into. The issues I highlight in review are in runtime the happy path, which makes me suspect there's more we haven't found yet.

Could we get some broad manual testing before this lands? Toggling the setting mid-session in both grid mode and the modal, across browsers, with filters and search active, and with at least one plugin that customises the media views. If that comes back clean, great. But if it feels like it needs more than a quick pass it could very well be a 7.2 item.

$( this.options.scrollElement ).on( 'scroll', this.scroll );

// The list may already be scrolled past the point where more are loaded.
this.scroll();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Noting that this.scroll() never checks this.options.infiniteScrolling so if there's already a queued throttled callback or pending collection.more(), they might call scroll() again, which will run unchecked. We might want to add a this.options.infiniteScrolling check to scroll() itself.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 431a1b9

Comment thread src/js/media/views/library-settings.js Outdated
* @type {boolean}
*/
isFiltered: !! settings.librarySettings &&
Boolean( settings.infiniteScrolling ) !== Boolean( settings.librarySettings.infiniteScrolling ),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, aren't we risking losing the filter precedence here? Disabling the checkbox here changes the UI to "Load More" despite the filter requiring infinite scrolling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in cf528d3. I made it explicit whether a filter exists using has_filter.


this.setStatus( __( 'Saving…' ) );

wp.ajax.post( 'set-media-library-settings', {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Multiple concurrent saves can result in saving the wrong value. Might need some throttling or disabling if a request is in progress here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 93997ba

view.setStatus( enabled ?
__( 'Infinite scrolling is on.' ) :
__( 'Infinite scrolling is off.' )
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this yet another place that might be out of sync with isFiltered? Looks like we're not covering the case when a filter has forced that infinite scroll is "on".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am considering this, and I have two proposals.

  • Notify that the filter is enabled when settings are updated. For example, Setting saved. Infinite scrolling is currently controlled by a plugin or theme.
  • When filtered, the toggle button should not be displayed at all.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To me, the first option is more clear.

}

this.attachments.setInfiniteScrolling( enabled );
this.$el.toggleClass( 'has-load-more', ! enabled );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We might need to cover a few more items/classes based on this logic, like .more-loaded, .found-media, and .new-media for example.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8562d2a

* A toolbar control opening a modal dialog with the personal options for the
* Media Library.
*
* @since 7.1.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Considering the 7.1 branch is only for bug fixes at this point, I doubt if this will make it. Should we consider it for 7.2?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This will need to be agreed upon and decided among the release leads.

Comment thread src/wp-includes/css/media-views.css Outdated
}

.wp-core-ui .media-toolbar-primary > .media-library-settings__toggle {
margin-left: 8px;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't the toggle absolute? Why use margins?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, this is something I forgot to remove when I absolutely positioned the toggle. Removed in cd6074a

Comment thread src/wp-includes/css/media-views.css Outdated
}

.wp-core-ui .mode-grid .media-toolbar-primary > .media-library-settings__toggle {
margin-left: 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same question as above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed in cd6074a

Comment on lines +135 to +136
this.attachments.setInfiniteScrolling( enabled );
this.$el.toggleClass( 'has-load-more', ! enabled );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wouldn't enabling infinite scrolling in the modal always flash the spinner and fetch an extra page? Should we swap those 2 lines to address that?

t-hamano and others added 6 commits August 11, 2026 19:33
…ling is off

Co-Authored-By: Claude <noreply@anthropic.com>
…akes precedence

Co-Authored-By: Claude <noreply@anthropic.com>
…he checkbox

Co-Authored-By: Claude <noreply@anthropic.com>
…rolling

Co-Authored-By: Claude <noreply@anthropic.com>
…absolutely positioned

Co-Authored-By: Claude <noreply@anthropic.com>
…g so it does not fetch an extra page

Co-Authored-By: Claude <noreply@anthropic.com>
@t-hamano

Copy link
Copy Markdown
Contributor Author

@tyxla Thanks for the review!

My main hesitation is timing rather than the code. We're at RC2, and this adds a new public JS API, a new AJAX endpoint, and runtime layout changes to one of the most-exercised screens in the admin, one that a lot of plugins hook into. The issues I highlight in review are in runtime the happy path, which makes me suspect there's more we haven't found yet.

I agree with this. While I understand the importance of improving accessibility, there is also a risk of causing problems by introducing new APIs.

This might go against the decision, but personally I propose punting this PR to 7.2. There is too little time to prepare.

cc @annezazu @joedolson

@t-hamano

t-hamano commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Could we get some broad manual testing before this lands? Toggling the setting mid-session in both grid mode and the modal, across browsers, with filters and search active, and with at least one plugin that customises the media views.

I tested the layout changes when the Media Library Assistant plugin, which extends the media toolbar, was enabled.

Before before-desktop before-tablet before-mobile image
After after-desktop after-tablet after-mobile image

@369work

369work commented Aug 11, 2026

Copy link
Copy Markdown

Test in Playground

I tested PR #12932 for the first time, and I don't think it would be immediately obvious to users that they need to click the gear icon.

I think the “Enable infinite scrolling” checkbox could be placed somewhere more visible and intuitive, so users can discover the setting more easily.

スクリーンショット 2026-08-12 003316 スクリーンショット 2026-08-12 004215

@annezazu

Copy link
Copy Markdown

Coming here to say that I agree we need to punt this from the 7.1 release. I am just uncomfortable with these kinds of changes at this stage of the release process when we need utmost stability. I also think this is worthy of being considered for a point release for 7.1 and that's something we can pursue without waiting months until 7.2.

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.

8 participants