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

Implement initial LaTeX support with Prosemirror-Math #36

Merged
merged 1 commit into from
Jul 26, 2022

Conversation

sawyerpollard
Copy link
Contributor

Hello!

This is a draft pull request for potentially adding math support to the Zotero Note Editor. This implementation uses Benjamin Bray's excellent prosemirror-math package.

prosemirror-math Considerations

  • Uses TypeScript and has an excellent codebase.
  • Modular and adheres to canonical Prosemirror plugin design.
  • Uses the hugely popular (and fastest) LaTeX rendering library KaTeX under the hood.
  • Still actively developed with 8 contributors, although last commit was a little under a year ago.

Progress

This package provides support for both inline math nodes and block math nodes, although I have only gotten block math nodes fully working.

Here are block math nodes:
Screen Shot 2022-06-28 at 12 46 05 AM
Screen Shot 2022-06-28 at 12 46 20 AM

The KaTeX and prosemirror-math CSS files are imported from node_modules/ into SCSS with the @import and tilde (~) syntax.

prosemirror-gapcursor was updated to meet prosemirror-math's dependency requirements.

The editor Schema was updated to accept math_display nodes.

Issues

Inline math nodes work great, except, after insertion, the cursor appears to move to the next line, although it continues in the correct place after the user types. This issue, not present in the prosemirror-math demo, is critical to fix. I have tried adjusting many options, tweaking CSS, and inspecting the DOM, but I am still working on a fix.

KaTeX requires 60 fonts, totaling 1.1 MB, to support the full LaTeX feature set. In order to prevent the initial load delay of these fonts, it seems that Webpack's url-loader can inline font URLs into CSS to lazily load them.

<math-display> HTML tags are used for the math nodes. "math-display" is considered an "unknown HTML tag" by the spec and is widely supported. The browser falls back to rendering it as an inline element.

Ultimately, I believe a more elegant solution is to use inline math nodes within paragraph nodes.

I think that I need some clarification on the different ways that people can export notes. How can they be exported as HTML in Zotero, for example? And can they be exported in other formats?

Additionally, rich text is correctly copied from the note editor into other word processors. A clipboardTextSerializer can be included to copy LaTeX code delimited by dollar signs for example.

Additional Thoughts

I would love to try out this current implementation in the Zotero client. Is there a workflow for compiling Zotero with a modified note-editor included? Is it just a matter of pointing the submodule in Zotero to a different commit.

Thank you for your work on this project!

@dstillman dstillman requested a review from mrtcode June 29, 2022 08:45
@dstillman
Copy link
Member

dstillman commented Jun 30, 2022

Great! I'll mostly let @mrtcode review this, but some initial notes:

  1. We'll want some more discoverable way to enable this than just an input mode. Maybe to start just an "Insert Math" menu item below "Insert Table"?

  2. Assuming we can fix the problems with inline math, you had mentioned the issue with $ triggering for phrases like "$300 or $400". I suggested we could consider using $$ for inline math too, since I assume we could differentiate between that and $$ on a new line. Would presumably need a custom input rule for that.

  3. We should allow Shift-Return to close the block node. (Right now it seems you have to use the arrow keys or click outside?) Not sure if we can customize this in our own code or if it requires patching prosemirror-math.

  4. We'll want to make sure we're exporting to Markdown with the $$ and $ delimiters, so that a Markdown processor that supports math would handle it. I assume this would need to go in markdown-translator-builder. (Don't need to worry about this at the moment. Just flagging it for later.)

A larger problem is figuring out what to do for HTML export, rich-text output (which is HTML copied to the clipboard as rich text), and word processor integration ("Add Note"). If we don't do anything, you'd get the raw equation input.

HTML

For HTML export, we could just document/provide the prosemirror-math/KaTeX JS and CSS that you'd need to add to your document to render these properly.

I know it's from prosemirror-math, but I'm not sure if using math-display (and math-inline) as the HTML serialization makes sense. It's technically OK — the hyphen makes it a custom element that just happens to not have any actual implementation behind it. But Zotero notes can be exported as HTML in various ways, and there's no guarantee that people will add CSS, let alone render with KaTeX — and if they don't, an unknown element would be treated as an inline element without any styling. That's not a huge problem if there are block elements above and below, but it could be a problem in some situations. It might make more sense to serialize block nodes as pres (to preserve spacing) with specific classes, and then customize the prosemirror-math JS and CSS accordingly. Or we could just decide that we don't care about the unrendered-LaTeX case and not go to the trouble of customizing this. Curious what @mrtcode thinks here.

Word processor output

The bigger problem, I think, is word processor integration. For this to be useful, you really should be able to use Add Note and see the rendered equations in your Word document, but it seems like KaTeX doesn't support SVG output (which we could convert to PNG after rendering to Canvas with something like canvg). MathJax does support SVG output, but prosemirror-math can't use MathJax: "In the future, prosemirror-math will also accept a custom callback that can be used to invoke alternative renderers like MathJax."

It's possible we could render in a hidden browser and take screenshots of each element, but that's getting pretty dicey. We should probably see if anyone has gotten prosemirror-math to work using MathJax, or look into what it would take to add support for that ourselves. If all fails, we could look into the hidden browser + screenshot option…

Rich text

Unclear. Could just leave it as the HTML export, maybe with the $ delimiters so that it's easier to render with something later. But ultimately I suspect many people would prefer the option to have inline images, same as with the word processor output.

@mrtcode
Copy link
Member

mrtcode commented Jun 30, 2022

So before moving further, I think the main question is if it's acceptable for us to load KaTeX libraries everywhere to show them rendered, or not. Because that determines if we have to render and store images and how much we need to modify prosemirror-math.

Notes can be exported/displayed in:

  • Zotero API. Third party apps can fetch note content. Raw TeX is fine.
  • Zotero item report. People want to print it, therefore we need to include KaTeX library to render TeX.
  • web-library, iOS app. We're already working on note-editor here.
  • Note import into a word processor. I think people probably want rendered TeX here, therefore we need already rendered images, because doing that at the import time would be too complicated.
  • Copying Rich Text to clipboard. It's another way to copy content into word processor, therefore the same rules apply.
  • Note export as raw HTML. Exporting HTML is a more technical thing, therefore it's fine to require additional libraries.

So it looks that rendered images are necessary for importing/copying into word processor, but there is one potential issue — rendered images might not be inserted into word processor in a way we expect. I'm mostly talking about inline math, while math blocks are probably fine. The result can be very chaotic. Which makes me question if we want rendered images at all. What people really want in word processors is probably active LaTeX objects. And I'm not sure if we can do that.


I would love to try out this current implementation in the Zotero client. Is there a workflow for compiling Zotero with a modified note-editor included? Is it just a matter of pointing the submodule in Zotero to a different commit.

Just fetch and checkout this branch into Zotero client note-editor submodule, and build Zotero client as usually.
Or you can do what I do when I want to quickly test note-editor:

# Assuming you are in note-editor dir
npm run build
rsync -a build/zotero/ .../zotero-client/build/resource/note-editor/

It builds and copies note-editor directly into Zotero client build directory and then you can just rebuild and run Zotero.

More observations:

  • KaTeX library should be added to note-editor package.json, otherwise it's not installed.
  • I think we can add support for inline math even if it doesn't work completely yet, and then we'll try to investigate the cause of the cursor issue.

@sawyerpollard
Copy link
Contributor Author

Thank you @mrtcode and @dstillman for your detailed feedback. I have started tackling a few of these issues today and will detail my findings, as well as some questions, tomorrow.

Thanks, Sawyer

@mrtcode
Copy link
Member

mrtcode commented Jul 1, 2022

Now when I think more about this, there isn't any good way to export inline math into word processors. Many small little images in separate lines won't be useful. I'm wondering what are people expectations here. Should we just do block level math? But that feels too limited.

@dstillman maybe we should ask in Zotero forums about this?

@dstillman
Copy link
Member

dstillman commented Jul 1, 2022

What's the issue? Just that the vertical alignment and line spacing are likely to be thrown off by inline images?

It looks like Word supports LaTeX math input, so I imagine we could just create those using the plugin when using Add Note. LibreOffice and Google Docs also support equations, but it doesn't seem like they support LaTeX input natively.

If we could actually create real equation objects in Word, and you can use external libraries for raw HTML export, I'd be fairly comfortable just outputting the raw LaTeX with delimiters the rest of the time. You'd either render it using some external tool or you'd manually recreate those in your document. Add Note is already a one-time operation, so if you have to go through and manually rewrite those using your word processor's preferred math syntax, so what?

@sawyerpollard
Copy link
Contributor Author

sawyerpollard commented Jul 2, 2022

@mrtcode @dstillman I will definitely do some more research into how Word supports LaTeX. I also want to see if exporting LaTeX objects as SVGs would work or if PNGs are better.

It seems that there will be times when people want to export the LaTeX code itself but other times when they want rendered images. Although, I think exporting images is a more common use case.

Does "Add Note" in Word use the DOMSerializer from buildClipboardSerializer() or buildToHTML() or neither?
Right now, buildClipboardSerializer() adds delimiters, but I haven't touched buildToHTML().

I've made some changes so far...

  • Include fonts as static assets
  • Add inline math
  • Create regex rules to pick between inserting inline math and block math
  • Create a block math menu item
  • Add pseudo-element to fix drag handle bug on block math
  • Serialize rich text copying (although not plain text) with double dollar signs
  • Use <pre> and <span> instead of <math-display> and <math-inline> tags. Side note: I am using the same toDom and parseDom rules as codeBlock in math_display. So, if HTML is imported, I'm not sure it can differentiate between them. I could add a class to avoid this.
  • Improve backspace by chaining in the prosemirror-math backspace command
  • Create text on the node to insert LaTex (probably should be internationalized)

If the build system were changed, the static fonts could also be transfered into the build from node_modules/ using Webpack.

Additionally, when you type $$my inline math here$$ the cursor appears to move to the next line. But if you type a character, it will insert directly after the inline math as expected and move the cursor.

@mrtcode
Copy link
Member

mrtcode commented Jul 2, 2022

Does "Add Note" in Word use the DOMSerializer from buildClipboardSerializer() or buildToHTML() or neither? Right now, buildClipboardSerializer() adds delimiters, but I haven't touched buildToHTML().

"Add Note" uses saved note HTML directly from note item. note-editor isn't involved here.

Anyway, good job, I'll review this soon.

@mrtcode
Copy link
Member

mrtcode commented Jul 4, 2022

@dstillman

  1. Assuming we can fix the problems with inline math, you had mentioned the issue with $ triggering for phrases like "$300 or $400". I suggested we could consider using $$ for inline math too, since I assume we could differentiate between that and $$ on a new line. Would presumably need a custom input rule for that.

I think the default math-prosemirror behaviour when you have to type the closing $ to trigger the input rule is better. Inline code and markdown italic/bold input rules already work like this. So I think we should just use $ for inline math.
Oh, I see the issue now. Although Github uses single $. So maybe we can do that as well.

  1. We'll want to make sure we're exporting to Markdown with the $$ and $ delimiters, so that a Markdown processor that supports math would handle it. I assume this would need to go in markdown-translator-builder. (Don't need to worry about this at the moment. Just flagging it for later.)

Are we sure that we don't want $ and $$ in note HTML? If we are storing TeX as preformatted text, because we expect to display it raw in various outputs, then maybe those delimiters also make sense? Although math-prosemirror modifications would probably be needed for that.

What's the issue? Just that the vertical alignment and line spacing are likely to be thrown off by inline images?

I doubt there exist a way to preserve images inline when copying HTML into word processors.

@dstillman
Copy link
Member

I think the default math-prosemirror behaviour when you have to type the closing $ to trigger the input rule is better.

The difference here is that there are regular sentences that would trigger this for people who don't know anything about math input, whereas people are by now fairly used to automatic Markdown in various programs (e.g., Slack). This probably wouldn't come up all that often, but it seems like it would be fairly confusing when it happened.

MathJax doesn't define these by default for this reason:

It also recognizes the TeX delimiters $$...$$ for displayed equations, but it does not define $...$ as in-line math delimiters. That is because dollar signs appear too often in non-mathematical settings, which could cause some text to be treated as mathematics unexpectedly.

(GitHub has a more technical audience that would be used to unexpected conversions when switching modes.)

But my suggestion of using $$ for inline as well is a bad idea, I think. Even if we handled that ourselves, it would make things more complicated when post-processing, since most of the auto-rendering tools just have you specify the delimiters declaratively. For HTML you could render specific elements (e.g., by class), but that wouldn't work for, say, Markdown.

So maybe we only support \(...\) for inline math, which it looks like KaTeX and MathJax support.

Are we sure that we don't want $ and $$ in note HTML? If we are storing TeX as preformatted text, because we expect to display it raw in various outputs, then maybe those delimiters also make sense?

You mean for raw HTML output? (I suggested we would include them for Rich Text.)

Yeah, I guess that makes sense. A little redundant with defined classes, but it's easier to use the auto-render extensions without needing to programmatically render specific elements.

I doubt there exist a way to preserve images inline when copying HTML into word processors.

The ones here seem to work, copied to Word: https://en.wikipedia.org/wiki/Wikipedia:Manual_of_Style/Images#MOS:BORDERIMAGE

content: "text*",
atom: true,
code: true,
toDOM: () => ["pre", 0],
Copy link
Member

Choose a reason for hiding this comment

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

We should use an additional class for math_display i.e. class="math". We should also move math_display before codeBlock, to avoid codeBlock grabbing all <pre> nodes.

inline: true,
atom: true,
toDOM: () => ["span", 0],
parseDOM: [{
Copy link
Member

Choose a reason for hiding this comment

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

class="math" is necessary here as well, because all <span> will be interpreted as math otherwise.

@@ -217,6 +217,7 @@ class Menu {
let insideBlockquote = nodeIsActive(state, schema.nodes.blockquote);
this.paragraph = this.buildBlock(schema.nodes.paragraph, {}, insideList || insideBlockquote);
this.codeBlock = this.buildBlock(schema.nodes.codeBlock);
this.math_display = this.buildBlock(schema.nodes.math_display);
Copy link
Member

Choose a reason for hiding this comment

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

Creating math_display doesn't properly focus the node and you have to click it again. Unclear why. Maybe we need to programmatically move selection position into the math node. I'll try to find later an example.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe insertMathCmd could help here.

@@ -14,11 +14,12 @@
"ff >= 60.9.0"
],
"dependencies": {
"@benrbray/prosemirror-math": "^0.2.2",
Copy link
Member

Choose a reason for hiding this comment

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

Add KaTeX package.

@mrtcode
Copy link
Member

mrtcode commented Jul 4, 2022

So maybe we only support \(...\) for inline math, which it looks like KaTeX and MathJax support.

Well, we could trigger input rule only if $f=x$ is surrounded with spaces. Github only needs one space after the closing $. And that basically solves the mentioned issues. And if not, you just hit Cmd-z (on note-editor). Additionally, it's going to be a bit weird if we allow to input with \(...\), but export to HTML/Markdown with $...$.

I doubt there exist a way to preserve images inline when copying HTML into word processors.

The ones here seem to work, copied to Word: https://en.wikipedia.org/wiki/Wikipedia:Manual_of_Style/Images#MOS:BORDERIMAGE

You are right, and that works in other word processors as well.
But again, do we want to generate image attachments for each math node or do we want to generate them in a hidden browser. Both options are very complicated actually.

@mrtcode
Copy link
Member

mrtcode commented Jul 4, 2022

@sawyerpollard I think woff2 is sufficient nowadays, and we can exclude woff and ttf fonts to save space.

@dstillman
Copy link
Member

But again, do we want to generate image attachments for each math node or do we want to generate them in a hidden browser. Both options are very complicated actually.

I would say neither, for now. We look into creating equations via the Word plugin for Add Note, and beyond that we add delimiters to output and people can post-process or manually replace in their word processor.

@dstillman
Copy link
Member

Well, we could trigger input rule only if $f=x$ is surrounded with spaces. Github only needs one space after the closing $. And that basically solves the mentioned issues.

I guess that's fine. And then if your post-processor converts other things, that's a problem for you to fix in your post-processor.

@mrtcode
Copy link
Member

mrtcode commented Jul 5, 2022

As we decided to serialize math nodes with surrounding $$ and $ delimiters, here is how we could do this:

The code below can be added to math_display node description:

math_display: {
  ...
  toDOM: node => {
  return ['pre', {
    class: 'math'
   }, '$$' + node.textContent + '$$']
  },
  parseDOM: [{
  tag: "pre.math",
  getContent(dom) {
    let text = dom.textContent;
    text = text.trim();
    if (text.slice(0, 2) === '$$' && text.slice(-2) === '$$') {
      text = text.slice(2, -2);
    }
    return Fragment.from(schema.text(text));
  }
  }]
  ...
}

Now when we'll do the above, we no longer need math_display in the clipboard serializer schema that was added in this PR, because the main schema will add $$ anyway.

But we need to include $$ for plain text serializer (that is triggered on copying and used for text/plain type), after this line:

else if (node.type === schema.nodes.math_display) {
	text += blockSeparator + '$$' + node.textContent + '$$' + blockSeparator;
	return false;
}

And then implement all the same code for math_inline and $.

Also do we want to use the additional paste rules that prosemirror-math provides? It seems it allows to paste math from Wikipedia.

@sawyerpollard
Copy link
Contributor Author

sawyerpollard commented Jul 8, 2022

Thank you for your detailed comments on the current implementation.
Here is a summary of the changes I have made so far.

  • Removed extra font files
  • Added math case in customTextBetween
  • Adjusted inline math input rule
  • Changed display math menu insertion logic
  • Changed toDOM and parseDOM logic (thanks mrtcode)
  • Removed math handling from clipboard serializer
  • Added CSS styles to remove display math outline

I changed the inline math input rule to $ but am still concerned about inadvertent triggering. It seems that GitHub Markdown doesn't have this worry, and they allow writing <span>$</span> to escape $, but this seems a little weird. Alternatively, we could trigger inline math input on $ and a space afterwards, which might be a good solution.

I spent a lot of time doing a deeper dive into the ProseMirror Selection, ResolvedPosition, Transaction, etc classes. Although the insertMathCmd function wasn't integrating how I'd like, I wrote some code to activate the math block on insertion. It's a little weird if one inserts a math block in the middle of a line, but it's not breaking. Also, if one selects text and then inserts a math block, the text disappears. I believe that with the setBlockType function, the text was actually being included in the LaTeX block. This is something I can investigate further if necessary.

Finally, I think it would be nice to include the provided Wikipedia parse rules. I created a PR on prosemirror-math to see if the author wants to export them, which would be nice. Alternatively, I can just copy the code over directly.

I have been playing with options in regards to "Add Note" integration but haven't finalized any of those implementations but will keep you updated.

Thanks,
Sawyer

}, '$$' + node.textContent + '$$'],
parseDOM: [{
tag: 'pre.math',
getContent(dom) {
Copy link
Member

Choose a reason for hiding this comment

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

I forgot to write it the last time, but it should be getContent(dom, schema) { as described here.

We should also add import { Fragment } from 'prosemirror-model'; at the top.

Comment on lines 446 to 447
if (text.slice(0, 2) === '$' && text.slice(-2) === '$') {
text = text.slice(2, -2);
Copy link
Member

Choose a reason for hiding this comment

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

Some tweaks here are necessary.

Comment on lines 593 to 594
else if (node.type === schema.nodes.math_inline) {
text += '$' + node.textContent + '$';
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

If I try to copy two paragraphs (one with inline math, and another with just text) and paste into a plain text editor, they appear in a single line.

Comment on lines 49 to 66

.math-node.empty-math .math-render::before {
content: "Click to insert LaTex ...";
color: black;
}

.math-node .math-render.parse-error::before {
content: "Cannot parse LaTeX.";
cursor: help;
}

.math-node .ProseMirror-focused {
outline: none;
}

math-display {
position: relative;

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can just move all the math related SCSS into components/core/math.scss. From both darwin.scss and generic.scss.

@mrtcode
Copy link
Member

mrtcode commented Jul 8, 2022

Alternatively, we could trigger inline math input on $ and a space afterwards, which might be a good solution.

Yes, we should do that.

I spent a lot of time doing a deeper dive into the ProseMirror Selection, ResolvedPosition, Transaction, etc classes.

ProseMirror learning curve is really steep. That's unrealistic to learning it in a short time span. If you can't find a solution on something in ProseMirror reference manual, their forums or other PM based projects in Github, just ask it here.

Finally, I think it would be nice to include the provided Wikipedia parse rules. I created a PR on prosemirror-math to see if the author wants to export them, which would be nice. Alternatively, I can just copy the code over directly.

I think we can just copy the code and keep a reference to it. It's already a plugin.

Any idea why after a math node there is a new line that can’t be deleted? This happens only on Chrome for me. Doesn't happen in their demo.

@sawyerpollard
Copy link
Contributor Author

Any idea why after a math node there is a new line that can’t be deleted? This happens only on Chrome for me. Doesn't happen in their demo.

Is this after an inline math node made with a single $? If you press space on this new line your cursor is brought back to the expected line. This appears to be very similar to what happens after pressing the Insert Citation button. I wonder if some code related to that is affecting math nodes.

Or is this a separate issue with math blocks made with $$ ?

@mrtcode
Copy link
Member

mrtcode commented Jul 10, 2022

Is this after an inline math node made with a single $? If you press space on this new line your cursor is brought back to the expected line. This appears to be very similar to what happens after pressing the Insert Citation button. I wonder if some code related to that is affecting math nodes.

Yes, it seems this is the same issue. I'll look into that.

Edit: This is fixed now.

Copy link
Member

@mrtcode mrtcode left a comment

Choose a reason for hiding this comment

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

We need to increment note-editor version here and later we'll need to do the same in Zotero client code, when we merge this PR. This is necessary to force older Zotero clients to open newer schema notes in read-only mode, to prevent removing unknown math nodes.

Next, @dstillman said he wants Shift-Enter to escape from math node, but prosemirror-math already has Ctrl-Enter for that, and even has Ctrl-Backspace to delete the whole math node, although Ctrl- possibly feels less common for macOS auditory. Let's see what he'll say.

@@ -57,7 +57,7 @@ export function buildInputRules({ enableSmartQuotes }) {
}),
linkInputRule(),
makeBlockMathInputRule(/^\$\$\s+$/, schema.nodes.math_display),
makeInlineMathInputRule(/\$(.+)\$/, schema.nodes.math_inline),
makeInlineMathInputRule(/\$\s((?:[^$\s]|[\s](?!\$))+)\s?\$$/, schema.nodes.math_inline),
Copy link
Member

Choose a reason for hiding this comment

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

I think it should work with $f(x)=5$ — a space after the closing $ and non obligatory spaces in between $. Well, not only a space after $ but also other symbols used in sentences like comma, dot, etc. Github is doing this. I still think this is a good idea, because unlike in Github you can easily revert this with Cmd-z. We are doing a similar thing for 1. input rule to start a list, and I don't see why $ can be more annoying than that.

@mrtcode
Copy link
Member

mrtcode commented Jul 12, 2022

It seems some code disappeared after the rebase. Fragment is no longer defined in schema/nodes.js. Anything else disappeared?

@sawyerpollard
Copy link
Contributor Author

@mrtcode Hi there,
I've been experimenting with the RegEx logic and handling edge cases like "$300 or $400" registering as math. I feel really good about the RegEx pattern I've built, which uses positive lookahead. Additionally, I had to alter the inline math input rule to prevent it from losing the last entered character. Sadly, I saw no way of handling the Enter key with the InputRule class, and this appears to be a quandary others have encountered. Therefore, I created a new command bound to the Enter key, as well as arrow keys. This handles inline math insertion in a way inspired by prosemirror-math and prosemirror-inputrules. Additionally, the function is fairly generic and could be used to insert other nodes on Enter.

I have been testing the Wikipedia math paste rules and so far it's not working that smoothly. It seems that Wikipedia is sometimes able to paste images of their math, however. I don't think this is a critical feature though.

There is a question of whether it makes sense to have inline math and block math. You should technically be able to do anything with inline math that you could with block math, because block math ignores new lines.

Excited to receive your feedback!

@mrtcode
Copy link
Member

mrtcode commented Jul 20, 2022

I get an error when typing $f(x)=1$ and pressing Enter.

Probably createInlineMath(/\$([^\$]*)\$$/, schema.nodes.math_inline) is interfering with customSplitBlock here.

index.es.js:863 Uncaught RangeError: Applying a mismatched transaction
    at EditorState.applyInner (index.es.js:863:33)
    at EditorState.applyTransaction (index.es.js:831:1)
    at EditorState.apply (index.es.js:807:1)
    at EditorView.dispatchTransaction (editor-core.js:253:31)
    at EditorView.dispatch (index.es.js:5278:29)
    at Array.customSplitBlock (commands.js:578:3)
    at index.es.js:663:8
    at Plugin.<anonymous> (index.es.js:85:1)
    at index.es.js:3563:59
    at EditorView.someProp (index.es.js:5121:1)

@mrtcode
Copy link
Member

mrtcode commented Jul 20, 2022

@mrtcode Hi there, I've been experimenting with the RegEx logic and handling edge cases like "$300 or $400" registering as math. I feel really good about the RegEx pattern I've built, which uses positive lookahead. Additionally, I had to alter the inline math input rule to prevent it from losing the last entered character. Sadly, I saw no way of handling the Enter key with the InputRule class, and this appears to be a quandary others have encountered. Therefore, I created a new command bound to the Enter key, as well as arrow keys. This handles inline math insertion in a way inspired by prosemirror-math and prosemirror-inputrules. Additionally, the function is fairly generic and could be used to insert other nodes on Enter.

Aside from the issue above, this works great.

I have been testing the Wikipedia math paste rules and so far it's not working that smoothly. It seems that Wikipedia is sometimes able to paste images of their math, however. I don't think this is a critical feature though.

Ok, we can ignore this feature.

There is a question of whether it makes sense to have inline math and block math. You should technically be able to do anything with inline math that you could with block math, because block math ignores new lines.

Yes, in theory you can do that, but:

  • You can still preserve new lines in your LaTeX, which is probably useful for larger formulas.
  • It's more consistent with other Markdown editors, to have math in a block.

Of course, if you are inserting a note into a word processor, the result is identical.

@sawyerpollard
Copy link
Contributor Author

@mrtcode Thanks for the feedback. I've corrected that error and refactored the code in the process. I hope it's not overstepping to create a single math.js file, which holds the majority of the logic.

@mrtcode
Copy link
Member

mrtcode commented Jul 21, 2022

Ok, I think now @dstillman can try this out for the final review.

And once we are ready, we should pick an appropriate time to release this, because we're increasing schema version number and therefore non-beta users will start seeing read-only notes.

@dstillman
Copy link
Member

And there's no way to only increase the schema version when using a feature that requires it, or something like that?

@mrtcode
Copy link
Member

mrtcode commented Jul 22, 2022

And there's no way to only increase the schema version when using a feature that requires it, or something like that?

That would introduce too much complexity. And we already need to be very careful with all schema changes because that can result in data loss.

@dstillman
Copy link
Member

Same problem with not allowing math in group notes in the beta, and using the earlier schema? I just don't think we can leave a beta up for more than a day or two if it's creating notes that say to upgrade, when there won't be upgrades available.

@mrtcode
Copy link
Member

mrtcode commented Jul 22, 2022

Same problem with not allowing math in group notes in the beta, and using the earlier schema?

I'm still afraid of the complexity in this sensitive part. One option would be to bundle and use an older version of note-editor, but, again, doesn't feel worth to do this.

We don't do schema updates often, so probably better to test this properly between us.

Another possibility is to silently introduce only the schema change. Push to production. And then introduce the feature for the beta.

@dstillman
Copy link
Member

Another possibility is to silently introduce only the schema change. Push to production. And then introduce the feature for the beta.

That sounds good.

@mrtcode
Copy link
Member

mrtcode commented Jul 22, 2022

Another possibility is to silently introduce only the schema change. Push to production. And then introduce the feature for the beta.

That sounds good.

All right, so I guess @sawyerpollard should extract schema/nodes.js changes into a separate PR, increase schema version in schema/index.js, and then we will need to rebase and resolve the conflict in this PR.

@mrtcode
Copy link
Member

mrtcode commented Jul 22, 2022

@dstillman, as @sawyerpollard suggested, do you think there would be any sense to only leave inline math?

There is a question of whether it makes sense to have inline math and block math. You should technically be able to do anything with inline math that you could with block math, because block math ignores new lines.

@dstillman
Copy link
Member

I think we want block. It seems to be what everyone does.

@sawyerpollard
Copy link
Contributor Author

I'm feeling good about the state of the PR now. The only thing that slightly bugs me is that I need to append \n\n to the math_display serializer in markdown-serializer.js for it format well. It's probably fine though.

@mrtcode mrtcode merged commit 2276cb4 into zotero:master Jul 26, 2022
@mrtcode
Copy link
Member

mrtcode commented Jul 26, 2022

Ok, merged, this is going to be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants