Yireo_NextGenImages icon indicating copy to clipboard operation
Yireo_NextGenImages copied to clipboard

[FEATURE] add support for background images

Open rommelfreddy opened this issue 2 years ago • 12 comments

Hey !

Magento does have a pagebuilder. Some image elements are placed with inline css as background image.

This PR will also handle CSS background images.

rommelfreddy avatar Jan 28 '22 22:01 rommelfreddy

Thanks! The NextGenImages is currently under heavy refactoring. I'll look into your PR once that refactoring is done, and I'll make sure the code is put to use. Thanks again!

jissereitsma avatar Jan 31 '22 08:01 jissereitsma

I thought at first it was a tag like <img src="data:image/gif;base64,foobar"/> but this is already skipped. Are you referring to something like <div style="background-image: url(foobar.png);" />? Because that's skipped as well? Or is there an img tag added with background-image?

I'm trying to proof that this is needed by extending upon a unit test Test\Unit\Util\HtmlReplacerTest, but if you could help me with creating the right HTML that would fail without this PR, but would succeed with this PR, then we have a test in place as well.

jissereitsma avatar Feb 21 '22 15:02 jissereitsma

By reading your PR (I should have done that already), I seem to understand that with your addition <div style="background-image: url(foobar.png);" /> would be turned into <div style="background-image: url(foobar.webp);" />. But this would modify the final HTML source, which is then cached and used for all browsers, right? And also, what if there are both WebP images and AVIF images added?

jissereitsma avatar Feb 21 '22 15:02 jissereitsma

oh, i see what you mean. on devices which does not support webp images, the image should not replaces by the webp image.

my code should be modified so that the background image contains multiple image formats too.

maybe i will find time for that the next two weeks, but i can not promise it.

rommelfreddy avatar Feb 21 '22 16:02 rommelfreddy

Exactly, different devices should be supported by the same HTML. With a <picture> tag this is possible. But I'm not sure if this is possible with inline CSS (or any kind of CSS for that matter). For instance, an HTML tag could support media queries:

<div style="background-image: url(foobar.png); @media (max-width:300px){ background-image: url(foobar-small.png); }" />

But to my knowledge there is no media query for matching a WebP image. Before diving into modifying the PR, we should first sort out a possible HTML/CSS combo that overcomes this problem. But I'm not sure if this is possible, if CSS media queries are not useful here.

jissereitsma avatar Feb 21 '22 17:02 jissereitsma

To add to this, the next version of this WebP module will add a little JS to add a new CSS class to the body, being either webp or no-webp. This would allow for a CSS rule like body.webp div.example { background-image: url(foobar-small.webp); }. But still this can't be done in an inline style (which is only able to influence that HTML element).

jissereitsma avatar Feb 21 '22 18:02 jissereitsma

i dont know any possiblities too.

but maybe this would be a nice beta feature which has to be explicit enabled in the administration. How do you think about this?

So the most of all browsers does support webp, so i think it is a good idea to move the decision to the admin of the shop. https://caniuse.com/webp

rommelfreddy avatar Feb 21 '22 18:02 rommelfreddy

Actually that sounds simple. But just like the IE discussions in the past, you don't want to block shops for older browsers. Even if the default would be to use WebP, this would still require a fallback. And the fallback is simply an issue with CSS backgrounds.

jissereitsma avatar Feb 22 '22 08:02 jissereitsma

@rommelfreddy Couldn't find $this->getAlternativeImagesByImageUrl($image); function on Util/HtmlReplacer.php

sanjayaccorin avatar Jul 01 '22 13:07 sanjayaccorin

@rommelfreddy Couldn't find $this->getAlternativeImagesByImageUrl($image); function on Util/HtmlReplacer.php

Hey @sanjayaccorin this function was implemented in the past. i guess it has been removed/renamed in the past.

if i see it correctly, it is required to replace it with $images = $this->imageCollector->collect($imageUrl);

rommelfreddy avatar Jul 04 '22 12:07 rommelfreddy

@sanjayaccorin i have updated the PR :) look forward to hear from you.

rommelfreddy avatar Aug 04 '22 12:08 rommelfreddy

@jissereitsma, @sanjayaccorin i have updated the PR again. look forward to hear from you.

rommelfreddy avatar Sep 12 '22 08:09 rommelfreddy