Make WordPress Core

Opened 6 years ago

Closed 2 days ago

#42203 closed task (blessed) (wontfix)

Ensure media & embeds in Text widget are styled properly across widget areas in bundled themes

Reported by: westonruter's profile westonruter Owned by:
Milestone: Priority: high
Severity: normal Version:
Component: Bundled Theme Keywords: needs-patch close
Focuses: Cc:

Description

The Text widget now allows for media to be embedded within it as of #40854. There is an Add Media button allowing for images, videos, and other embeds to be added to the content. Themes may not have styles in them that expect media to appear in a Text widget context, since previously they may have only appeared in post content. As noted in §Default Themes Updates for Media Widgets for Images, Video, and Audio:

Themes that add custom styles to the MediaElement.js player (namely Twenty Thirteen and Twenty Fourteen) were updated from just styling it within syndicated content, to also include instances within widgets. Most themes don’t restrict styles for captioned images or media players to just post content, that is, limit CSS selectors to classes output by post_class(). If your theme does, make sure to either remove that constraint or include a .widget selector.

In the same way for the embeds in the Text widget, we need to make sure that all of the core themes are compatible. Text widgets need to be tested in all core themes and in each of their widget areas.

Once the core themes are updated then the changes will may be instructive for theme authors to make similar changes in their themes.

See same task for Gallery widget in #41969.

Change History (30)

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


6 years ago

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


6 years ago

#3 @alexvorn2
6 years ago

  1. Can we add gallery functionality to Text Widget (maybe to wp editor too)?
  2. Will everything work on Customizer page (embeds, audios, videos, adding images, tweets, YouTube videos etc)?

#4 @alexvorn2
6 years ago

Or we have already, I have not tested...

#5 @westonruter
6 years ago

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

#6 @SergeyBiryukov
6 years ago

  • Component changed from Widgets to Bundled Theme

#7 @davidakennedy
6 years ago

Overall thoughts

  • Some of the margin issues should be easy to solve.
  • The bigger challenge is dealing with widget areas that are extremely narrow, like in Twenty Fourteen and Twenty Fifteen in some cases.
  • Also, handling responsive iframes, objects, etc will probably be a challenge without adding a lot of extra CSS or a jQuery library like FitVids. Many of the video embeds didn't look bad in the widget areas, they just weren't full width in all cases.
  • I'm going to focus on the newer themes first since they likely have a higher install base and should be easier to fix in most cases.

I'm going to test #41969 tomorrow as well and come up with some patches.

Test results follow:

2017

Sidebar: Video iframes in sidebar not full width. Would need to have extra code to make full width.
Footer 1: Video iframes in sidebar not full width. Would need to have extra code to make full width.
Footer 2: Video iframes in sidebar not full width. Would need to have extra code to make full width.

The is especially noticeable in the footer widgets areas on certain screen sizes.

Screen shot: https://cloudup.com/cf0xK0ilIvv

2016

Sidebar: Video iframes in sidebar not full width. Would need to have extra code to make full width.
Footer 1: Video iframes in sidebar not full width. Would need to have extra code to make full width.
Footer 2: Video iframes in sidebar not full width. Would need to have extra code to make full width.

The is especially noticeable in the footer widgets areas on certain screen sizes.

Screen shot: https://cloudup.com/ckDlQsJL8mO

2015

The standard desktop widths in the one widget area look good. However, at smaller screen sizes, the iframes aren't full width.

Screenshot: https://cloudup.com/cA-JHysSvs5

At smaller widths with the sidebar to the left, some of the styles for the MediaElement.js player hit some stress cases.

Screenshot: https://cloudup.com/cVCIQyl-fb6

The Instagram embed styles look broken at narrow widths.

Screenshot: https://cloudup.com/clyPiTiDxBM

2014

Primary Sidebar: This sidebar has a narrow width, and some of the styles for the MediaElement.js player hit some stress cases.

Screenshot: https://cloudup.com/cM_SiDVw40S

Primary Sidebar, Content Sidebar Footer Widget Area: The YouTube embed breaks.

Screenshot: https://cloudup.com/c7TqKMb0A-A

2013

Main Widget Area: Similar to other narrow widget areas in default themes, the Instagram widget looks broken.

Screenshot: https://cloudup.com/cTSNn7gnK3T

Secondary Widget Area: Video iframes in sidebar not full width. Would need to have extra code to make full width.

Screenshot: https://cloudup.com/cmJ3M-YEmty

Also, some of the MediaElement.js containers need a margin.

2012

Main Sidebar: Some of the MediaElement.js containers need a margin. Video iframes in sidebar not full width. Would need to have a extra code to make full width. The is especially noticeable in the footer widgets areas on certain screen sizes. Same for both Front Page Widget Areas.

Screenshot: https://cloudup.com/cbl-F7BLkiv

2011

Main Sidebar: Some of the MediaElement.js containers need a margin. Video iframes in sidebar not full width. Would need to have a extra code to make full width. This is more noticable at smaller screen sizes when the sidebar drops under the main content areas.

Screenshot: https://cloudup.com/caTAILX2ZmG

Showcase Sidebar: This area has similar issues as other narrow widget areas.

Screenshot: https://cloudup.com/crdIXxAalFI

Footer Area One, Two and Three: These have the same issues as previously noted.

Screenshot: https://cloudup.com/c-QxyfB3iVO

2010

All the widget areas in this theme have similar issues to the other themes. Most notable are needing margins on the MediaElement.js containers. Although, this theme isn't responsive so it doesn't have as many width issues as other themes.

Screenshot: https://cloudup.com/cibKJF4PQ1n

#8 follow-up: @laurelfulford
6 years ago

@westonruter - I'm looking at this with @davidakennedy and one thing we noticed is that the Youtube/Vimeo embeds don't have a width set at all. This is causing the issues above where those embeds aren't filling the available space -- though no width appears to be set, they display at a maximum of 300px wide.

The Video widget uses MediaElement.js on YouTube/Vimeo embeds to make them responsive. Would this be a reasonable fix for these embeds in the Text widget?

Is it something that can be fixed at the widget level at this point, or better to address in the themes for now and circle back?

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


6 years ago

This ticket was mentioned in Slack in #core-media by davidakennedy. View the logs.


6 years ago

#11 follow-up: @westonruter
6 years ago

  • Keywords needs-patch added

@laurelfulford Interesting. This is probably why @obenland opted to continue using the video shortcode for displaying YouTube and Vimeo videos in [41765]. I think if we basically rewrote the embed shortcodes to video shortcodes specifically for YouTube and Vimeo it would only be a partial fix as other oEmbeds would continue to be broken so we should try to find a more general solution.

What if the width were to be injected into the embeds? We currently have logic in the Text widget to basically override the width that an embed may explicitly provide. But it isn't providing something in return.

See also what was done in the audio widget in [41886] to add a max-width style to the player. But it seems like something needs to be done at a lower-level to give it width to begin with?

I think it's something that should probably be fixed at the widget level, but we can do that as part of this ticket. What width injection would fix the issue?

#12 in reply to: ↑ 8 @alexvorn2
6 years ago

  • Keywords needs-patch removed

Replying to laurelfulford:
This plugin may help making videos wider on posts - https://wordpress.org/plugins/fluid-video-embeds/
So we can import the functionality here to the widget from the plugin...

#13 @westonruter
6 years ago

  • Keywords needs-patch added

#14 @alexvorn2
6 years ago

oh! the tag added itself! This new trac update is not good.

#15 in reply to: ↑ 11 @joemcgill
6 years ago

Replying to westonruter:

What if the width were to be injected into the embeds? We currently have logic in the Text widget to basically override the width that an embed may explicitly provide. But it isn't providing something in return.

This logic seems ok when all we're expecting to pass to the callback is an instance of a media embed, for which that particular logic seemed to be written. In the text widget, however, it seems to apply much too widely, trying to match any HTML element included in the text widget.

A possible approach here might be to try and add a wrapping div around iframes that appear to contain videos. That said, it's worth noting that if you remove the inject_video_max_width_style logic from the text widget, the same max-width issues that affect the text widget also seem to apply to videos included in content, unless handled by the theme. I think that's a much better situation than Core trying to over-engineer a solution here.

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


6 years ago

This ticket was mentioned in Slack in #core-media by davidakennedy. View the logs.


6 years ago

#19 @westonruter
6 years ago

I've limited the width/height replacements logic to only video, iframe, object, and embed elements. See [42001].

#20 @davidakennedy
6 years ago

  • Milestone changed from 4.9 to 4.9.1

I had a few Slack discussions with @melchoyce and @westonruter about this ticket. This could be fixed by bundling FitVids with default themes, or something similar. But it might be better to bundle the needed functionality with core itself, making all the embeds, videos, etc. responsive by default. Themes could opt into it with an add_theme_support statement. This makes even more sense with Gutenberg coming in the future, and all kinds of blocks potentially needing this functionality.

For that reason, I'd say we punt this until 4.9.1, and aim for something like that.

Until then, the default themes and the embeds will still look okay in most cases, they just won't be full width or fluid.

Happy to hear other suggestions and thoughts though.

cc: @joemcgill @bradyvercher in case they want to chime in.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 years ago

#22 @bradyvercher
6 years ago

@davidakennedy I'm all for a core-based solution considering this is something every theme has to deal with. I also like the add_theme_support() suggestion. While I'm a fan of FitVids.js, I would really prefer to see a vanilla JS alternative in core.

#23 @johnbillion
6 years ago

  • Milestone changed from 4.9.1 to 5.0

This ticket was mentioned in Slack in #core-editor by xkon. View the logs.


6 years ago

#25 @johnbillion
5 years ago

  • Milestone changed from 5.0 to 5.1

#26 @laurelfulford
5 years ago

  • Milestone changed from 5.1 to 5.2

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


5 years ago

#28 @audrasjb
5 years ago

  • Milestone changed from 5.2 to Future Release

Moved to Future release as per today's bug scrub.

#29 @karmatosed
12 days ago

  • Keywords close added
  • Owner davidakennedy deleted

Looking at this ticket it hasn’t been active for few years and the following is my recommendation as to why this should be a candidate to close.

  • It was blessed due to a default theme which has now passed.
  • It was related to the default theme release which now has happened those years ago.
  • This is related to a specific feature in WordPress 4.8.

That said, if anyone wishes to work on a patch or has issues with this please raise them as a discussion whilst leaving this keyword on during discussion.

@davidakennedy for now I am going to remove you as owner and you can add yourself again if you want to continue working, however due to the inactivity this feels the correct move and opens up the ticket to anyone who might want to pick it up.

@westonruter if you have anything to add for context please do on this ticket also.

#30 @karmatosed
2 days ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

After leaving this for feedback, I am now going to move this to close. Thank you everyone for your collaboration on this.

Note: See TracTickets for help on using tickets.