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

Conflict between GeneralHtmlSupport and FontBackgroundColor #15757

Closed
sunesimonsen opened this issue Jan 25, 2024 · 8 comments · Fixed by #15868
Closed

Conflict between GeneralHtmlSupport and FontBackgroundColor #15757

sunesimonsen opened this issue Jan 25, 2024 · 8 comments · Fixed by #15868
Assignees
Labels
package:font package:html-support package:table squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@sunesimonsen
Copy link

sunesimonsen commented Jan 25, 2024

📝 Provide detailed reproduction steps (if any)

On the branch ssimonsen/general-html-support the editor renders the background color for the heading:

Screenshot 2024-01-25 at 10 49 58

But when the BackgroundFontColor plugin is added the background color is no longer rendered:

Screenshot 2024-01-25 at 10 53 32

I tracked the problem do to this line, when removed everything works again.

Minimal reproduction can be found here.

Notice that the table plugin also introduces the bug, as it also makes use of addBackgroundRules.

This bug is currently blocking Zendesk customers from applying background styles.

✔️ Expected result

That the background color is maintained.

❌ Actual result

The background inline style is stripped.

❓ Possible solution

Make addBackgroundRules compatible with the general HTML support plugin.

📃 Other details

  • First affected CKEditor version: probably all
  • Installed CKEditor plugins: @ckeditor/ckeditor5-basic-styles, @ckeditor/ckeditor5-essentials, @ckeditor/ckeditor5-paragraph, @ckeditor/ckeditor5-html-support, @ckeditor/ckeditor5-font

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

@sunesimonsen sunesimonsen added the type:bug This issue reports a buggy (incorrect) behavior. label Jan 25, 2024
@lslowikowska lslowikowska added the support:2 An issue reported by a commercially licensed client. label Jan 29, 2024
@FilipTokarski
Copy link
Member

I was able to reproduce it.

The only way to make it work for me, was to change GHS config to:

allow: [
  {
       name: "h1",
       styles: true,
   }
]

This also converted original background property to background-color.

cc @Witoso

@FilipTokarski FilipTokarski added the squad:core Issue to be handled by the Core team. label Jan 29, 2024
@sunesimonsen
Copy link
Author

@Witoso the background also conflicts with the table plugin, so maybe this issue should also have the package:table label.

@niegowski niegowski self-assigned this Feb 12, 2024
@CKEditorBot CKEditorBot added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Feb 12, 2024
@niegowski
Copy link
Contributor

I was able to reproduce it.

The only way to make it work for me, was to change GHS config to:

allow: [
  {
       name: "h1",
       styles: true,
   }
]

This also converted original background property to background-color.

This would also work with the following GHS config:

allow: [
  {
       name: "h1",
       styles: [ 'background-color' ],
   }
]

The above would properly handle both background: red and background-color: red.

This is happening because the editor (while loading data) parses the styles and normalizes values so that shorthand notations can be handled transparently as longhand notations (background: red => background-color: red). Since we handle only the background-color style then we can't reduce it to the shorthand notation because we do not know if there is no other rule for background that could interfere.

@sunesimonsen
Copy link
Author

This is happening because the editor (while loading data) parses the styles and normalizes values so that shorthand notations can be handled transparently as longhand notations (background: red => background-color: red). Since we handle only the background-color style then we can't reduce it to the shorthand notation because we do not know if there is no other rule for background that could interfere.

I just checked, using the following configuration also conflicts:

      htmlSupport: {
        allow: [
          {
            name: "h1",
            styles: ["background-color"],
          },
        ],
      },

@sunesimonsen
Copy link
Author

@Witoso do you have any timeline for when this bug will be fixed? The reason why I'm asking is because Zendesk is currently holding back on realizing a new feature because of this bug ☺️

@Witoso
Copy link
Member

Witoso commented Feb 15, 2024

@niegowski is doing a research right now, as it turned out to be more complex than we anticipated. Should know soon the scope, and then we will decide on the next steps. cc @arkflpc

niegowski added a commit that referenced this issue Feb 22, 2024
Fix (html-support): Background color style should be properly preserved by GHS while the `FontBackgroundColor` plugin is enabled. It should be able to preserve a partly defined style. Closes #15757. Closes #10399.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Feb 22, 2024
@CKEditorBot CKEditorBot added this to the iteration 72 milestone Feb 22, 2024
@sunesimonsen
Copy link
Author

🥇 thanks a lot - I'll test it as soon as it have been released.

@sunesimonsen
Copy link
Author

The new release seems to have fixed the problem - thank you so much 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:font package:html-support package:table squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants