CKEDITOR.dom.element attribute containing " (double quotation mark) breaks basicWriter HTML output

[Peter]

Example:

element.setAttribute( 'onclick', 'document.location="/products/fa-3819";' );

Expected HTML output:

onclick="document.location="/products/fa-3819";"

Generated HTML output:

onclick="document.location="/products/fa-3819";"

(I use textarea replacement mode in my setup.)

DOM handling functions like setAttribute/getAttribute are expected to handle values as literals. All the required string processing should be made by the underlying framework.

I think the problem affects only CKEDITOR.htmlParser.basicWriter. CKEDITOR.dom.element and its setAttribute implementation has nothing to do with the problem.

See: /core/htmlparser/basicwriter.js Line 61-64:

		/**
		 * Writes an attribute. This function should be called after opening the
		 * tag with {@link #openTagClose}.
		 * @param {String} attName The attribute name.
		 * @param {String} attValue The attribute value.
		 * @example
		 * // Writes ' class="MyClass"'.
		 * writer.attribute( 'class', 'MyClass' );
		 */
		attribute : function( attName, attValue )
		{
			this._.output.push( ' ', attName, '="', attValue, '"' );
		},

Please add proper escaping for attributes. (CKEDITOR.tools.htmlEncode is not applicable since it leaves double quotes untouched in Firefox)

​http://www.w3.org/TR/html4/charset.html#h-5.3.2
"Some authors use the character entity reference """ to encode instances of the double quote mark (") since that character may be used to delimit attribute values."

[Garry Yao]

Fixed with [5007].

[Garry Yao]

[5007] has brought a regression:

Reproducing Procedures

1. Open any page sample with the following source codes:

   <a name="anchor">anchor</a>
   

• Actual Result: JavaScript error thrown;

[Garry Yao]

Regression fixed with [5028].

[Alfonso Martínez de Lizarrondo]

The change in the htmlwriter plugin isn't right.

On one hand, it doesn't seems to fail at that code with the testcase, so we could avoid that check. But if we prefer to be cautious and protect it, then the previous statement should be covered also by the same check as it could also fail :

			if ( typeof attValue == 'string' )
			{
				if ( this.forceSimpleAmpersand )
					attValue = attValue.replace( /&/, '&' );

				// Browsers don't always escape quote in attribute values. (#4683)
				attValue = attValue.replace( /"/g, '"' )
			}

[Garry Yao]

On one hand, it doesn't seems to fail at that code with the testcase.

It's the basicWriter (input) change in [5007] that exactly failed the above TC here, while there's a huge possibility that same kind of problem could also affect htmlWriter (output) as well.

But if we prefer to be cautious and protect it, then the previous statement should be covered also by the same check as it could also fail.

You're right, but that's not part of this regression.

[Garry Yao]

Nice catch though, I've opened #5096 for it.