Skip to content

Media: Remove inline event handlers from the image editor - #14767

Open
aeonframework wants to merge 1 commit into
WordPress:trunkfrom
aeonframework:image-editor-no-inline-handlers
Open

aeonframework wants to merge 1 commit into
WordPress:trunkfrom
aeonframework:image-editor-no-inline-handlers

Conversation

@aeonframework

Copy link
Copy Markdown

The image editor markup printed by wp_image_editor() binds every control with an inline on* attribute: 48 handlers covering crop, scale, rotate and flip, the popup menu keyboard navigation, undo and redo, save and cancel, the help toggles, the numeric crop and selection fields, and the preview image's onload. The "Edit Image" buttons printed by edit_form_image_editor() and get_media_item() open the editor with an inline onclick too.

A site that sends a nonce-based Content Security Policy without 'unsafe-inline' blocks all of these (script-src-attr). Core already supports adding nonces to every script it prints, through wp_script_attributes and wp_inline_script_attributes. Inline event handlers cannot carry a nonce, though. As a result:

  • On Media > Edit (post.php?post=ID&action=edit), "Edit Image" does nothing.
  • With &image-editor, the editor is printed with the page, but the crop tool never initializes because the preview's onload is blocked.

The image editor is the largest single group of inline handlers left in wp-admin. It had 48 of the 83 on*= attributes in wp-admin/ and wp-includes/ PHP at this base.

Changes

  • wp-admin/includes/image-edit.php: every inline handler is replaced by a data-imgedit-action attribute (buttons), a data-imgedit-input attribute (numeric fields) or a data-imgedit-preview attribute (preview image). A data-imgedit-value attribute carries an argument where one is needed (rotation angle, flip axis, scale/restore, ratio axis). .imgedit-wrap gets data-postid. The nonce is read from the existing #imgedit-nonce-{id} field, so it is no longer interpolated into JavaScript inside HTML attributes.
  • wp-admin/includes/media.php: both "Edit Image" buttons get data-imgedit-open, data-postid and data-nonce.
  • js/_enqueues/lib/image-edit.js: delegated listeners on document call the same imageEdit methods with the same arguments as before:
    • clicks on [data-imgedit-open] and .imgedit-wrap [data-imgedit-action];
    • keydown/blur on the rotate menu;
    • keyup blur on [data-imgedit-input].
  • The preview image's load event does not bubble, so it is caught in the capture phase. A check on DOM ready catches a preview that finished loading before the footer script ran. The attribute is removed when the image is handled, so imgLoaded() runs exactly once either way. The images that refreshEditor() builds are unaffected.
  • The imageEdit method signatures, element IDs and markup structure are unchanged. Code that triggers clicks on these elements keeps working because the listeners are delegated.

Testing

  • New tests/phpunit/tests/admin/includesImageEdit.php, 3 tests:

    • wp_image_editor() and edit_form_image_editor() output has no on* attribute (checked with WP_HTML_Tag_Processor);
    • every control exposes its action, the wrapper's data-postid matches the attachment, and the nonce fields verify for image_editor-{id}.

    Before the change all 3 tests fail. After it, vendor/bin/phpunit tests/phpunit/tests/admin/includesImageEdit.php reports OK (3 tests, 26 assertions).

  • Full PHPUnit suite, vendor/bin/phpunit against MySQL 8.0 on PHP 8.3:

    • before: 30761 tests, 28 failures;
    • after: 30764 tests, the same 28 failures;
    • in both runs the failures are in Tests_Template (23), Tests_Dependencies_jQuery (4) and Tests_Script_Modules_WpScriptModules (1), and they come from the environment (the src/ tree was not built).
  • vendor/bin/phpcs on the changed PHP files: no new warnings. The 3 existing warnings in media.php are unchanged.

  • JSHint 2.13.6 with the repo's .jshintrc on image-edit.js: nothing new. The only 2 warnings are the existing == at line 1035.

  • Browser check: headless Chromium against a 7.2-beta1 nightly with the three changed files copied in, SCRIPT_DEBUG on, CONCATENATE_SCRIPTS off. A mu-plugin sends Content-Security-Policy: script-src 'self' 'nonce-…' and adds the nonce through wp_script_attributes and wp_inline_script_attributes.

    • Before: "Edit Image" is blocked (script-src-attr) and the editor never opens. With &image-editor the crop tool never initializes. Three script-src-attr violations came from the editor.
    • After: all of these work, with 0 violations from the image editor:
      • open over Ajax; crop initialized;
      • rotate menu, including ArrowDown to move focus;
      • rotate 90° left, which adds a history step and enables Save;
      • undo and redo;
      • scale width 100, which updates the height (480 → 75);
      • help toggle;
      • Save ("Image saved");
      • server-rendered &image-editor: crop initialized, Cancel closes.
    • The violations still left on that screen come from code outside this patch: the raw <script> tags in wp-admin/includes/misc.php (_wpColorScheme, wp_admin_canonical_url(), the back-forward cache reload) and the TinyMCE init in class-wp-editor.php.

Follow-ups (not in this PR)

  • misc.php's three admin-head scripts go through wp_print_inline_script_tag(), the same pattern as r63545.
  • The remaining inline handlers in the legacy media-upload.php screens (get_media_item() Delete/Cancel and WPSetAsThumbnail, and the addExtImage form), in meta-boxes.php and in the list tables.

Trac ticket: https://core.trac.wordpress.org/ticket/59446

Use of AI Tools

AI assistance: Yes
Tool(s): MiniAeon (omp agent harness)
Model(s): Claude Opus 5.5
Used for: finding the remaining inline handlers, drafting the markup and listener changes, the PHPUnit tests and the headless-browser CSP check. The tests and the headless-browser CSP check listed above were run during that work, and the full diff was reviewed by a person before this PR was opened.


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.

The image editor markup printed by `wp_image_editor()` bound every control
with an inline `on*` attribute (48 of them), and the "Edit Image" buttons
printed by `edit_form_image_editor()` and `get_media_item()` opened the editor
the same way. A Content Security Policy that allows scripts by nonce, without
`'unsafe-inline'`, blocks all of these, so the editor could not be opened and,
when printed with the page, its crop tool never initialized.

The controls now carry `data-imgedit-*` attributes and are bound by delegated
listeners in `image-edit.js`. The editor wrapper exposes the post ID, and the
nonce is read from the existing `imgedit-nonce-{id}` field instead of being
interpolated into JavaScript inside HTML attributes. The preview image's load
event is handled in the capture phase, with a check on DOM ready for an image
that loaded before the script ran.

See #59446.
@github-actions

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.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @aeonframework.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

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


Props Bot commands

You can trigger Props Bot actions by commenting on this PR:

  • @propsbot add <username> to give props to a GitHub user. Requires write access.
  • @propsbot remove <username> to leave a user out of the props. Requires write access.
  • @propsbot refresh to refresh this list. Adding the props-bot label works too.

@github-actions

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.

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.

1 participant