Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#10664 closed New Feature (fixed)

MathJax widget

Reported by: Piotr Jasiun Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.3 Beta
Component: General Version:
Keywords: Cc: wim.leers@…

Description

Widget allows you insert and edit mathematical equations in TeX or MathML and display them with MathJax library.

Change History (18)

comment:1 Changed 5 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: newassigned

comment:2 Changed 5 years ago by Wim Leers

Cc: wim.leers@… added

comment:3 Changed 5 years ago by Piotrek Koszuliński

I made a short review and I have some comments.

  1. data.math - I'm not sure if this is a good name. It contains equation/formula's source and "math" seems to be meaningless.
  2. data.math = el.children[ 0 ].value; such careless traversing structure coming from data is unsafe. In this case it only causes displaying default equation, but some check what's there would be better and if span's content is corrupted we should rather not upcast it.
  3. getAbsolutePath - I've seen similar hack somewhere else. It'd be great to not have it inside widget.
  4. The code lacks comments.
Last edited 5 years ago by Piotrek Koszuliński (previous) (diff)

comment:4 in reply to:  3 Changed 5 years ago by Piotr Jasiun

Replying to Reinmar:

I made a short review and I have some comments.

  1. data.math - I'm not sure if this is a good name. It contains equation/formula's source and "math" seems to be meaningless.

I was looking for a good name for this. But it could be equation, inequality, expression, variable... I checked how MathJax call it and they use 'mathematics' ('open source JavaScript display engine for mathematics that works in all browsers.'). But if you have any better idea I will be be grateful.

  1. data.math = el.children[ 0 ].value; such careless traversing structure coming from data is unsafe. In this case it only causes displaying default equation, but some check what's there would be better and if span's content is corrupted we should rather not upcast it.

Fixed.

  1. getAbsolutePath - I've seen similar hack somewhere else. It'd be great to not have it inside widget.

We decided to remove it.

  1. The code lacks comments.

I know. I will work on it.

comment:5 Changed 5 years ago by Piotr Jasiun

Status: assignedreview

Widget is ready to review. I will keep working on it but my changes shouldn't be critical.

My TODO list:

  • more text in sample,
  • comments & documentation,
  • fix same origin policy issue,
  • tests for: inline editor
  • tests for: undo/redo
  • tests for: absolute path and preview plugin
  • and some minior tests.

comment:6 Changed 5 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

I've just pushed small changes. Still we have issues.

With Firefox:

  1. Clean the document.
  2. Click the Math button.
  3. Click ok to insert the default math.
  4. Check the source.

You will have this:

<p><span class="math-tex">\(x = {-b \pm \sqrt{b^2-4ac} \over 2a}\)</span><img src="data:image/gif;base64,R0lGODlhAQABAPABAP///wAAACH5BAEKAAAALAAAAAABAAEAAAICRAEAOw%3D%3D" /><span style="background:url(&quot;http://ckeditor.dev/plugins/widget/images/drag.png&quot;) repeat scroll 0% 0% rgba(220, 220, 220, 0.5); left:0px; top:-14px"><img draggable="true" src="data:image/gif;base64,R0lGODlhAQABAPABAP///wAAACH5BAEKAAAALAAAAAABAAEAAAICRAEAOw%3D%3D" style="height:14px; width:14px" /></span></p>

I'll keep the review going here meanwhile.

comment:7 Changed 5 years ago by Piotrek Koszuliński

I pushed a fix to t/10664 for the issue mentioned in comment:6. I added some points to the https://docs.google.com/document/d/1U4oUCuk4gLuXUzx5BFyN1VlDEmVEggK-ovapSqpXkBQ/edit?usp=sharing

comment:8 Changed 5 years ago by Wim Leers

Very nice :)

Feedback:

  • The dialog title says "Mathematics in TeX". The same text is repeated just above the <textarea>.
  • The dialog should probably link to TeX documentation, to educate the user about the syntax?
  • Why not get rid of the dialog all together? As soon as the cursor reaches a MathJax widget, the formula itself could be displayed *right there*, and the user would be able to edit it. No need for a dialog. Direct manipulation. Of course, that would mean the user won't get a preview. But a preview is only really necessary for more complex formulas. So why not allow double-clicking the widget to get a dialog, otherwise in-place editing? If you don't do that, there's little reason to use a Widget for this, isn't there? Widgets are about removing the need for dialogs (out-of-place editing) where possible, aren't they.
Last edited 5 years ago by Piotr Jasiun (previous) (diff)

comment:9 in reply to:  8 Changed 5 years ago by Piotr Jasiun

Thanks for your feedback :)

  • The dialog title says "Mathematics in TeX". The same text is repeated just above the <textarea>.

I also don't like this. Do you have any better idea?

  • The dialog should probably link to TeX documentation, to educate the user about the syntax?

Good idea. I will add this.

  • Why not get rid of the dialog all together? As soon as the cursor reaches a MathJax widget, the formula itself could be displayed *right there*, and the user would be able to edit it. No need for a dialog. Direct manipulation. Of course, that would mean the user won't get a preview. But a preview is only really necessary for more complex formulas. So why not allow double-clicking the widget to get a dialog, otherwise in-place editing?
    If you don't do that, there's little reason to use a Widget for this, isn't there? Widgets are about removing the need for dialogs (out-of-place editing) where possible, aren't they.

