Opened 5 years ago

Closed 5 years ago

#10937 closed Bug (fixed)

MathJax widget improvements

Reported by: Piotr Jasiun Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.3
Component: General Version:
Keywords: Cc:

Description (last modified by Piotr Jasiun)

Umbrella ticket for MathJax widget. Because most of these ticket are related to each other they will be done in one branch.

Ticket Summary Owner Type Status Priority Component
#10840 MathJax widget - same origin policy issue Piotr Jasiun Bug closed Normal General
#10841 MathJax widget - support subtree changes Piotr Jasiun Bug closed Must have (possibly next milestone) General
#10842 MathJax widget - undo change done through a dialog Bug closed Normal General
#10843 MathJax widget tests Piotr Jasiun Bug closed Normal General
#10844 MathJax widget - IE8 support Piotr Jasiun Bug closed Normal General
#10857 MathJax widget [FF,IE9] - clipboard totally broken Piotr Jasiun Bug closed Must have (possibly next milestone) Core : Pasting
#10930 IE9 MathJax crash on undo Piotr Jasiun Bug closed Normal General
#10948 MathJax widget - loading indicators Piotr Jasiun New Feature closed Normal General
#11078 MathJax sample should mention that MathJax widget does not support IE8 Piotr Jasiun Task closed Normal Documentation & Samples

Change History (11)

comment:1 Changed 5 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: newassigned
Summary: MathJax widget improvmetsMathJax widget improvements

comment:2 Changed 5 years ago by Piotr Jasiun

Description: modified (diff)

comment:3 Changed 5 years ago by Piotr Jasiun

All of changes and tests are in t/10937 branch and corresponding branch in tests project.

Both branches have been rebased on the top of the major and tested with IE11. Everything there is fine.

comment:4 Changed 5 years ago by Piotr Jasiun

Status: assignedreview

MathJax is stable now. Only IE8 makes some problems but it seems to be pointless to spend more time in this. All of the tests are green in all supported browsers in both dev and build versions.

All of the changes are in: ​t/10937 and corresponding test branch.

comment:5 Changed 5 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

I think we reached a good situation with this feature, therefore I'm not going so hard on the review.

There are some minor things though.


Just for simplification, I would do this:

node.hasAttributes[ 'data-cke-survive' ]

... instead of this:

node.attributes[ 'data-cke-survive' ] === 'true'

I would then simply use "1" instead of "true".

In this way this attribute would behave just like a HTML "boolean". In fact, the current "false" behavior is misleading, because it is not a guarantee that the element will get removed if empty.


The loader image is in the "icons" folder and this is wrong. That folder is reserved to toolbar icons. It should be into a new "images" folder.


Other than this, we should be ready to close this ticket and all its siblings.

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

Wrong quotes used:

document.domain = "ckeditor.dev";

iframe.getName() != 'iframe'

Could be changed to:

!iframe.is( 'iframe' )

Missing spaces:

if( CKEDITOR.env.ie ) // Twice!

attrs['data-cke-survive'] = 'true';

return  match.replace(/(<iframe.*?\/iframe>)/i, ""); // Plus wrong quotes.

preview.setHtml( '<img src=' + CKEDITOR.plugins.mathjax.loadingIcon + ' alt=' + editor.lang.mathjax.loading +'>' ); // After '+'.

if( doc.getById( 'preview' ) )

There's a space in indentation after this line:

CKEDITOR.plugins.mathjax.fixSrc =

We don't use curly braces in this case:

+                                       if ( val ) {
+                                               preview.setStyle( key, val );
+                                       }
Last edited 5 years ago by Piotrek Koszuliński (previous) (diff)

comment:7 Changed 5 years ago by Piotr Jasiun

There is no hasAttributes method in CKEDITOR.htmlParser.element (it exists only in CKEDITOR.dom.element) so changed to:

if( node.attributes[ 'data-cke-survive' ] )

I agree that data-cke-survive=false does not make much sense.

I also moved loader.gif form icons to images.

comment:8 in reply to:  7 Changed 5 years ago by Piotrek Koszuliński

Replying to pjasiun:

There is no hasAttributes method in CKEDITOR.htmlParser.element (it exists only in CKEDITOR.dom.element) so changed to:

if( node.attributes[ 'data-cke-survive' ] )

Space!

comment:9 Changed 5 years ago by Piotr Jasiun

Status: review_failedreview

Code style fixes applied and everything seems to work.

comment:10 Changed 5 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:11 Changed 5 years ago by Piotr Jasiun

Resolution: fixed
Status: review_passedclosed
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy