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

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

Closed
2 tasks done
thisisannie opened this issue May 30, 2024 · 7 comments
Closed
2 tasks done

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

thisisannie opened this issue May 30, 2024 · 7 comments

Comments

@thisisannie
Copy link

thisisannie commented May 30, 2024

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

  • Check that there isn't already an issue that reports the same bug
  • Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

PRs

@emteknetnz
Copy link
Member

emteknetnz commented May 30, 2024

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
Copy link
Member

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
Copy link
Member

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.

@GuySartorelli GuySartorelli transferred this issue from silverstripe/silverstripe-framework May 30, 2024
@thisisannie
Copy link
Author

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

@emteknetnz
Copy link
Member

The alt attribute issue might be resolved post merging of silverstripe/silverstripe-framework#11217

@GuySartorelli GuySartorelli self-assigned this Jun 4, 2024
@GuySartorelli
Copy link
Member

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

@emteknetnz
Copy link
Member

@thisisannie Linked PR has been merged

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

No branches or pull requests

3 participants