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

Add style (Styles-Plugin) to anchor leads to unuseable behaviour #14683

Closed
sascha-meissner opened this issue Jul 28, 2023 · 13 comments · Fixed by #15226
Closed

Add style (Styles-Plugin) to anchor leads to unuseable behaviour #14683

sascha-meissner opened this issue Jul 28, 2023 · 13 comments · Fixed by #15226
Assignees
Labels
domain:ui/ux This issue reports a problem related to UI or UX. intro Good first ticket. package:html-support squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@sascha-meissner
Copy link

sascha-meissner commented Jul 28, 2023

📝 Provide detailed reproduction steps (if any)

Make a ckeditor5 build with the Styles-plugin included

take this codesnippet

<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Document</title>
    <style>
      .button {
        padding:25px;
        background:#eaeaea;
      }
    </style>
  </head>
<body>
  <div id="editor"></div>
  <!-- Make sure this points to the built ckeditor.js -->
  <script src="/ckeditor/ckeditor.js"></script>
  <script>
    ClassicEditor
      .create( document.querySelector( '#editor' ), {
        toolbar: {
            items: [
                'style',
                'link'
            ],
        },
        style: {
            definitions: [
                {
                  name: 'Button',
                  element: 'a',
                  classes: [ 'button' ]
                }
            ]
        }
      } )
      .then( editor => {
        window.editor = editor;
      } )
      .catch( err => {
        console.error( err.stack );
      } );
  </script>
</body>
</html>

Now, add some text, select it and make a link out of it, now when selecting the link-text, the style "button" will get avaiblable in the styles-dropdown, select the style - Now see the "Actual result" part of this issue.

✔️ Expected result

When applying the "button-style" to an anchor, a hit on return should "leave the anchor and exit the style" (like in ckeditor4)

❌ Actual result

When applying the "button-style" to an anchor, there is no way to continue writing without the style, when hitting return, the style will further be applied but cannot be removed (the style in the styles button is greyed out, even though you see visually that it is applied).

jithub__1

❓ Possible solution

When hitting enter the style should not be further applied

📃 Other details

  • Browser: FF / Chrome
  • OS: Windows/Mac
  • First affected CKEditor version: Ckeditor5
  • Installed CKEditor plugins: Styles

I really hope this is no duplicate, i can reproduce this with the latest 38.1 Version

@sascha-meissner sascha-meissner added the type:bug This issue reports a buggy (incorrect) behavior. label Jul 28, 2023
@niegowski
Copy link
Contributor

The GHS has an invalid schema definition for htmlA:


This line should not be here same as for the linkHref attribute. cc @Witoso

@niegowski niegowski added the intro Good first ticket. label Aug 1, 2023
@Witoso
Copy link
Member

Witoso commented Aug 7, 2023

I wonder @sascha-meissner why don't you use Link plugin? With it, everything works correctly, and escape is possible.

Screen.Recording.2023-08-07.at.10.00.43.mov

(@niegowski 's case is to fix anyway)

@Witoso Witoso added package:html-support domain:ui/ux This issue reports a problem related to UI or UX. labels Aug 7, 2023
@sascha-meissner
Copy link
Author

@niegowski Thanks for your answer and @Witoso Thanks for your suggestion, My example code here was to break the error down to its root cause. Actually i´m using Ckeditor5 within Drupal10 (Drupal ships this with it´s core and has an own link-implementation which will supply an autocomplete to target content, and for this link-implementation the same error appears like described in my minimal code example)

@wimleers
Copy link

wimleers commented Aug 8, 2023

This seems like a similar behavior/bug to #14216, but based on #14683 (comment) by @niegowski, it sounds like it'll need to be fixed upstream anyway?

I wonder @sascha-meissner why don't you use Link plugin? With it, everything works correctly, and escape is possible.

I'm confused by this statement 😅 @sascha-meissner is showing that he's using the link + style plugins, and based on your video, you're doing the same? 🤔

What am I missing? 🙈


Upstream Drupal issue: https://www.drupal.org/project/drupal/issues/3376167

@Witoso
Copy link
Member

Witoso commented Aug 8, 2023

@sascha-meissner showed an example w/o Link plugin, only with GHS*, where a link is an htmlA element in the model. I'm not certain how impactful this is for Drupal, do you have a link plugin that behaves differently than our link (uses htmlA?)

*GHS is a dependency of Style.

@niegowski
Copy link
Contributor

@sascha-meissner showed an example w/o Link plugin, only with GHS*, where a link is an htmlA element in the model. I'm not certain how impactful this is for Drupal, do you have a link plugin that behaves differently than our link (uses htmlA?)

@Witoso. It's not that. The Link feature uses the linkHref attribute but GHS can extend the link behavior by storing additional data in htmlA attribute. The existence of both is completely valid and does not mean that the Link feature is not used.

@Witoso
Copy link
Member

Witoso commented Aug 8, 2023

Gotcha, got confused about the minimal build mentioned above, I thought it's very minimal :) 

  1. Link plugin and GHS are enabled.
  2. Additional attributes htmlA may be added:
  3. And are copied on enter which shouldn't happen (This line should not be here same as for the linkHref attribute.)

@Witoso
Copy link
Member

Witoso commented Aug 8, 2023

Workaround: removing the flag from the schema after initialization:

editor.model.schema.setAttributeProperties( 'htmlA', { copyOnEnter: false } )

@wimleers
Copy link

wimleers commented Aug 9, 2023

@Witoso Is that enough to completely fix the problem, without side effects for other functionality?

@Witoso
Copy link
Member

Witoso commented Aug 9, 2023

Attaching video of what happens after the workaround:

Screen.Recording.2023-08-09.at.15.07.49.mov
  • Pressing enter inside the link, splits it and the style is copied. Possible to disable style (as before).
  • Pressing enter at the end of the link creates a new line without any htmlA properties (fix).

without side effects for other functionality?

No, it shouldn't affect other features unless someone created something that expects this is the way it works 🤔. But that would be an extremely rare case, I think. This is a bug, for sure.

@wimleers
Copy link

wimleers commented Oct 3, 2023

If it's really a one-liner to fix, can we please get this in the upcoming release? 🙏

@Witoso Witoso added the squad:core Issue to be handled by the Core team. label Oct 3, 2023
@el7cosmos
Copy link

apply the suggestion workaround, but still have the problem when clicking outside the link

and also when backspace after hitting the enter button.

Screen.Recording.2023-10-09.at.13.34.57.mov

@Witoso
Copy link
Member

Witoso commented Oct 10, 2023

May be related: #15051.

@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 Oct 18, 2023
@mmotyczynska mmotyczynska self-assigned this Oct 24, 2023
@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 Oct 24, 2023
niegowski added a commit that referenced this issue Oct 25, 2023
…o-unusable-behaviour

Fix (html-support): Additional attributes for the link element (e.g. CSS class) should not be applied after pressing Enter. Closes #14683.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Oct 25, 2023
@CKEditorBot CKEditorBot added this to the iteration 68 milestone Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ui/ux This issue reports a problem related to UI or UX. intro Good first ticket. package:html-support squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants