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

maxime-rainville · 2024-04-29T06:22:30Z

This PR aligns how ImageShortcodeProvider generates its HTML markup to be aligned with how regular images are generated.

This reduces complexity and makes it easy to customise the HTML markups.

Parent issue

Create an advanced image rendering solution .github#230

Depends on

ENH Update AttributesHTML to output alt attribute even if it's empty silverstripe-framework#11217
- Review combined build

maxime-rainville

Template tree

templates/DBFile_image.ss is what was used to render the mark-up for images in templates. There's a related templates/DBFile_download.ss that renders plain file like.

I concluded that those files were misplaced. They should be following the namespace structure.

What I did

Created matching DBFile and DBFile_Image templates in the correct folder.
ImageShortcodeProvider now generates its markup by calling its own template that includes the namespaced image template.
I moved all the mark up from the original templates and moved it to the template in the correct namespace.
- If you've customised image rendering by overriding templates/DBFile_image.ss, your custom template will still be used.
- Because the ImageShortcodeProvider doesn't call templates/DBFile_image.ss at all, your custom template won't start bleeding into TinyMCE images.

Things to change in CMS 6

Once this PR is merged, I'll do a follow up PR for CMS 6:

This method in DBFile should reference the name spaced template.
templates/DBFile_image.ss and templates/DBFile_download.ss will be deleted.
ImageShortcodeProvider::createImageTag() will be deleted since it's now deprecated.

Other

I haven't added any test because the output of this thing is meant to be identical to the original.

I guess the only test I could run is try to override some templates and validate that I still get the expected output. Not sure how much value that would provide.

maxime-rainville · 2024-04-29T06:26:57Z

src/Shortcodes/ImageShortcodeProvider.php

@@ -75,10 +75,10 @@ public static function handle_shortcode($args, $content, $parser, $shortcode, $e
        }

        // Check if a resize is required
+        $manipulatedRecord = $record;


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.

maxime-rainville · 2024-04-29T06:28:18Z

src/Shortcodes/ImageShortcodeProvider.php

@@ -87,7 +87,7 @@ public static function handle_shortcode($args, $content, $parser, $shortcode, $e
                $resized = $record->ResizedImage($width, $height);
                // Make sure that the resized image actually returns an image
                if ($resized) {
-                    $src = $resized->getURL($grant);


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

maxime-rainville · 2024-04-29T06:29:15Z

src/Shortcodes/ImageShortcodeProvider.php

@@ -102,7 +102,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' => '']


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

maxime-rainville · 2024-04-29T06:31:58Z

src/Shortcodes/ImageShortcodeProvider.php


-        $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');


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

src/Shortcodes/ImageShortcodeProvider.php

maxime-rainville

Still a WIP, but here's some pointers while I have them in my mind.

The results between the old way and new way should be functionally equivalent. However, because shortcode and plain images were being rendered differently they are a couple situation where you will get slightly different results.

No size in short code

[img id=1] would be converted to <img src="..." alt="">

Because ImageManipulation autodetects the size of the image the output will now be <img src="..." alt="" width="123" height="123" loading="lazy">,

In practice, it should not be possible to not provide sizes for images unless you bypass the UI. So this should be a very marginal use case.

In any case, I'm calling this an improvement. An image in this scenario would now be lazy loaded.

Height without width or Width without height

[img id=1 width="300"] would be converted to <img src="..." alt="" width="300">

It will now render to <img src="..." alt="" width="300" height="auto">

ImageManipulation wants to auto-detect the size of the image if it's not provided. This could lead to a wrong aspect ratio.

So I updated the shortcode to explicitly set the missing value to auto. This should cause the browser to render the missing dimension to preserve the aspect ratio of the physical file.

It's worth pointing out that you can not lazy load the image in this situation because you can not predict the final size of the image.

Rendering missing image

ImageManipulation seem to assume that the image would always be there. It was making no effort to handle the scenario where the dimension of the image could not be determined.

I updated the logic so that if the size of an image can no be determined, ImageManipulation will now disable lazy loading on it.

In theory this could affect the scenario where you are trying to render a broken image.

maxime-rainville · 2024-05-02T11:09:36Z

src/ImageManipulation.php

@@ -1150,8 +1150,8 @@ protected function getDefaultAttributes(): array
        $attributes = [];
        if ($this->getIsImage()) {
            $attributes = [
-                'width' => $this->getWidth(),
-                'height' => $this->getHeight(),
+                'width' => $this->getWidth() ?: null,


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.

maxime-rainville · 2024-05-02T11:15:08Z

src/ImageManipulation.php

-            unset($attributes['loading']);
+        if (isset($attributes['loading'])) {
+            // Check if the dimensions for the image have been set
+            $dimensionsUnset = (


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.

src/Shortcodes/ImageShortcodeProvider.php

maxime-rainville · 2024-05-02T11:33:52Z

tests/php/Shortcodes/ImageShortcodeProviderTest.php

-        $this->assertEquals(
-            sprintf(
-                '<img src="%s" alt="">',
-                $image->Link()
-            ),
-            $parser->parse(sprintf('[image id=%d]', $image->ID))


Most of these test were throwing errors because ImageManipulation was returning attributes in a different order.

ImageManipulation does a few things a bit differently which also led to slightly different result in some context.

Because none of these test actually included messages, it was really hard to figure out what each test was meant to be doing.

So I basically ended rewriting all the test with more descriptive message and to uso a utility assert method.

maxime-rainville · 2024-05-02T11:36:05Z

tests/php/Shortcodes/ImageShortcodeProviderTest.php

-
-        $this->assertEquals(
-            sprintf(
-                '<img src="%s" alt="" width="50%%" height="auto">',


This confused me for a bit. The way to escape a % in sprintf is to double it.

maxime-rainville · 2024-05-02T11:50:13Z

tests/php/Shortcodes/ImageShortcodeProviderTest.php

+     * @param array $attrs Key pair values of attributes and values.
+     *                     If the value is FALSE we assume that it is not present
+     */
+    private function assertImageTag(array $attrs, string $shortcode, $message = ""): void


Instead of testing whether the HTML generated by a specific shortcode exactly matches the expected HTML, this method will parse the HTML and check the individual attributes.

This is better because we shouldn't care about the order of the attributes. What we care about is whether the attribute is there or not and whether it has the correct value.

maxime-rainville · 2024-05-02T11:54:44Z

tests/php/Shortcodes/ImageShortcodeProviderTest.php

+            $nodeAttr = $nodeAttrs->getNamedItem($key);
+            if ($value === false) {
+                if ($nodeAttr !== null) {
+                    $this->fail("$message\nImage should not contain attribute '$key'\n$tag");


Instead of doing straight assertion here, I decided to directly fail the test. This has a few advantages:

When I was asserting something about the node it would give a very unpleasant about what was failing which made it difficult to figure out what was actually wrong.

I'm building much more specific messages here:

I'm specifying what key is expected and what its expected value is.

I'm printing out the generated image tag.

I'm also including the overarching message coming from the original test case to better understand what's the actual scenario we are testing.

maxime-rainville · 2024-05-02T11:55:18Z

tests/php/Shortcodes/ImageShortcodeProviderTest.php

+                    $this->fail("$message\nImage attribute '$key' should have value '$value'\n$tag");
+                }
+            }
+            $this->assertTrue(true, 'Suppress warning about not having any assertion');


I need an actual assertion, otherwise PHPUnit thinks the test is broken.

…h generic Image generation

src/Shortcodes/ImageShortcodeProvider.php

maxime-rainville · 2024-05-13T09:24:33Z

src/Shortcodes/ImageShortcodeProvider.php

-     * @param int|null $height
-     * @return array
-     */
-    private static function updateLoadingValue(array $args, ?int $width, ?int $height): array


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

maxime-rainville commented Apr 29, 2024

View reviewed changes

maxime-rainville mentioned this pull request Apr 30, 2024

Create an advanced image rendering solution silverstripe/.github#230

Open

GuySartorelli reviewed Apr 30, 2024

View reviewed changes

src/Shortcodes/ImageShortcodeProvider.php Show resolved Hide resolved

maxime-rainville force-pushed the pulls/2/render-image-unify branch from 3c18bb9 to 186d529 Compare May 2, 2024 11:07

maxime-rainville commented May 2, 2024

View reviewed changes

maxime-rainville mentioned this pull request May 7, 2024

ENH Add ability to render any image using a default picture template and automatically render retina and webp version kinglozzer/silverstripe-picture#1

Open

ENH Align rendering of image genercated by ImageShortCodeProvider wit…

ac8a75d

…h generic Image generation

maxime-rainville force-pushed the pulls/2/render-image-unify branch from 9985383 to ac8a75d Compare May 13, 2024 09:22

maxime-rainville commented May 13, 2024

View reviewed changes

maxime-rainville marked this pull request as ready for review May 14, 2024 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

maxime-rainville commented Apr 29, 2024 •

edited

maxime-rainville left a comment

maxime-rainville Apr 29, 2024

maxime-rainville Apr 29, 2024

maxime-rainville Apr 29, 2024

maxime-rainville Apr 29, 2024

maxime-rainville left a comment

maxime-rainville May 2, 2024

maxime-rainville May 2, 2024

maxime-rainville May 2, 2024 •

edited

maxime-rainville May 2, 2024

maxime-rainville May 2, 2024

maxime-rainville May 2, 2024

maxime-rainville May 2, 2024

maxime-rainville May 13, 2024

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

Are you sure you want to change the base?

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

Conversation

maxime-rainville commented Apr 29, 2024 • edited

Parent issue

Depends on

maxime-rainville left a comment

Choose a reason for hiding this comment

Template tree

What I did

Things to change in CMS 6

Other

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxime-rainville left a comment

Choose a reason for hiding this comment

No size in short code

Height without width or Width without height

Rendering missing image

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxime-rainville May 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxime-rainville commented Apr 29, 2024 •

edited

maxime-rainville May 2, 2024 •

edited