Opened 10 years ago
Closed 10 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)
Change History (54)
#2
@
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
@
10 years ago
28905.3.diff loads editor stylesheets - this is all a rough draft.
#4
@
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
@
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
@
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
#8
@
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
@
10 years ago
Tons of red on the way - breaking it up into digestable pieces.
#13
@
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
@
10 years ago
If you have more than one of the same audio view, only the last one will display.
#15
@
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.
#17
@
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.
#18
@
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. :)
#23
@
10 years ago
.4: Don't attach nodes to the view instance and check if the iframe still exists before resizing.
This ticket was mentioned in IRC in #wordpress-dev by avryl. View the logs.
10 years ago
#28
@
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
.
#34
@
10 years ago
Not sure if I should reopen this. Here's an issues caused by loading the editor stylesheet in all iframes: #29048.
#36
@
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 inwp_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()
andwp_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()?
#37
follow-up:
↓ 39
@
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
@
10 years ago
- Resolution set to fixed
- Status changed from reopened to closed
Replying to wonderboymusic:
Sounds good.
How will themes style it?