Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XmlDataProcessor unable to convert data to xml #1275

Closed
mtilsted opened this issue Oct 2, 2018 · 5 comments · May be fixed by #16581
Closed

XmlDataProcessor unable to convert data to xml #1275

mtilsted opened this issue Oct 2, 2018 · 5 comments · May be fixed by #16581
Labels
package:engine resolution:expired This issue was closed due to lack of feedback. status:stale type:bug This issue reports a buggy (incorrect) behavior.

Comments

@mtilsted
Copy link

mtilsted commented Oct 2, 2018

Is this a bug report or feature request? (choose one)

🐞 Bug report
XmlDataProcessor is unable to convert data to xml. (Importing existing xml does work).

This can bee seen in 2 ways: The easy is just to do:
editor.data.processor=new XmlDataProcessor();
console.log(editor.getData());

which returns html instead of xml. The exact same thing happens if I create my own XmlDataProcessor instance and calls toData()

💻 Version of CKEditor

Cke version:
"@ckeditor/ckeditor5-core": {
"version": "11.0.0",
}

It's a bug i standard CKEditor without any plugins.

📋 Steps to reproduce

Either Just do:
editor.data.processor=new XmlDataProcessor();
console.log(editor.getData())

or
execute this plugin:

class MtExampleCommand extends Command {
execute() {
const editor=this.editor;
const model=editor.model;
const modelDocument = editor.model.document;
const view = editor.editing.view;
const viewDocument = view.document;
const p=new XmlDataProcessor();
const vRoot=viewDocument.getRoot();
const xml=p.toData(vRoot);
}
}

✅ Expected result

Valid xml.

❎ Actual result

Valid html (tags such as img not closed. attributes values and & are not escaped.

📃 Other details that might be useful

The XmlDataProcessor.toData() method which should convert to xml just does this:

	const domFragment = this._domConverter.viewToDom( viewFragment, document );
	return this._htmlWriter.getHtml( domFragment );

So it looks like the method to convert to xml newer got a real implementation.

@vdeygas
Copy link

vdeygas commented Jul 11, 2019

Hello,
A quick fix to this issue is to use XMLSerializer to convert the HTML output to XML.

So xmlDataProcessor could be :

* XMLSerializer instance used to convert DOM elements to an XML string.
* 
* @private
* @member {XMLSerializer}
*/
this._xmlSerializer = new XMLSerializer();

...

toData( viewFragment ) {
    // Convert view DocumentFragment to DOM DocumentFragment.
    const domFragment = this._domConverter.viewToDom( viewFragment, document );

    // Convert DOM DocumentFragment to XML output.
    const doc = document.implementation.createHTMLDocument( '' );
    const container = doc.createElement( 'div' );
    container.appendChild( domFragment );
    return this._xmlSerializer.serializeToString(container);
}

Is the resolution of this issue could be planed ?

Thx

@Mgsy
Copy link
Member

Mgsy commented Jul 16, 2019

Hello, first of all, I'd like to apologize for the late response, somehow we've missed this ticket.

Indeed, the problem exists and our XmlDataProcessor is returning HTML code instead of XML. There is a high chance that we've never tested it this way, because we use this processor mostly in our dev utils. Fixing this issue might cause our tests to fail, but anyway, we should definitely fix it and adjust our tests to those changes.

@vdeygas, pull request with a fix for this case (with tests and documentation) will be much appreciated and we will be more than happy to review it.

@Mgsy Mgsy added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. labels Jul 16, 2019
@Mgsy Mgsy added this to the next milestone Jul 16, 2019
@mlewand
Copy link
Contributor

mlewand commented Mar 25, 2021

I stumbled on this problem recently, where the prospect reported that our XML processor returns <br> instead of <br/>.

The problem is with the fact that we're simply using basic (HTML) writer for XML. Using a XML writer will improve it. I made a PoC like that:

class CustomWriter {
	/**
	 * Returns an HTML string created from the document fragment.
	 *
	 * @param {DocumentFragment} fragment
	 * @returns {String}
	 */
	getHtml( fragment ) {
		const parser = new DOMParser();
		const doc = parser.parseFromString( '<root></root>', 'text/xml' );

		const container = doc.children[ 0 ];
		container.appendChild( fragment );

		return container.innerHTML;
	}
}

function customPlugin( editor ) {
	console.log( 'using a custom processor' );
	editor.data.processor = new XmlDataProcessor( editor.editing.view.document, { namespaces: [ 'foo' ] } );

	editor.data.processor._htmlWriter = new CustomWriter();
}

By adding the customPlugin plugin you'll have the output like that:

<h2 xmlns="http://www.w3.org/1999/xhtml">Lists in the table</h2>
<hr xmlns="http://www.w3.org/1999/xhtml" />

<p xmlns="http://www.w3.org/1999/xhtml">foodaskljd
	<br /><br /> </p>

<figure xmlns="http://www.w3.org/1999/xhtml" class="image">
	<img src="../assets/img/fields.jpg" alt="Autumn fields" />
</figure>

Which produces proper markup for self closing tags, but adds namespace URL for top level elements.

@pomek pomek removed this from the nice-to-have milestone Feb 21, 2022
@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added the resolution:expired This issue was closed due to lack of feedback. label Nov 7, 2023
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 7, 2023
ChristianAdamski added a commit to ChristianAdamski/ckeditor5 that referenced this issue Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine resolution:expired This issue was closed due to lack of feedback. status:stale type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants