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

[Plain table output]: The PlainTableOutput overrides downcast converters of other features #11394

Closed
Reinmar opened this issue Mar 3, 2022 · 3 comments · Fixed by #11407
Closed
Assignees
Labels
package:image package:table release:blocker This issue blocks the upcoming release (is critical). squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@Reinmar
Copy link
Member

Reinmar commented Mar 3, 2022

Why

This ticket is a follow-up to #11240.

There's a bug in the converter that it affects conversion of <caption> elements in other widgets than table.

Use this below code (excerpt from ImageCaptionEditing):

// Model -> view converter for the data pipeline.
editor.conversion.for( 'dataDowncast' ).elementToElement( {
    model: 'caption',
    view: ( modelElement, { writer } ) => {
        const imageUtils = editor.plugins.get( 'ImageUtils' );
        if ( !imageUtils.isBlockImage( modelElement.parent ) ) {
            return null;
        }

        return writer.createContainerElement( 'foobar' );
    },
    converterPriority: 'high'
} );

It should override ImageCaptionEditing's converter to produce the <foobar> element instead.

It works without the PlainTableOutput plugin but fails with it (if the order in which plugins are loaded is PlainTableOutput, ImageFooBar).

@Reinmar Reinmar added type:bug This issue reports a buggy (incorrect) behavior. package:image package:table squad:core Issue to be handled by the Core team. release:blocker This issue blocks the upcoming release (is critical). labels Mar 3, 2022
@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Mar 3, 2022
@Reinmar
Copy link
Member Author

Reinmar commented Mar 3, 2022

Let's merge this to the release branch because it seems to be a blocker.

@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Mar 4, 2022
@dawidurbanski
Copy link
Contributor

@Reinmar Pull request is ready to review: #11407

@dawidurbanski
Copy link
Contributor

dawidurbanski commented Mar 4, 2022

This was happening due to the fact that the PlainTableOutput downcast converter was unnecessary creating <figcaption> element for all model elements other than table.

Since this converter is registered with the high priority, only higher priority converters would actually be able to override this behaviour. In case of other high priority converters it was a plugin loading order issue.

The solution was to just simply do nothing inside the converter callback if we detect we are within an element other than table.

@CKEditorBot CKEditorBot added status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Mar 7, 2022
arkflpc added a commit that referenced this issue Mar 7, 2022
…mage-caption-override

Fix (table): Prevent plain table output converter from interfering with other features caption converters. Closes #11394.
@arkflpc arkflpc closed this as completed Mar 7, 2022
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Mar 7, 2022
@Reinmar Reinmar added this to the iteration 51 milestone Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:image package:table release:blocker This issue blocks the upcoming release (is critical). squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
4 participants