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

Adding toolbar to widget fails #4608

Closed
ansorensen opened this issue May 30, 2019 · 4 comments · Fixed by ckeditor/ckeditor5-widget#88
Closed

Adding toolbar to widget fails #4608

ansorensen opened this issue May 30, 2019 · 4 comments · Fixed by ckeditor/ckeditor5-widget#88
Assignees
Labels
package:widget type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@ansorensen
Copy link

I'm attempting to add a toolbar to my formula widget with this code in my plugin class:

afterInit() {
        const editor = this.editor;
        const widgetToolbarRepository = editor.plugins.get( WidgetToolbarRepository );

        widgetToolbarRepository.register( 'formula', {
            items: editor.config.get( 'formula.items' ),
            getRelatedElement: () => {
                let selectedElement = editor.model.document.selection.getSelectedElement();
                return selectedElement && selectedElement.name === 'formula' ? selectedElement : null;
            }
        } );
 }

I see the getRelatedElement function getting called and successfully returning an element when my formula widget is selected.

However, I get the following stack trace:
domconverter.js:254 Uncaught TypeError: Cannot read property 'createElement' of undefined at DomConverter.viewToDom (domconverter.js:254) at getBalloonPositionData (widgettoolbarrepository.js:333) at WidgetToolbarRepository._showToolbar (widgettoolbarrepository.js:283) at WidgetToolbarRepository._updateToolbarsVisibility (widgettoolbarrepository.js:244) at ClassicEditorUI.<anonymous> (widgettoolbarrepository.js:109) at ClassicEditorUI.fire (emittermixin.js:242) at ClassicEditorUI.update (editorui.js:103) at Document.<anonymous> (editorui.js:73) at Document.fire (emittermixin.js:242) at View.<anonymous> (view.js:201)

I've traced the issue to line 246 of ckeditor5-widget/src/widgettoolbarrepository.js

target: editingView.domConverter.viewToDom( relatedElement )

This function call only passes in one parameter, but viewToDom expects two:

viewToDom(viewNode, domDocument)

When viewToDom later calls:

domElement = domDocument.createElement(viewNode.name);

It throws the error "Cannot read property 'createElement' of undefined"

And no toolbar appears.

@Mgsy
Copy link
Member

Mgsy commented Jun 3, 2019

@jodator, can you take a look at this?

@Reinmar
Copy link
Member

Reinmar commented Jun 24, 2019

Ping 🔔

@jodator jodator self-assigned this Jun 24, 2019
@jodator
Copy link
Contributor

jodator commented Jun 24, 2019

Sorry for that - if might loos track of this.

So there are two issues:

  1. API usage - the getRelatedElement() must return a View element not the Model element. So you should either use editor.editing.mapper.toViewElement() or use the selection passed to the getRelatedElement() callback or use the editor.editing.view.document.selection directly (the passed selection to the callback is the best option).

  2. In this particular case passing the domDocument does not make sense as we cannot create a view node to attach a toolbar to it. It should be an instance already. So the side effect of throwing an error indicates some errors in code (see point 1).

Anyway the scenario in 2 should be handled better. The domConverter.viewToDom() shouldn't be used here AFAICS. It should only operate on existing view elements the widgetToolbarRepository should call mapViewToDom() instead.

@ansorensen
Copy link
Author

Thanks, I'll give these updates a try.

oleq referenced this issue in ckeditor/ckeditor5-widget Jun 27, 2019
Fix: A proper `DomConverter` method should be used to map a view to DOM when getting balloon position data. Closes #87.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-widget Oct 9, 2019
@mlewand mlewand added this to the iteration 25 milestone Oct 9, 2019
@mlewand mlewand added resolution:solved type:bug This issue reports a buggy (incorrect) behavior. package:widget labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:widget type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants