ckeditor / ckeditor5

Powerful rich text editor framework with a modular architecture, modern integrations, and features like collaborative editing.
https://ckeditor.com/ckeditor-5
Other
9.33k stars 3.68k forks source link

[GHS] Improve compatibility with link decorators #12394

Open Mgsy opened 2 years ago

Mgsy commented 2 years ago

πŸ“ Provide detailed reproduction steps (if any)

  1. Open a sample with GHS + enable all + addTargetToExternalLinks configuration.
  2. Insert a link with the rel attribute, e.g. editor.setData('<a href="https://google.com" rel="test">Foo</a>').
  3. Call editor.getData();.

❌ Actual result

The link has been divided: <a target="_blank" rel="noopener noreferrer" href="https://google.com"><a rel="test">Foo</a></a>.


If you'd like to see this fixed sooner, add a πŸ‘ reaction to this post.

Reinmar commented 1 year ago

The inner link (<a rel>) is outputted by GHS. The outer one by the automatic decorator. Those two aren't merged together because there's a conflict - both use the rel attribute.

It should happen here:

https://github.com/ckeditor/ckeditor5/blob/56c56070d0caa0989fd8bfa83accb5ac3f024912/packages/ckeditor5-engine/src/view/downcastwriter.ts#L1429-L1439

We treat only class and style granularly and merge them. See:

https://github.com/ckeditor/ckeditor5/blob/56c56070d0caa0989fd8bfa83accb5ac3f024912/packages/ckeditor5-engine/src/view/downcastwriter.ts#L1708-L1719

All other attributes are not merged and prevent merging elements.

Reinmar commented 1 year ago

Related issue: ckeditor/ckeditor5#6436.

Reinmar commented 1 year ago

The problem is that the logic to treat these attributes granularly would need to change in several places. A bit like the history with StylesProcessor that is now widely used in the view logic. In other words, this would be a big change to make rel work granularly.

Additionally, we'd need to make this extensible, just like we did with StylesProcessor. Here we have a problem with rel, but what if someone expected data-rels to also work granularly?

sateesh-r commented 1 year ago

This issue can be reproduced using the Manual decorator using below steps

  1. Create ckeditor from sources (https://ckeditor.com/docs/ckeditor5/latest/installation/getting-started/quick-start-other.html#building-the-editor-from-source)

  2. Add Link, GHS and SourceEdit plugins.

  3. In the app.js file add below Link and GHS configuration and rebuild it

    link: {
    decorators: {
        openInNewTab: {
            mode: 'manual',
                label: 'Open in a new tab',
                    defaultValue: true,
                        attributes: {
                target: '_blank',
                }
        }
    }
    },
    htmlSupport: {
    allow: [
        {
            name: /.*/,
            attributes: true,
            classes: true,
            styles: true
        }
    ],
    } 
  4. Open index.html, in the editor using the Link plugin create link for http://www.google.com/

  5. Click Source button and we see below <a href="http://www.google.com" target="_blank">Link</a>

  6. In source Mode change the target from _blank to _self

  7. Exit Source Mode

  8. Edit the link in Normal mode (WYSWIG ) and observe the β€œOpen in a new window” is toggled to off, which is expected.

  9. Now toggle it to ON to make the link open in new tab.

  10. Click on the Source Button and we see the link has been divided (even though we don't use rel here

@Reinmar I think issue is with not only rel, probably with target also

sateesh-r commented 1 year ago

Another example, where it will fail with existing data for manual decorator (created using above my previous comment)

  1. Create the below content and set it in index.html inside editor div <a href="http://www.google.com" target="_self">Link</a>
  2. Edit the link in WYSIWYG editor with link plugin, Turn on the toggle to open in new window.
  3. Go to Source mode and and observe the same issue, see below <a href="http://www.google.com" target="_self"><a target="_blank">Link</a></a>

I think this will be major issue, If some one edits the link through link plugin on existing data.