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

markup/goldmark: Render strikethrough and emojis in TOC entries #11784

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

lyind
Copy link
Contributor

@lyind lyind commented Dec 5, 2023

Configure the TOC (table of contents, toc.go) goldmark renderer instance to always enable the Strikethrough extension.

This allows handling ast.KindStrikethrough within an AST node avoiding a crash.

Fixes #7169
Fixes #8087
Fixes #11783

@jmooring
Copy link
Member

jmooring commented Dec 6, 2023

@lyind There's a failing test, and you need to add conditions to test the issues that this PR addresses. Also, please capitalize the first letter of the commit message (after the xxx/xxx: part) per the commit message guidelines.

@lyind lyind force-pushed the fix-toc-strikethrough-crash branch from cb25276 to 49a8c90 Compare December 7, 2023 12:03
@lyind lyind changed the title markup/goldmark: add strikethrough extension to toc renderer markup/goldmark: Add strikethrough extension to toc renderer Dec 7, 2023
@lyind
Copy link
Contributor Author

lyind commented Jan 26, 2024

It seems like the rendering test fails, because the output is more correct now:

...
        line diff (-got +want):
            []string{
            	"<nav id=\"TableOfContents\">\n",
            	"  <ul>\n",
            	"    <li><a href=\"#a--b--c--d\">A &lt; B &amp; C &gt; D</a></li>\n",
            	strings.Join({
            		`    <li><a href="#a--b--c--d-divfoodiv">A &lt; B &amp; C &gt; D `,
            		"<",
          - 		"!-- raw HTML omitted -->foo<!-- raw HTML omitted --",
          + 		"div>foo</div",
...

But I might confuse "got" with "want", so will have another look at the code of that test case.

@github-actions github-actions bot removed the Stale label Jan 27, 2024
@lyind lyind force-pushed the fix-toc-strikethrough-crash branch from 49a8c90 to c6cc57b Compare February 9, 2024 11:58
@lyind
Copy link
Contributor Author

lyind commented Feb 9, 2024

@jmooring I applied the following changes to fix the test and be more aligned in general:

  • Also pass rendering options (HardWraps, XHTML, Unsafe) to the TOC rendering extension
  • Last test in TestEscapeToc() (unsafe renderer): expect <div> element to correctly be excluded from the TOC output

Is my assumption correct that the test expectations were slightly incorrect, in regard to the <div> and possibly similar block elements to be passed through?

@lyind lyind force-pushed the fix-toc-strikethrough-crash branch from 635fb4d to a6d8a5c Compare February 9, 2024 12:17
@jmooring
Copy link
Member

jmooring commented Feb 9, 2024

Also pass rendering options (HardWraps, XHTML, Unsafe) to the TOC rendering extension

Why do we need to do this? The xhtml and unsafe options are already honored when rendering a TOC entry, and you would never have a newline within a heading so the hardWraps setting is irrelevant.

Although I have not looked into the details, I find it curious that the strikethrough and emoji extensions are not used when enabled, but the typographer extension is used when enabled. Can you explain why that is?

Here's simple but (I think) comprehensive test:

git clone --single-branch -b hugo-github-issue-11784 https://github.com/jmooring/hugo-testing hugo-github-issue-11784
cd hugo-github-issue-11784
hugo server

Neither the strikethrough nor the emoji extensions are used when rendering the TOC entry. I think we broke the emoji part when we started using yuin's extension (#11593).

@jmooring
Copy link
Member

jmooring commented Feb 9, 2024

Let's re-task this PR to fix these open issues:

I've revised the test cases, replacing toc_test.go with toc_integration_test.go. The last two cases are commented out; they fail due to the first two open issues above.

https://github.com/gohugoio/hugo/blob/master/markup/goldmark/toc_integration_test.go#L255-L265

Without digging into the details, I am surprised that the typographer extension affects TOC entries, but the emoji and strikethrough extensions do not.

@jmooring jmooring changed the title markup/goldmark: Add strikethrough extension to toc renderer markup/goldmark: Render strikethrough and emojis in TOC entries Feb 10, 2024
@lyind
Copy link
Contributor Author

lyind commented Feb 20, 2024

The xhtml and unsafe options are already honored when rendering a TOC entry, and you would never have a newline within a heading so the hardWraps setting is irrelevant.

True. Just passed the options down out of "stubborn completeness", will change that again.

Configure the TOC (TableOfContents, toc.go) goldmark renderer to always
enable the Strikethrough and Emoji extensions. This allows handling
ast.KindStrikethrough and ast.KindEmoji AST nodes when rendering the TOC.

Fixes #7169
Fixes #11783
Fixes #12022
@lyind lyind force-pushed the fix-toc-strikethrough-crash branch from a6d8a5c to bf22c3e Compare February 20, 2024 15:56
@lyind
Copy link
Contributor Author

lyind commented Feb 20, 2024

@jmooring
Copy link
Member

Functionally this is great! Thank you.

The next step is a review by bep.

@jmooring jmooring requested a review from bep February 21, 2024 03:56
@bep bep merged commit 134e7d1 into gohugoio:master Mar 7, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants