Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing descriptor in webp source srcset #43

Closed
Manours opened this issue Jun 29, 2022 · 9 comments
Closed

Missing descriptor in webp source srcset #43

Manours opened this issue Jun 29, 2022 · 9 comments
Labels

Comments

@Manours
Copy link
Contributor

Manours commented Jun 29, 2022

Hi there 👋

I just found a curious behaviour when I use the get_image function with the webp format enabled.

This line :

{{ get_image(file, {
    src: 'figure-regular',
    srcset: 'figure-small 500w, figure-regular 1000w, figure-medium 1500w, figure-large 2000w',
    sizes: '(max-width: 1048px) calc(100vw - 48px), 1000px'
}) }}

Generate this HTML :

<picture>
  <source
    srcset="/uploads/media/figure-regular/01/1-image.webp?v=1-0, /uploads/media/figure-small/01/1-image.webp?v=1-0 500w, /uploads/media/figure-regular/01/1-image.webp?v=1-0 1000w, /uploads/media/figure-medium/01/1-image.webp?v=1-0 1500w, /uploads/media/figure-large/01/1-image.webp?v=1-0 2000w"
    type="image/webp"
  >
  <img
    alt="image"
    title="image"
    loading="lazy"
    src="/uploads/media/figure-regular/01/1-image.jpg?v=1-0"
    srcset="/uploads/media/figure-small/01/1-image.jpg?v=1-0 500w, /uploads/media/figure-regular/01/1-image.jpg?v=1-0 1000w, /uploads/media/figure-medium/01/1-image.jpg?v=1-0 1500w, /uploads/media/figure-large/01/1-image.jpg?v=1-0 2000w"
    sizes="(max-width: 1048px) calc(100vw - 48px), 1000px"
    width="1000"
    height="563"
  >
</picture>

If you look at the srcset in the webp source, the first url doesn't have a descriptor (like 1000w) which causes strange behavior by the browser.
I guess this url matches the src argument but I can't do whitout it.

Is there a way to remove it  ?

@alexander-schranz
Copy link
Member

alexander-schranz commented Jun 29, 2022

Hello @Manours,

looks like a bug the figure-regular is defined as 1000w but as it is not the first element of the srcset something strange is happening.

Can you create a failing test case for it? You can copy exist test case like the following and adjust it:

public function testComplexImageTag(): void
{
$imageExtension = new ImageExtension('/lazy');
$this->assertSame(
'<img alt="Logo"' .
' title="Description"' .
' src="/uploads/media/sulu-400x400/01/image.jpg?v=1-0"' .
' srcset="/uploads/media/sulu-400x400/01/image.jpg?v=1-0 1024w, /uploads/media/sulu-170x170/01/image.jpg?v=1-0 800w, /uploads/media/sulu-100x100/01/image.jpg?v=1-0 460w"' .
' sizes="(max-width: 1024px) 100vw, (max-width: 800px) 100vw, 100vw"' .
' id="image-id"' .
' class="image-class">',
$imageExtension->getImage(
$this->image,
[
'src' => 'sulu-400x400',
'srcset' => 'sulu-400x400 1024w, sulu-170x170 800w, sulu-100x100 460w',
'sizes' => '(max-width: 1024px) 100vw, (max-width: 800px) 100vw, 100vw',
'id' => 'image-id',
'class' => 'image-class',
'alt' => 'Logo',
]
)
);
}

@Manours
Copy link
Contributor Author

Manours commented Jun 29, 2022

I've created that test :

public function testComplexWebpPictureTag(): void
{
    $imageExtension = new ImageExtension('/lazy');
    $imageExtension = new ImageExtension(null, [], ['webp' => 'image/webp']);

    $this->assertSame(
        '<picture>' .
        '<source srcset="/uploads/media/sulu-100x100/01/image.webp?v=1-0 460w, /uploads/media/sulu-170x170/01/image.webp?v=1-0 800w, /uploads/media/sulu-400x400/01/image.webp?v=1-0 1024w"' .
        ' type="image/webp">' .
        '<img alt="Logo"' .
        ' title="Description"' .
        ' src="/uploads/media/sulu-400x400/01/image.jpg?v=1-0"' .
        ' srcset="/uploads/media/sulu-100x100/01/image.jpg?v=1-0 460w, /uploads/media/sulu-170x170/01/image.jpg?v=1-0 800w, /uploads/media/sulu-400x400/01/image.jpg?v=1-0 1024w"' .
        ' sizes="(max-width: 1024px) 100vw, (max-width: 800px) 100vw, 100vw"' .
        ' id="image-id"' .
        ' class="image-class">' .
        '</picture>',
        $imageExtension->getImage(
            $this->image,
            [
                'src' => 'sulu-400x400',
                'srcset' => 'sulu-100x100 460w, sulu-170x170 800w, sulu-400x400 1024w',
                'sizes' => '(max-width: 1024px) 100vw, (max-width: 800px) 100vw, 100vw',
                'id' => 'image-id',
                'class' => 'image-class',
                'alt' => 'Logo',
            ]
        )
    );
}

And this is the result :

1) Sulu\Twig\Extensions\Tests\Unit\ImageExtensionTest::testComplexWebpPictureTag
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<picture><source srcset="/uploads/media/sulu-100x100/01/image.webp?v=1-0 460w, /uploads/media/sulu-170x170/01/image.webp?v=1-0 800w, /uploads/media/sulu-400x400/01/image.webp?v=1-0 1024w" type="image/webp"><img alt="Logo" title="Description" src="/uploads/media/sulu-400x400/01/image.jpg?v=1-0" srcset="/uploads/media/sulu-100x100/01/image.jpg?v=1-0 460w, /uploads/media/sulu-170x170/01/image.jpg?v=1-0 800w, /uploads/media/sulu-400x400/01/image.jpg?v=1-0 1024w" sizes="(max-width: 1024px) 100vw, (max-width: 800px) 100vw, 100vw" id="image-id" class="image-class"></picture>'
+'<picture><source srcset="/uploads/media/sulu-400x400/01/image.webp?v=1-0, /uploads/media/sulu-100x100/01/image.webp?v=1-0 460w, /uploads/media/sulu-170x170/01/image.webp?v=1-0 800w, /uploads/media/sulu-400x400/01/image.webp?v=1-0 1024w" type="image/webp"><img alt="Logo" title="Description" src="/uploads/media/sulu-400x400/01/image.jpg?v=1-0" srcset="/uploads/media/sulu-100x100/01/image.jpg?v=1-0 460w, /uploads/media/sulu-170x170/01/image.jpg?v=1-0 800w, /uploads/media/sulu-400x400/01/image.jpg?v=1-0 1024w" sizes="(max-width: 1024px) 100vw, (max-width: 800px) 100vw, 100vw" id="image-id" class="image-class"></picture>'

@alexander-schranz
Copy link
Member

@Manours can you create a pull request with it? Would be great.

@alexander-schranz
Copy link
Member

Thank you, fixed in 2.5.4.

@Manours
Copy link
Contributor Author

Manours commented Jun 29, 2022

Thank you 👍

@Manours
Copy link
Contributor Author

Manours commented Sep 1, 2022

I'm back @alexander-schranz … 👋

I just remember that the sizes attributes should be added to the <source> tags too.

Capture d’écran 2022-08-31 à 15 46 12

Could you please handle this ?

@alexander-schranz
Copy link
Member

@Manours Yeah definitly a bug when the validators says that. Can you create a test case with the sizes attribute matching this usecase?

@Manours
Copy link
Contributor Author

Manours commented Oct 20, 2022

Is this okay @alexander-schranz ?
#46

@alexander-schranz
Copy link
Member

Thx for the reproducer. PR is merged will fix another issue and great a release soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants