WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 4 days ago

#40854 assigned task (blessed)

Allow media to be embedded in Text widget

Reported by: alexvorn2 Owned by: azaozz
Milestone: 4.9 Priority: high
Severity: normal Version: 4.8
Component: Widgets Keywords: has-patch needs-testing
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 (8)

40854.0.diff (2.0 KB) - added by westonruter 5 weeks ago.
https://github.com/xwp/wordpress-develop/pull/253
text-widget-with-media.png (195.7 KB) - added by westonruter 5 weeks ago.
40854.2.diff (2.7 KB) - added by azaozz 5 weeks ago.
40854-widget-shortcodes.diff (3.1 KB) - added by westonruter 4 weeks ago.
40854-widget-shortcodes.2.diff (11.1 KB) - added by westonruter 4 weeks ago.
40854-widget-shortcodes.3.diff (12.6 KB) - added by westonruter 4 weeks ago.
Account for shortcode_unautop()
error-upon-switch-to-visual-tab-containing-embed.png (394.0 KB) - added by westonruter 5 days ago.
error-upon-pasting-embed-shortcode.png (306.7 KB) - added by westonruter 5 days ago.

Download all attachments as: .zip

Change History (45)

#1 @alexvorn2
5 months ago

Before 4.8 Release *

#2 @ocean90
5 months 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
5 months 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
5 months 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.


4 months ago

#6 @westonruter
4 months ago

  • Milestone changed from Awaiting Review to 4.8.1

#7 @westonruter
4 months ago

Adding oEmbeds depends on #34115.

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


3 months ago

#9 @westonruter
3 months ago

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

#10 in reply to: ↑ 3 ; follow-up: @adriannees
2 months 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
2 months 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
2 months 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
5 weeks 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
5 weeks 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.
  • “Add Media” button needs to be translated.

Supporting oEmbed is dependent on #34115.

Last edited 5 weeks ago by westonruter (previous) (diff)

@azaozz
5 weeks ago

#15 in reply to: ↑ 14 @azaozz
5 weeks 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
5 weeks ago

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

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

#17 @azaozz
5 weeks 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
5 weeks 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 5 weeks ago by azaozz (previous) (diff)

#19 in reply to: ↑ 18 @westonruter
5 weeks 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
5 weeks 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
4 weeks 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
4 weeks ago

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

@westonruter
4 weeks ago

Account for shortcode_unautop()

#23 @westonruter
4 weeks 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
4 weeks 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
3 weeks 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.


11 days ago

#27 @westonruter
10 days 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
10 days 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 days ago

#30 @westonruter
6 days ago

  • Type changed from enhancement to task (blessed)

#31 @westonruter
5 days 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.


5 days ago

#33 @westonruter
5 days 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
4 days 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
4 days 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
4 days 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
4 days 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”.

Note: See TracTickets for help on using tickets.