Add support for lazy loading images #2211

sinofp · 2023-05-04T22:16:35Z

IMPORTANT: Please do not create a Pull Request adding a new feature without discussing it first.

The place to discuss new features is the forum: https://zola.discourse.group/
If you want to add a new feature, please open a thread there first in the feature requests section.

Sanity check:

Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

(Delete or ignore this section for documentation changes)

Are you doing the PR on the next branch?

If the change is a new feature or adding to/changing an existing one:

Have you created/updated the relevant documentation page(s)?

Recently I learnt loading="lazy" decoding="async" and wanted to try them in my Zola-powered blog, then I found issue #2055 requesting loading="lazy".
Given that issue has been marked by “help wanted”, I decided to give it a try, hence this PR.

This PR adds an option lazy_async_image to set loading="lazy" decoding="async" for images.
The loading="lazy" part is discussed in the issue, I can delete decoding="async" part if that’s too far.

The alt text in pulldown-cmark is enclosed within Start(Image) and End(Image), and can contain styles (pulldown-cmark/pulldown-cmark#394 (comment)).
I think most people will only use plain text in alt text so I didn’t bother cleaning the styles because “Having something simple and easy to use for 90% of the usecases is more interesting than covering 100% usecases after sacrificing simplicity.”
That said, there might be people want to use styles, links or even inline HTML for alt text, so the documentation includes a note on the limitation of the lazy_async_image option.

I added a small test to demonstrate the difference between the output of pulldown-cmark and mine. I also tested the program against my blog.

In theory, they can make the page load faster and show content faster. There’s one problem: CommonMark allows arbitrary inline elements in alt text. If I want to get the correct alt text, I need to match every inline event. I think most people will only use plain text, so I only match Event::Text.

This is the reason why we should use plain text when lazy_async_image is enabled.

Keats

That's really cool, I didn't know about decoding="async"

Keats · 2023-05-05T20:39:53Z

components/markdown/src/markdown.rs

@@ -326,7 +329,13 @@ pub fn markdown_to_html(
        for (event, mut range) in Parser::new_ext(content, opts).into_offset_iter() {
            match event {
                Event::Text(text) => {
-                    if let Some(ref mut _code_block) = code_block {
+                    if lazy_async_image && is_image_alt {


Can we not just push back the Text event in that case and let pulldown-cmark handle it?

Indeed, this addition is redundant. I have removed it.

Keats · 2023-05-05T20:48:03Z

components/markdown/src/markdown.rs

+                            cmark::escape::escape_href(&mut img_before_alt, &title)
+                                .expect("Could not write to buffer");
+                        }
+                        img_before_alt.write_str("\" alt=\"").expect("Could not write to buffer");


It's not certain there will be an alt right?

Yes, the user might not input any alt text, but we still need to emit an empty alt attribute for the screen reader to ignore the picture: https://www.boia.org/blog/images-that-dont-need-alternative-text-still-need-alt-attributes

Keats · 2023-05-05T20:48:59Z

components/markdown/tests/img.rs

+    .unwrap()
+    .body;
+    assert_eq!(rendered, "<p><img src=\"https://example.com/abc.jpg\" alt=\"ha<a href=\"https://example.com\">ha</a>\" loading=\"lazy\" decoding=\"async\" /></p>\n");
+}


Can you add a test with an empty alt?

Done! I added it for both pulldown-cmark and mine.

Sorry I forgot zola was already using insta for snapshot testing. Can you check eg can_render_basic_markdown to see how it works? Otherwise I'll do it myself later

No worries, I want to try it, so I’ve converted the test.

I totaly forgot one can leave the alt text empty. I thought I need to eliminate the alt attribute in that case, but actually empty alt text is better than not having an alt attribute at all: https://www.w3.org/TR/WCAG20-TECHS/H67.html https://www.boia.org/blog/images-that-dont-need-alternative-text-still-need-alt-attributes Thus I will leave the empty alt text. Another test is added to ensure alt text is properly escaped. I will remove the redundant escaping code after this commit.

After removing the if-else inside the arm of Event::Text(text), the alt text is still escaped. Indeed they are redundant.

Keats · 2023-05-06T14:37:34Z

components/markdown/tests/img.rs

+    .unwrap()
+    .body;
+    assert_eq!(rendered, "<p><img src=\"https://example.com/abc.jpg\" alt=\"ha<a href=\"https://example.com\">ha</a>\" loading=\"lazy\" decoding=\"async\" /></p>\n");
+}


Sorry I forgot zola was already using insta for snapshot testing. Can you check eg can_render_basic_markdown to see how it works? Otherwise I'll do it myself later

`cargo insta review` looks cool! I wanted to dedup the cases variable, but my Rust skill is not good enough to declare a global vector.

Keats · 2023-05-06T21:04:42Z

Thanks!

* Add optional decoding="async" loading="lazy" for img In theory, they can make the page load faster and show content faster. There’s one problem: CommonMark allows arbitrary inline elements in alt text. If I want to get the correct alt text, I need to match every inline event. I think most people will only use plain text, so I only match Event::Text. * Add very basic test for img This is the reason why we should use plain text when lazy_async_image is enabled. * Explain lazy_async_image in documentation * Add test with empty alt and special characters I totaly forgot one can leave the alt text empty. I thought I need to eliminate the alt attribute in that case, but actually empty alt text is better than not having an alt attribute at all: https://www.w3.org/TR/WCAG20-TECHS/H67.html https://www.boia.org/blog/images-that-dont-need-alternative-text-still-need-alt-attributes Thus I will leave the empty alt text. Another test is added to ensure alt text is properly escaped. I will remove the redundant escaping code after this commit. * Remove manually escaping alt text After removing the if-else inside the arm of Event::Text(text), the alt text is still escaped. Indeed they are redundant. * Use insta for snapshot testing `cargo insta review` looks cool! I wanted to dedup the cases variable, but my Rust skill is not good enough to declare a global vector.

sinofp added 3 commits May 4, 2023 22:04

Add very basic test for img

6127764

This is the reason why we should use plain text when lazy_async_image is enabled.

Explain lazy_async_image in documentation

dd45fb3

Keats requested changes May 5, 2023

View reviewed changes

sinofp added 2 commits May 6, 2023 13:20

Remove manually escaping alt text

e7ae995

After removing the if-else inside the arm of Event::Text(text), the alt text is still escaped. Indeed they are redundant.

Keats requested changes May 6, 2023

View reviewed changes

Use insta for snapshot testing

b351471

`cargo insta review` looks cool! I wanted to dedup the cases variable, but my Rust skill is not good enough to declare a global vector.

Keats approved these changes May 6, 2023

View reviewed changes

Keats merged commit b5a90db into getzola:next May 6, 2023

ISSOtm mentioned this pull request Oct 23, 2023

Lazy-load images in longer pages ISSOtm/issotm.github.io#27

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for lazy loading images #2211

Add support for lazy loading images #2211

sinofp commented May 4, 2023

Keats left a comment

Keats May 5, 2023

sinofp May 6, 2023

Keats May 5, 2023

sinofp May 6, 2023

Keats May 5, 2023

sinofp May 6, 2023

Keats May 6, 2023

sinofp May 6, 2023

Keats May 6, 2023

Keats commented May 6, 2023

Add support for lazy loading images #2211

Add support for lazy loading images #2211

Conversation

sinofp commented May 4, 2023

Code changes

Keats left a 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

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

Keats commented May 6, 2023