Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#27016 closed task (blessed) (fixed)

Make placeholders/MCE views for Audio/Video shortcodes

Reported by: wonderboymusic Owned by:
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.6
Component: Media Keywords: has-patch
Focuses: javascript Cc:


This is an offshoot of #26628, which was doing too much in the patch.

Attachments (8)

27016.diff (6.8 KB) - added by wonderboymusic 8 years ago.
27016.2.diff (5.7 KB) - added by wonderboymusic 8 years ago.
27016.3.diff (29.6 KB) - added by wonderboymusic 8 years ago.
27016.4.diff (30.0 KB) - added by wonderboymusic 8 years ago.
27016.5.diff (32.3 KB) - added by wonderboymusic 8 years ago.
27016.6.diff (36.9 KB) - added by wonderboymusic 8 years ago.
27016.7.diff (5.4 KB) - added by wonderboymusic 8 years ago.
27016.8.diff (509 bytes) - added by Jayjdk 8 years ago.
Added i18n to untranslatable string

Download all attachments as: .zip

Change History (44)

#1 @wonderboymusic
8 years ago

In 27097:

When a video shortcode has content in its body, append it as inner HTML in the resulting <video>.

Reverts [27096].
Fixes #26628.
See #27016.

This ticket was mentioned in IRC in #wordpress-dev by azaozz. View the logs.

8 years ago

8 years ago

#3 @wonderboymusic
8 years ago

  • Keywords has-patch added; needs-refresh removed
  • Milestone changed from Awaiting Review to 3.9

#4 @wonderboymusic
8 years ago

27016.2.diff restores this to its previously working state with some of the TinyMCE init extras. I just realized you can't click to edit the shortcode. We should fix that as well. Perhaps in a later commit. I will leave this here until @azaozz or @gcorne weighs in.

#5 @azaozz
8 years ago

In [27169]:

TinyMCE: add image based placeholders for audio and video shortcodes. Props wonderboymusic, see #27016.

#6 @wonderboymusic
8 years ago

This is killer. Thank you. One caveat: the closing of the shortcode is optional. My test post that had the chromeless YouTube videos used shortcodes with no closer, so the placeholders didn't appear until I added closing tags.

#7 @azaozz
8 years ago

The idea is to eventually remove/refactor the 'wpgallery' TinyMCE plugin and (perhaps) make a 'wpmedia' plugin instead that will use wpViews.

We will also need another "single" mode in the media modal to handle editing of audio/video. Currently title, caption, description and embedding preference (link to or embed) can be set for media files that are in the library. Not sure what could be edited if the media file was added from external site.

One caveat: the closing of the shortcode is optional.

Do we want to support that or convert all shortcodes without a closing tag on editing? Looks like converting would be better.

#8 @wonderboymusic
8 years ago

Good question - any shortcode inserted by editor would have closing, but when written by hand, I always exclude them.

#9 @azaozz
8 years ago

...when written by hand, I always exclude them.

Then the regex will be quite a bit more interesting. Will need a recursion or a loop to catch edge cases like [audio src="1.mp3"][audio src="2.mp3"][audio src="3.mp3"][/audio]. Will fix it tomorrow.

#10 @azaozz
8 years ago

In 27177:

TinyMCE: add support for audio and video shortcodes without closing, fix jshint warning, see #27016.

#11 @wonderboymusic
8 years ago

  • Focuses javascript added

27016.5.diff gave me many headaches, but I think it kind of rocks now:

  • Introduces MVC for audio and video shortodes/placeholders
  • Placeholders for audio and video are now clickable and editable in the media modal
  • The settings in the media modal map to new namespace defaults: wp.media.audio.defaults and wp.media.video.defaults
  • Piggybacking on @gcornes amazing work - allows you to replace the audio or video in the media modal, it then updates your shortcode accordingly
  • Adds the necessary back-and-forth in the TinyMCE wpgallery plugin, which is where all of our media parsing is living
  • Adds media.model.PostVideo and media.model.PostAudio
  • Adds media.controller.AudioDetails and media.controller.VideoDetails
  • Turns media.controller.ReplaceImage into a factory called media.controller.ReplaceMedia
  • Turns media.view.MediaFrame.ImageDetails into a factory called media.view.MediaFrame.MediaDetails
  • Turns media.view.ImageDetails into a factory called media.view.MediaDetails

Without the factories, the amount of code duplication that would be necessary is breathtaking.

The image manipulation @gcorne is doing is about 3 million times more complicated than what I am doing - since it is altering existing HTML inline in TinyMCE. The code I am introducing is pretty good boilerplate for turning shortcodes into models into settings and back.

Will let this simmer for thoughts.

This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.

8 years ago

#13 @gcorne
8 years ago

The functionality looks to be a good addition, although it would be nice if the modal also included a player above the various settings in the details view so that the user could preview the audio or video file.

In terms of the code, as I tried to illustrate in 26631.15.patch, I am not super fond of having a function that generates constructors. I think it is better for the particular instance to take options that change the behavior of the instance or to declare separate constructors and use either composition, mixins, or inheritance (ordered in terms of preference) to keep things DRY.

#14 @wonderboymusic
8 years ago

I am going to work up a patch that has un-abstracted code in media-views.js, then we can do an autopsy on it.

#15 @wonderboymusic
8 years ago

In 27411:

Add TinyMCE placeholders for audio and video shortcodes.

  • Add wp.media.mixin.
  • Add wp.media.audio and wp.media.video.
  • Add wp.media.model.PostAudio and wp.media.model.PostVideo
  • Add wp.media.controller.AudioDetails and wp.media.controller.VideoDetails.
  • Add wp.media.controller.ReplaceAudio and wp.media.controller.ReplaceVideo.
  • Add wp.media.view.MediaFrame.AudioDetails and wp.media.view.MediaFrame.VideoDetails.
  • Add wp.media.view.AudioDetails and wp.media.view.VideoDetails.
  • Update the wpgallery TinyMCE plugin.
  • Display audio and video players in the media modal when shortcode is clicked.
  • Provide a UI to edit shortcode attributes in the media modal.
  • Provide a UI to replace the src media file in an audio or video shortcode.

