search

silverstripe / silverstripe-assets

Silverstripe Assets component
BSD 3-Clause "New" or "Revised" License
9 stars 65 forks source link

Image convert('webp') conflicting with LazyLoad(false) #608

Closed thisisannie closed 4 weeks ago

thisisannie commented 1 month ago

Module version(s) affected

5.x-dev#8886a3a93ddf5d9ad0b677f7403ca600894bbe00

Description

Aim: To (selectively) disable lazy loading in SS template and convert the image to webp.

Issue: When both attributes are applied, the tag loading="lazy" is incorrectly applied to the image tag.

How to reproduce

In the .ss file:

<% include SideBar %>
<div class="content-container unit size3of4 lastUnit">

    <%-- regular image --%>
    0<br>
    $MyImage<br>
    0<br>

    <%-- as expected: this produces a webp and applies lazy loading tag by default --%>
    1<br>
    $MyImage.Convert('webp')<br>
    1<br>

    <%-- as expected: this correctly omits lazy loading tag  --%>
    2<br>
    $MyImage.LazyLoad(false)<br>
    2<br>

    <%-- problem: this produces a webp but incorrectly applies lazy loading tag --%>
    3<br>
    $MyImage.LazyLoad(false).Convert('webp')<br>
    3<br>

</div>

Possible Solution

No response

Additional Context

No response

Maybe related

Validations

PRs

emteknetnz commented 1 month ago

Have replicated locally

Page.php

<?php

use SilverStripe\Assets\File;
use SilverStripe\AssetAdmin\Forms\UploadField;
use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\Assets\Image;

class Page extends SiteTree
{
    private static $db = [];

    private static $has_one = [
        'MyImage' => Image::class,
    ];

    private static $owns = [
        'MyImage',
    ];

    public function getCMSFields()
    {
        $fields = parent::getCMSFields();
        $fields->addFieldsToTab(
            'Root.Main',
            [
                UploadField::create('MyImage'),
            ]
        );
        return $fields;
    }
}

Page.ss

<% include SideBar %>
<div class="content-container unit size3of4 lastUnit">
    <article>
        <h1>$Title</h1>
        <div class="content">$Content</div>
    </article>

    <% if $MyImage %>
        <%-- regular image --%>
        0<br>
        $MyImage<br>
        0<br>

        <%-- as expected: this produces a webp and applies lazy loading tag by default --%>
        1<br>
        $MyImage.Convert('webp')<br>
        1<br>

        <%-- as expected: this correctly omits lazy loading tag  --%>
        2<br>
        $MyImage.LazyLoad(false)<br>
        2<br>

        <%-- problem: this produces a webp but incorrectly applies lazy loading tag --%>
        3<br>
        $MyImage.LazyLoad(false).Convert('webp')<br>
        3<br>
    <% end_if %>

</div>
emteknetnz commented 1 month ago

Note that as a probably seperate issue $MyImage.Convert('webp').LazyLoad(false) will throw an exception [Emergency] Uncaught Error: Call to a member function setOriginal() on null

GuySartorelli commented 1 month ago

It'll be worth checking if this is related to convert() specifically, or if other manipulations such as Fit after setting LazyLoad also have the same problem.

Also, moving to assets since that's where the code for this is.

thisisannie commented 1 month ago

Probably related, the alt value is also incorrectly omitted when convert is applied

emteknetnz commented 1 month ago

The alt attribute issue might be resolved post merging of https://github.com/silverstripe/silverstripe-framework/pull/11217

GuySartorelli commented 1 month ago

Turns out all attributes have this bug. I'm about to make a PR which will fix it.

emteknetnz commented 4 weeks ago

@thisisannie Linked PR has been merged