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

Upcasting a $text element over an already upcasted property should work #13383

Closed
idiazroncero opened this issue Feb 2, 2023 · 3 comments
Closed
Labels
resolution:expired This issue was closed due to lack of feedback. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@idiazroncero
Copy link

📝 Provide detailed reproduction steps (if any)

This is related to Upcasting an element that has multiple attributes may not work #4306 and it might be considered a feature request instead of a bug.

Summary: Issue #4306 solved a bug where multiple upcasts couldn't address the same $text element because the first one to act would consume the tagname.

The solution was to consume the item when view only specifies name and, if not, consume the property/properties (i.e class, ID, data-attribute, etc)

The problem is that this also prevents upcasting the same property several times, which might make sense for plugins wanting to act independently of each other on the same properties.

Scenario we use a plugin that stores on linkClass any existing link classes on an attribute to expose an UI for handling them. This upcast acts on a elements with classes: true.

    conversion.for('upcast')
      .elementToAttribute( {
        model: {
          key: 'linkClass',
          value: viewElement => {
            console.log('Upcasted classes');
            return viewElement.getAttribute('class');
          }
        },
        view: {
          name: 'a',
          classes: true,
        },
      });

We have another plugin that looks for an specific class to populate a linkSize attribute. This upcast acts on a classes that have an specific class:

    conversion.for('upcast')
      .elementToAttribute( {
        model: {
          key: 'linkSize',
          value: viewElement => {
            console.log('Upcasted size');
            return 'lg';
          },
        },
        view: {
          name: 'a',
          classes: 'btn-lg',
        }
      });

✔️ Expected result

Data upcasting should populate both attributes:

Screenshot_20230202_135411

❌ Actual result

Data upcasting will only happen for the first occurence (or higher priority) upcast. It will consume the a.classes and further upcasts will fail to act.

Screenshot_20230202_135552

❓ Possible solution

  1. Don't consume $text properties. Of course I'm aware that this might have tons of architectural implications but at the same time I find counterintuitive the idea that each element property can be consumed only once.

  2. Provide developers with an API or a way to easily "bypass" property consumption.

  3. ...or maybe Iim just missing something.

📃 Other details

CKEDITOR_VERSION: 36.0.0
Installed plugins: Essentials, Paragraph, Heading, List, Bold, Italic, Link and a custom testing plugin that pretty much includes the aforementioned code.


If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@idiazroncero idiazroncero added the type:bug This issue reports a buggy (incorrect) behavior. label Feb 2, 2023
@idiazroncero
Copy link
Author

I tried to solve this with a custom dispatcher that should consume and then revert the classes: ['btn-lg'], but it didn't work.

    conversion.for('upcast')
      .add(dispatcher => {
        dispatcher.on('element:a', (evt, data, conversionApi) => {
          if ( conversionApi.consumable.consume(data.viewItem, { classes: ['btn-lg'] })) {
            const { modelRange } = conversionApi.convertChildren( data.viewItem, data.modelCursor );

            for ( let item of modelRange.getItems() ) {
              if ( conversionApi.schema.checkAttribute( item, 'linkSize' ) ) {
                conversionApi.writer.setAttribute( 'linkSize', 'custom-lg', item );
              }
            }
            conversionApi.consumable.revert(data.viewItem, { classes: ['btn-lg'] });
          }
        });
      });

This upcast actually prevents other upcasts from acting on links (see the lacking linkHref on links 2 and 3, which have the btn-lg class)

Screenshot_20230202_144838

@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 still 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. 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 resolution:expired This issue was closed due to lack of feedback. and removed status:stale labels Mar 5, 2024
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolution:expired This issue was closed due to lack of feedback. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

2 participants