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

Accommodate recent change to Goldmark's strikethrough extension #12597

Closed
jmooring opened this issue Jun 14, 2024 · 10 comments · Fixed by #12604
Closed

Accommodate recent change to Goldmark's strikethrough extension #12597

jmooring opened this issue Jun 14, 2024 · 10 comments · Fixed by #12604

Comments

@jmooring
Copy link
Member

jmooring commented Jun 14, 2024

yuin/goldmark#455

Prior to this change in v1.7.2, Goldmark's strikethrough extension was triggered by wrapping text within a pair of double-tildes:

~~deleted~~

This behavior was compatible with Hugo's extras extension, so you could use strikethrough and subscripts at the same time:

~~deleted~~
H~2~O

Goldmark's strikethrough extension is now triggered by either single- or double-tildes:

~~deleted~~
~deleted~

That means you can no longer use use strikethrough and subscripts at the same time.

Possible solutions:

  1. Look at prioritization, though initial review by @bowman2001 was not promising
  2. Implement a double-tilde strikethrough feature in the extras extension, and use this feature instead of Goldmark's strikethrough extension when subscripts are enabled (or some similar mechanism)

Regardless of the subscript conflict, the Goldmark change is a breaking change.

@bowman2001
Copy link

About the suggested prioritization: There already was a unit test case to check the compatibility and coexistence of subscript and strike-through for an expression with mixed usage. And I just added another one with a simple strike-through. For now, they are both fine.

When I raise the priority of subscript above strike-through (locally), they both fail (as expected).

@jmooring
Copy link
Member Author

For now, they are both fine.

Please clarify what this means.

@bowman2001
Copy link

With the current prioritization, both tests pass.

They will fail if the priority of subscript is raised above strike-through. (Tested locally)

@bowman2001
Copy link

bowman2001 commented Jun 14, 2024

As far as I can see, the only option is our own strike-through implementation. I think, this extension should be able to process both kinds of usages of the ~ like Goldmark handles both types of emphasis. This would be the most efficient way.

And because you already implemented a detailed configuration structure, it would also be possible to accommodate different needs with an additional config parameter.
This one could decide, if the new strike-through works as in the GFM spec or treats single ~ as subscripts.

The default configuration could be compatible with GFM and something like subscript: true could enable the current behavior when both extensions are enabled.

@jmooring
Copy link
Member Author

jmooring commented Jun 14, 2024

Putting aside the configuration settings for a moment, this is how I think it should work:

  1. With a fresh Hugo install, both single- and double-tilde strikethrough should be enabled. This is consistent with the current default and provides GFM compatibility.
  2. As soon as you enable subscripts, anything wrapped in single-tildes is a subscript

Assuming we have to add strikethrough to the extras extension, the default config should be:

[markup.goldmark.extensions.extras.strikethrough]
enable = true

Which leaves us with this to figure out:

[markup.goldmark.extensions]
strikethrough = true/false

I suspect that very few users have disabled strikethrough... I can't remember seeing it. So the easiest approach would be to completely remove the Goldmark strikethrough extension/config, and note the breaking change in the release notes, applicable only if they currently have strikethrough disabled.

@bowman2001
Copy link

I could implement a new strike-through extension which includes the subscript without the configuration switch for a start. I am currently occupied and won't be able to finish this before the middle of the next month. But this is important to me and I would be happy to contribute.

@jmooring
Copy link
Member Author

jmooring commented Jun 15, 2024

Just in case there are other near-term upstream bug fixes, I'd rather not wait, so perhaps we can do this in phases.

Phase 1

Remove the Goldmark "strikethrough" extension and create an extras "delete" (different name to avoid confusion) extension that only handles double-tildes. This would allow us to bump to Goldmark v1.7.2 without losing current capabilities.

There's some added complexity here because we also use the Goldmark "strikethrough" extension when rendering TOC entries, but TOC entries don't currently honor any of the extras features anyway. So maybe this is a separate issue.

We need to make sure we handle this test case too:

https://github.com/yuin/goldmark/blob/master/extension/_test/strikethrough.txt#L25-L30

Phase 2

Make the "delete" extension handle single-tildes if "subscript" is not enabled. This would give us full GFM compatibility, but in my view there's no rush for this. The primary objective should be to maintain current behavior when we bump to Goldmark v1.7.2.

@bowman2001
Copy link

bowman2001 commented Jun 16, 2024

I already have an implementation of the new delete extension which mimics the behavior of strike-through in the last version 1.7.1 of Goldmark.

But the new test case in 1.7.2 does complicate things a little, because there is a corresponding change in the original strike-through parser to exclude the rendering of more than 2 markup characters like ~~~. Because we use the same parser for all extra inline tags, this change would also affect edge cases like +++, ===, ++++, and ====.

I think, that we can adopt this change, because I can't imagine that anyone wants to use +++insertion++ to produce <ins>+insertion</ins> or similar cases with ===. And nested inline tags like <del><del>doubly deleted</del></del> which are possible at the moment don't make any sense.

There is one reasonable edge case which can not be implemented with the current approach in phase 1: To use something like ~~CO~2~~~ to produce <del>CO<sub>2</sub></del> is reasonable but won't work.

What's your opinion on this?

@jmooring
Copy link
Member Author

jmooring commented Jun 16, 2024

First, thank you very much for the rapid response.

Second, please submit a PR to the hugo-goldmark-extensions repository. Although I think I understand what you've described above, I'd like to take it for a spin to be sure.

Third, how we handle this on the Hugo side is probably the tricky part, and I've been giving this more thought. If we assume that only 5% of all sites enable the subscript feature, it would probably be best for the other 95% to continue to use the official Goldmark strikethrough extension instead of the extras extension delete feature. Options:

1) Handle this with documentation. For example:

If you enable the extras extension "subscript" feature you must disable the Goldmark strikethrough extension. To support both subscript and strikethrough, enable the extras extension "delete" feature.

2) Make the above happen automatically. While this sounds good, I'm afraid we'll end up with some spaghetti code. The additional complexity may not make sense given my 5% usage estimate above.

I'm inclined to handle this with documentation, at least for now. We can always revisit this in the future.

@bep
Copy link
Member

bep commented Jun 16, 2024

I'm inclined to handle this with documentation, at least for now. We can always revisit this in the future.

Agree.

jmooring added a commit to jmooring/hugo that referenced this issue Jun 17, 2024
When enabling the Hugo Goldmark Extras "subscript" extension, if you
want to render subscripts and strikethrough text concurrently, you must:

1. Disable the Goldmark "strikethrough" extension
2. Enable the Hugo Goldmark Extras "delete" extension

Closes gohugoio#12597
jmooring added a commit to jmooring/hugo that referenced this issue Jun 17, 2024
With Goldmark v1.7.1 and earlier, the Goldmark "strikethrough" extension was
triggered by wrapping text within a pair of double-tilde characters. With
Goldmark v1.7.2 and later, to provide full GFM compatibility, the Goldmark
"strikethrough" extension is triggered by wrapping text within a pair of
single- or double-tilde characters.

This change created a conflict with the Hugo Goldmark Extras "subscript"
extension.

When enabling the Hugo Goldmark Extras "subscript" extension, if you
want to render subscript and strikethrough text concurrently, you must:

1. Disable the Goldmark "strikethrough" extension
2. Enable the Hugo Goldmark Extras "delete" extension

Closes gohugoio#12597
jmooring added a commit to jmooring/hugo that referenced this issue Jun 17, 2024
With Goldmark v1.7.1 and earlier, the Goldmark "strikethrough" extension was
triggered by wrapping text within a pair of double-tilde characters. With
Goldmark v1.7.2 and later, to provide full GFM compatibility, the Goldmark
"strikethrough" extension is triggered by wrapping text within a pair of
single- or double-tilde characters.

This change created a conflict with the Hugo Goldmark Extras "subscript"
extension.

When enabling the Hugo Goldmark Extras "subscript" extension, if you
want to render subscript and strikethrough text concurrently, you must:

1. Disable the Goldmark "strikethrough" extension
2. Enable the Hugo Goldmark Extras "delete" extension

Closes gohugoio#12597
@bep bep closed this as completed in 8efc75b Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants