ResponsiveWrapper: Fixed rendering SVGs as a featured image. · Pull Request #48307 · WordPress/gutenberg

ghost · 2023-02-22T11:03:31Z

The feature image gets rendered within a ResponsiveWrapper. That relies on the image's height and width being able to be determined. In the PostFeaturedImage component, it isn't able to do that for an SVG. This leads the ResponsiveWrapper assigning an undefined value to the paddingBottom style.

As suggested, I added a check for an undefined value and in that case set padding bottom to 100%.

Props aaronrobertshaw, Mamaduka.
Fixes #48184, #5477.

What?

Fixing the rendering of SVG images when set as a featured image.

Why?

The natural height and width of an SVG can't reliably be determined, resulting in a CSS bug. See #48184.

How?

As suggested by @aaronrobertshaw, I added a check for an undefined value on the naturalWidth in the <ResponsiveWrapper /> component and in that case set padding-bottom to 100%.

Testing Instructions

Add support for svg to your website (see the code snippet below)
Create a page or post
Add an SVG as a featured image

function toms_mime_types( $t ) {
	$t['svg'] = 'image/svg+xml';
	return $t;
}
add_filter( 'upload_mimes', 'toms_mime_types' );

The feature image gets rendered within a ResponsiveWrapper. That relies on the image's height and width being able to be determined. In the PostFeaturedImage component, it isn't able to do that for an SVG. This leads the ResponsiveWrapper assigning an undefined value to the paddingBottom style. As suggested, I added a check for an undefined value and in that case set padding bottom to 100%. Props aaronrobertshaw, Mamaduka. Fixes #48184, #5477.

github-actions · 2023-02-22T11:03:56Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @tomdevisser! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

aaronrobertshaw

Thanks for taking the time and effort to work on this @tomdevisser 👍

Unfortunately, it looks like the old comment of mine that you were directed to, might have put this PR on the wrong track. At the time, I suspected the use of paddingBottom: 100% would have its shortcomings but I didn't have the bandwidth to explore it and since then the issue has fallen off my radar.

To test this PR and the updates to the ResponsiveWrapper component in a more isolated manner, I tweaked the component's storybook example to use various SVGs.

So long as the SVG's height is <= to its width, the SVG appears to display ok. Once the height is greater than the width however paddingBottom: 100% doesn't provide enough real estate and the bottom of the image gets cropped.

Quick diff adding storybook example used above

diff --git a/packages/components/src/responsive-wrapper/stories/index.tsx b/packages/components/src/responsive-wrapper/stories/index.tsx
index 5f48b91d27..f6b86289ec 100644
--- a/packages/components/src/responsive-wrapper/stories/index.tsx
+++ b/packages/components/src/responsive-wrapper/stories/index.tsx
@@ -36,3 +36,35 @@ Default.args = {
 		/>
 	),
 };
+
+export const WithoutNaturalDimensions = Template.bind( {} );
+WithoutNaturalDimensions.args = {
+	children: (
+		<svg xmlns="http://www.w3.org/2000/svg" height="320px" width="140px">
+			<rect
+				x="20"
+				y="20"
+				width="100"
+				height="280"
+				style={ { fill: 'blue' } }
+			/>
+			<g>
+				<circle style={ { fill: 'red' } } cx="70" cy="80" r="30" />
+				<circle style={ { fill: 'yellow' } } cx="70" cy="160" r="30" />
+				<circle style={ { fill: '#40CC40' } } cx="70" cy="240" r="30" />
+			</g>
+		</svg>
+	),
+};

Following some of the other links from the original issue, there does appear to be an alternate approach that might be worth exploring here as well.

Whichever approach we end up settling upon to fix this issue, it would be good to add a storybook example that demonstrates the behaviour with SVGs, along with a changelog entry for the components package.

Thanks again for submitting the PR. I'll be happy to review again when needed 🙂

ghost · 2023-02-24T10:06:08Z

@aaronrobertshaw Thanks for reviewing. Will be interesting to try and create a Storybook example, I have no experience with it yet.

While searching I found 10up also has a pretty easy solution for this in their plugin:
https://github.com/10up/safe-svg/blob/develop/safe-svg.php#L329
https://github.com/10up/safe-svg/blob/develop/assets/safe-svg.css#L4

Would you say that's the right way to go? If so, I could implement this, but I'd like your opinion on it before diving into the code again. As far as I can see, the only thing they've changed is to set the width to 100% if the image source is an SVG.

aaronrobertshaw · 2023-02-24T10:23:31Z

Will be interesting to try and create a Storybook example, I have no experience with it yet.

The Storybook docs are pretty helpful and worth taking a look at before you jump in.

There are also some examples you can follow in the components package, though I'd tend to focus more on those components whose stories are in TypeScript as they'll be more up-to-date with best practices. Luckily, the ResponsiveWrapper is one of those 🙂

Speaking of best practices, the components package has its own contributing readme you might find handy, complete with a section on the Storybook.

Would you say that's the right way to go?

Unfortunately, I can't say at the moment. I'd need to take a closer look at the featured image use case and the linked approach but I'm out of time today.

At a quick glance, it looks a little hacky to be wrapping the image in additional markup. My gut feeling is it would be better to solve the issue a little closer to its source if possible.

ghost · 2023-02-24T11:47:41Z

At a quick glance, it looks a little hacky to be wrapping the image in additional markup. My gut feeling is it would be better to solve the issue a little closer to its source if possible.

I think the extra markup is just for the plugin to be sure it keeps working if Gutenberg changes the wrapper/classes around it. We can apply the width 100% directly to the image using an existing class or an inline style, I don't know what the standard approach is in the Gutenberg project.

ciampo · 2023-02-24T11:56:07Z

Thank you @tomdevisser for working on this!

@aaronrobertshaw shared some great feedback, pretty much the same that I would have also shared.

Regarding an alternative approach, have we considered object-fit?

ghost · 2023-02-24T12:04:37Z

Regarding an alternative approach, have we considered object-fit?

I haven't yet, but worth trying! When writing new styles in Gutenberg, is there a common practice in choosing what selector to use? And the specificity, should it be as low as needed? Would love some pointers on this!

ciampo · 2023-02-24T13:31:22Z

As suggested from Aaron, the fix should be applied as close to the source as possible (in this case, in the ResponsiveWrapper component — as you've already done with your first attempt).

When writing new styles in Gutenberg, is there a common practice in choosing what selector to use?

We usually use classnames, following a BEM-like convention (as you can also see in the style.scss file for the ResponsiveWrapper component.

More specifically regarding the @wordpress/components package, we have some contributing guidelines that you may find useful. With regards to styling, you'll notice that the guidelines suggest using CSS-in-JS where possible. In this specific case, since the component already has Sass styles, you can continue writing any new styles in the styles.scss file.

And the specificity, should it be as low as needed? Would love some pointers on this!

In terms of specificity, it's good practice to keep it as low as necessary, in order to prevent "specificity wars" in the library. Having said that, we strongly discourage any style overrides outside of the library (and especially the overrides that rely on the internal components' class names), since they are prone to breakages any time the internal implementation of the component changes.

ghost · 2023-02-24T13:55:28Z

More specifically regarding the @wordpress/components package, we have some contributing guidelines that you may find useful.

Thank you for linking this, and for sharing the other helpful information. Learning more about contributing and the project every day.

I have checked out the branch again, reverted my previous changes and tried the object-fit, but without any luck. I also tried different solutions using fit-content, different display values, etc. The only fix that seems to work with different SVGs, also when height is bigger than width, is the padding-bottom: 100%, a fix that was already there partially, but could also be applied through CSS on the .components-responsive-wrapper.

I'll keep trying some other solutions, but if any of you has more ideas they're more than welcome!

ghost · 2023-02-24T14:55:52Z

I think I found a fix, using a position of relative. I did have to remove the padding bottom (which seemed like a weird way to achieve the height anyway), but this also removes the ResizeObserver.

This might be too big a change and I don't know what implications this has to this component. But after testing with several images of different mime types everything seems to work well, so I wanted to give it a chance anyway. 🙂

It does do this one weird thing when replacing from svg to image, not sure why.

buggy-img.mov

ghost · 2023-02-24T15:47:39Z

Update: Looking at the StoryBook locally I see how this obviously breaks things as I could've expected.

Starting to think with the current markup it's near to impossible to fix, and when changing the markup there's too much that'll possibly break as everything is depending on each other.

We could make a separate component just for the Featured Image wrapper, so we can't break anything else, but I'm not sure if that's the right solution.

I would almost propose to go with the padding fix for now as that seems to work for almost everything without breaking anything, and it's just a preview so it isn't too bad that it gets cropped. It gives us more time to think about a better implementation while it "works".

ciampo · 2023-02-24T18:55:55Z

Could you commit the changes to Storybook that introduce a new example with the SVG image? I may also attempt to have a look :)

ghost · 2023-02-24T19:00:34Z

@ciampo Sure thing! But where can I grab a good svg for adding to the Storybook? The URL being used right now is https://s.w.org/style/images/about/WordPress-logotype-standard.png, is there like a place to upload images to w.org to use here? Or a shared folder with assets we can use?

ghost · 2023-02-24T19:16:16Z

Grabbed an SVG I could find online of the WordPress logo, should be fine temporarily, but it's probably better to replace it before any merge happens.

ciampo · 2023-02-24T19:19:40Z

Aaron posted a diff earlier today in which he had created an ad-hoc SVG image for the Storybook example, if it can be useful

aaronrobertshaw · 2023-02-27T08:18:11Z

It might also be worth noting that if the SVG defines viewBox and preserveAspectRatio attributes, the SVG is contained within the available area using only padding-bottom: 100%.

The ResponsiveWrapper component clones its children injecting the components-responsive-wrapper__content class. Could it be possible to reduce the chances of some SVGs not scaling here by checking for an SVG child, and if it is missing the viewbox prop but has width/height dimensions, adding the viewbox and preserveAspectRatio props?

The following diff contains an updated SVG for testing with my earlier Storybook example snippet.

Click for SVG code

		<svg
			xmlns="http://www.w3.org/2000/svg"
			viewBox="0 0 140 320"
			preserveAspectRatio="xMinYMin meet"
			height="320px"
			width="140px"
		>
			<rect
				x="0"
				y="0"
				width="140"
				height="320"
				style={ { fill: 'blue' } }
			/>
			<g>
				<circle style={ { fill: 'red' } } cx="70" cy="80" r="30" />
				<circle style={ { fill: 'yellow' } } cx="70" cy="160" r="30" />
				<circle style={ { fill: '#40CC40' } } cx="70" cy="240" r="30" />
			</g>
		</svg>

ghost · 2023-02-27T08:22:13Z

Could it be possible to reduce the chances of some SVGs not scaling here by checking for an SVG child, and if it is missing the viewbox prop but has width/height dimensions, adding the viewbox and preserveAspectRatio props?

Is it possible to get these values on upload and add them to the WP Image? Cause in that case we don't have to change anything in the ResponsiveWrapper component. This is getting a bit out of my scope, so if anyone else wants to try a commit I encourage you to, I will stay active here and try to think along.

Mamaduka · 2023-03-08T04:49:12Z

@tomdevisser, can we close this PR now that #48573 is merged?

ghost requested a review from ajitbohra as a code owner February 22, 2023 11:03

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Feb 22, 2023

ghost mentioned this pull request Feb 22, 2023

Using SVG as a featured image #48184

Closed

Mamaduka requested review from Mamaduka and ciampo February 22, 2023 11:06

Merge branch 'WordPress:trunk' into fix/featured-image-svg-fix

10bc2ff

ghost added [Feature] Document Settings Document settings experience [Feature] Media Anything that impacts the experience of managing media [Type] Bug An existing feature does not function as intended labels Feb 22, 2023

aaronrobertshaw reviewed Feb 24, 2023

View reviewed changes

ciampo added the [Package] Components /packages/components label Feb 24, 2023

ciampo assigned ghost Feb 24, 2023

ciampo requested a review from mirka February 24, 2023 13:01

Removed ResizeObserver and added relative positioning

e6b115b

Restored the component to original state and added storybook variation

7263bc9

ghost marked this pull request as draft February 27, 2023 11:37

ciampo mentioned this pull request Feb 28, 2023

ResponsiveWrapper: use aspect-ratio CSS prop and support SVG elements #48573

Merged

ghost closed this Mar 8, 2023

This pull request was closed.

Conversation

ghost commented Feb 22, 2023

What?

Why?

How?

Testing Instructions

Uh oh!

github-actions bot commented Feb 22, 2023

Uh oh!

aaronrobertshaw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Feb 24, 2023

Uh oh!

aaronrobertshaw commented Feb 24, 2023

Uh oh!

ghost commented Feb 24, 2023

Uh oh!

ciampo commented Feb 24, 2023

Uh oh!

ghost commented Feb 24, 2023

Uh oh!

ciampo commented Feb 24, 2023

Uh oh!

ghost commented Feb 24, 2023

Uh oh!

ghost commented Feb 24, 2023

Uh oh!

ghost commented Feb 24, 2023

Uh oh!

ciampo commented Feb 24, 2023

Uh oh!

ghost commented Feb 24, 2023

Uh oh!

ghost commented Feb 24, 2023

Uh oh!

ciampo commented Feb 24, 2023

Uh oh!

aaronrobertshaw commented Feb 27, 2023

Uh oh!

ghost commented Feb 27, 2023

Uh oh!

Mamaduka commented Mar 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aaronrobertshaw left a comment •

edited

Loading