Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#40854 closed task (blessed) (fixed)

Allow media to be embedded in Text widget

Reported by: alexvorn2's profile alexvorn2 Owned by: biskobe's profile biskobe
Milestone: 4.9 Priority: high
Severity: normal Version: 4.8
Component: Widgets Keywords: needs-testing has-patch
Focuses: ui Cc:

Description (last modified by ocean90)

Adding images, videos and audio files into Text widget without having 3 additional widgets I think will be better, because in a Text Widget you can add a description or additional information, just we need to add a Upload media button to insert media into the text field.
(Ticket related to Media Widgets - #32417) We can revert that ticked and continue to improve Text widget.
What do you think??

Attachments (11)

40854.0.diff (2.0 KB) - added by westonruter 7 years ago.
https://github.com/xwp/wordpress-develop/pull/253
text-widget-with-media.png (195.7 KB) - added by westonruter 7 years ago.
40854.2.diff (2.7 KB) - added by azaozz 7 years ago.
40854-widget-shortcodes.diff (3.1 KB) - added by westonruter 7 years ago.
40854-widget-shortcodes.2.diff (11.1 KB) - added by westonruter 7 years ago.
40854-widget-shortcodes.3.diff (12.6 KB) - added by westonruter 7 years ago.
Account for shortcode_unautop()
error-upon-switch-to-visual-tab-containing-embed.png (394.0 KB) - added by westonruter 6 years ago.
error-upon-pasting-embed-shortcode.png (306.7 KB) - added by westonruter 6 years ago.
40854-no-postid-requirement-in-parse-embed-ajax.diff (894 bytes) - added by biskobe 6 years ago.
40854-no-postid-requirement-in-parse-embed-ajax.2.diff (994 bytes) - added by biskobe 6 years ago.
40854-no-postid-requirement-in-parse-embed-ajax.3.diff (1.1 KB) - added by westonruter 6 years ago.
Consolidate error checks

Download all attachments as: .zip

Change History (64)

#1 @alexvorn2
7 years ago

Before 4.8 Release *

#2 @ocean90
7 years ago

  • Description modified (diff)
  • Focuses accessibility removed
  • Summary changed from Before 4.8, Integrate media widgets functionality into Text widget to Integrate media widgets functionality into Text widget

#3 follow-up: @westonruter
7 years ago

  • Summary changed from Integrate media widgets functionality into Text widget to Allow media to be embedded in Text widget

The rationale for having a separate widget for each media type is explained in https://core.trac.wordpress.org/ticket/32417#comment:136

That being said, the ability to embed media inside of a Text widget should also probably facilitated.

#4 @karmatosed
7 years ago

I would +1 having media embed inside, but also agree it should not mean merging the widgets.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


7 years ago

#6 @westonruter
7 years ago

  • Milestone changed from Awaiting Review to 4.8.1

#7 @westonruter
7 years ago

Adding oEmbeds depends on #34115.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 years ago

#9 @westonruter
7 years ago

  • Keywords needs-patch added
  • Milestone changed from 4.8.1 to 4.9

#10 in reply to: ↑ 3 ; follow-up: @adriannees
7 years ago

I don't disagree that separate widgets are useful HOWEVER I'm currently in a project where I need to add an image to a text widget because the client made a last minute change request and I don't want to nor feel it should be necessary to completely re-configure the widget area and fiddle with all of the media queries and styling just to be able to add an image widget NEXT to the text. This widget is now more annoying than helpful because it's a bastard child of both a wysiwyg and a text widget.

Replying to westonruter:

The rationale for having a separate widget for each media type is explained in https://core.trac.wordpress.org/ticket/32417#comment:136

That being said, the ability to embed media inside of a Text widget should also probably facilitated.

#11 in reply to: ↑ 10 ; follow-up: @melchoyce
7 years ago

Replying to adriannees:

I don't disagree that separate widgets are useful HOWEVER I'm currently in a project where I need to add an image to a text widget because the client made a last minute change request and I don't want to nor feel it should be necessary to completely re-configure the widget area and fiddle with all of the media queries and styling just to be able to add an image widget NEXT to the text.

You can still flip over to the text tab and manually add an image to your widget:

<img src="your image url" alt="image description" />

#12 in reply to: ↑ 11 @adriannees
7 years ago

Yes I know, I'm aware of that - but it requires looking up the media URL before doing doing so and doesn't have the wysiwyg image options making it just as difficult as the text editor was before in this regard while also having the added inconvenience of it stripping your code or adding additional code if you flip to the visual editor. I know developers aren't really being considered in this widgets evolution so just imagine if one of our non-technical clients decides they want to change the picture down the line? You didn't want them to have to deal with code if they didn't want to but you're going to force them to if they want pictures (which can be confusing as hell to them)

The point is this is either too much or not enough. So for now I've switched begrudgingly back to using the Black Studio plugin (which works great no knocking their work, just preferred to use the built in)

Replying to melchoyce:

You can still flip over to the text tab and manually add an image to your widget:

<img src="your image url" alt="image description" />

#13 @westonruter
7 years ago

Just to re-summarize...

Follow up on #32417 (add media widget), #35243 (extending widget with visual mode), and #35760 (providing API to instantiate editor dynamically).

The work in this ticket probably will be focused on extending wp.editor.initialize() (introduced in #35760).

Note that in order for oEmbeds to be allowed, we'd have to implement #34115.

#14 follow-up: @westonruter
7 years ago

  • Owner set to azaozz
  • Status changed from new to assigned

@azaozz I started looking into this. I was happy with how easy it was to get the button to work initially. See 40854.0.diff. Nevertheless, you're much more familiar with it than I am, so any help would be appreciated as there are some issues that need additional work:

  • As seen in text-widget-with-media.png, the video and audio shortcodes aren't getting converted into TinyMCE views. Similarly, inserting a gallery just shows a placeholder without any preview.
  • Insert media window should show “Insert into widget” instead of “Insert into post”.
  • Media button should be shown even if rich text editing is disabled.

Supporting oEmbed is dependent on #34115.

Version 0, edited 7 years ago by westonruter (next)

@azaozz
7 years ago

#15 in reply to: ↑ 14 @azaozz
7 years ago

Replying to westonruter:

Hm, if I remember right there were strong objections to adding support for media to the Text widget. This gives this editor pretty much equivalent capabilities as the "main" editor and makes the (new) image, video and audio widgets somewhat redundant.

40854.diff looks good, just have to enable the wpview plugin to show "previews" for galleries and embeds. Added that in 40854.2.diff.

Yeah, we can't do any oEmbeds as currently there is no way to cache the responses. Once this is solved all embeddable content will work properly inside the editor.

#16 @azaozz
7 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

Going to commit 40854.2.diff so this can be tested easier.

#17 @azaozz
7 years ago

In 41344:

Text widget: add the Add Media button and enable the wpview plugin to show embedded media previews in the editor.

Props westonruter, azaozz.
See #40854.

#18 follow-up: @azaozz
7 years ago

Still to be done: shortcodes are not parsed on the front-end there, so audio, video and galleries fail to show up in the Customizer preview. Also still somewhat unclear about having several widgets with overlapping features.

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

#19 in reply to: ↑ 18 @westonruter
7 years ago

Thank you for working that out.

Replying to azaozz:

Still to be done: shortcodes are not parsed on the front-end there, so audio, video and galleries fail to show up in the Customizer preview.

Actually, it's not just in the Customizer preview. It's on the frontend generally, as rendering shortcodes isn't currently supported in the Text widget in core, and plugins currently are needed to add support for them. It did not occur to me that shortcodes would be required when adding media, but it slipped my mind.

Also still somewhat unclear about having several widgets with overlapping features.

It does seem somewhat of a shame to go to the trouble of implementing a Gallery widget when a user could also just add a gallery into a Text widget. Maybe it's about being able to provide guided user experiences to accomplish certain tasks.

There is overlap to be sure, but I think there may be enough use cases for separate widgets. It seems clear that users need to be able to add images along their text, in particular floating images around text in the sidebar. But maybe core shouldn't support this, and core should instead just require two Text widgets with an image widget in between if that is the desired effect (though it wouldn't be semantically right).

@melchoyce Thoughts?

#20 @melchoyce
7 years ago

Yeah, I don’t think being able to embed media in the text widget makes the media widgets pointless. I don’t mind the redundancy here at all. Sometimes you just want one thing, like a banner in your sidebar. Sometimes you want something a little more complex, like a photo of yourself and a short bio. If you’re working on your site and search for image, or video, in your widget list, the standalone media widgets provide a faster and more straightforward experience. (Plus, you’d need to know about Text supporting media to take advantage of it in the first place.) I'm totally happy to have this move forward and get committed early & often.

#21 @westonruter
7 years ago

Cool. So then next we'll need to support for (a subset of) shortcodes for the widget. Given that shortcodes often rely on a $post global, we'd need to potentially temporarily unset this global variable so that the widget's doesn't attempt to use an unexpected post as context, e.g. the last post in The Loop on an archive page.

The shortcodes and their handlers in core are:

  • [wp_caption] => img_caption_shortcode()
  • [caption] => img_caption_shortcode()
  • [gallery] => gallery_shortcode()
  • [playlist] => wp_playlist_shortcode()
  • [audio] => wp_audio_shortcode()
  • [video] => wp_video_shortcode()
  • [embed] => WP_Embed::shortcode()

The one I know for sure will needs work here to work without a $post global is embed. And for that see #34115.

#22 @westonruter
7 years ago

40854-widget-shortcodes.diff adds support for shortcodes in widgets, but unit tests need updating before committing that one.

@westonruter
7 years ago

Account for shortcode_unautop()

#23 @westonruter
7 years ago

In 41361:

Widgets: Add shortcode support inside Text widgets.

  • Used now in core to facilitate displaying inserted media. See #40854.
  • The [embed] shortcode is not supported because there is no post context for caching oEmbed responses. This depends on #34115.
  • Add do_shortcode() to the widget_text_content filter in the same way it is added for the_content at priority 11, with shortcode_unautop() called at priority 10 after wpautop().
  • For Text widget in legacy mode, manually apply do_shortcode() (and shortcode_unautop() if auto-paragraph checked) if the core-added widget_text_content filter remains, unless a plugin added do_shortcode() to widget_text to prevent applying shortcodes twice.
  • Ensure that global $post is null while filters apply in the Text widget so shortcode handlers won't run with unexpected contexts.

Props westonruter, nacin, aaroncampbell.
See #40854, #34115.
Fixes #10457.

#24 @westonruter
7 years ago

After [41361] now the main thing left is to support the [embed] shortcode. While this depends on #40854, it will also require incorporating some of the logic from WP_Embed, specifically WP_Embed::run_shortcode().

#25 @westonruter
7 years ago

  • Priority changed from normal to high

Bumping priority to high for visibility and alignment with 4.9 goals, and given proximity to beta 1 deadline.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 years ago

#27 @westonruter
6 years ago

In 41651:

Embeds: Cache oEmbeds in an oembed_cache custom post type instead of postmeta when there is no global $post.

Add processing of embeds to rich Text widget.

Props swissspidy, westonruter, ocean90, johnbillion.
See #40854, #39994, #40935.
Fixes #34115.

#28 @westonruter
6 years ago

With [41651] for #34115 there is now embed handling added for the Text widget.

@azaozz Would you please update TinyMCE for the Text widget to enable embed previews?

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


6 years ago

#30 @westonruter
6 years ago

  • Type changed from enhancement to task (blessed)

#31 @westonruter
6 years ago

Videos embedded in the Text widget on the frontend need to have the same width/height constraints added as is done in the WP_Widget_Media_Video::inject_video_max_width_style() method: https://github.com/WordPress/wordpress-develop/blob/f17fcad6e1788a7afab7ffb72ea969a2599ca759/src/wp-includes/widgets/class-wp-widget-media-video.php#L140

This ticket was mentioned in Slack in #core by westonruter. View the logs.


6 years ago

#33 @westonruter
6 years ago

@azaozz Actually I'm seeing oEmbed previews work, but only when the embed shortcode is used. The thing I'm not seeing is the URL expansion to embeds.

Nevertheless, I am seeing an issue with the embed shortcode previews.

If I try pasting [embed]https://youtu.be/KU_Jdts5rL0[/embed] into the Visual tab of the Text widget, I get an error inside of mce-view.js. See error-upon-pasting-embed-shortcode.png.

Otherwise, if I try pasting the same embed into the Text tab and then switch to Visual, I get a different error in editor.js via the scrollVisualModeToStartElement. See error-upon-switch-to-visual-tab-containing-embed.png.

#34 @westonruter
6 years ago

In 41779:

Widgets: Apply the same container-constraining embed resizing logic from the Video widget to embeds in the Text widget.

See #40854, #39994.

#35 @westonruter
6 years ago

See also #42118 which is a needed style fix to prevent extra margins from being added to the bottom of embed iframes in the Text widget's paragraphs.

#36 @azaozz
6 years ago

In 41783:

Editor:

  • Fix keeping text selection and scroll position when there are embeds from URL.
  • Add editor setting to disable keeping selection and scroll position.
  • Remove dependency on Underscore.js.
  • Fix error in the Text widget editor.

Props biskobe.
Fixes #42059, see #40854.

#37 @westonruter
6 years ago

I know this issue wasn't marked as fixed with [41783] but I just wanted to say I do see the scrollVisualModeToStartElement issue apparently being fixed now, but pasting [embed]https://youtu.be/KU_Jdts5rL0[/embed] still results in an error, and bare URLs are not “unfurled”.

#38 @westonruter
6 years ago

@iseulde is this something you'd also be able to assist with?

#39 @westonruter
6 years ago

@biskobe Is this scrollVisualModeToStartElement issue something you'd be able to look into?

#40 @westonruter
6 years ago

  • Owner changed from azaozz to biskobe

#41 @westonruter
6 years ago

  • Keywords needs-patch added; has-patch removed

Update: @biskobe will be patching this tomorrow and we will plan to include in 4.9-beta3.

#42 @biskobe
6 years ago

  • Keywords has-patch added; needs-patch removed

40854-no-postid-requirement-in-parse-embed-ajax.diff modifies the AJAX check to have the post_ID optional when making a parse-embed request.

This way the Text Widget's Rich Text editor properly receives the rendered Embed code.

#43 @biskobe
6 years ago

40854-no-postid-requirement-in-parse-embed-ajax.2.diff improves the permission check to validate that the user has edit access to a specific post, if a post ID is provided.

#44 @westonruter
6 years ago

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

In 41913:

Widgets: Fix previewing embeds in Text widget by allowing parse-embed admin ajax requests with an empty post_ID just as WP_oEmbed_Controller::get_proxy_item_permissions_check() allows.

As of #34115 if there is no post context the oEmbed will be cached in an oembed_cache custom post type, so having a post as context is no longer a requirement for caching.

Props biskobe, westonruter.
See #34115, #40450.
Fixes #40854.

#45 @westonruter
6 years ago

In 41985:

Editor: Specify maxwidth in parse-embed requests based on width of editor iframe so that TinyMCE view embeds fit, particularly in Text widgets.

See #40854, #34115.

#46 @westonruter
6 years ago

In 42001:

Widgets: Limit container-constraining logic in Text widget to video, iframe, object, and embed elements.

Amends [41779].
See #40854.

#47 @westonruter
6 years ago

In 42390:

Editor: Fix determining TinyMCE editor width when in inline mode instead of iframe mode.

See #40854.
Amends [41985].
Props aduth.
Fixes #42416 for trunk.

#48 @westonruter
6 years ago

In 42391:

Editor: Fix determining TinyMCE editor width when in inline mode instead of iframe mode.

See #40854.
Amends [41985].
Props aduth.
Fixes #42416 for 4.9.

#49 @tristanleboss
6 years ago

Is it on the roadmap to allow developpers to add an Image or a Video field to their custom widget?

#50 @westonruter
6 years ago

In 42545:

Widgets: Ensure media is explicitly enqueued from Text widget in case Media widgets are unregistered.

Amends [41344].
See #40854.
Fixes #43125.

#51 @westonruter
6 years ago

In 42546:

Widgets: Ensure media is explicitly enqueued from Text widget in case Media widgets are unregistered.

Amends [41344].
See #40854.
Fixes #43125 for 4.9 branch.

#52 @westonruter
6 years ago

In 42613:

Customize: Ensure media playlists get initialized after selective refresh; expose new wp.playlist.initialize() API.

In particular allows audio and video playlists to be added to the Text widget and previewed.

Props bpayton, westonruter.
See #40854.
Fixes #42495.

#53 @SergeyBiryukov
6 years ago

In 42622:

Customize: Ensure media playlists get initialized after selective refresh; expose new wp.playlist.initialize() API.

In particular allows audio and video playlists to be added to the Text widget and previewed.

Props bpayton, westonruter.
See #40854.
Merges [42613], [42617] to the 4.9 branch.
Fixes #42495.

Note: See TracTickets for help on using tickets.