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 selection over on MathJax interaction #15

Merged
merged 2 commits into from
Aug 1, 2024
Merged

Conversation

dat-boris
Copy link
Collaborator

@dat-boris dat-boris commented Aug 1, 2024

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:

  • Chrome
  • Safari
  • Firefox

We noticed that for MathJax only interaction it could cause issue
as the `trimLeft/Right` function gets into infinite loop (wow!).

This is a fix for that.  We also ensure that the space around the
text is displayed properly, that give some indication of selection.
@dat-boris dat-boris self-assigned this Aug 1, 2024
@dat-boris dat-boris requested review from lizfaubell and a team August 1, 2024 20:42
@dat-boris dat-boris marked this pull request as ready for review August 1, 2024 20:48
@dat-boris dat-boris changed the title Fix selection over pure MathJax interaction Fix selection over on MathJax interaction Aug 1, 2024
Copy link

@lizfaubell lizfaubell left a comment

Choose a reason for hiding this comment

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

Ahh interesting fix. Small request: can testing steps include which browsers you tested this on?

Comment on lines +46 to +48
currentRange.startContainer !== lastContainer &&
(!isTextNode(currentRange.startContainer) || isSpace(currentRange.startContainer.textContent[currentRange.startOffset]))
);

Choose a reason for hiding this comment

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

Could use a brief comment.

@dat-boris
Copy link
Collaborator Author

Ahh interesting fix. Small request: can testing steps include which browsers you tested this on?

LOL - am just testing on Firefix as I see this review :) shall do!

Copy link

@lizfaubell lizfaubell left a comment

Choose a reason for hiding this comment

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

Thanks!

@dat-boris dat-boris merged commit 37a79e1 into master Aug 1, 2024
4 of 5 checks passed
khan-actions-bot pushed a commit to Khan/label-studio that referenced this pull request Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants