-
Notifications
You must be signed in to change notification settings - Fork 991
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
Add support for lazy loading images #2211
Changes from 3 commits
06e9a40
6127764
dd45fb3
bd85ca4
e7ae995
b351471
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -252,6 +252,9 @@ pub fn markdown_to_html( | |
|
||
let mut stop_next_end_p = false; | ||
|
||
let lazy_async_image = context.config.markdown.lazy_async_image; | ||
let mut is_image_alt = false; // For alt text between Event::Start(Tag::Image(..)) and Event::End(Tag::Image(..)) | ||
|
||
let mut opts = Options::empty(); | ||
let mut has_summary = false; | ||
opts.insert(Options::ENABLE_TABLES); | ||
|
@@ -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 { | ||
let mut alt = String::new(); | ||
cmark::escape::escape_html(&mut alt, &text) | ||
.expect("Could not write to buffer"); | ||
|
||
events.push(Event::Html(alt.into())); | ||
} else if let Some(ref mut _code_block) = code_block { | ||
if contains_shortcode(text.as_ref()) { | ||
// mark the start of the code block events | ||
let stack_start = events.len(); | ||
|
@@ -387,13 +396,38 @@ pub fn markdown_to_html( | |
events.push(Event::Html("</code></pre>\n".into())); | ||
} | ||
Event::Start(Tag::Image(link_type, src, title)) => { | ||
if is_colocated_asset_link(&src) { | ||
let link = if is_colocated_asset_link(&src) { | ||
let link = format!("{}{}", context.current_page_permalink, &*src); | ||
events.push(Event::Start(Tag::Image(link_type, link.into(), title))); | ||
link.into() | ||
} else { | ||
events.push(Event::Start(Tag::Image(link_type, src, title))); | ||
} | ||
src | ||
}; | ||
|
||
events.push(if lazy_async_image { | ||
is_image_alt = true; | ||
|
||
let mut img_before_alt: String = "<img src=\"".to_string(); | ||
cmark::escape::escape_href(&mut img_before_alt, &link) | ||
.expect("Could not write to buffer"); | ||
if !title.is_empty() { | ||
img_before_alt | ||
.write_str("\" title=\"") | ||
.expect("Could not write to buffer"); | ||
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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not certain there will be an alt right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
Event::Html(img_before_alt.into()) | ||
} else { | ||
Event::Start(Tag::Image(link_type, link, title)) | ||
}); | ||
} | ||
Event::End(Tag::Image(..)) => events.push(if lazy_async_image { | ||
is_image_alt = false; | ||
Event::Html("\" loading=\"lazy\" decoding=\"async\" />".into()) | ||
} else { | ||
event | ||
}), | ||
Event::Start(Tag::Link(link_type, link, title)) if link.is_empty() => { | ||
error = Some(Error::msg("There is a link that is missing a URL")); | ||
events.push(Event::Start(Tag::Link(link_type, "#".into(), title))); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
mod common; | ||
use config::Config; | ||
|
||
#[test] | ||
fn can_transform_image() { | ||
// normal image | ||
let rendered = common::render("![haha](https://example.com/abc.jpg)").unwrap().body; | ||
assert_eq!(rendered, "<p><img src=\"https://example.com/abc.jpg\" alt=\"haha\" /></p>\n"); | ||
|
||
// alt text with style | ||
let rendered = common::render("![__ha__*ha*](https://example.com/abc.jpg)").unwrap().body; | ||
assert_eq!(rendered, "<p><img src=\"https://example.com/abc.jpg\" alt=\"haha\" /></p>\n"); | ||
|
||
// alt text with link | ||
let rendered = | ||
common::render("![ha[ha](https://example.com)](https://example.com/abc.jpg)").unwrap().body; | ||
assert_eq!(rendered, "<p><img src=\"https://example.com/abc.jpg\" alt=\"haha\" /></p>\n"); | ||
} | ||
|
||
#[test] | ||
fn can_add_lazy_loading_and_async_decoding() { | ||
// normal alt text | ||
let mut config = Config::default_for_test(); | ||
config.markdown.lazy_async_image = true; | ||
let rendered = | ||
common::render_with_config("![haha](https://example.com/abc.jpg)", config.clone()) | ||
.unwrap() | ||
.body; | ||
assert_eq!(rendered, "<p><img src=\"https://example.com/abc.jpg\" alt=\"haha\" loading=\"lazy\" decoding=\"async\" /></p>\n"); | ||
|
||
// Below is acceptable, but not recommended by CommonMark | ||
|
||
// alt text with style | ||
let rendered = | ||
common::render_with_config("![__ha__*ha*](https://example.com/abc.jpg)", config.clone()) | ||
.unwrap() | ||
.body; | ||
assert_eq!(rendered, "<p><img src=\"https://example.com/abc.jpg\" alt=\"<strong>ha</strong><em>ha</em>\" loading=\"lazy\" decoding=\"async\" /></p>\n"); | ||
|
||
// alt text with link | ||
let rendered = common::render_with_config( | ||
"![ha[ha](https://example.com)](https://example.com/abc.jpg)", | ||
config.clone(), | ||
) | ||
.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"); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a test with an empty alt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! I added it for both pulldown-cmark and mine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I forgot zola was already using insta for snapshot testing. Can you check eg There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries, I want to try it, so I’ve converted the test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not just push back the Text event in that case and let pulldown-cmark handle it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this addition is redundant. I have removed it.