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

fix(auto-render): concatenate content of successive text nodes #3422

Merged
merged 12 commits into from Aug 29, 2022

Conversation

marcoesters
Copy link
Contributor

Webkit-based browsers like Chrome, Edge, or Safari do not parse very large math expressions (limit appears to be around 48k characters) because they split large text nodes into smaller ones so that auto-render misses delimiters. This PR fixes this by concatenating all successive text nodes.

Tested on:

  • Windows 10 with Chrome 96.0.4664.45, Edge 95.0.1020.53, and Firefox 93.0
  • MacOS 12.0.1 with Chrome 96.0.4664.55 and Safari 15.1
  • MacOS 10.13.6 with Safari 11.1.2
  • Android 12 with Chrome 95.0.4638.74 Firefox 94.1.2

What is the previous behavior before this PR?

Webkit-based browsers (Chrome, Edge, Safari) split large text nodes into smaller ones. This can cause delimiters to be in separate child nodes, which the auto-render parser will miss.

What is the new behavior after this PR?

All successive text modes will be concatenated into one node and the concatenated textContent will be used to render math.

Concatenate successive text nodes to prevent auto-render from skipping
math input when browsers split text nodes with long textContent.
Copy link
Member

@edemaine edemaine left a comment

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/67738121/in-what-cases-do-browsers-create-multiple-adjacent-text-nodes seems to confirm this issue; do you have other documentation, or an example where this is failing?

Overall, this seems like a good idea, though potentially slow/inefficient, better than not working. But we need to not touch such text if it has no math expressions in it; see comment.

contrib/auto-render/auto-render.js Outdated Show resolved Hide resolved
marcoesters added 3 commits November 19, 2021 15:51
Only remove siblings when math expressions were found to prevent removal
of nodes that do not contain math.
Copy link
Member

@edemaine edemaine left a comment

Choose a reason for hiding this comment

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

Thanks -- this is looking good! Would you be able to add tests to contrib/auto-render/test/auto-render-spec.js? If not, I can try.

@codecov
Copy link

codecov bot commented Nov 21, 2021

Codecov Report

Merging #3422 (d5ae92c) into main (99be728) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3422      +/-   ##
==========================================
+ Coverage   92.98%   93.00%   +0.02%     
==========================================
  Files          90       90              
  Lines        6739     6750      +11     
  Branches     1568     1570       +2     
==========================================
+ Hits         6266     6278      +12     
+ Misses        435      434       -1     
  Partials       38       38              
Impacted Files Coverage Δ
contrib/auto-render/auto-render.js 79.03% <100.00%> (+6.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99be728...d5ae92c. Read the comment docs.

@marcoesters
Copy link
Contributor Author

Thanks -- this is looking good! Would you be able to add tests to contrib/auto-render/test/auto-render-spec.js? If not, I can try.

I'm working on a test with two strings long enough to break on Chrome (including and not including math to render). I'm not familiar enough with jest to know if it's possible to reproduce the issue in a unit test since the problem is browser-specific.

My tests pass with the current version of katex as well, so I could provide a much smaller string if a browser-specific test is not possible.

@edemaine
Copy link
Member

I'm not sure what DOM the tests use, but it might be virtual. I had in mind tests where you manually make multiple adjacent text nodes, not necessarily large, and make sure the right substitutions happen (or don't happen).

@marcoesters
Copy link
Contributor Author

That makes a lot more sense than what I was trying to do.

The current tests should do the trick. The math parser fails in the current KaTeX version but passes with my mods.

contrib/auto-render/auto-render.js Outdated Show resolved Hide resolved
@ylemkimon ylemkimon enabled auto-merge (squash) August 29, 2022 21:46
@ylemkimon
Copy link
Member

Thank you for the PR!

@ylemkimon ylemkimon merged commit 4d3fdd8 into KaTeX:main Aug 29, 2022
KaTeX-bot added a commit that referenced this pull request Aug 29, 2022
## [0.16.2](v0.16.1...v0.16.2) (2022-08-29)

### Bug Fixes

* **auto-render:** concatenate content of successive text nodes ([#3422](#3422)) ([4d3fdd8](4d3fdd8))
* Implement \pmb via CSS text-shadow ([#3505](#3505)) ([176552a](176552a))
@KaTeX-bot
Copy link
Member

🎉 This PR is included in version 0.16.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

4 participants