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

fix: shortcode in inline elements #2601

Closed
wants to merge 2 commits into from
Closed

fix: shortcode in inline elements #2601

wants to merge 2 commits into from

Conversation

amtanq
Copy link
Contributor

@amtanq amtanq commented Aug 7, 2024

Fixes Issue: #2565

BTS

Step 1: Minimum reproducible example (to break shortcodes)

<a href="{{ basic() }}">{{ basic() }}</a>

Step 2: Investigating code which references @@ZOLA_SC_PLACEHOLDER@@

  • markdown.rs (renders markdown, point of interest)
  • mod.rs
  • parser.rs

Step 3: Investigating shortcode.render call in markdown.rs

  • for samples which are rendered correctly the shortcode.render was invoked for each shortcode
  • for samples which render @@ZOLA_SC_PLACEHOLDER@@, shortcode.render was NOT invoked
  • source of shortcodes: html_shortcodes list (which had the shortcode references even if they were not rendered)

Step 4: Investigating missing shortcodes

  • !$range.contains(&shortcode.span.start) skips shortcodes which are not in the range
  • Understanding range and shortcode.span
  • Understanding parsed content (cmark markdown parser)
    • iterator(event, range) where range -> range in the markdown source

Step 5: Investigating ranges

content: <pre>•  <a href="@@ZOLA_SC_PLACEHOLDER@@">@@ZOLA_SC_PLACEHOLDER@@</a>•</pre>••<a href="@@ZOLA_SC_PLACEHOLDER@@">@@ZOLA_SC_PLACEHOLDER@@</a>
note: • represents newline (replaced for easy offset calculation)
parse_tree:
  Start(HtmlBlock) 0..77
  Html(Borrowed(<pre>•)) 0..6
    Html(Borrowed(  <a href="@@ZOLA_SC_PLACEHOLDER@@">@@ZOLA_SC_PLACEHOLDER@@</a>•)) 6..70
  Html(Borrowed(</pre>•)) 70..77
  End(HtmlBlock) 0..77
  Start(Paragraph) 78..140
    InlineHtml(Borrowed(<a href="@@ZOLA_SC_PLACEHOLDER@@">)) 78..112
    Text(Borrowed(@@ZOLA_SC_PLACEHOLDER@@)) 112..135
    InlineHtml(Borrowed(</a>)) 135..139
  End(Paragraph) 78..140
shortcodes:
  17..40
  42..65
  87..110
  112..135
logs:
  Start(HtmlBlock) 0..77
  Html(Borrowed(<pre>•)) 0..6
  Html(Borrowed(  <a href="@@ZOLA_SC_PLACEHOLDER@@">@@ZOLA_SC_PLACEHOLDER@@</a>•)) 6..70
    html_render_shortcodes: Borrowed(  <a href="@@ZOLA_SC_PLACEHOLDER@@">@@ZOLA_SC_PLACEHOLDER@@</a>•) 6..70
      render_compare 17..40 6..70
      render_compare 42..65 40..70
      render_compare 87..110 65..70 [break]
      html: [Start(HtmlBlock), Html(Borrowed(<pre>•)), Html(Boxed(  <a href=")), Html(Boxed(<h4>Basic shortcode</h4>•)), Html(Boxed(">)), Html(Boxed(<h4>Basic shortcode</h4>•)), Html(Boxed(</a>•))]
  Html(Borrowed(</pre>•)) 70..77
  End(HtmlBlock) 0..77
  Start(Paragraph) 78..140
  InlineHtml(Borrowed(<a href="@@ZOLA_SC_PLACEHOLDER@@">)) 78..112
  ^^^^^^^^^^ ISSUE: render_shortcodes not called (issue propogates to next offsets)
  Text(Borrowed(@@ZOLA_SC_PLACEHOLDER@@)) 112..135
    text_render_shortcodes: Borrowed(@@ZOLA_SC_PLACEHOLDER@@) 112..135
      render_compare 87..110 112..135 [break]
      html: [Start(HtmlBlock), Html(Borrowed(<pre>•)), Html(Boxed(  <a href=")), Html(Boxed(<h4>Basic shortcode</h4>•)), Html(Boxed(">)), Html(Boxed(<h4>Basic shortcode</h4>•)), Html(Boxed(</a>•)), Html(Borrowed(</pre>•)), End(HtmlBlock), Start(Paragraph), InlineHtml(Borrowed(<a href="@@ZOLA_SC_PLACEHOLDER@@">)), Text(Boxed(@@ZOLA_SC_PLACEHOLDER@@))]
  InlineHtml(Borrowed(</a>)) 135..139
  End(Paragraph) 78..140

