fix(copy-tex): Use JS (instead of CSS) to select full equation, solving display glitches #3586
Conversation
fonsp
changed the title
fonsp
changed the title
Co-authored-by: Erik Demaine <edemaine@mit.edu>
BREAKING CHANGE: copy-tex extension no longer has (or requires) a CSS file.
There was a problem hiding this comment.
Sorry for the delay. I cleaned up your code (in particular, lint was failing, and we weren't using Flow, but are now).
In experimenting with use of this rewrite, I found the undesirable behavior that, if your selection starts within one equation and ends within a different equation, we get ugly text again. So I rewrote the code to separately extend both the start and end of the range, instead of just looking at the common ancestor. This should restore the intended behavior.
Could you take a look and make sure you're good with my changes, before I merge and release?
|
Looks great, thanks for the help! |
Codecov Report
@@ Coverage Diff @@
## main #3586 +/- ##
==========================================
- Coverage 93.53% 93.39% -0.15%
==========================================
Files 90 90
Lines 6669 6679 +10
Branches 1549 1554 +5
==========================================
Hits 6238 6238
- Misses 400 403 +3
- Partials 31 38 +7
Continue to review full report at Codecov.
|
# [0.16.0](v0.15.6...v0.16.0) (2022-06-06) ### Bug Fixes * **copy-tex:** Use JS (instead of CSS) to select full equation, solving display glitches ([#3586](#3586)) ([8c2d852](8c2d852)) ### BREAKING CHANGES * **copy-tex:** copy-tex extension no longer has (or requires) a CSS file. * Code cleanup, lint fixes, port to Flow * Rewrite to extend both start and end of range * Remove contrib/**/*.css linting Co-authored-by: Erik Demaine <edemaine@mit.edu>
|
馃帀 This PR is included in version 0.16.0 馃帀 The release is available on: Your semantic-release bot 馃摝馃殌 |
|
Ooh and an instant release after the merge, that's awesome! 馃専 |
|
Yeah, semantic-release is pretty cool. Makes for a lot of releases though. 馃槄 |
|
Hi @fonsp (contributor) and @edemaine (approver), curiously to find out that this PR is made a long time ago. Currently the file is written in flow (I guess?), but the file is obviously not a valid js file for most browsers and it's publishing to npm as-is. Can anyone try to fix it by compiling it to a standard JS file when publishing? See main branch content and you will get what I am talking:
related issue #3948 |
What is the previous behavior before this PR?
When selecting a subrange of a KaTeX equation, we currently use
user-select: allto ensure that the entire equation is selected, which is needed to get the tex code correctly.I found this behaviour to be unstable on Chrome (not just on Safari, like mentioned in the README), and I would often get a partial selection, leading to strange HTML content:
Schermopname.2022-03-20.om.16.28.50.mov
What is the new behavior after this PR?
This PR uses JS to guarantee that when a subrange of an equation is selected, the parent equation is used for the copying logic. We use
.closest('.katex')to find this parent.When the selection is not fully contained inside a katex equation element, the old behaviour is used, which was working great!
This means that the
user-select: allclass (and in fact, the entire CSS file), can be be removed 馃帀 This fixes the display bugs:Schermopname.2022-03-20.om.16.35.45.mov
Removing the CSS file should not be breaking, as importing it from a future version will just give a 404, which is fine. (Right?) If not, we can put the file back with empty contents.
Let me know what you think!