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

When creating dom elements the xml namespace should be respected #1842

Closed
Sebobo opened this issue Jun 28, 2019 · 4 comments
Closed

When creating dom elements the xml namespace should be respected #1842

Sebobo opened this issue Jun 28, 2019 · 4 comments
Labels
type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@Sebobo
Copy link

Sebobo commented Jun 28, 2019

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

🐞 Bug report

💻 Version of CKEditor

CKEditor Engine 13.1.1

📋 Steps to reproduce

  1. Similar to the placeholder example do the following:

        function createIconView(modelItem, viewWriter) {
            const icon = modelItem.getAttribute('icon');

            const svgTag = viewWriter.createContainerElement('svg', {
                'class': 'icon icon--medium',
                'viewbox': '0 0 100 100',
                'xmlns': 'http://www.w3.org/2000/svg',
            });
            const useTag = viewWriter.createEmptyElement('use', {
                'href': '#' + icon,
                'xmlns': 'http://www.w3.org/2000/svg',
            });

            viewWriter.insert(viewWriter.createPositionAt(svgTag, 0), useTag);

            return svgTag;
        }

✅ Expected result

The icon should be rendered and the SVG and USE tags should be valid SVG nodes instead of simple document nodes.

❎ Actual result

The icon is not shown and the SVG and USE nodes are normal DOM elements not SVGSVGElement when inspecting the nodes.

📃 Other details that might be useful

The issue is because in domconverter.js and createelement.js only createElement is used and no check is there that looks for an xmlns attribute and then uses createElementNS instead.

I can create a PR for this. It worked well when I modified the two mentioned classes. I'm not sure if there are other places I have to check.

@Reinmar Reinmar added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. labels Jun 28, 2019
@Reinmar Reinmar added this to the next milestone Jun 28, 2019
@Reinmar
Copy link
Member

Reinmar commented Jun 28, 2019

Thanks! Interesting use case.

Am I right that it seems something pretty simple to implement? Did you try patching those classes already? Curious if it'll be enough.

@Sebobo
Copy link
Author

Sebobo commented Jun 28, 2019

Yes it worked for me. Basically it's this:

createelement.js

export default function createElement( doc, name, attributes = {}, children = [] ) {
    const namespace = attributes.xmlns;
    const element = namespace ? doc.createElementNS(namespace, name ) : doc.createElement( name );

and the domconverter.js viewToDom method:

// Create DOM element.
const namespace = viewNode.getAttribute( 'xmlns' );
domElement = namespace ? domDocument.createElementNS(namespace, viewNode.name ) : domDocument.createElement( viewNode.name );

@Sebobo
Copy link
Author

Sebobo commented Jun 28, 2019

Creating a PR might take a while for me as I don't have the contributions cla yet :/

@mlewand mlewand modified the milestones: nice-to-have, backlog Oct 7, 2019
@Reinmar Reinmar modified the milestones: backlog, iteration 27 Oct 8, 2019
Reinmar added a commit to ckeditor/ckeditor5-utils that referenced this issue Oct 8, 2019
Feature: Introduced support for creating elements in other XML namespaces. See ckeditor/ckeditor5#1842.

Thanks @Sebobo!
@Reinmar
Copy link
Member

Reinmar commented Oct 8, 2019

I think that ckeditor/ckeditor5-utils#307 and ckeditor/ckeditor5-engine#1798 are enough to close this ticket. If support for xmlns will be needed in other places too, let's open new tickets for those.

Thanks @Sebobo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

3 participants