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

h4, h5, and h6 fail to downcast GHS added attributes #10539

Closed
lauriii opened this issue Sep 16, 2021 · 1 comment · Fixed by #10664
Closed

h4, h5, and h6 fail to downcast GHS added attributes #10539

lauriii opened this issue Sep 16, 2021 · 1 comment · Fixed by #10664
Assignees
Labels
squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@lauriii
Copy link
Contributor

lauriii commented Sep 16, 2021

📝 Provide detailed reproduction steps (if any)

  1. Enable html-support, source-editing, and headings plugins
  2. Configure GHS to allow all elements and attributes
  3. Configure headings plugins with the example from https://ckeditor.com/docs/ckeditor5/latest/features/headings.html#configuring-toolbar-buttons
  4. Add a h2 element via the headings plugin.
  5. Enter source mode and give it a class of "test".
  6. Exit source mode.
  7. Via the headings plugin, change the h2 to an h3.
  8. Enter source mode and observe that the class "test" is still there.
  9. Exit source mode.
  10. Via the headings plugin, change the h3 to an h4.
  11. Enter source mode and observe that the class "test" is gone!.
  12. Exit source mode.
  13. Via the headings plugin, change the h4 to an h3.
  14. Enter source mode.
  15. Observe that the class "test" is now back!

✔️ Expected result

class attribute should downcast on the heading regardless of its level.

❌ Actual result

class attribute doesn't downcast on h4, h5, and h6.

❓ Possible solution

It seems like https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-html-support/src/schemadefinitions.js maps heading views to elements. However, not all supported heading views are mapped. Also, the mapping assumes that heading1 is mapped to h2 while its mapping can be configured.


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

@lauriii lauriii added the type:bug This issue reports a buggy (incorrect) behavior. label Sep 16, 2021
@Reinmar Reinmar added the squad:core Issue to be handled by the Core team. label Sep 28, 2021
@Reinmar
Copy link
Member

Reinmar commented Sep 29, 2021

We need to remove this:

{
model: 'heading1',
view: 'h2'
},
{
model: 'heading2',
view: 'h3'
},
{
model: 'heading3',
view: 'h4'
},

And add GHS's integration with the Heading feature, because the list of headings that it supports is configurable via config.heading.items. The model and view element names are fully configurable there, so a static definition in schemadefinitions.js makes no sense.

Flow:

  1. Introduce HeadingIntegration plugin (see src/integrations/).
  2. Check whether Heading is enabled. Abort if not.
  3. Read config.heading.options (ignore potential validation/normalization of invalid config)
  4. Add these options via DataSchema.registerBlockElement() with just the model and view properties
  5. Remove the existing heading1-heading3 definitions from schemadefinitions.js.
  6. Add HeadingIntegration in GeneralHtmlSupport's requirements.

Scope:

  • Solution
  • Tests

@arkflpc arkflpc self-assigned this Oct 5, 2021
niegowski added a commit that referenced this issue Oct 13, 2021
Fix (html-support): Adds HTML support for all headings given in the configuration of the headings feature. Closes #10539.
@Reinmar Reinmar added this to the iteration 48 milestone Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants