WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#29048 closed defect (bug) (fixed)

Twenty Thirteen: editor styles for audio format cause issues in wpviews

Reported by: celloexpressions Owned by: azaozz
Milestone: 4.0 Priority: normal
Severity: major Version: 4.0
Component: TinyMCE Keywords: has-patch
Focuses: Cc:

Description

See screenshots. Very simple fix, but fairly broken without the fix.

(needs a more specific selector so that the styling doesn't also apply to <body>s inside mce views, not an issue for background colors or anything else we're doing here).

Attachments (10)

29048.before.png (22.3 KB) - added by celloexpressions 6 years ago.
29048.after.png (23.2 KB) - added by celloexpressions 6 years ago.
29048.diff (512 bytes) - added by celloexpressions 6 years ago.
29048.patch (1.9 KB) - added by iseulde 6 years ago.
29048.2.patch (2.2 KB) - added by iseulde 6 years ago.
29048.3.patch (1.9 KB) - added by azaozz 6 years ago.
29048.4.patch (3.6 KB) - added by azaozz 5 years ago.
29048.5.patch (5.5 KB) - added by azaozz 5 years ago.
29048.6.patch (7.3 KB) - added by iseulde 5 years ago.
Screen Shot 2014-08-25 at 21.20.23.png (22.0 KB) - added by iseulde 5 years ago.

Download all attachments as: .zip

Change History (41)

#1 @iseulde
6 years ago

  • Component changed from Bundled Theme to TinyMCE
  • Version changed from 3.9 to trunk

I don't think we should fix this in Twenty Thirteen. We should try to fix this in core first because this affects all themes. All audio, video and embed views were recently moved to iframes which also have the editor stylesheet loaded.

#2 @iseulde
6 years ago

Related: #28905.

#3 follow-up: @wonderboymusic
6 years ago

it is physically impossible to predict what every theme will do here - a couple of options:

  • Ditch the post-specific body classes in the MCE sandboxes
  • Ask Lance what he thinks

I'd rather someone say, "I had to change a CSS rule to ADD some dazzle to my editor stylesheet", rather than: "I don't know what happened, but this looks jacked"

#4 @iseulde
6 years ago

Even if we ditch the post specific classes, there might be some other issues. The theme will always assume this is the main body, and it's not. They could add and icon to it regardless of the post format. What about resetting all the body styles, as if there were no body?

#5 in reply to: ↑ 3 @azaozz
6 years ago

Replying to wonderboymusic:

it is physically impossible to predict what every theme will do here...

Yeah, looks like loading the theme's editor-style.css into the iframe is not that good. Would break things in some themes for sure.

What exactly are we trying to accomplish with that? Maybe a better option is to have another stylesheet for use specifically in these iframes? Yeah, that would probably mean add_theme_support(), etc.

Also, we can set the iframe (and the iframe's html, body) with transparent background, so it will always have the editor's background color.

#6 @iseulde
6 years ago

Some themes style the av player and playlist.
And yes, we could let the theme load separate styles for that, since they're all in frames. Adding css for it in the main editor is also completely useless now.

#7 @wonderboymusic
6 years ago

Some background on why I decided to use iframe sandboxes: https://core.trac.wordpress.org/ticket/28905#comment:6

.... one of the most common theme things that happens to audio/video is the coloring of the progress bar with CSS. The only way to represent that in the iframe sandbox is to also load the editor stylesheet (if it exists).

So... what should we do here? I still don't have a bulletproof answer.

#8 @azaozz
6 years ago

As far as I see we have two options:

  1. Load editor-style.css into the iframes and add another <style> to reset/override some of it, primarily things applied to the body, etc. This is easier to implement and will add all needed styles, but may add some styling that we don't want in there.
  1. Insert some HTML that resembles the ME.js output, grab styles from it, remove the HTML, construct a <style> tag with the styles we got, add that tag to all iframes. This is more complex and has a limitation as we can't grab all styles, will have to decide which selectors/styles to get. But should be safer.

Thinking we can go with (1) for now. Will have to add an ID to the iframe body so it is easy to target from the reset CSS. Having an ID will also let themes target the iframes directly, eventually.

@iseulde
6 years ago

#9 @iseulde
6 years ago

Most themes don't have styling for this and the one that do only add a splash of colour. Let's remove the editor styles for it for now and come back to this later. The cleanest solution imo is to add a separate stylesheet for the media elements.

@iseulde
6 years ago

#10 @azaozz
6 years ago

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

In 29543:

TinyMCE wpView sandbox iframes:

  • Make them transparent.
  • Don't load tons of unrelated styles that can break them. This could result in minimal styling mismatch to the front-end, but keeps the views working well and looking good.

Props avryl, fixes #29048.

#11 @azaozz
6 years ago

In 29545:

TinyMCE wpView: scale iframes to 100% width and ensure they have transparent background. See #29048.

#12 @celloexpressions
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Twenty Fourteen makes several changes to audio and video players beyond the colors, including using Genericons for the icons, for example. I don't think we should just throw out styles that theme authors have carefully designed for the editor. azaozz's first suggestion in 8 seems like a much better solution, or if that doesn't work, I don't think many themes do things as crazy as Twenty Thirteen here, so I still think we could just fix it in the theme. And when those styles were added to Twenty Thirteen, we knew that they could potentially cause some issues down the road since they push the boundaries of what should be done in editor styles, so other themes that do similar things would probably be okay with adapting here.

If we are going to keep [29543], we need to remove the corresponding styles from Twenty Thirteen and Fourteen as having them there implies that they should work in all themes, since they're bundled with core. But that's a big bummer for anyone who's created these styles before.

#13 @azaozz
6 years ago

loading a ton of unrelated css in these iframes is not the best solution... Much "cleaner" would be to load a specific stylesheet. However that means we will require yet another stylesheet for the editor from the themes.

The best solution by far would be to use seamless for the iframes. Unfortunately at present this is not yet supported in any browser. However we should be using that when it becomes available. (I'm actually thinking we can still add it now.)

In that terms, revisiting this in 4.1 and weighting the options again would be best, IMHO.

#14 @wonderboymusic
6 years ago

Maybe we add a filter in wp_media_mce_styles() - seems like the only way at this point...?

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


6 years ago

@azaozz
6 years ago

#16 @azaozz
6 years ago

In 29048.3.patch: add a filter on the stylesheets URLs so themes can override the default ME styling. Try to make the function and filter names a bit more self-explanatory.

#17 @azaozz
6 years ago

In 29559:

TinyMCE wpView: add a filter for the stylesheet URLs loaded in the sandbox iframes. See #29048.

#18 follow-up: @wonderboymusic
6 years ago

In 29564:

After [29543], the iframe sandboxes for media need to load Open Sans. Playlists look aggressively bad without it. Adds font styles for the body.

See #29048.

#19 in reply to: ↑ 18 @azaozz
6 years ago

Replying to wonderboymusic:

...the iframe sandboxes for media need to load Open Sans. Playlists look aggressively bad without it.

By default TinyMCE doesn't use Open Sans. It is added by the themes. Not sure if we should force it in the sandbox iframes.

On the other hand maybe we should change the default TinyMCE font to Open Sans, or at least increase the size a bit. Testing with a theme that doesn't have editor-style.css, text in the editor looks definitely worse.

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

#20 @iseulde
6 years ago

+1 for increasing the default font-size to at least 14px. And yes, we could either set Georgia, "Times New Roman", "Bitstream Charter", Times, serif as the font in the iframes, or set both the editor and iframes to Open Sans.

#21 @nacin
5 years ago

Is there anything else we need to do for 4.0 here? I'm not sure what the state is of this ticket.

#22 @iseulde
5 years ago

Working on this one right now. I'll add the default font and add a stylesheet for 2013 and 2014.

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


5 years ago

#24 @iseulde
5 years ago

Not sure what to do here actually. I started making an add_editor_view_style() function and add a stylesheet for 2013, but it just makes things more complicated for theme authors. If it only needs styles for the audio player, it's OK, but the playlist also needs link styles etc., which can change based on the category or format... And so can colours for the player actually.

It might be better to revert to adding editor-style.css and just do body and html reset, because other styles aren't harmful. On the other side, it's still not possible to get the format and category classes there. Not sure what the best way to go is. Both are problematic.

#25 @wonderboymusic
5 years ago

I have always believed we should just load the editor stylesheet

#26 @azaozz
5 years ago

Okay, lets go with (1) from comment:8 above. Seems that is the "least worst" option at the moment.

Not sure if we need to load the default editor stylesheets: skins/lightgray/content.min.css and skins/wordpress/wp-content.css. Seems we will have to load all the rest. Also can add a MutationObserver for attributes changes on the editor's body tag and reset the iframe body classes when changed. I know this won't work in older browsers, but.. Progressive enhancement :)

@azaozz
5 years ago

#27 @azaozz
5 years ago

In 29048.4.patch:

  • Import the stylesheets from the editor iframe to the sandbox iframe.
  • Remove the (recently introduced) filter for adding more stylesheets in the AJAX response.

@azaozz
5 years ago

#28 @azaozz
5 years ago

29048.5.patch also adds observer for changes to the body classes.

@iseulde
5 years ago

#29 @iseulde
5 years ago

29048.6.patch move the mejs styles from the body to the top of the head, so they don't override theme styles.

#30 @iseulde
5 years ago

Screenshot for 2014 above, with colour, genericons etc.

#31 @wonderboymusic
5 years ago

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

In 29615:

MCE View sandboxes:

  • Use a MutationObserver to listen to the body class of the parent editor frame.
  • In wpview_media_sandbox_styles(), only return the MEjs stylesheets.
  • In wp_ajax_parse_media_shortcode() and wp_ajax_parse_embed(), return an object instead of an HTML blob to allow passing body and head separately

Props avryl, azaozz.
Fixes #29048.

Note: See TracTickets for help on using tickets.