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

Added mathematics tool. #124

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

dazsmith
Copy link

@dazsmith dazsmith commented Aug 24, 2020

By opening a pull request, I certify that I hold the intellectual property of the code I am submitting, and I am granting the initial authors of WBO a perpetual, worldwide, non-exclusive, royalty-free, and irrevocable license to this code.

This code adds a "mathematics tool" which accepts (La)TeX code in a text box, formats it using mathjax, and displays it on the page. This is designed to address #109.

Explanation of code

  • The input LaTeX is stored in the aria-label attribute of the svg element created by mathjax. I don't think this is contrary to the designated use of this accessibility attribute, as it is reasonable to expect that a screen reader would benefit from accessing the input LaTeX code.
  • The color of the displayed mathematics is the color selected by the user, but the usual LaTeX color commands can be used to (locally) change the color. The user's color is stored in the stroke attribute of the rectangle of class clickhelper.
  • The rectangle of class clickhelper is appended to the output of mathjax to make it easier to select the displayed mathematics for future edits & moves using the move tool.
  • The edits to ellipse, pencil and rectangle tools and associated CSS and board.css are because mathjax outputs an svg element containing many other elements, whose fill attribute should not be set to none. Therefore, a new class, nofill, was created for those tools providing the original functionality.
  • The edits to the hand tool are to enable selection of the "parent" svg element containing all the mathjax output, so that not just a single part of the mathjax output is moved.

Some comments

  • I don't know whether or not line 7 of the new mathematics.js should be edited to include my name too. Please advise.

@dazsmith
Copy link
Author

Perhaps the failed checks failed because of the edits to the pencil tool?

@dazsmith
Copy link
Author

dazsmith commented Sep 2, 2020

It was not the change to the pencil tool.
The issue was that the main canvas svg node required property xmlns:xlink="http://www.w3.org/1999/xlink" to be able to contain output of mathematics tool.
Solving that problem revealed a typo in a line I added.

I'm not sure why this issue only arises in the preview and not the boards.

@droundy
Copy link
Contributor

droundy commented Sep 12, 2020

This sounds great. Would you guess that this pull request is in a state where I could reasonably integrate it intro my whiteboards for classes starting in under two weeks?

@dazsmith
Copy link
Author

I've been using it in my classes for 5 weeks with no problem.

@droundy
Copy link
Contributor

droundy commented Sep 12, 2020

If the mathjax a standard 2.7, or did you have to customize it? If it's standard, is there any reason not to source it from a CDN?

@droundy
Copy link
Contributor

droundy commented Sep 12, 2020

When I merge this with my code, I'm getting a problem in the preview, which unfortunately is pretty high priority for my plans to manage my dozen breakout rooms. :(

Edit: Never mind. I now realize I was looking at an incorrect path. (Embarrassing!)

@droundy
Copy link
Contributor

droundy commented Sep 12, 2020

I'm not sure how to directly contribute to someone else's pull request, so I'll note the one change I had to make here:

$ git diff
diff --git a/client-data/view.html b/client-data/view.html
index f595660..a22ff21 100644
--- a/client-data/view.html
+++ b/client-data/view.html
@@ -21,6 +21,7 @@
        <link rel="alternate" hreflang="{{.}}" href="{{../boardUriComponent}}?lang={{.}}" />
        {{/languages}}
        <script src="../polyfill.min.js"></script>
+       <script type="text/javascript" id="MathJax-script" src="https://mj.dasmithmaths.com/mathjax/tex-svg-full.js"></script>
 </head>
 
 <body>
@@ -94,6 +95,7 @@
        <script src="../tools/rect/rect.js"></script>
        <script src="../tools/ellipse/ellipse.js"></script>
        <script src="../tools/text/text.js"></script>
+       <script src="../tools/mathematics/mathematics.js"></script>
        <script src="../tools/eraser/eraser.js"></script>
        <script src="../tools/hand/hand.js"></script>
        <script src="../tools/grid/grid.js"></script>

Thanks for this great tool!

@droundy
Copy link
Contributor

droundy commented Sep 12, 2020

I notice that when I use math like $\frac{\sin x}{\cos x}$ the rendered math is often hidden by the edit field. Would it be possible to shift the edit field down just a bit more to help with this? It's even worse if I use an align environment or similar, so maybe we could just make the edit a bit transparent?

@dazsmith
Copy link
Author

If the mathjax a standard 2.7, or did you have to customize it? If it's standard, is there any reason not to source it from a CDN?

It is standard mathjax, called with <script type="text/javascript" id="MathJax-script" src="https://mj.dasmithmaths.com/mathjax/tex-svg-full.js"></script>. I have never used mathjax before, so did not know if I could trust a public server, so just rolled my own. I haven't tried it with a public server, but I don't see why it wouldn't work, as long as they don't rate limit the mathjax requests in any way.

@dazsmith
Copy link
Author

I'm not sure how to directly contribute to someone else's pull request, so I'll note the one change I had to make here:
...
Thanks for this great tool!

Thanks for the contribution. I can't find /client-data/view.html in either of lovasoa's branches. Where is this file?

Once I understand that ... I also don't know the best way for you to edit a pr, but I guess you could submit a pr to my repository, which I accept, thereby updating the branch for this pr too.

@dazsmith
Copy link
Author

I notice that when I use math like $\frac{\sin x}{\cos x}$ the rendered math is often hidden by the edit field. Would it be possible to shift the edit field down just a bit more to help with this? It's even worse if I use an align environment or similar, so maybe we could just make the edit a bit transparent?

I had thought of trying to move the textbox up a bit but couldn't quickly figure it out so gave up. Transparency is a much better idea. I'll give it a go in the coming week.

@droundy
Copy link
Contributor

droundy commented Sep 13, 2020

Apparently I didn't make a pull request that I thought I had. At least I can't find record of it. So it seems that view.html is only in my repository. I should put together a pull request...

@droundy
Copy link
Contributor

droundy commented Sep 14, 2020

Hi @dazsmith , I just sent a pull request to you with some useful changes, hopefully you can merge it into this just fine.

@dazsmith
Copy link
Author

I'm not sure why some of the checks failed and others passed. Can you comment, @lovasoa or @droundy ?

@sents
Copy link
Contributor

sents commented Sep 6, 2022

Note that this PR uses setting of innerHTML of the data sent to the server. This allows potentially setting arbitrary HTML in the svg element. Setting innerHTML in an <svg> environment doesn't do anything so it doesn't allow arbitrary code to be executed to everybody seeing the board. However if somebody were to download the svg and embed in in a page a script tag would be executed. If there is interest in merging this eventually I can add code to filter <script>, <image>, <a> on the server side.

@sents
Copy link
Contributor

sents commented Sep 6, 2022

@lovasoa I would really like to work on this PR but I can not commit to this pull request. Should I open another pull request?
@dazsmith Are you still interested on working on this?

@dazsmith
Copy link
Author

dazsmith commented Sep 7, 2022

@sents I would like to come back to working on this. I think if you submit a pull request to me, then I can merge it into this one.
I expect that with the upstream updates there is quite a bit of work to be done.
I'm not sure why it always failed the checks, but my guess is that downloading mathjax takes so long that it causes a timeout.

Your plan to filter <script> etc sounds good.

@holema
Copy link
Contributor

holema commented Dec 28, 2022

Will you merge this PR @lovasoa? This PR seems to be great

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.

4 participants