Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#36521 closed defect (bug) (fixed)

Setting media control value through JavaScript API does not update control template

Reported by: timothyblynjacobs's profile TimothyBlynJacobs Owned by: westonruter's profile westonruter
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.2
Component: Customize Keywords: has-patch commit
Focuses: javascript Cc:

Description

When setting the value of a MediaControl through the JS API, the control in the customizer still displays the old image previews. This is because the internal params.attachment data is not set to the new value.

Attached is a patch that will update the internal params.attachment data if necessary. This could be worked around in the client code, but it is certainly unexpected behavior, and since there is already a call to wp.media.attachment( value ).fetch() it seems like this should belong in the control itself.

Attachments (3)

36521.diff (1018 bytes) - added by TimothyBlynJacobs 9 years ago.
35612.1.diff (1.1 KB) - added by TimothyBlynJacobs 9 years ago.
36521.2.diff (2.5 KB) - added by westonruter 9 years ago.

Download all attachments as: .zip

Change History (12)

#1 @TimothyBlynJacobs
9 years ago

  • Keywords has-patch added

#2 @westonruter
9 years ago

  • Milestone changed from Awaiting Review to 4.6

#3 @TimothyBlynJacobs
9 years ago

I realized my first patch wouldn't work if the value was set to an empty value.

@westonruter
9 years ago

#4 @westonruter
9 years ago

@TimothyBlynJacobs I found a couple more scenarios that meant further iteration on your patch.

First of all, the control needed to handle the scenario for when it is dynamically instantiated with JS without passing the attachmentData as a param up front.

Secondly, I noticed that there were Ajax calls being made to get the attachment data with a URL being supplied as the attachment ID. Turns out the BackgroundImage control, as a subclass of UploadControl, uses the URL of the attachment as the value, and it also manually sets the attachment data before rendering. So in its case, we can just assume the attachment data is already set.

Please give it a try.

#5 @westonruter
9 years ago

  • Keywords needs-testing added

#6 @TimothyBlynJacobs
9 years ago

Thanks for taking a look and the much more thorough approach. I will certainly give it a try.

#7 @westonruter
9 years ago

  • Keywords commit added; needs-testing removed
  • Owner set to westonruter
  • Status changed from new to accepted

I've been using a patched version of wp.customize.MediaControl.prototype.ready for several weeks. I'm going to commit this.

#8 @westonruter
9 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 37701:

Customize: Ensure MediaControl fetches the necessary attachment data for rendering when dynamically added via JS.

Fixes #36521.
Props TimothyBlynJacobs, westonruter.

#9 @celloexpressions
8 years ago

I unfortunately just discovered that this breaks the background image cropper plugin pretty badly. There were some pretty hacky things in there, but deleting the attachment param based on the value may not be the safest approach. Probably not worth changing anything in core for at this point, and unlikely to affect others if they haven't reported it yet, but I want to mention it here for posterity.

Note: See TracTickets for help on using tickets.