Opened 3 years ago

Closed 3 years ago

#11777 closed Bug (fixed)

MathJax plugin converts & to simple &

Reported by: j.swiderski Owned by: pjasiun
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 j.swiderski

  • Status changed from new to confirmed

comment:2 Changed 3 years ago by fredck

  • Milestone set to CKEditor 4.4.1

comment:3 Changed 3 years ago by pjasiun

  • Owner set to pjasiun
  • Status changed from confirmed to assigned

comment:4 Changed 3 years ago by pjasiun

  • Status changed from assigned to review

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 follow-up: Changed 3 years ago by Reinmar

  • Status changed from review to review_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 pjasiun

  • Status changed from review_failed to assigned

comment:7 in reply to: ↑ 5 Changed 3 years ago by pjasiun

  • Status changed from assigned to review

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 Reinmar

  • Status changed from review to review_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 Reinmar (previous) (diff)

comment:9 Changed 3 years ago by Reinmar

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 pjasiun

  • Status changed from review_failed to review

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

comment:11 Changed 3 years ago by Reinmar

  • Status changed from review to review_passed

I rebased both branches and pushed additional commit to tests.

comment:12 Changed 3 years ago by pjasiun

  • Resolution set to fixed
  • Status changed from review_passed to closed
Note: See TracTickets for help on using tickets.
© 2003 – 2016 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy