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.

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 · 2024-05-28T08:16:18Z

The builds are red because they depend on the framework PR. I've created a branch of the sink to test this https://github.com/creative-commoners/recipe-kitchen-sink/actions/runs/9265818257

emteknetnz

Still have failing unit tests are merging the framework PR and rerunning

emteknetnz

Cannot use width/height="auto", it's invalid html

You can test that here https://www.freeformatter.com/html-validator.html

<!DOCTYPE html>
<head>
<meta charset="utf-8"/>
<title>test</title>
</head>
<body>
<img src="http://example.com/abc.jpg" alt="" width="600" height="auto" />
</body>

Bad value “auto” for attribute “height” on element “img”: Expected a digit but saw “a” instead. From line 7, column 1 to line 7, column 73 Code Extract: d>↩<body><img src="http://example.com/abc.jpg" alt="" width="600" height="auto" />↩</bo

maxime-rainville · 2024-06-05T09:48:29Z

Not sure what I was thinking there. I added a bit of logic to suppress the width/height attribute instead of outputting auto.

emteknetnz

Has the following integration failures in CI:

phpunit framework

SilverStripe\Forms\Tests\HTMLEditor\HTMLEditorFieldTest::testReadonlyField

endtoend asset-admin

vendor/silverstripe/asset-admin/tests/behat/features/insert-an-image.feature:108

endtoend cms

vendor/silverstripe/cms/tests/behat/features/edit-a-page.feature:80

maxime-rainville · 2024-06-19T09:13:41Z

silverstripe/silverstripe-framework#11286 fixes the framework issue.
silverstripe/silverstripe-cms#2971 fixes the cms issue.
My implementation was double escaping entities. I fixed it and added a unit test to cover this scenario 9d529c7

emteknetnz

Merge conflict

…h generic Image generation

…h generic Image generation (silverstripe#596)

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

Closed

GuySartorelli reviewed Apr 30, 2024

View reviewed changes

src/Shortcodes/ImageShortcodeProvider.php Outdated 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

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

emteknetnz requested changes May 29, 2024

View reviewed changes

maxime-rainville force-pushed the pulls/2/render-image-unify branch from ac8a75d to 4edc6d7 Compare June 1, 2024 09:48

emteknetnz requested changes Jun 4, 2024

View reviewed changes

maxime-rainville force-pushed the pulls/2/render-image-unify branch 2 times, most recently from 1e88f22 to ebbb8bf Compare June 5, 2024 09:46

maxime-rainville force-pushed the pulls/2/render-image-unify branch from ebbb8bf to 6089cea Compare June 5, 2024 10:04

emteknetnz requested changes Jun 12, 2024

View reviewed changes

maxime-rainville mentioned this pull request Jun 19, 2024

MNT ALter order of expected attribute on image test silverstripe/silverstripe-framework#11286

Merged

maxime-rainville force-pushed the pulls/2/render-image-unify branch 2 times, most recently from 9d529c7 to 4380406 Compare June 20, 2024 00:01

emteknetnz requested changes Jun 20, 2024

View reviewed changes

ENH Align rendering of image genercated by ImageShortCodeProvider wit…

cd54355

…h generic Image generation

maxime-rainville force-pushed the pulls/2/render-image-unify branch from 4380406 to cd54355 Compare June 26, 2024 10:08

emteknetnz approved these changes Jun 30, 2024

View reviewed changes

emteknetnz merged commit cf787b0 into silverstripe:2 Jun 30, 2024
9 checks passed

emteknetnz deleted the pulls/2/render-image-unify branch June 30, 2024 22:36

GuySartorelli pushed a commit to creative-commoners/silverstripe-assets that referenced this pull request Jul 17, 2024

ENH Align rendering of image genercated by ImageShortCodeProvider wit…

85402b1

…h generic Image generation (silverstripe#596)

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

Loading

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

Loading

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

maxime-rainville commented May 28, 2024

emteknetnz left a comment

emteknetnz left a comment •

edited

Loading

maxime-rainville commented Jun 5, 2024

emteknetnz left a comment

maxime-rainville commented Jun 19, 2024 •

edited

Loading

emteknetnz left a comment

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

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

Conversation

maxime-rainville commented Apr 29, 2024 • edited Loading

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 Loading

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 May 28, 2024

emteknetnz left a comment

Choose a reason for hiding this comment

emteknetnz left a comment • edited Loading

Choose a reason for hiding this comment

maxime-rainville commented Jun 5, 2024

emteknetnz left a comment

Choose a reason for hiding this comment

maxime-rainville commented Jun 19, 2024 • edited Loading

emteknetnz left a comment

Choose a reason for hiding this comment

maxime-rainville commented Apr 29, 2024 •

edited

Loading

maxime-rainville May 2, 2024 •

edited

Loading

emteknetnz left a comment •

edited

Loading

maxime-rainville commented Jun 19, 2024 •

edited

Loading