Opened 3 years ago

Closed 3 years ago

#11777 closed Bug (fixed)

MathJax plugin converts & to simple &

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

Description

Steps to reproduce:

  1. Open MathJax sample, open Math dialog and paste below:
    \begin{equation*} f(n) = \begin{cases} 0 & n = 0\\ 1 & n = 1\\ f(n-1) + f(n-2) & \text{otherwise} \end{cases} \end{equation*}
    
  2. Switch to source, wysiwyg, source.

Result: You will see that & got converted to simple &.

I haven't noticed any difference in final formula results but & is definitely more correct than simple '&'.

Change History (12)

comment:1 Changed 3 years ago by Jakub Ś

Status: newconfirmed

comment:2 Changed 3 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 4.4.1

comment:3 Changed 3 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:4 Changed 3 years ago by Piotr Jasiun

Status: assignedreview

The problem was native encoding for & and lack of such encoding for &.

I added encoding on downcast and decoding on upcast and fixed creating data attribute for widgets.

Now if user put & in MathJax dialog it will be automatically encoded to & (and & will be encoded to &).

Changes in t/11777 and corresponding test branch.

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

Status: reviewreview_failed
  1. #11811 must go before this ticket.
  2. This ticket must not touch widget plugin.
  3. There's a typo in tests names.
  4. Do we need to test dialog? The TC may be simplified a lot with
    • editor.setData();
    • assert editor.getData()
  5. Since we have tools.setUpEditors we should avoid creating two editors with exactly the same config for tests. Remember about tests performance.
  6. The integration tests which you implemented does not show precisely which changes in dev are required and which are not, because there's no link between what was required for fixing data output, what for loading data (value stored in widget data is not controlled).

comment:6 Changed 3 years ago by Piotr Jasiun

Status: review_failedassigned

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

Status: assignedreview

Replying to Reinmar:

  1. #11811 must go before this ticket.

Done.

  1. This ticket must not touch widget plugin.

Thanks to #11811 this ticket does not need to change anything in the widget plugin.

  1. There's a typo in tests names.

I renamed tests.

  1. Do we need to test dialog? The TC may be simplified a lot with
    • editor.setData();
    • assert editor.getData()

Fixed. I used widget.setData() instead of dialog.

  1. Since we have tools.setUpEditors we should avoid creating two editors with exactly the same config for tests. Remember about tests performance.

Fixed.

  1. The integration tests which you implemented does not show precisely which changes in dev are required and which are not, because there's no link between what was required for fixing data output, what for loading data (value stored in widget data is not controlled).

I've added checking if the data attribute contains proper content.

Changes in t/11777 and corresponding test branch.

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

Status: reviewreview_failed

The tests are still more complicated than they have to be.

  1. This makes no sense:
    widget.setData( 'math', '\\\(&\\\)' );
    assert.areSame( '\\\(&\\\)', tools.getAttributeData( widget ).math );
    

You're testing how widget stringify data attribute - that's totally internal widget system thing. You should not be interested in that.

  1. You switch modes twice what... is equal to setData( getData() ). You should have something like:
    assert.areSame( ..., editor.getData() );
    bot.setData( editor.getData(), function() {
        assert.areSame( '\\\(&\\\)', tools.getAttributeData( widget ).math );
    } );
    

That's all.

  1. You tested only &. What about other entities? What about other special characters? I pasted \u00a0 (real, not encoded) into the MathJax dialog and after switching between modes it was encoded. The reason is that you need to use the same trick with iframe as I did in code snippet.
Last edited 3 years ago by Piotrek Koszuliński (previous) (diff)

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

I changed my decision regarding last point. It turned out that:

  1.   is the only character we do not decode right now on Chrome and Firefox. And on IEs there are only couple more. We should think about making a general improvement for that.
  2. We do not encode special characters in MathJax when downcasting. That's because entities filter is applied before widget is downcasted. That's another thing we may need to fix, although it hits architectural limitations very badly.

Therefore, we'll not focus on other special characters than these handled by tools.htmlDecode at the moment.

comment:10 Changed 3 years ago by Piotr Jasiun

Status: review_failedreview

I simplified and improved performance of tests. Changes in t/11777 test branch.

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

Status: reviewreview_passed

I rebased both branches and pushed additional commit to tests.

comment:12 Changed 3 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