The idea of this widget was to show power of widget system, without plenty of code. In-place-editing (without dialog) would be a great feature, but we can add it in the future. Let keep it simple at the begging.

Last edited 5 years ago by Piotr Jasiun (previous) (diff)

comment:10 Changed 5 years ago by Piotrek Koszuliński

Why not get rid of the dialog all together? As soon as the cursor reaches a MathJax widget, the formula itself could be displayed *right there*, and the user would be able to edit it. No need for a dialog. Direct manipulation. Of course, that would mean the user won't get a preview. But a preview is only really necessary for more complex formulas. So why not allow double-clicking the widget to get a dialog, otherwise in-place editing? If you don't do that, there's little reason to use a Widget for this, isn't there? Widgets are about removing the need for dialogs (out-of-place editing) where possible, aren't they.

In this case widgets system is not used to enable in-place editing, but to solve a little bit different problem. MathJax library is quite destructive and demanding - it does A LOT of operations on DOM and therefore it would simply destroy editor. Thanks to widgets we have control over life cycle of an iframe which we used to separate MathJax context from editor context. This wasn't possible before.

So we are now able to render equations directly in the editor, which is already a great improvement. They can also be edited and during edition there's a live preview. Yes, this doesn't happen directly in editor's area, but it's consistent with other entities which are editable using dialog.

I also don't know where else could we display an input for a TeX format. In a small popup over editable? That would be something completely new and perhaps this is a way for the future, but now we don't have such system with a11y support and so on, that's why we chose the dialogs system.

comment:11 Changed 5 years ago by Wim Leers

I also don't like this. Do you have any better idea?

Only keep the one in the dialog title.

Let keep it simple at the begging.

I agree with the principle. KISS FTW. But at the same time, the demos have to be convincing. Wowing, preferably.

I also don't know where else could we display an input for a TeX format.

Replace the <span class="mathjax">MATHEMATICAL FORMULA HERE</span> with <span class="mathjax-editing">MATHEMATICAL FORMULA HERE</span>, so that MathJax doesn't do its magic, and the user can edit the formula right there. Upon pressing the return key, or giving focus to something else, transform to <span class="mathjax">…` again, so that MathJax once again performs its magic.

comment:12 in reply to:  11 Changed 5 years ago by Piotr Jasiun

Replying to Wim Leers:

I also don't like this. Do you have any better idea?

Only keep the one in the dialog title.

We cannot do this because of accessibility. But I changed the second header.

Let keep it simple at the begging.

I agree with the principle. KISS FTW. But at the same time, the demos have to be convincing. Wowing, preferably.

I also don't know where else could we display an input for a TeX format.

Replace the <span class="mathjax">MATHEMATICAL FORMULA HERE</span> with <span class="mathjax-editing">MATHEMATICAL FORMULA HERE</span>, so that MathJax doesn't do its magic, and the user can edit the formula right there. Upon pressing the return key, or giving focus to something else, transform to <span class="mathjax">…` again, so that MathJax once again performs its magic.

comment:13 Changed 5 years ago by Piotr Jasiun

Status: review_failedreview

Piotrek fixes problem which was the reason of R-. I also added some text to sample, documentation and code comments. I made some changes in dialog as well.

On my TODO list is (still):

  • adding some more tests (I will work on them during the review),
  • fix same origin policy (but it will be done after beta release).

comment:14 Changed 5 years ago by Wim Leers

We cannot do this because of accessibility.

Just make it only visible for screenreaders? :)

comment:15 Changed 5 years ago by Piotrek Koszuliński

More comments:

  1. When a DOM subtree in which widget is attached changes, widget is unloaded. The preview disappears and widget is not responding on doubleclicks. To reproduce - select few paragraphs and apply bold/list/whatever.
  2. On Chrome very often scrolling by mouse the browser's viewport stops working :| It can be unblocked only by dragging the scrollbar by mouse - then it works. Not sure if we can do anything about this, but it's weird ;/
  3. On Chrome and IE I wasn't able to undo change done through a dialog to the widgets which are alone in their paragraphs. This is most likely caused by #10812, because there's an error thrown from the selection.js. And it works fine on FF (because there's a bogus br after widget, so it doesn't fall into #10812 case).

None of these is a beta blocker although 1. is very uncool.

Last edited 5 years ago by Piotrek Koszuliński (previous) (diff)

comment:16 Changed 5 years ago by Piotr Jasiun

Replying to Reinmar:

None of these is a beta blocker although 1. is very uncool.

I know about this and agree that this is very uncool. I talked with Fred about this and we decided to fix it after beta.

comment:17 Changed 5 years ago by Piotrek Koszuliński

Status: reviewreview_passed

Please report tickets for every still opened issue which we mentioned above and assign them to 4.3.

comment:18 Changed 5 years ago by Piotr Jasiun

Resolution: fixed
Status: review_passedclosed
Last edited 5 years ago by Piotr Jasiun (previous) (diff)
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy