Fix forceSimpleAmpersand config implementation

Run ampersand replacement after the htmlEncodeAttr, otherwise the results are overwritten (#965)
ckeditor · Sep 14, 2018 · 48eeb5f · 48eeb5f
1 parent 7880739
commit 48eeb5f
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 2 deletions.
diff --git 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( /&amp;/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( /&amp;/g, '&' ) );
 			}
 
 			this._.output.push( ' ', attName, '="', attValue, '"' );

diff --git 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 = '<p><a href="http://www.blah.com?foo=1&bar=2">Test link</a></p>';
+		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);
+			});
+		});
 	}
-} );
+} );