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

Align rendering of image genercated by ImageShortCodeProvider with generic Image generation #596

Open
wants to merge 1 commit into
base: 2
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 19 additions & 5 deletions src/ImageManipulation.php
Original file line number Diff line number Diff line change
Expand Up @@ -1150,8 +1150,8 @@ protected function getDefaultAttributes(): array
$attributes = [];
if ($this->getIsImage()) {
$attributes = [
'width' => $this->getWidth(),
'height' => $this->getHeight(),
'width' => $this->getWidth() ?: null,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the short code was rendering an non-existent image, getWidth and getHeight would return 0. This would lead to a width="0" attribute in the HTML.

Defaulting to null here fixes this.

'height' => $this->getHeight() ?: null,
'alt' => $this->getTitle(),
'src' => $this->getURL(false)
];
Expand All @@ -1176,11 +1176,25 @@ public function getAttributes()

$attributes = array_merge($defaultAttributes, $this->attributes);

// We need to suppress the `loading="eager"` attribute after we merge the default attributes
if (isset($attributes['loading']) && $attributes['loading'] === 'eager') {
unset($attributes['loading']);
if (isset($attributes['loading'])) {
// Check if the dimensions for the image have been set
$dimensionsUnset = (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shortcode had some logic to make sure that if no size was provided, the image would be eager loaded. This logic made more sense here in Image manipulation than in the original one.

--

The shortcode also had some logic to handle the scenario where only the height or only the width was provided. In that scenario, the missing attribute would be omitted.

Unfortunately, ImageManipulation really wants to read the size from the actual Image. This would lead to the aspect ratio of the image being changed. So I updated ImageShortcodeProvider to explicitely set the missing value to auto in those situation.

empty($attributes['width']) ||
empty($attributes['height']) ||
$attributes['width'] === 'auto' ||
$attributes['height'] === 'auto'
);

// We need to suppress the `loading="eager"` attribute after we merge the default attributes
// or if dimensions are not set
if ($attributes['loading'] === 'eager' || $dimensionsUnset) {
unset($attributes['loading']);
}
}




$this->extend('updateAttributes', $attributes);

return $attributes;
Expand Down
71 changes: 35 additions & 36 deletions src/Shortcodes/ImageShortcodeProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use SilverStripe\Assets\Image;
use SilverStripe\Core\Flushable;
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Dev\Deprecation;
use SilverStripe\View\Parsers\ShortcodeHandler;
use SilverStripe\View\Parsers\ShortcodeParser;

Expand Down Expand Up @@ -73,26 +74,43 @@ public static function handle_shortcode($args, $content, $parser, $shortcode, $e
return null; // There were no suitable matches at all.
}

// Grant access to file if necessary
if (static::getGrant($record)) {
$record->grantFile();
}

// Check if a resize is required
$manipulatedRecord = $record;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old approach was basically building the <img> tag by hand.

I'm using standard Image manipulation to assign the correct size and attribute to a manipulated image instead.

$width = null;
$height = null;
$grant = static::getGrant($record);
$src = $record->getURL($grant);
maxime-rainville marked this conversation as resolved.
Show resolved Hide resolved
if ($record instanceof Image) {
$width = isset($args['width']) ? (int) $args['width'] : null;
$height = isset($args['height']) ? (int) $args['height'] : null;

// Resize the image if custom dimensions are provided
$hasCustomDimensions = ($width && $height);
if ($hasCustomDimensions && (($width != $record->getWidth()) || ($height != $record->getHeight()))) {
$resized = $record->ResizedImage($width, $height);
$resized = $manipulatedRecord->ResizedImage($width, $height);
// Make sure that the resized image actually returns an image
if ($resized) {
$src = $resized->getURL($grant);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The grant isn't needed here. Session grants apply to the original file and all its variant.

$manipulatedRecord = $resized;
}
}

// If only one of width or height is provided, set the other to auto
if ($width && !$height) {
$args['height'] = 'auto';
} elseif (!$width && $height) {
$args['width'] = 'auto';
}
}

// Determine whether loading="lazy" is set
$args = self::updateLoadingValue($args, $width, $height);
maxime-rainville marked this conversation as resolved.
Show resolved Hide resolved
// Set lazy loading attribute
if (!empty($args['loading'])) {
$loading = strtolower($args['loading']);
unset($args['loading']);
$manipulatedRecord = $manipulatedRecord->LazyLoad($loading !== 'eager');
}

// Build the HTML tag
$attrs = array_merge(
Expand All @@ -101,7 +119,7 @@ public static function handle_shortcode($args, $content, $parser, $shortcode, $e
// Use all other shortcode arguments
$args,
// But enforce some values
['id' => '', 'src' => $src]
['id' => '', 'src' => '']
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The src attirbutes need to be unset here because we want the resize picture to pick up its own URL.

);

// If file was not found then use the Title value from static::find_error_record() for the alt attr
Expand All @@ -111,11 +129,14 @@ public static function handle_shortcode($args, $content, $parser, $shortcode, $e

// Clean out any empty attributes (aside from alt) and anything not whitelisted
$whitelist = static::config()->get('attribute_whitelist');
$attrs = array_filter($attrs ?? [], function ($v, $k) use ($whitelist) {
return in_array($k, $whitelist) && (strlen(trim($v ?? '')) || $k === 'alt');
}, ARRAY_FILTER_USE_BOTH);
foreach ($attrs as $key => $value) {
if (in_array($key, $whitelist) && (strlen(trim($value ?? '')) || $key === 'alt')) {
$manipulatedRecord = $manipulatedRecord->setAttribute($key, $value);
}
}

$markup = self::createImageTag($attrs);
// We're calling renderWith() with an explicit template in case someone wants to use a custom template
$markup = $manipulatedRecord->renderWith(self::class . '_Image');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're using a ImageShortcodeProvider specific template here. This would allow you to customise each template individually.

GuySartorelli marked this conversation as resolved.
Show resolved Hide resolved

// cache it for future reference
if ($fileFound) {
Expand All @@ -131,9 +152,12 @@ public static function handle_shortcode($args, $content, $parser, $shortcode, $e

/**
* Construct and return HTML image tag.
*
* @deprecated 2.3.0
*/
public static function createImageTag(array $attributes) : string
{
Deprecation::notice('2.3.0', 'Will be removed without equivalent functionality to replace it.');
$preparedAttributes = '';
foreach ($attributes as $attributeKey => $attributeValue) {
if (strlen($attributeValue ?? '') > 0 || $attributeKey === 'alt') {
Expand Down Expand Up @@ -209,29 +233,4 @@ protected static function find_error_record($errorCode)
'Title' => _t(__CLASS__ . '.IMAGENOTFOUND', 'Image not found'),
]);
}

/**
* Updated the loading attribute which is used to either lazy-load or eager-load images
* Eager-load is the default browser behaviour so when eager loading is specified, the
* loading attribute is omitted
*
* @param array $args
* @param int|null $width
* @param int|null $height
* @return array
*/
private static function updateLoadingValue(array $args, ?int $width, ?int $height): array
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is private so no need to deprecate the method. We can just nuke it.

{
if (!Image::getLazyLoadingEnabled()) {
return $args;
}
if (isset($args['loading']) && $args['loading'] == 'eager') {
// per image override - unset the loading attribute unset to eager load (default browser behaviour)
unset($args['loading']);
} elseif ($width && $height) {
// width and height must be present to prevent content shifting
$args['loading'] = 'lazy';
}
return $args;
}
}
2 changes: 1 addition & 1 deletion templates/DBFile_download.ss
Original file line number Diff line number Diff line change
@@ -1 +1 @@
<a href="$URL.ATT" title="$Title" <% if $Basename %>download="$Basename.ATT"<% else %>download<% end_if %>>$Title</a>
<% include SilverStripe\Assets\Storage\DBFile %>
2 changes: 1 addition & 1 deletion templates/DBFile_image.ss
Original file line number Diff line number Diff line change
@@ -1 +1 @@
<img $AttributesHTML />
<% include SilverStripe\Assets\Storage\DBFile_Image %>
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<% include SilverStripe\Assets\Storage\DBFile_Image %>
1 change: 1 addition & 0 deletions templates/SilverStripe/Assets/Storage/DBFile.ss
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<a href="$URL.ATT" title="$Title" <% if $Basename %>download="$Basename.ATT"<% else %>download<% end_if %>>$Title</a>
1 change: 1 addition & 0 deletions templates/SilverStripe/Assets/Storage/DBFile_Image.ss
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<img $AttributesHTML />