Opened 10 years ago
Closed 10 years ago
#29048 closed defect (bug) (fixed)
Twenty Thirteen: editor styles for audio format cause issues in wpviews
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| 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)
Change History (41)
#3
follow-up:
↓ 5
@
10 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
@
10 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
@
10 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
@
10 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
@
10 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
@
10 years ago
As far as I see we have two options:
- 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.
- 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.
#9
@
10 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.
#10
@
10 years ago
- Owner set to azaozz
- Resolution set to fixed
- Status changed from new to closed
In 29543:
#12
@
10 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
@
10 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
@
10 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.
10 years ago
#16
@
10 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.
#19
in reply to:
↑ 18
@
10 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.
#20
@
10 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
@
10 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
@
10 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.
10 years ago
#24
@
10 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.
#26
@
10 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 :)
#27
@
10 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.
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.