See #27016.

#16 @wonderboymusic
8 years ago

There is a super-obnoxious bug, not immediately obvious - related to #26779:

  • If you have a video shortcode, open the modal by clicking the placeholder, close the modal, open the modal, click play: the video is in a loading state forever
  • If you have a video shortcode, open the modal by clicking the placeholder, play the video, close the modal, open the modal, click play: it works

I think there are 2 instances of an object pointing at the same local video, for some reason playing it does something special: looking into it.

#17 @wonderboymusic
8 years ago

In 27414:

Audio doesn't have the same quirks as video in the media modal. We don't have to aggressively destroy the mejs instance.

See #27016.

#18 @wonderboymusic
8 years ago

In 27415:

Add some missing JSDoc blocks to media-related code.

See #27016.

#19 @nacin
8 years ago

  • Type changed from enhancement to task (blessed)

#20 @wonderboymusic
8 years ago

In 27420:

Add a timestamp to the urls passed to <audio> and <video> in the modal to ensure that cached view instances aren't referenced by MEjs. Pause the player when closing the controller's modal.

See #27016, #26779.

#21 @wonderboymusic
8 years ago

In 27421:

Ensure that all a/v attribute names have a leading space when the tags are dynamically-generated via Underscore templates.

See #27016.

#23 @wonderboymusic
8 years ago

In 27440:

Cleanup audio/video shortcodes in the media modal:

  • On the controller's update, replace, and close events, call detach() on the frame
  • Cleanup the HTML ouput of the Underscore templates.
  • Move some logic from the Underscore template to the VideoDetails view class.

See #27016.

#24 @wonderboymusic
8 years ago

In 27443:

Call the correct remove() method on the MEjs instance when exiting the media modal.

See #27016.

#25 @wonderboymusic
8 years ago

In 27450:

Support the autoplay attribute, even when the plugin type for a MediaElement instance is Flash. In the media modal, wp.media.view.VideoDetails and wp.media.view.AudioDetails now extend a unified wp.media.view.MediaDetails class which contains all of the player creation and destruction logic. The remove() method mimics the mejs.MediaElementPlayer.remove() method, but does not re-add the audio/video tag to the DOM. The MEjs method is especially problematic when the tag has autoplay="true" and the view has been detached but not destroyed.

Fixes #25077.
See #27016.

#26 @wonderboymusic
8 years ago

In 27453:

JSHint addendum to [27450].

See #27016.

#27 @wonderboymusic
8 years ago

In 27476:

Audio/Video shortcodes in the media modal:

  • Add embedMimes to _wpMediaViewsL10n
  • Use escape instead of interpolate when setting attributes in Underscore templates
  • When creating the <audio> and <video> tags dynamically, set inner <source> nodes instead of the src attribute and properly set the mime-type per source as the type attribute. This is also drastically reduces the amount of code used to generate the tags.

See #27016.

#28 @wonderboymusic
8 years ago

In 27477:

In wp.media.model.PostAudio and wp.media.model.PostVideo, use Underscore's unset method when clearing out properties when the attachment changes.

See #27016.

#29 @wonderboymusic
8 years ago

In 27478:

Audio/Video shortcodes in the media modal:

  • Make a generic model, wp.media.model.PostMedia, which replaces wp.media.model.PostAudio and wp.media.model.PostVideo
  • Make a generic library, wp.media.controller.MediaLibrary, which replaces wp.media.controller.ReplaceVideo and wp.media.controller.ReplaceAudio
  • wp.media.controller.MediaLibrary is used to create new states that want to load a library filtered by type, making it incredibly simple to add states to frames. See wp.media.view.MediaFrame.VideoDetails and wp.media.view.MediaFrame.AudioDetails.

UX changes:

  • Add the ability to manage HTML5 fallbacks - add additional <source>s or remove specific <source>s
  • In the Video Details state, add the ability to select a poster image

See #27016.

#30 @wonderboymusic
8 years ago

In 27479:

Add wp.media.view.MediaFrame.MediaDetails, which wp.media.view.MediaFrame.AudioDetails and media.view.MediaFrame.VideoDetails extend. The subclasses subsequently only need to set createStates() and bindHandlers(), as well as any missing toolbar views.

See #27016.

#31 @wonderboymusic
8 years ago

In 27480:

Since audio and video shortcodes don't point at actual attachments, don't persist the library's selection. Move the media instructions in the block that scrolls.

See #27016.

#32 @wonderboymusic
8 years ago

In 27481:

Video editing in the media modal:

  • Add a state: Add Subititles
  • Add text/vtt to the list of allowed mime-types, files end in .vtt. .srt files are served as text/plain.
  • The content body of a video shortcode should be used for adding <track> elements only. This happens dynamically in the modal. If added by hand, they can still be parsed and managed.

See #27016.

8 years ago

Added i18n to untranslatable string

#33 @Jayjdk
8 years ago

The "There are no associated subtitles" string from [27481] is not translatable. I've added a patch.

#34 @wonderboymusic
8 years ago

In 27490:

A string was missing its translation wrapper in the Underscore template for video details.

Props Jayjdk.
See #27016.

#35 @wonderboymusic
8 years ago

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

New tickets for individual issues.

#36 @wonderboymusic
8 years ago

In 27642:

Add JSDoc blocks to media-audiovideo.js.

See #27016.

Note: See TracTickets for help on using tickets.