Step 6: Adding a test for the issue

  • can_use_shortcodes_in_inline_elements in components/markdown/tests/shortcodes.rs
failed test

Step 7: Fixing the issue by accounting for InlineHtml

logs:
  Start(HtmlBlock) 0..77
  Html(Borrowed(<pre>•)) 0..6
  Html(Borrowed(  <a href="@@ZOLA_SC_PLACEHOLDER@@">@@ZOLA_SC_PLACEHOLDER@@</a>•)) 6..70
    html_render_shortcodes: Borrowed(  <a href="@@ZOLA_SC_PLACEHOLDER@@">@@ZOLA_SC_PLACEHOLDER@@</a>•) 6..70
      render_compare 17..40 6..70
      render_compare 42..65 40..70
      render_compare 87..110 65..70 [break]
      html: [Start(HtmlBlock), Html(Borrowed(<pre>•)), Html(Boxed(  <a href=")), Html(Boxed(<h4>Basic shortcode</h4>•)), Html(Boxed(">)), Html(Boxed(<h4>Basic shortcode</h4>•)), Html(Boxed(</a>•))]
  Html(Borrowed(</pre>•)) 70..77
  End(HtmlBlock) 0..77
  Start(Paragraph) 78..140
  InlineHtml(Borrowed(<a href="@@ZOLA_SC_PLACEHOLDER@@">)) 78..112
    html_render_shortcodes: Borrowed(<a href="@@ZOLA_SC_PLACEHOLDER@@">) 78..112
      render_compare 87..110 78..112
      render_compare 112..135 110..112 [break]
      html: [Start(HtmlBlock), Html(Borrowed(<pre>•)), Html(Boxed(  <a href=")), Html(Boxed(<h4>Basic shortcode</h4>•)), Html(Boxed(">)), Html(Boxed(<h4>Basic shortcode</h4>•)), Html(Boxed(</a>•)), Html(Borrowed(</pre>•)), End(HtmlBlock), Start(Paragraph), Html(Boxed(<a href=")), Html(Boxed(<h4>Basic shortcode</h4>•)), Html(Boxed(">))]
  Text(Borrowed(@@ZOLA_SC_PLACEHOLDER@@)) 112..135
    text_render_shortcodes: Borrowed(@@ZOLA_SC_PLACEHOLDER@@) 112..135
      render_compare 112..135 112..135
      html: [Start(HtmlBlock), Html(Borrowed(<pre>•)), Html(Boxed(  <a href=")), Html(Boxed(<h4>Basic shortcode</h4>•)), Html(Boxed(">)), Html(Boxed(<h4>Basic shortcode</h4>•)), Html(Boxed(</a>•)), Html(Borrowed(</pre>•)), End(HtmlBlock), Start(Paragraph), Html(Boxed(<a href=")), Html(Boxed(<h4>Basic shortcode</h4>•)), Html(Boxed(">)), Html(Boxed(<h4>Basic shortcode</h4>•))]
  InlineHtml(Borrowed(</a>)) 135..139
  End(Paragraph) 78..140
passes test

@amtanq amtanq changed the title fix: shortcode in inline elements draft: shortcode in inline elements Aug 7, 2024
@amtanq amtanq changed the base branch from master to next August 7, 2024 09:13
@amtanq amtanq changed the title draft: shortcode in inline elements fix: shortcode in inline elements Aug 7, 2024
@@ -679,7 +679,7 @@ pub fn markdown_to_html(
event
});
}
Event::Html(text) => {
Event::Html(text) | Event::InlineHtml(text) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sadly it's not that easy. There is #2581 which solves both (I did miss it solved the shortcode issue sorry :() but I still don't know whether the continue reading in inline html makes sense.
To keep the fix simple, it should probably be a separate branch that doesn't contain the continue reading stuff and we can decide later if we want the full #2581

Copy link
Contributor

Choose a reason for hiding this comment

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

You're more the welcome to copy my code and remove the continue-reading bit, FWIW. (Aimed at both amtanq/Keats if either of you want to do it first.)

Otherwise, I'll get around to it at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since you have more context on it, would appreciate if you can pick this whenever you get time. LMK if you're okay with it, else would be happy to continue myself too!

@amtanq
Copy link
Contributor Author

amtanq commented Aug 10, 2024

Closing in favor of #2606
Thanks @clarfonthey

@amtanq amtanq closed this Aug 10, 2024
@amtanq amtanq deleted the fix-shortcode-in-inline-elements branch August 10, 2024 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants