Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#28905 closed defect (bug) (fixed)

Audio/Video MCE views should use iframe sandboxes

Reported by: wonderboymusic's profile 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 10 years ago.
28905.2.diff (14.9 KB) - added by wonderboymusic 10 years ago.
28905.3.diff (18.3 KB) - added by wonderboymusic 10 years ago.
28905.4.diff (28.6 KB) - added by wonderboymusic 10 years ago.
28905.5.diff (27.9 KB) - added by wonderboymusic 10 years ago.
28905.6.diff (29.3 KB) - added by wonderboymusic 10 years ago.
28905.7.diff (28.1 KB) - added by wonderboymusic 10 years ago.
28905.8.diff (23.8 KB) - added by wonderboymusic 10 years ago.
28905.patch (5.3 KB) - added by iseulde 10 years ago.
28905.2.patch (5.3 KB) - added by iseulde 10 years ago.
28905.3.patch (1.5 KB) - added by iseulde 10 years ago.
28905.4.patch (1.7 KB) - added by iseulde 10 years ago.
28905.5.patch (8.1 KB) - added by iseulde 10 years ago.
28905.6.patch (1.3 KB) - added by azaozz 10 years ago.
28905.9.diff (490 bytes) - added by kovshenin 10 years ago.

Download all attachments as: .zip

Change History (54)

#1 @iseulde
10 years ago

How will themes style it?

#2 @wonderboymusic
10 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
10 years ago

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

#4 @iseulde
10 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
10 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
10 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
10 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
10 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
10 years ago

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

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

#10 @wonderboymusic
10 years ago

In 29177:

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

See #28905.

#11 @wonderboymusic
10 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
10 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
10 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
10 years ago

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

#15 @wonderboymusic
10 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
10 years ago

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

#17 @iseulde
10 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
10 years ago

#18 @iseulde
10 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 10 years ago by iseulde (previous) (diff)

@iseulde
10 years ago

#19 @iseulde
10 years ago

Just removed some whitespace in .2.

#20 @wonderboymusic
10 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
10 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
10 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
10 years ago

@iseulde
10 years ago

#23 @iseulde
10 years ago

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

#24 @wonderboymusic
10 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
10 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
10 years ago

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


10 years ago

#27 @wonderboymusic
10 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
10 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 10 years ago by azaozz (previous) (diff)

@azaozz
10 years ago

#29 @azaozz
10 years ago

In 29202:

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

#30 @azaozz
10 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
10 years ago

#31 @kovshenin
10 years ago

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

#32 @wonderboymusic
10 years ago

In 29277:

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

Props kovshenin.
See #28905.

#33 @wonderboymusic
10 years ago

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

#34 @iseulde
10 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
10 years ago

Let's handle it over there

#36 @azaozz
10 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 10 years ago by azaozz (previous) (diff)

#37 follow-up: @wonderboymusic
10 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.


10 years ago

#39 in reply to: ↑ 37 @azaozz
10 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.