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

Fence KaTeX math expressions #1576

Merged
merged 30 commits into from
Feb 19, 2022
Merged

Fence KaTeX math expressions #1576

merged 30 commits into from
Feb 19, 2022

Conversation

fgtham
Copy link
Contributor

@fgtham fgtham commented Feb 3, 2022

Flexmark would parse and corrupt the contents of KaTeX math expressions.
This commit circumvents the problem by fencing the math expressions.

Fixes #1389 and #1393.

Could use more testing.

Flexmark would parse and corrupt the contents of KaTeX math expressions.
This commit circumvents the problem by fencing the math expressions.

Fixes gsantner#1389 and gsantner#1393.
@fgtham
Copy link
Contributor Author

fgtham commented Feb 3, 2022

Ok, this doesn't work.

@gsantner
Copy link
Owner

gsantner commented Feb 3, 2022 via email

@fgtham
Copy link
Contributor Author

fgtham commented Feb 3, 2022

It does work, at least outside of code blocks. So all the examples in the two issues render correctly. The problem is that $$...$$ and $...$ are also replaced inside of code blocks, which gives "interesting" results pretty quickly.

Florian Tham added 5 commits February 4, 2022 07:51
This reverts commit b620407.

Works fine outside of code blocks, but not inside. Needs another
approach instead.
Stripped down and modified flexmark-ext-gitlab to support KaTeX.

Works: math inline mode. Doesn't work: math display mode.
@fgtham
Copy link
Contributor Author

fgtham commented Feb 4, 2022

Messing with regular expressions just doesn't work here. I think the correct approach is to implement a flexmark extension that handles math environments where it is appropriate, and just leaves them alone where it is not (e.g. within code blocks).

Fortunately the GitLab Flavoured Markdown extension already has great support for both inline and display math modes. I stripped the extension of everything that's not needed for math rendering and adopted it to support KaTex inline $...$ and display $$...$$ modes.

It looks good on my notes so far.

What doesn't work:

  • Display math mode in the markor-markdown-reference.md Template is wrapped in a <div>...</div> block and not rendered. After removing the surrounding divs it works. Apparently <div>$$ ... $$</div> breaks. I don't know why this is, but I also don't understand why the divs are there in the first place.
  • KaTeX needs custom JS to render math elements. I tried to add the script below to JS_KATEX in format.markdown.MarkdownTextConverter.java, but could not get it to work. I then injected the following snippet from the Markor app settings (View mode / inject head):
<script>
(function () {
    document.addEventListener("DOMContentLoaded", function () {
        var mathElems = document.getElementsByClassName("katex");
        var elems = [];
        for (const i in mathElems) {
            if (mathElems.hasOwnProperty(i)) elems.push(mathElems[i]);
        }

        elems.forEach(elem => {
            katex.render(elem.textContent, elem, { throwOnError: false, displayMode: elem.nodeName !== 'SPAN', });
        });
    });
 })();
</script>

The math font size needed a bit adjusting as well:

<style>
.katex { font-size: 105%; }
</style>

@gsantner
Copy link
Owner

gsantner commented Feb 4, 2022

yeah it's a bit tricky, as both KaTex (at view/javascript-time) and Markdown parser (at generator-time) are greedy for everything they can get, and mess up the other one :D.

Generally don't want to mess too much with the (KaTex) dependencies, it took quite some effort to make users somewhat happy. If now the one or other big chunk isn't working anymore guess bugreports are flooded again ... instead of these 2 small ones :D.

Creating a new Flexmark extension is something I currently wouldn't prefer todo. We are kind of forced to an older version currently, and upgrading the version requires much rewrites then again. This is due the flexmark version Markor currently uses .. works fine with older Java and thus Android version. Newer versions require newer Java version, which would mean I need to drop 🔪 many users and their devices .

Fortunately the GitLab Flavoured Markdown extension already has great support for both inline and display math modes. I stripped the extension of everything that's not needed for math rendering and adopted it to support KaTex inline $...$ and display $$...$$ modes.

Is there something in the way when directly enabling the GitLab extension, or that part? I think copy-pasting some sections of it might just make it hard to maintain, and gets left behind when flexmark themself might have things fixed already


Preferably, if turned out we kinda fork the flexmark katex code ... please combine it to one file, with nested/inner classes:

public class FlexmarkKatexSupport {
   class ....BlockQuote { }
   class ....InLineMath { }
   class ....Extension { }
  ............
}

@fgtham
Copy link
Contributor Author

fgtham commented Feb 4, 2022

Generally don't want to mess too much with the (KaTex) dependencies, it took quite some effort to make users somewhat happy. If now the one or other big chunk isn't working anymore guess bugreports are flooded again ... instead of these 2 small ones :D.

What about adding a toggle in the settings to switch between current and new math mode handling? If something breaks, users have an option to switch back.

Is there something in the way when directly enabling the GitLab extension, or that part?

GitLab Flavoured Markdown does not support $...$ and $$...$$ for inline and display math modes. Instead, inline mode is triggered with $`...`$, display math mode is a code block of type "math":

```math
\ldots
```

Switching from current to GFM would have each user adjust their documents.

Unfortunately the math environments are hardcoded in flexmark-ext-gitlab, that's why I duplicated, stripped down and modified the extension.

I think copy-pasting some sections of it might just make it hard to maintain, and gets left behind when flexmark themself might have things fixed already

Yes, it's a pain. If not right now, then later.

Preferably, if turned out we kinda fork the flexmark katex code ... please combine it to one file, with nested/inner classes:

I can have a go at it. If you think it's not worth it, or too much hassle in the long run to maintain a separate KaTeX extension, I can fully understand it. So, what do you think? Should I continue?

Florian Tham and others added 10 commits February 5, 2022 17:31
I could not figure out how to make the JS work from `JS_KATEX`, so I
placed it in a separate file instead.
In addition to `$...$` and `$$...$$`, math rendering can now be enabled
with `\(...\)` for inline and `\[...¸]` for display mode.
This reverts commit 9ece193.

Does not work reliably.
@fgtham
Copy link
Contributor Author

fgtham commented Feb 9, 2022

The problematic examples render correctly. I did lots of testing and couldn't find any problems.

It fixes an additional issue with dollar signs in text: currently, both "$foo $bar" and "\$foo \$bar" are interpreted as math mode in Markor. Instead, with this PR applied, the escaped dollar signs prevent math mode.

The issue with <div>$ ... $</div> not working can be resolved by adding empty lines between the divs and math mode environment. Or by removing the divs altogether. The Markor markdown reference probably should be updated accordingly.

I tried to consolidate the source files into one using nested classes and failed. I need to do some reading first, as I have no prior knowledge of Java. It will take time.

I could ask the flexmark devs if they would accept a math extension. Maybe that would ease things in the long run.

@gsantner
Copy link
Owner

gsantner commented Feb 9, 2022

The divs are from me. They were there to mark an explicit section where markdown parser should not do its job, so at JS "runtime" the katex-js can pick up the text.

@gsantner
Copy link
Owner

I zipped the files a bit up .. to just one file

@gsantner gsantner force-pushed the master branch 5 times, most recently from c4f83d5 to 74c7bb8 Compare February 10, 2022 21:33
@gsantner gsantner linked an issue Feb 12, 2022 that may be closed by this pull request
Copy link
Owner

@gsantner gsantner left a comment

Choose a reason for hiding this comment

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

do you feel this is safe enough to give to users? so it not breaks existing rules? 😄

otherwise guess it's gonna be in soon

@fgtham
Copy link
Contributor Author

fgtham commented Feb 15, 2022

It really does make a difference.

Current Markor v2.8.5 from f-droid

Screenshot_20220215-082547_Markor

Mordor without CSS_KATEX

Screenshot_20220215-082507_Mordor

Mordor with CSS_KATEX

Screenshot_20220215-082637_Mordor

I don't know where the change in font size comes from, but the huge math equations are quite ugly. Of course I can remove CSS_KATEX if you prefer.

Other than that, I habe been using this PR for several days now and didn't notice any obvious problems (apart from <div>$$ ... $$</div> not working). It should be safe to roll out. If you're not confident, I could mark it as experimental, so it will only be used if the user ticked the Experimental Functions checkbox?

@gsantner
Copy link
Owner

gsantner commented Feb 15, 2022

what about font-size: inherit? like .katex, .katex > * { font-size: inherit; }. I mean, the direction of neutralization of implied font-sizes, instead of adding another one

@fgtham
Copy link
Contributor Author

fgtham commented Feb 16, 2022

You are right, inherit makes sense.

@gsantner
Copy link
Owner

gsantner commented Feb 16, 2022

ok so it works? good, then it scales also fine with global font-size rules

something else remaining?

@fgtham
Copy link
Contributor Author

fgtham commented Feb 16, 2022

Seems good from my side, nothing remains so far.

Copy link
Owner

@gsantner gsantner left a comment

Choose a reason for hiding this comment

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

screenshot-2022-02-19__14-15-52

@gsantner gsantner added this to the 2.9 milestone Feb 19, 2022
@gsantner gsantner merged commit 5ac79f9 into gsantner:master Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants