Skip to content
This repository has been archived by the owner on Jan 31, 2024. It is now read-only.

KaTeX support #174

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

KaTeX support #174

wants to merge 1 commit into from

Conversation

MoshiBar
Copy link

katex support, for math typesetting

What

add katex support to allow for proper displaying of math expressions

Why

so that math expressions can be properly shown (feature parity with firefish)

Additional info (optional)

example:
before change
image
after change
image

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

@ShittyKopper
Copy link
Contributor

using composition api / setup sugar / <script setup> seems to be the approach used by misskey (https://github.com/transfem-org/Sharkey/blob/develop/CONTRIBUTING.md#vue), though it doesn't seem hard to rewrite this to work with that

my main concern is the use of v-html, this could end up with an xss if used improperly. i wonder if there's a pre-existing katex component for vue we could use?

@ShittyKopper
Copy link
Contributor

so i couldn't find any prebuilt component (the ones i did find seem to be for older versions of vue and/or unmaintained)

i did stumble upon https://katex.org/docs/security though, which seems to imply katex seems to care about security so as long as we keep it up to date it should be Relatively Fine™

i would recommend explicitly setting trust to false though. it's the default sure but being explicit won't hurt

throwOnError: false,
} as any);
return this.block
? `<div style="text-align:center">${katexString}</div>`
Copy link
Contributor

Choose a reason for hiding this comment

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

i feel like this would be better as part of the vue template instead of being inline html, with text-align:center applied via <style> instead of being inline

@Mar0xy
Copy link
Contributor

Mar0xy commented Nov 27, 2023

I agree with each statement made by kopper here.

Also the fact that it's split into two components seems redundant when one component is literally just there for one small task that can all be accomplished in one single file.

@CPlusPatch
Copy link
Contributor

Is there anything stopping us from just running HTML sanitization on all KaTeX embeds? I dont think performance would be an issue, since I doubt math expressions are that common

throwOnError: false,
} as any);
return this.block
? `<div style="text-align:center">${katexString}</div>`
Copy link
Contributor

Choose a reason for hiding this comment

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

Some kind of simple HTML sanitization could be made here, depending on what KaTeX renders to you might be able to just drop all non-math HTML tags and their attributes

@ShittyKopper
Copy link
Contributor

Also the fact that it's split into two components seems redundant when one component is literally just there for one small task that can all be accomplished in one single file.

depending on how heavy katex is as a library it does make sense to load it asynchronously for tree shaking purposes (as i don't imagine it being something that'll be used often enough)

it may be possible to use a single component and import it asynchronously there, though i'm not sure how difficult that would be

@Mar0xy
Copy link
Contributor

Mar0xy commented Nov 28, 2023

though i'm not sure how difficult that would be

You just simply make an async function a lot of stuff misskey does is already done with async in mind.

codingneko pushed a commit to codingneko/Sharkey that referenced this pull request Dec 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants