WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#28905 closed defect (bug) (fixed)

Audio/Video MCE views should use iframe sandboxes

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

Description

  • Iframes allow us to load all of the JS we want without borking Tiny MCE
  • I can get rid of a ton of compat code.
  • Audio and video previews via [embed]s are busted right now.

Attachments (15)

28905.diff (12.0 KB) - added by wonderboymusic 6 years ago.
28905.2.diff (14.9 KB) - added by wonderboymusic 6 years ago.
28905.3.diff (18.3 KB) - added by wonderboymusic 6 years ago.
28905.4.diff (28.6 KB) - added by wonderboymusic 6 years ago.
28905.5.diff (27.9 KB) - added by wonderboymusic 6 years ago.
28905.6.diff (29.3 KB) - added by wonderboymusic 6 years ago.
28905.7.diff (28.1 KB) - added by wonderboymusic 6 years ago.
28905.8.diff (23.8 KB) - added by wonderboymusic 6 years ago.
28905.patch (5.3 KB) - added by iseulde 6 years ago.
28905.2.patch (5.3 KB) - added by iseulde 6 years ago.
28905.3.patch (1.5 KB) - added by iseulde 6 years ago.
28905.4.patch (1.7 KB) - added by iseulde 6 years ago.
28905.5.patch (8.1 KB) - added by iseulde 6 years ago.
28905.6.patch (1.3 KB) - added by azaozz 6 years ago.
28905.9.diff (490 bytes) - added by kovshenin 6 years ago.

Download all attachments as: .zip

Change History (54)

@wonderboymusic
6 years ago

#1 @iseulde
6 years ago

How will themes style it?

#2 @wonderboymusic
6 years ago

We could collect script and style URLs for media + editor stylesheet in _wpmejsSettings or something - I am not 100% sure where I am going with this, but I got it working for audio/video shortcodes and embeds that return audio/video shortcodes (working on playlists now). If I get this to work, I can delete a bunch of hacks that check your current browser and fix some issues where YouTube is the source for video shortcodes.

I am going to do a lot of testing to see if this is a good idea or not. I am not gonna commit anything weird.

#3 @wonderboymusic
6 years ago

28905.3.diff loads editor stylesheets - this is all a rough draft.

#4 @iseulde
6 years ago

I've been trying to do something similar during GSoC, but for general use. One thing that won't work is changing styles based on the category or format of a post. You'll need to detect changes and reset the right classes for every iframe body etc.. Not so nice. :)

#5 @wonderboymusic
6 years ago

28905.4.diff is a few steps away from being really good. All audio/video/playlists are loaded in iframe sandboxes with all the styles and scripts their hearts desire. Audio styles are weird, but I working on it...

The shortcodes are parsed via AJAX, so there is no need for the parsing logic in mce-view.js. Because each document has its own window property, the players will destroy themselves when the iframes die (I will confirm this with more testing).

Because they are iframes with the scripts in scope, I can delete all of the crazy browser-specific logic for compatibility. This also fixes a bunch of issues: YouTube works as the video source, no placeholders need to be shown when Flash can't communicate with window.mejs for plugin-type media sources, and no compatibility layer is required for Playlists on front-end vs Tiny MCE-loaded.

None of the shortcode logic needs to be branched for MCE views, and no JS needs to be branched for init'ing the players on load.

Another reason this is good: eventually, it would be good for all sites to have the ability to be an oEmbed endpoint themselves. If I have a player of my band's music on my site and other people want to embed it on their site, this same iframe-type loading will be useful. Not as a sandbox, but knowing the basic logic required to render these things in an iframe.

Gonna throw some cold water on my face

#6 @wonderboymusic
6 years ago

  • Keywords has-patch added

28905.7.diff produces the following "After" results:

Before

Video URL = broken
Video as [embed] = broken
Video as [video] = works, not sandbox'd
Use an "unsupported" type like Windows Media = no player
YouTube as src for [video] = no player

Audio URL = broken
Audio as [embed] = broken
Audio as [audio] = works
Use an "unsupported" type like Windows Media = no player

Embed as URL = works
Embed as [embed] = works

Playlist as [playlist]= works, not sandbox'd
Playlist as [playlist type="video"] = works, not sandbox'd

After

Video URL = works
Video as [embed] = works
Video as [video] = works
Use an "unsupported" type like Windows Media = works
YouTube as src for [video] = works

Audio URL = works
Audio as [embed] = works
Audio as [audio] = works
Use an "unsupported" type like Windows Media = works

Embed as URL = works
Embed as [embed] = works

Playlist as [playlist] = works
Playlist as [playlist type="video"] = works

#7 @wonderboymusic
6 years ago

In 29176:

Make audio and video URLs/embed handlers work in <iframe>-sandbox'd MCE views.

Introduce:
get_editor_stylesheets()
wp_media_mce_styles().

See #28905.

#8 @nacin
6 years ago

Tons of green in [29176] with those functions but not sure where it is derived from. If this is meant to DRY up some stuff, where is that stuff?

#9 @wonderboymusic
6 years ago

Tons of red on the way - breaking it up into digestible pieces.

Last edited 6 years ago by wonderboymusic (previous) (diff)

#10 @wonderboymusic
6 years ago

In 29177:

_WP_Editors::editor_settings() should make use of get_editor_stylesheets().

See #28905.

#11 @wonderboymusic
6 years ago

In 29178:

Add a new AJAX action: parse-media-shortcode. This async call will replace JS rendering of audio/video/playlist shortcodes.

See #28905.

#12 @wonderboymusic
6 years ago

In 29179:

Simplify creation of audio, video, and playlist MCE views by placing them in iframe sandboxes.

Wins:

  • Eliminates duplication of code between PHP and JS
  • Views can load JS without messing with TinyMCE and scope
  • MEjs doesn't break when it loads a file plugin-mode. This allows any file type the MEjs supports to play in MCE views.
  • YouTube now works as the source for video.
  • Users can still style the views, editor stylesheets are included in these sandboxes.
  • Audio and Video URLs and [embed]s are no longer broken.
  • Remove the crazy compat code necessary to determine what file types play in what browser.
  • Remove unneeded Underscore templates.
  • Remove the compat code for playlists.

See #28905.

#13 @iseulde
6 years ago

Just wondering, but if there are hacks necessary to make it work in every browser without an iframe, why are these hacks not needed for an iframe? Sorry, I don't know anything about mejs.

Problem 2: the time "label" that overflows the player is now cut off, because you can't overflow an iframe.

#14 @iseulde
6 years ago

If you have more than one of the same audio view, only the last one will display.

#15 @wonderboymusic
6 years ago

The JS outside of the iframe is inaccessible when MEjs loads media in Flash or Silverlight. In another ticket, I got it to work, but barely and the solution was hacky. Files like Windows Media worked everywhere in every browser except when included in MCE views. Because these sandboxes load the JS with HTML, they are in the same scope if Flash is used.

#16 @iseulde
6 years ago

Okay, thanks. :) Ugh, on the front-end, none of this would be a problem.

#17 @iseulde
6 years ago

So having more than one of the same view won't work because right now it's creating an iframe and tries to insert the exact same iframe in the editor for every view... It should create a new iframe for every view.
I also see that the main body's class is copied instead of the editor's body class. We'll need the editor instance there to get the body. This will also need to update whenever you change the post format etc. There are now events for that since the editor scrolling "feature".
Let me make a patch.

@iseulde
6 years ago

#18 @iseulde
6 years ago

This will set and iframe for every node in every editor. I used the editor's body class for the iframe body class, I assume that's what we need there, instead of the main body class. I commented it though, since it seems to mess with the rendering of the media. I'll let you figure that out. :)

Last edited 6 years ago by iseulde (previous) (diff)

@iseulde
6 years ago

#19 @iseulde
6 years ago

Just removed some whitespace in .2.

#20 @wonderboymusic
6 years ago

In 29187:

Cleanup after [29179]:

  • _WP_Editors::editor_settings() no longer needs to load MEjs styles
  • Make sure each identical shortcode with multiple instances also has an iframe sandbox for each instance
  • For the time being, make audio and video shortcodes bypass the loading placeholder to avoid whiplash visually

Props avryl, wonderboymusic.
See #28905.

#21 @wonderboymusic
6 years ago

In 29189:

Cleanup after [29179]:

  • Cleanup players when the editor is hidden - window scope is unique to each frame
  • Add the editor body class to each iframe sandbox
  • Remove unneeded code from wp-mediaelement.js

See #28905.

#22 @wonderboymusic
6 years ago

In 29191:

Cleanup after [29179]:

  • Pause players when media is edited via modal
  • Remove players on unind
  • Account for failure when an empty node is passed to an mce.view.View

See #28905.

@iseulde
6 years ago

@iseulde
6 years ago

#23 @iseulde
6 years ago

.4: Don't attach nodes to the view instance and check if the iframe still exists before resizing.

#24 @wonderboymusic
6 years ago

In 29193:

Cleanup after [29179]:

Don't attach nodes to the view instance and check if the iframe still exists before resizing.

Props avryl.
See #28905.

#25 @wonderboymusic
6 years ago

In 29194:

Cleanup after [29179]:

  • Remove wp.media.mixin.pauseAllPlayers() - was only ever intended to be used in MCE views, reaches too far into global scope. Rendered irrelevant since [28364].
  • Remove wp.media.mixin.pausePlayers() - added in [28364]. MCE views no longer keep track of player instances and call mixin methods, they refer to their child iframes and call MEjs methods on the iframe's window.mejs.players.

See #28905.

@iseulde
6 years ago

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


6 years ago

#27 @wonderboymusic
6 years ago

In 29198:

Make sure that the ready event only fires once for relevant MCE views that are sandbox'd in iframes. Move some editor callbacks to initialize.

Props avryl.
See #28905.

#28 @azaozz
6 years ago

This bit: var p, win = $( 'iframe', content ).get(0).contentWindow; in both pausePlayers() and unsetPlayers() breaks all external embeds that use iframe (most of them?). If the iframe is not "same origin" we can't access contentWindow.

On top of that Chrome throws Uncaught SecurityError: Blocked a frame with origin... making it impossible to use even try{} catch(e){} there.

Thinking a better solution would be to fire an editor event for pausing/removing the ME.js players. Then listen for it from inside the (local) iframes we've added. That would make it asynchronous, i.e. you don't need to select any players, don't even need to know if there are any.

Another (easy?) workaround would be to add a className to the local iframes we created and look for that before trying to access contentWindow.

Last edited 6 years ago by azaozz (previous) (diff)

@azaozz
6 years ago

#29 @azaozz
6 years ago

In 29202:

TinyMCE wpView: prevent fatal (security) errors when trying to access iframe.contentWindow in pausePlayers() and unsetPlayers(). See #28905.

#30 @azaozz
6 years ago

In 29272:

wpView: consolidate pausePlayers() and unsetPlayers(), they are almost the same. Prevent errors when instead of a player ME.js shows only a "Download File" placeholder (in IE). See #28905.

@kovshenin
6 years ago

#31 @kovshenin
6 years ago

The next audio file in a playlist won't play after r29179. See 28905.9.diff.

#32 @wonderboymusic
6 years ago

In 29277:

After [29179], remove the last instance of this.isCompatibleSrc().

Props kovshenin.
See #28905.

#33 @wonderboymusic
6 years ago

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

#34 @iseulde
6 years ago

Not sure if I should reopen this. Here's an issues caused by loading the editor stylesheet in all iframes: #29048.

#35 @wonderboymusic
6 years ago

Let's handle it over there

#36 @azaozz
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm still at a loss here.

  • All audio/video/playlist views seem to use wp_ajax_parse_media_shortcode(). Is the block about audio and video in wp_ajax_parse_embed() still needed (https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/ajax-actions.php#L2678).
  • If yes, wp_ajax_parse_embed() and wp_ajax_parse_media_shortcode() seem to repeat quite a bit of code. Merge them? Or perhaps parse_media_shortcode() should be a helper function called from wp_ajax_parse_embed()?
Last edited 6 years ago by azaozz (previous) (diff)

#37 follow-up: @wonderboymusic
6 years ago

The [embed] shortcode/handler for audio and video files returns an [audio] or [video] via $wp_embed->autoembed() at priority 8 of the_content. The shortcode is parsed at priority 11.

wp_ajax_parse_embed() does both of these steps for audio/video, not for playlist, which has no embed handler.

The only shared code is the MCE style loading and the nuking of $wp_scripts->done

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


6 years ago

#39 in reply to: ↑ 37 @azaozz
6 years ago

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

Replying to wonderboymusic:

Sounds good.

Note: See TracTickets for help on using tickets.