Skip to content

Commit 37a79e1

Browse files
authored
Fix selection over on MathJax interaction (#15)
## Summary: We noticed that for MathJax only interaction it could cause issue as the `trimLeft/Right` function gets into infinite loop (wow!). This might be the contribution of some of the slowness issue we see. This is a fix for that. We also ensure that the space around the text is displayed properly, that give some indication of selection. I also took a look at the upstream, but did not notice any relevant fixes that is related. Issue: https://khanacademy.atlassian.net/browse/DI-1513 ## Test plan: Run `yarn start` Selecting on the "Pure mathjax" interaction will show this issue without the fix. Tested on: - [x] Chrome - [x] Safari - [x] Firefox Author: dat-boris Reviewers: lizfaubell Required Reviewers: Approved By: lizfaubell Checks: ⌛ Tests / E2E Tests, ✅ Build JS Bundle / Build, ✅ Lint / Run ESLint, ✅ Tests / Unit Tests, ✅ Build JS Bundle / Build Coverage Pull Request URL: #15
1 parent 984a8f3 commit 37a79e1

File tree

3 files changed

+26
-4
lines changed

3 files changed

+26
-4
lines changed

src/examples/rich_text_table/tasks.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
[
22
{
33
"data": {
4-
"text": "[[\"Asking question?<a href=\\\"javascript:alert('pwned')\\\">click</a>\", \"Answer! Look at some LaTex\\n\\n\\\\(\\\\dfrac{1}{2k - 6} = \\\\dfrac{1}{3}\\\\) and \\\\(\\\\dfrac{1.5}{2k - 6} = \\\\dfrac{1}{3}\\\\)\\n\\n\"], [\"Asking more question in LaTex?\\n\\n\\\\[\\n\\\\begin{array}{r}\\n 3.050 \\\\\\\\\\n- 0.338 \\\\\\\\\\n\\\\hline\\n\\\\end{array}\\n\\\\]\\n\\n\", \"More LaTex! $4 to $5. \\n\\n\\\\(\\\\dfrac{3}{2k - 6} = \\\\dfrac{1}{3}\\\\)\\n\\n\"], [\"Following is not Math\", \"$4 to $5\"]]"
4+
"text": "[[\"Asking question?<a href=\\\"javascript:alert('pwned')\\\">click</a>\", \"Answer! Look at some LaTex\\n\\n\\\\(\\\\dfrac{1}{2k - 6} = \\\\dfrac{1}{3}\\\\) and \\\\(\\\\dfrac{1.5}{2k - 6} = \\\\dfrac{1}{3}\\\\)\\n\\n\"], [\"Asking more question in LaTex?\\n\\n\\\\[\\n\\\\begin{array}{r}\\n 3.050 \\\\\\\\\\n- 0.338 \\\\\\\\\\n\\\\hline\\n\\\\end{array}\\n\\\\]\\n\\n\", \"More LaTex! $4 to $5. \\n\\n\\\\(\\\\dfrac{3}{2k - 6} = \\\\dfrac{1}{3}\\\\)\\n\\n\"], [\"Following is not Math\", \"$4 to $5\"], [\"\\\\(\\\\dfrac{3}{2k - 6} = \\\\dfrac{1}{3}\\\\)\", \"\\\\(\\\\dfrac{3}{2k - 6} = \\\\dfrac{1}{3}\\\\)\"]]"
55
},
66
"predictions": [
77
{

src/tags/object/RichText/table.js

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,10 @@ const NO_MATHJAX_CLASS = 'tex2jax_ignore';
3737
// Extract math from conversation, alternate between math and non-math
3838
// Khanmigo uses "\(.*?\)" and "\[.*?\]" as the marker for math
3939
// For example, "What is \(2 + 2\)?" will split into ["What is ", "2 + 2", "?"]
40+
//
41+
// Note that even if this only contains MathJax, this will be wrapped in two
42+
// empty strings, which is what we want.
43+
// e.g. "\\(2 + 2\\)" will split into ["", "2 + 2", ""]
4044
const parseConvoWithMath = (str) => {
4145
// About the capture group: a cool behaviour of str.split is that if there's
4246
// capturing group, the group is captured into the group, which is perfect
@@ -79,7 +83,13 @@ const renderTableValue = (val) => {
7983
convoAndMathList.map((convo, i) => {
8084
if (i % 2 === 0) {
8185
// Non math
82-
return <span key={`eq=${i}`} className={NO_MATHJAX_CLASS}>{convo}</span>;
86+
if (!convo) {
87+
// Need to present space here, so if selection happens we can
88+
// still observe it. We tested that en quad space gives the most
89+
// obvious selection.
90+
return <span key={`eq-${i}`} className={NO_MATHJAX_CLASS}>&#x3000;</span>;
91+
}
92+
return <span key={`eq-${i}`} className={NO_MATHJAX_CLASS}>{convo}</span>;
8393
} else {
8494
// So for Math, we need to create a span as we want 2 piece of dom:
8595
// 1. The hidden raw MathJax expression, to allow slot Label to work

src/utils/selection-tools.js

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,18 @@ const trimSelectionLeft = (selection) => {
3535
selection.removeAllRanges();
3636
selection.collapse(resultRange.startContainer, resultRange.startOffset);
3737
let currentRange = selection.getRangeAt(0);
38+
let lastContainer = currentRange.startContainer;
3839

3940
do {
4041
selection.collapse(currentRange.endContainer, currentRange.endOffset);
4142
selection.modify('extend', 'forward', 'character');
4243
currentRange = selection.getRangeAt(0);
43-
} while (!isTextNode(currentRange.startContainer) || isSpace(currentRange.startContainer.textContent[currentRange.startOffset]));
44+
lastContainer = currentRange.startContainer;
45+
} while (
46+
// check that the modify is indeed moving selection forward
47+
currentRange.startContainer !== lastContainer &&
48+
(!isTextNode(currentRange.startContainer) || isSpace(currentRange.startContainer.textContent[currentRange.startOffset]))
49+
);
4450
resultRange.setStart(currentRange.startContainer, currentRange.startOffset);
4551
selection.removeAllRanges();
4652
selection.addRange(resultRange);
@@ -51,12 +57,18 @@ const trimSelectionRight = (selection) => {
5157
selection.removeAllRanges();
5258
selection.collapse(resultRange.endContainer, resultRange.endOffset);
5359
let currentRange = selection.getRangeAt(0);
60+
let lastContainer = currentRange.startContainer;
5461

5562
do {
5663
selection.collapse(currentRange.startContainer, currentRange.startOffset);
5764
selection.modify('extend', 'backward', 'character');
5865
currentRange = selection.getRangeAt(0);
59-
} while (!isTextNode(currentRange.startContainer) || isSpace(currentRange.startContainer.textContent[currentRange.startOffset]));
66+
lastContainer = currentRange.startContainer;
67+
} while (
68+
// check that the modify is indeed moving selection forward
69+
currentRange.startContainer !== lastContainer &&
70+
(!isTextNode(currentRange.startContainer) || isSpace(currentRange.startContainer.textContent[currentRange.startOffset]))
71+
);
6072
resultRange.setEnd(currentRange.endContainer, currentRange.endOffset);
6173
selection.removeAllRanges();
6274
selection.addRange(resultRange);

0 commit comments

Comments
 (0)