Make WordPress Core

Opened 4 weeks ago

Last modified 4 weeks ago

#65825 new defect (bug)

Media: the edit-image content handler is bound twice on the Post and ImageDetails media frames

Reported by: bejignesh Owned by:
Priority: normal Milestone: Awaiting Review
Component: Media Version: 5.3
Severity: normal Keywords: has-patch
Cc: Focuses:

Description (last modified by bejignesh)

wp.media.view.MediaFrame.Post and wp.media.view.MediaFrame.ImageDetails each register content:render:edit-image twice, so editImageContent() runs twice for a single "Edit Image" click.

MediaFrame.Select.bindHandlers() binds it directly:

// src/js/media/views/frame/select.js
bindHandlers: function() {
    ...
    this.on( 'content:render:edit-image', this.editImageContent, this );
},

Both subclasses call the parent and then bind the same pair again. MediaFrame.Post.bindHandlers() does it through its handlers map:

// src/js/media/views/frame/post.js
bindHandlers: function() {
    Select.prototype.bindHandlers.apply( this, arguments );
    ...
    handlers = {
        content: {
            'embed':          'embedContent',
            'edit-image':     'editImageContent',
            'edit-selection': 'editSelectionContent'
        },
        ...
    };

    _.each( handlers, function( regionHandlers, region ) {
        _.each( regionHandlers, function( callback, handler ) {
            this.on( region + ':render:' + handler, this[ callback ], this );
        }, this );
    }, this );
},

MediaFrame.ImageDetails.bindHandlers() does it directly:

// src/js/media/views/frame/image-details.js
bindHandlers: function() {
    Select.prototype.bindHandlers.apply( this, arguments );
    ...
    this.on( 'content:render:edit-image', this.editImageContent, this );
    ...
},

Both subclasses define their own editImageContent, at post.js:407 and image-details.js:81. Select.prototype.bindHandlers() binds this.editImageContent, which resolves on the instance, so both registrations on a given frame point at that subclass's own override rather than the one on Select. On constructed Post and ImageDetails frames the two registered callbacks are the same function reference, and each is identical to the subclass's own override and not to Select.prototype.editImageContent. The duplicate is therefore two registrations of the same method.

Counting the listeners on freshly constructed frames:

frame listeners on content:render:edit-image
wp.media.view.MediaFrame.Select 1
wp.media.view.MediaFrame.Post 2
wp.media.view.MediaFrame.ImageDetails 2

Each invocation constructs a wp.media.view.EditImage, sets it on the content region and calls loadEditor(), which issues an image-editor Ajax request. The second view replaces the first, so what the user ends up seeing is correct, but the first view and its request are wasted.

Measured in the Classic block's Add Media modal, which goes through wp.media.editor and involves no block editor media frame: one click on "Edit Image" issues two action=image-editor&do=open requests for the same attachment, and a single imgedit-panel-* element survives in the DOM.

Introduced in [46461] (5.3), which added editImageContent and its listener to the select frame for #48028. post.js already carried 'edit-image': 'editImageContent' in its handlers map at that point, and since MediaFrame.Post extends MediaFrame.Select the binding has been duplicated ever since.

Removing the 'edit-image' entry from the MediaFrame.Post handlers map, and the duplicate this.on() call in MediaFrame.ImageDetails.bindHandlers(), leaves the inherited select binding in place and the handler runs once on every frame.

Steps to reproduce:

  1. Add a Classic block to a post, click inside it, then activate "Add Media".
  2. Select an image so the Attachment Details panel appears.
  3. Click "Edit Image".
  4. In the browser network panel, filter on admin-ajax.php and observe two POST requests carrying action=image-editor and do=open for the same postid.

Noticed while tracing #65816, which has an unrelated cause in @wordpress/media-utils. Filed separately so the two do not get mixed up.

Change History (3)

#1 @bejignesh
4 weeks ago

  • Description modified (diff)
  • Summary Media: MediaFrame.Post binds the edit-image content handler twice, so editImageContent runs twiceMedia: the edit-image content handler is bound twice on the Post and ImageDetails media frames

Correcting the scope of this ticket. I originally described only MediaFrame.Post. MediaFrame.ImageDetails also extends MediaFrame.Select, calls Select.prototype.bindHandlers() and then binds content:render:edit-image again at src/js/media/views/frame/image-details.js:44, so it has the same duplicate. Summary and description updated.

Listener counts on freshly constructed frames, before and after removing the two redundant bindings:

frame before after
MediaFrame.Select 1 1
MediaFrame.Post 2 1
MediaFrame.ImageDetails 2 1

In the Classic block's Add Media modal, one click on "Edit Image" goes from two action=image-editor&do=open requests to one, the editor still renders with all its controls, and a single imgedit-panel-* element remains in the DOM.

grunt jshint:media reports 98 files lint free. grunt qunit:compiled passes 566/566 with 0 failures on both compiled.html and index.html.

This ticket was mentioned in PR #12913 on WordPress/wordpress-develop by @bejignesh.


4 weeks ago
#2

  • Keywords has-patch added

MediaFrame.Select.bindHandlers() binds content:render:edit-image to editImageContent. MediaFrame.Post and MediaFrame.ImageDetails both extend MediaFrame.Select and call Select.prototype.bindHandlers(), then bind the same pair a second time: Post through the 'edit-image': 'editImageContent' entry in its handlers map, ImageDetails through a direct this.on() call.

Neither subclass defines its own editImageContent, so both registrations resolve to the same inherited method. It runs twice for one "Edit Image" click, building two wp.media.view.EditImage views and calling loadEditor() twice, which issues two image-editor Ajax requests. The second view replaces the first, so the visible result is correct and the first view and its request are simply discarded.

This PR removes the two redundant bindings and leaves the inherited one from MediaFrame.Select.

Listeners on content:render:edit-image, counted on freshly constructed frames:

frame before after
MediaFrame.Select 1 1
MediaFrame.Post 2 1
MediaFrame.ImageDetails 2 1

Introduced in [46461] (5.3), which added editImageContent and its listener to the select frame for #48028. post.js already carried the entry in its handlers map at that point, and image-details.js already had its own this.on() call, so both have been duplicated since.

## Testing

  1. Add a Classic block to a post, click inside it, then activate "Add Media".
  2. Select an image so the Attachment Details panel appears.
  3. Click "Edit Image".
  4. In the network panel, filter on admin-ajax.php.

Before: two POST requests with action=image-editor and do=open for the same postid.
After: one.

In both cases the image editor renders with Crop, Scale and Image Rotation, and a single imgedit-panel-* element is present in the DOM.

The Classic block path is used above because it goes through wp.media.editor and involves no block editor media frame.

Results on this branch:

  • grunt jshint:media: 98 files lint free
  • grunt qunit:compiled: 566/566 passed, 0 failed, on both compiled.html and index.html

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Driving the browser to count event listeners and Ajax requests before and after the change, tracing the duplicate binding back to [46461], and drafting this description. The change itself is two line removals. I reviewed the reasoning, ran the lint and QUnit suites, and confirmed the before and after request counts myself.

#3 @bejignesh
4 weeks ago

  • Description modified (diff)

Correcting a claim I made in the description. I wrote that neither subclass defines its own editImageContent. That is wrong: both do, at src/js/media/views/frame/post.js:407 and src/js/media/views/frame/image-details.js:81, and both differ from the one on MediaFrame.Select.

The mechanism is that Select.prototype.bindHandlers() binds this.editImageContent, which resolves on the instance, so on these frames both registrations point at the subclass's own override, not at Select's. Checked on constructed frames:

frame registrations same function reference is the subclass's own override is Select.prototype.editImageContent
MediaFrame.Post 2 yes yes no
MediaFrame.ImageDetails 2 yes yes no

So the duplicate is two registrations of the same method, and removing the second leaves exactly one call to the subclass's own override. The fix and every measurement already reported are unaffected. Description updated and the pull request description corrected the same way.

Note: See TracTickets for help on using tickets.