italic text usings underscore vs. attrs also containing an underscore

[schneekluth]

I have discovered a bug that occurs when italic text formatting using underscores _ is used after using attrs that also contain underscores, e.g. {target="_blank"}.

A adtailed issue was raised in the Zensical Studio repository: zensical/studio#110

A technical explanation of the bug was provided by Squidfunk in this comment: zensical/studio#110 (comment)

[facelessuser]

While the way Python Markdown works, and how it runs inline processors, may contribute to this issue, a rewrite of emphasis that abandons the approach of applying emphasis with multiple passes of different regex will greatly improve performance and general behavior.

I've been able to verify this issue would be resolved with this currently unreleased approach.

I'm working a new approach over in these 3rd party extensions: https://github.com/facelessuser/pymdown-extensions, specifically BetterEm. I plan to compare this new approach against Python Markdown's internal tests, and if it makes sense, may propose it as a potential change here, assuming it would be accepted. If not, it would remain as a third party solution.

As a note, this solution would require a recent fix in Python Markdown: 152a16f. We take advantage of this commits behavior and performance.

[facelessuser]

I've been working on this prototype for about a week, and a quick test shows only two test differences in the Python Markdown test suite.

The top result is the new result:

Case 1

This appears to fix a case where we do not match PHP Markdown Extra. The fact that it was added as a question indicates there was some doubt here.

THIS___SHOULD___STAY?

- <p>THIS___SHOULD___STAY?</p>
+ <p>THIS<strong><em>SHOULD</em></strong>STAY?</p>

Case 2

This may be the most contested case, as it does deviate from PHP Markdown. This would now match GitHub/CommonMark.

This is text __bold_italic bold___ with more text

- <p>This is text <strong>bold_italic bold</strong>_ with more text</p>
+ <p>This is text __bold_italic bold___ with more text</p>

If Case 2 is unacceptable, then I'd likely just keep this over in Pymdown Extensions. But if the Case 2 deviation is OK, we would have an approach that eliminates some pathological slowness and has better nested results in general.

If there is strong reservations, then I won't bother with a PR, but if I hear nothing, I'll likely put one up for review once I feel I have everything ironed out. If it gets rejected, it gets rejected 🙂.

[waylan]

The second case is related to Python-Markdown's non-standard behavior of ignoring middle_word_emphasis. While I don't have strong feelings about it, we have been advertising that as a feature since before I became involved in the project. Therefore, we are likely to have a large number of users who have been creating documents for years without escaping underscores in the middle of words. If we change that behavior now we could break many thousands of documents. Although I find it odd that your new implementation only encounters this issue when combined with a triple underscore. Without seeing your new implementation, I wonder why you can't apply the same logic as you do for the other cases.

Note that we only ignore single underscores which do not either start or end with a word boundary. Double and triple underscores are assumed to only exist as Markdown markup and are expected to always be parsed as Markdown. That explains the question mark on the first test case. Presumably that should be parsed as Markdown and it was surprising when it was found to not be. I think we can call the current behavior a bug. Although, apparently at some time in the past it was determined to be a not-very-serious bug. Unfortunately, I have no recollection of how we got here in this case.

[waylan]

So I just took a second look at the output of the second test and I missed that the middle single underscore is ignored in both scenarios. I would say that the document author clearly made an error and each output highlights that in a slightly different way. Either would be an acceptable solution. Although, the less surprising output would match the behavior of __foo___, which explains the existing output. But, if there are significant gains to the had by switching to the alternate output, I'm okay with that.

[facelessuser]

Yep, the second case's change is due to how the algorithm works. It looks at delimiters ___, __, _ and pairs and consumes them. In this case it found a valid __ to start, and resolved it with___ having a leftover of _. It avoids middle word _s. It seems most modern-day parsers behave this way. I lean on existing implementation behavior to understand how to handle ambiguous delimiters (ones that can look like an opening or a closing).

We should be able to process complex nesting, and if we have to terminate early due to being unable to close all the opening spans, we render the ones we could complete, and we begin parsing later without reparsing what we gave up on.

If it was just a different, opinionated approach, I'd probably just keep it third-party, but since it actually speeds up some pathological scenarios while generally. It also allows us to stop fiddling with complex regex patterns for odd cases here and there.

[facelessuser]

While we've done a lot of testing, the plan is to release the new implementation first in Pymdown Extensions. We will shoulder the initial risk of any potential uncaught issues first. Once we feel good about the implementation, we will push to get it in here.

In order for us to release this new implementation, we require a new release of Python Markdown that contains: 152a16f. Here's hoping to see that hit sooner rather than later. It makes a lot of the time savings we employ possible.