From 48eeb5fb205f6bb441544327bc4bd5746dd7c19a Mon Sep 17 00:00:00 2001 From: Alex Maris Date: Tue, 26 Sep 2017 11:08:32 -0500 Subject: [PATCH 1/8] Fix forceSimpleAmpersand config implementation Run ampersand replacement after the htmlEncodeAttr, otherwise the results are overwritten (https://github.com/ckeditor/ckeditor-dev/issues/965) --- plugins/htmlwriter/plugin.js | 4 ++- tests/plugins/htmlwriter/htmlwriter.js | 36 +++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/plugins/htmlwriter/plugin.js b/plugins/htmlwriter/plugin.js index 36f98d8de67..9a798d286c9 100644 --- a/plugins/htmlwriter/plugin.js +++ b/plugins/htmlwriter/plugin.js @@ -179,9 +179,11 @@ CKEDITOR.htmlWriter = CKEDITOR.tools.createClass( { attribute: function( attName, attValue ) { if ( typeof attValue == 'string' ) { - this.forceSimpleAmpersand && ( attValue = attValue.replace( /&/g, '&' ) ); // Browsers don't always escape special character in attribute values. (https://dev.ckeditor.com/ticket/4683, https://dev.ckeditor.com/ticket/4719). attValue = CKEDITOR.tools.htmlEncodeAttr( attValue ); + + // Run ampersand replacement after the htmlEncodeAttr, otherwise the results are overwritten (https://github.com/ckeditor/ckeditor-dev/issues/965) + this.forceSimpleAmpersand && ( attValue = attValue.replace( /&/g, '&' ) ); } this._.output.push( ' ', attName, '="', attValue, '"' ); diff --git a/tests/plugins/htmlwriter/htmlwriter.js b/tests/plugins/htmlwriter/htmlwriter.js index 84f4fdcae3b..3812fe272e5 100644 --- a/tests/plugins/htmlwriter/htmlwriter.js +++ b/tests/plugins/htmlwriter/htmlwriter.js @@ -39,5 +39,39 @@ bender.test( { assert.areSame( afterFormat, bot.getData( false, false ) ); } ); } ); + }, + 'test editor config.forceSimpleAmpersand in html element attributes': function () { + var data = '

Test link

'; + bender.editorBot.create({ + name: 'basic_forceSimpleAmpersand', + formattedOutput: true, + + config: { + allowedContent: true, + forceSimpleAmpersand: true, + + on: { + instanceReady: function (evt) { + var wrtierConfig = { + indent: true, + breakBeforeOpen: false, + breakAfterOpen: false, + breakBeforeClose: false, + breakAfterClose: false + }; + + evt.editor.dataProcessor.writer.setRules('p', wrtierConfig); + evt.editor.dataProcessor.writer.setRules('div', wrtierConfig); + } + } + } + }, function (bot) { + bot.setData( data, function () { + var afterFormat = bot.getData( false, false); + + // Trigger getData a second time to reveal bug. + assert.areSame( afterFormat, data); + }); + }); } -} ); +} ); \ No newline at end of file From 4056e6b413bda5ee49c334c8975b0178b29b7007 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Fri, 14 Sep 2018 10:22:05 +0200 Subject: [PATCH 2/8] Pull request code style corrections. --- plugins/htmlwriter/plugin.js | 6 ++++-- tests/plugins/htmlwriter/htmlwriter.js | 29 ++++++++++++++------------ 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/plugins/htmlwriter/plugin.js b/plugins/htmlwriter/plugin.js index 9a798d286c9..53230c5d212 100644 --- a/plugins/htmlwriter/plugin.js +++ b/plugins/htmlwriter/plugin.js @@ -182,8 +182,10 @@ CKEDITOR.htmlWriter = CKEDITOR.tools.createClass( { // Browsers don't always escape special character in attribute values. (https://dev.ckeditor.com/ticket/4683, https://dev.ckeditor.com/ticket/4719). attValue = CKEDITOR.tools.htmlEncodeAttr( attValue ); - // Run ampersand replacement after the htmlEncodeAttr, otherwise the results are overwritten (https://github.com/ckeditor/ckeditor-dev/issues/965) - this.forceSimpleAmpersand && ( attValue = attValue.replace( /&/g, '&' ) ); + // Run ampersand replacement after the htmlEncodeAttr, otherwise the results are overwritten (#965). + if ( this.forceSimpleAmpersand ) { + attValue = attValue.replace( /&/g, '&' ); + } } this._.output.push( ' ', attName, '="', attValue, '"' ); diff --git a/tests/plugins/htmlwriter/htmlwriter.js b/tests/plugins/htmlwriter/htmlwriter.js index 3812fe272e5..e08cb0a3cd9 100644 --- a/tests/plugins/htmlwriter/htmlwriter.js +++ b/tests/plugins/htmlwriter/htmlwriter.js @@ -40,9 +40,12 @@ bender.test( { } ); } ); }, - 'test editor config.forceSimpleAmpersand in html element attributes': function () { + + // (#965) + 'test config.forceSimpleAmpersand works in HTML element attributes': function() { var data = '

Test link

'; - bender.editorBot.create({ + + bender.editorBot.create( { name: 'basic_forceSimpleAmpersand', formattedOutput: true, @@ -51,7 +54,7 @@ bender.test( { forceSimpleAmpersand: true, on: { - instanceReady: function (evt) { + instanceReady: function( evt ) { var wrtierConfig = { indent: true, breakBeforeOpen: false, @@ -59,19 +62,19 @@ bender.test( { breakBeforeClose: false, breakAfterClose: false }; - - evt.editor.dataProcessor.writer.setRules('p', wrtierConfig); - evt.editor.dataProcessor.writer.setRules('div', wrtierConfig); + + evt.editor.dataProcessor.writer.setRules( 'p', wrtierConfig ); + evt.editor.dataProcessor.writer.setRules( 'div', wrtierConfig ); } } } - }, function (bot) { - bot.setData( data, function () { - var afterFormat = bot.getData( false, false); + }, function( bot ) { + bot.setData( data, function() { + var afterFormat = bot.getData( false, false ); // Trigger getData a second time to reveal bug. - assert.areSame( afterFormat, data); - }); - }); + assert.areSame( afterFormat, data ); + } ); + } ); } -} ); \ No newline at end of file +} ); From 8d8ee21229a2288ccab65454456adf7b0fc881f6 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Fri, 14 Sep 2018 10:58:41 +0200 Subject: [PATCH 3/8] Tests: simplified unit test. --- tests/plugins/htmlwriter/htmlwriter.js | 32 ++++++++------------------ 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/tests/plugins/htmlwriter/htmlwriter.js b/tests/plugins/htmlwriter/htmlwriter.js index e08cb0a3cd9..2dfa18911fa 100644 --- a/tests/plugins/htmlwriter/htmlwriter.js +++ b/tests/plugins/htmlwriter/htmlwriter.js @@ -46,34 +46,20 @@ bender.test( { var data = '

Test link

'; bender.editorBot.create( { - name: 'basic_forceSimpleAmpersand', + name: 'forceSimpleAmpersand', formattedOutput: true, - config: { - allowedContent: true, - forceSimpleAmpersand: true, - - on: { - instanceReady: function( evt ) { - var wrtierConfig = { - indent: true, - breakBeforeOpen: false, - breakAfterOpen: false, - breakBeforeClose: false, - breakAfterClose: false - }; - - evt.editor.dataProcessor.writer.setRules( 'p', wrtierConfig ); - evt.editor.dataProcessor.writer.setRules( 'div', wrtierConfig ); - } - } + extraAllowedContent: 'a[href]', + forceSimpleAmpersand: true } }, function( bot ) { - bot.setData( data, function() { - var afterFormat = bot.getData( false, false ); + bot.editor.dataProcessor.writer.setRules( 'p', { + indent: false, + breakAfterClose: false + } ); - // Trigger getData a second time to reveal bug. - assert.areSame( afterFormat, data ); + bot.setData( data, function() { + assert.areSame( data, bot.getData( false, false ) ); } ); } ); } From dcf8dd13c2eee3136b44d54bf45e41b7ded7d8c4 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Fri, 14 Sep 2018 11:09:10 +0200 Subject: [PATCH 4/8] Tests: added manual test. --- .../htmlwriter/manual/positioning.html | 33 +++++++++++++++++++ .../plugins/htmlwriter/manual/positioning.md | 13 ++++++++ 2 files changed, 46 insertions(+) create mode 100644 tests/plugins/htmlwriter/manual/positioning.html create mode 100644 tests/plugins/htmlwriter/manual/positioning.md diff --git a/tests/plugins/htmlwriter/manual/positioning.html b/tests/plugins/htmlwriter/manual/positioning.html new file mode 100644 index 00000000000..cdaa55cb6d2 --- /dev/null +++ b/tests/plugins/htmlwriter/manual/positioning.html @@ -0,0 +1,33 @@ +

Divarea editor:

+ + +

classic editor:

+ + +

Inline editor:

+ + + diff --git a/tests/plugins/htmlwriter/manual/positioning.md b/tests/plugins/htmlwriter/manual/positioning.md new file mode 100644 index 00000000000..639fe606524 --- /dev/null +++ b/tests/plugins/htmlwriter/manual/positioning.md @@ -0,0 +1,13 @@ +@bender-ui: collapsed +@bender-tags: 4.10.2, bug, htmlwriter +@bender-ckeditor-plugins: wysiwygarea,toolbar,link,htmlwriter,sourcearea + +1. Click the "Source" button. + ## Expected + + `a[href]` attribute is equal to `https://foo.bar?foo=1&bar=2` note `&` character it should not be encoded into an entity. + + ## Unexpected + + `&` character in `a[href]` gets encoded to `&`. +1. Repeat steps for other editors. \ No newline at end of file From 5ddcd18ad36a64111089385f36bc2121548aa001 Mon Sep 17 00:00:00 2001 From: Marek Lewandowski Date: Fri, 14 Sep 2018 11:31:08 +0200 Subject: [PATCH 5/8] Changelog entry. --- CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.md b/CHANGES.md index 6120d1661aa..c1c62c4cdd1 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -13,6 +13,7 @@ Fixed Issues: * [#2380](https://github.com/ckeditor/ckeditor-dev/issues/2380) Fixed: Styling HTML comments in top level element result with extra paragraphs. * [#2294](https://github.com/ckeditor/ckeditor-dev/issues/2294) Fixed: Pasting content from MS Outlook and then bolding it results with an error. * [#2035](https://github.com/ckeditor/ckeditor-dev/issues/2035) [Edge] Fixed: `Permission denied` is thrown when opening [Panel](https://ckeditor.com/cke4/addon/panel) instance. +* [#2035](https://github.com/ckeditor/ckeditor-dev/issues/2035) Fixed: The [`config.forceSimpleAmpersand`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_config.html#cfg-forceSimpleAmpersand) option does no longer work. Thanks to [Alex Maris](https://github.com/alexmaris)! API Changes: From 62b3e049eaecc5af81e3d5118513fb6216f39b6c Mon Sep 17 00:00:00 2001 From: Tomasz Jakut Date: Fri, 14 Sep 2018 16:22:32 +0200 Subject: [PATCH 6/8] Update changelog entry. --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index c1c62c4cdd1..31951f20e39 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -13,7 +13,7 @@ Fixed Issues: * [#2380](https://github.com/ckeditor/ckeditor-dev/issues/2380) Fixed: Styling HTML comments in top level element result with extra paragraphs. * [#2294](https://github.com/ckeditor/ckeditor-dev/issues/2294) Fixed: Pasting content from MS Outlook and then bolding it results with an error. * [#2035](https://github.com/ckeditor/ckeditor-dev/issues/2035) [Edge] Fixed: `Permission denied` is thrown when opening [Panel](https://ckeditor.com/cke4/addon/panel) instance. -* [#2035](https://github.com/ckeditor/ckeditor-dev/issues/2035) Fixed: The [`config.forceSimpleAmpersand`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_config.html#cfg-forceSimpleAmpersand) option does no longer work. Thanks to [Alex Maris](https://github.com/alexmaris)! +* [#965](https://github.com/ckeditor/ckeditor-dev/issues/965) Fixed: The [`config.forceSimpleAmpersand`](https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_config.html#cfg-forceSimpleAmpersand) option does no longer work. Thanks to [Alex Maris](https://github.com/alexmaris)! API Changes: From a64843430fcd68365719856049987aaf143eee6d Mon Sep 17 00:00:00 2001 From: Tomasz Jakut Date: Fri, 14 Sep 2018 16:23:56 +0200 Subject: [PATCH 7/8] Rename manual test. --- .../manual/{positioning.html => forcesimpleampersand.html} | 0 .../htmlwriter/manual/{positioning.md => forcesimpleampersand.md} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename tests/plugins/htmlwriter/manual/{positioning.html => forcesimpleampersand.html} (100%) rename tests/plugins/htmlwriter/manual/{positioning.md => forcesimpleampersand.md} (100%) diff --git a/tests/plugins/htmlwriter/manual/positioning.html b/tests/plugins/htmlwriter/manual/forcesimpleampersand.html similarity index 100% rename from tests/plugins/htmlwriter/manual/positioning.html rename to tests/plugins/htmlwriter/manual/forcesimpleampersand.html diff --git a/tests/plugins/htmlwriter/manual/positioning.md b/tests/plugins/htmlwriter/manual/forcesimpleampersand.md similarity index 100% rename from tests/plugins/htmlwriter/manual/positioning.md rename to tests/plugins/htmlwriter/manual/forcesimpleampersand.md From 898126e6b69ec6fc6c6d117f69bd44694c0249d9 Mon Sep 17 00:00:00 2001 From: Tomasz Jakut Date: Fri, 14 Sep 2018 16:50:12 +0200 Subject: [PATCH 8/8] Add issue reference to the manual test. --- tests/plugins/htmlwriter/manual/forcesimpleampersand.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/plugins/htmlwriter/manual/forcesimpleampersand.md b/tests/plugins/htmlwriter/manual/forcesimpleampersand.md index 639fe606524..a2843975d48 100644 --- a/tests/plugins/htmlwriter/manual/forcesimpleampersand.md +++ b/tests/plugins/htmlwriter/manual/forcesimpleampersand.md @@ -1,5 +1,5 @@ @bender-ui: collapsed -@bender-tags: 4.10.2, bug, htmlwriter +@bender-tags: 4.10.2, 965, bug,htmlwriter @bender-ckeditor-plugins: wysiwygarea,toolbar,link,htmlwriter,sourcearea 1. Click the "Source" button. @@ -10,4 +10,4 @@ ## Unexpected `&` character in `a[href]` gets encoded to `&`. -1. Repeat steps for other editors. \ No newline at end of file +1. Repeat steps for other editors.