Opened 6 years ago

Closed 6 years ago

#10367 closed Bug (fixed)

Editable#insertText loses $ characters

Reported by: Piotrek Koszuliński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.1.2
Component: General Version: 4.0 Beta
Keywords: Cc:

Description

Based on: http://stackoverflow.com/questions/16161826/ckeditor-issue-with-inserttext

Call editor.insertText( '$$' ) in styled text (bold, italic, etc). One of the dollars will be lost.

The reason is this line: https://github.com/ckeditor/ckeditor-dev/blob/master/core/editable.js#L1616

Dollar characters make some troubles when used in replace string.

Change History (10)

comment:1 Changed 6 years ago by Jakub Ś

Status: newconfirmed

Problem can be reproduced in all browsers form CKEditor 4.0 beta (works in 3.x branch).

Below can be executed in browser console:

CKEDITOR.instances.editor1.insertText( '$$' ); 

comment:2 Changed 6 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 4.1.2
Owner: set to Frederico Caldeira Knabben
Status: confirmedreview

Pushed t/10367 in cksource and tests.

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

> git co t/10367
error: pathspec 't/10367' did not match any file(s) known to git.

Have you pushed test branch?

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

Status: reviewreview_failed

BTW. the patch cannot be correct:

> 'a$bcde'.replace( /$/g, '$$$$' )
> "a$bcde$$"

Dollar character needs to be escaped of course.

Also, I haven't seen tests yet, but they should cover all special replacement patterns: $$, $&, $, $', $n (where n is 0-99)`.

In case of more issues, I'd propose to fix this by using split, splice and join.

comment:5 Changed 6 years ago by Frederico Caldeira Knabben

Status: review_failedreview

Well.. don't know what to say. I was sure I had a test for it, but it got lost. My mistake, for sure.

In any case, the review is definitely correct.

I've just pushed it again to t/10367 in cksource and tests.

comment:6 Changed 6 years ago by Olek Nowodziński

Status: reviewreview_failed

The fix fails on IE7, because:

alert( 'foo'.replace( 'foo', '$$$$' ) );
  • regular browser: $$
  • IE7: $$$$

comment:7 Changed 6 years ago by Olek Nowodziński

Owner: changed from Frederico Caldeira Knabben to Olek Nowodziński
Status: review_failedassigned

comment:8 Changed 6 years ago by Olek Nowodziński

Status: assignedreview

Rebased both branches on latest master.

  • Added MT for the ticket.
  • Proposed split&join solution instead of regex replace.

comment:9 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

I don't think we need this mt as it is a plain duplication of the introduced automated tests.

As for the rest, we're definitely ok.

comment:10 Changed 6 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Merged fix into master, dev git:a4c8d0f133, tests 6cc696129ed8fc.

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