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

react-markdown can't render MathJax CHTML #745

Closed
4 tasks done
alex-titarenko opened this issue Jun 13, 2023 · 12 comments
Closed
4 tasks done

react-markdown can't render MathJax CHTML #745

alex-titarenko opened this issue Jun 13, 2023 · 12 comments
Labels
👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on

Comments

@alex-titarenko
Copy link

Initial checklist

Affected packages and versions

react-markdown: 8.0.7

Link to runnable example

https://codesandbox.io/s/crimson-architecture-8qkw9w?file=/src/app.tsx

Steps to reproduce

See the CodeSandbox link above

Expected behavior

I'm expecting MathJax CHTML to be properly rendered with React-Markdown

Actual behavior

You see blank space when rendered math expression should be.

After the investigation, I see where the problem is:

MathJax (one of the most popular libraries out there to render Latex) when outputting the result to CHTML is using custom HTML elements (example: mjx-container, mjx-math, etc).
The problem happens in ast-to-react.js when it tries to translate AST properties into react properties. In the case of className or class it will always translate it to className, but React can't handle className property for non-standard HTML tags.
See GitHub issue here: facebook/react#4933
And instead, React will just render classname property instead of class.

image

What I'm expecting from React-Markdown library in case of custom HTML tags in AST (custom tags always should have at list one dash character, so should be easy to check, like mjx-container) it should rename className to class or if class property already there don't change it to className. After this change, it should properly render in React.

Runtime

Node v17

Package manager

npm 8

OS

Windows, macOS

Build and bundle tools

Create React App

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jun 13, 2023
@wooorm
Copy link
Member

wooorm commented Jun 13, 2023

  • Would be probably tough to solve in a good way: we don’t know if class or className was intended
  • I understand why React doesn’t touch className, given that a web component could indeed accept such a classname prop, but it does feel weird that React does className everywhere else but not here: custom elements too use the className DOM prop to represent the class HTML attribute
  • Seems like custom elements don’t have good React support anyway, and are going to change in the future: https://react.dev/reference/react-dom/components#custom-html-elements

@alex-titarenko
Copy link
Author

alex-titarenko commented Jun 13, 2023

@wooorm ok, if you don't want to automatically change className to class, you may skip automatical change class to className if it's a custom component. This will help as well since I'm in control when creating AST and can make sure to have class property, but currently React-Markdown will change it to className and will break everything.

@wooorm
Copy link
Member

wooorm commented Jun 13, 2023

just

yeah, no, that’s not how it works.

@alex-titarenko
Copy link
Author

Sorry, your answer was not clear for me, may you consider my latest proposal, does it sound reasonable?

@wooorm
Copy link
Member

wooorm commented Jun 13, 2023

Your idea doesn’t work. Take a look at the code. react-markdown isn’t responsible for doing that. The AST used here specifies that className must be used, so some other place also shouldn‘t do it. react doesn’t support web components well anyway and is going to change how they support web components so perhaps any work done here is useless?

@alex-titarenko
Copy link
Author

@wooorm React-Markdown is absolutely responsible for this.
Look at the code here:

const info = find(ctx.schema, prop)

If property name is class then info will get className.
What I was proposing is in case of custom html tag (easy to check, just look for dashes in the name) make a quick check and keep class property name.

React-Markdown library is all about rendering markdown in React. What I'm describing is absolutely valid scenario for React and React library supports my scenario. That is why I'm not quite sure I understand your opposition for the proposal. React supports this scenario, but you don't want even so library is all about rendering in React?

@wooorm
Copy link
Member

wooorm commented Jun 14, 2023

valid scenario

I have no problem with your problem.

React-Markdown is absolutely responsible for this.

I understand that the code looks like that, but no. The plugin rehype-mathjax chooses className. So addProperty is called with className. Your idea expects it to be called with class, which doesn’t happen.


My concerns are about more than 500 repos. An ecosystem with lots of plugins made by users doing arbitrary things. We at unified work on many projects that integrate with React. I am concerned with how to support custom elements everywhere. Correctly. In all cases.

@alex-titarenko
Copy link
Author

alex-titarenko commented Jun 14, 2023

I have my own implementation of rehype-mathjax where I make sure to have class property instead of className, but ReactMarkdown auto-converts class to className even so React does not support className for custom tag names.

The concern about backward compatibility is valid on your case, but I can't imagine a case where your current users expecting to have classname (with lowercase) property in rendered React when they provided class property name. That is unexpected behavior.

P.S. There is no problem with rehype-mathjax library since it's agnostic of rendering engine like React, but the problem with React-Markdown which produces invalid result in React.

@wooorm
Copy link
Member

wooorm commented Jun 14, 2023

I have my own implementation of rehype-mathjax

That breaks our specification of hast — you are not supposed to do that. react-markdown fixes your bug.

The concern about backward compatibility is valid on your case

I have no concern with backwards compat. I have concerns with solving this correctly. Your ideas do not solve this correctly. I am looking for ideas that solve this.

That is unexpected behavior.

Sure.

There is no problem with rehype-mathjax library since it's agnostic of rendering engine like React

Correct.

but the problem with React-Markdown which produces invalid result in React.

Please see my first comment: #745 (comment).
The problem is either the design of the entire ecosystem or React.
It’s not just react-markdown.

@wooorm
Copy link
Member

wooorm commented Oct 27, 2023

Let’s track this over at remarkjs/remark-math#85. As it has more to do with what that project generates!

@wooorm wooorm closed this as completed Oct 27, 2023
@wooorm wooorm added the 👀 no/external This makes more sense somewhere else label Oct 27, 2023
@github-actions

This comment has been minimized.

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Oct 27, 2023
@igloo1505
Copy link

Hey everybody... this is still an issue in 14+, but I thought I'd leave this here. It's definitely not a permanent solution, but it's a bandaid that works for those using Next and having this issue.

import { getRandomId } from '#/utils/ui'
import Script from 'next/script'
import React from 'react'


interface ImmediateNoteContentContainerProps {
    children: React.ReactNode
}



const ImmediateNoteContentContainer = ({ children }: ImmediateNoteContentContainerProps) => {
    const id = `nt-cnt-${getRandomId(8)}`
    return (
        <>
            <Script strategy="lazyOnload" id={`${id}-scrt`}>{`let em = document.getElementById("${id}");
let children = em.querySelectorAll("[classname]");
for (const k of children){
    let cl = k.getAttribute("classname")?.split(" ")
    if(cl && cl.length > 0){
        cl.forEach((c) => {
            k.classList.add(c)
        })
    }
}
`}</Script>
            <div
                className={"note-base-layout pt-8 space-y-2 md:space-y-3"}
                id={id}
            >
                {children}
            </div>
        </>
    )
}


ImmediateNoteContentContainer.displayName = "ImmediateNoteContentContainer"


export default ImmediateNoteContentContainer;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on
Development

No branches or pull requests

3 participants