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

<ul type> and <ol type> not supported by List nor ListProperties #11615

Closed
wimleers opened this issue Apr 11, 2022 · 12 comments · Fixed by #11699
Closed

<ul type> and <ol type> not supported by List nor ListProperties #11615

wimleers opened this issue Apr 11, 2022 · 12 comments · Fixed by #11699
Assignees
Labels
package:list squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@wimleers
Copy link

📝 Provide detailed reproduction steps (if any)

While bringing the <ol start> and <ol reversed> functionality to Drupal core, we’re also looking at <ol type> and <ul type>, because as https://ckeditor.com/blog/ckeditor-5-v32.0.0-with-new-list-properties-support-for-the-script-tag-and-enhanced-mentions/ mentions, this is now supported too and has a magnificent UX.

https://ckeditor.com/docs/ckeditor5/latest/api/module_list_listproperties-ListPropertiesConfig.html#member-styles says:

When set, the list style feature will be enabled. It allows changing the list-style-type HTML attribute of the lists.

But:

  1. There is no list-style-type HTML attribute. Only a type attribute.
  2. There seems to be no way to configure CKEditor 5 to generate a type attribute, only list-style-type.
  3. Searching the CKE5 codebase finds plenty of matches for the strings <ul style="list-style-type and <ol style="list-style-type, but zero for <ul type and <ol type

Conclusion:

Because ListProperties' support for list style types does not work for <ol type> or <ul type>, despite that being in the HTML spec:

→ No downcast support, but also, more worryingly still, no upcast support.

Therefore the only work-around is to use GHS to add support for these. But … that also does not work:

  1. Load the following CKE5 plugins: htmlSupport.GeneralHtmlSupport, list.List, with the following htmlSupport config:
{"allow":[{"name":"ul","attributes":{"type":true},"classes":true,"styles":true}]}
  1. Instantiate CKEditor 5 on a textarea containing markup like:
<ol type="A">
  <li>test</li>
</ol>

I strongly suspect that v32 caused this to not work with GHS anymore, because I do recall testing this a long time ago and it worked fine then.

✔️ Expected result

CKEditor 5 retains the type attribute.

❌ Actual result

It does not.

📃 Other details

  • Browser: N/A — reproduced in Chrome and Firefox.
  • OS: N/A — reproduced in macOS Big Sur
  • First affected CKEditor version: v33.0.0
  • Installed CKEditor plugins: see above.

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

@wimleers wimleers added the type:bug This issue reports a buggy (incorrect) behavior. label Apr 11, 2022
@wimleers
Copy link
Author

Sibling Drupal issue: https://www.drupal.org/project/drupal/issues/3274651

@wimleers
Copy link
Author

Related: #11595.

@Reinmar
Copy link
Member

Reinmar commented Apr 11, 2022

Quick note about GHS + list: The old list feature had no integration with GHS. Only the new one (document lists) has. 

I strongly suspect that v32 caused this to not work with GHS anymore, because I do recall testing this a long time ago and it worked fine then.

AFAIR it didn't work when we discussed it the last time. Also, I think it could never work via GHS because we never had integration with lists for it.

@Reinmar Reinmar added the squad:core Issue to be handled by the Core team. label Apr 11, 2022
@Reinmar
Copy link
Member

Reinmar commented Apr 13, 2022

I've just checked that loading the below content to CKEditor 4:

<ol type="A">
    <li>sdfsdf</li>
    <li>sdfsdf</li>
    <li>sfsdf</li>
</ol>

Makes it return this data:

<ol style="list-style-type:upper-alpha">
    <li>sdfsdf</li>
    <li>sdfsdf</li>
    <li>sfsdf</li>
</ol>

In other words, CKEditor 4 normalizes type to list-style-type.

We could add similar logic to the CKEditor 5 document list's upcast.

Random thoughts:

  • Content in which both things are used (type and list-style-type) -> Do the same what CKE4 does.
  • Probably to be handled in a strategy (getAttributeOnUpcast()).

@wimleers
Copy link
Author

Quick note about GHS + list: The old list feature had no integration with GHS. Only the new one (document lists) has.

Does that mean that per this bit of #2973 (comment):

This feature will be a part of the upcoming v34.0.0 release. However, due to its size and complexity, we'll keep it on an "experimental" level for 1-2 releases, meaning that we don't recommend using it in production without assessing the risk. For now, it isn't even covered in the documentation, although this should change sooner.

… Drupal updating to CKEditor 5 v34.0.0 didn't actually solve this for us?

@wimleers
Copy link
Author

It definitely worked fine in CKEditor 4 + ACF — this is a freshly installed Drupal 9, with no configuration changes at all. This is how it works out of the box.

ol.type.in.CKE4.mov

@lauriii
Copy link
Contributor

lauriii commented Apr 21, 2022

I did some research on whether it would be feasible to override the downcast and upcast for this per request from @wimleers.

We could do a pretty scoped override if we could either override createAttributeStrategies or the array returned by that function. Is that something that CKEditor 5 could provide us as an API for this use case?

If that's not feasible, we could still override the upcast and downcast for the listStyle attribute and hardcode the assumptions from list/listproperties/listpropertiesediting in our code. What could be helpful is if you could provide at least areRepresentingSameList function as an exported function so we wouldn't have to duplicate that, since it seems like something that would change in case the list plugin receives new features.

@Reinmar
Copy link
Member

Reinmar commented Apr 27, 2022

We discussed this on a refinement meeting whether it'll be easier for us to make the current downcast/upcast strategies easily extensible/overridable or simply implementing the support for config.list.properties.styles.useAttribute = true.

We came to a conclusion that the config option will be easier.

@Reinmar
Copy link
Member

Reinmar commented Apr 27, 2022

Scope:

  • The changes will affect only DocumentList.
  • Extend the upcast callback for the type attribute. In case when both the attribute and style are specified, follow what the browser prioritizes.
  • Add support for the config.list.properties.styles.useAttribute = true option.
  • Document this in the API docs for ListPropertiesConfig. Explain that it works only in DocumentListProperties.
  • Make sure to map the values on upcast and downcast. We should use the same set of values in the model, regardless of whether a style or an attribute is used in the data.

@Reinmar
Copy link
Member

Reinmar commented Apr 27, 2022

There's a problem that the `type` attribute supports a subset of the options supported by CSS and by our UI.

Values supported by the attribute:

After https://html.spec.whatwg.org/multipage/grouping-content.html#attr-ol-type

Keyword State Description Examples for values 1-3 and 3999-4001
1 (U+0031) decimal Decimal numbers 1.
a (U+0061) lower-alpha Lowercase latin alphabet a.
A (U+0041) upper-alpha Uppercase latin alphabet A.
i (U+0069) lower-roman Lowercase roman numerals i.
I (U+0049) upper-roman Uppercase roman numerals I.

Our UI:

So the attribute does not support the 01. option.

Proposed approach:

  • Hide the 01. option in the UI when the useAttribute option is used.
  • When upcasting from list-style-type, when we have a value unsupported by the type attribute, use DEFAULT_LIST_TYPE. In other words, when useAttribute is true we may have only 5 values in the model.

@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 Apr 27, 2022
@arkflpc arkflpc self-assigned this Apr 27, 2022
@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 Apr 27, 2022
@wimleers
Copy link
Author

and not retained in GHS #11615

This piece of the title can be removed — see #11595 (comment) 😊

Scope:

  • The changes will affect only DocumentList.
  • Extend the upcast callback for the type attribute. In case when both the attribute and style are specified, follow what the browser prioritizes.
  • Add support for the config.list.properties.styles.useAttribute = true option.
  • Document this in the API docs for ListPropertiesConfig. Explain that it works only in DocumentListProperties.
  • Make sure to map the values on upcast and downcast. We should use the same set of values in the model, regardless of whether a style or an attribute is used in the data.

👍 Sounds like a great plan!

There's a problem that the type attribute supports a subset of the options supported by CSS and by our UI.
[…]
Proposed approach:

  • Hide the 01. option in the UI when the useAttribute option is used.
  • When upcasting from list-style-type, when we have a value unsupported by the type attribute, use DEFAULT_LIST_TYPE. In other words, when useAttribute is true we may have only 5 values in the model.

👍 This seems totally reasonable 😊

@wimleers wimleers changed the title <ul type> and <ol type> not supported by List nor ListProperties, *and* not retained in GHS <ul type> and <ol type> not supported by List nor ListProperties Apr 28, 2022
@wimleers
Copy link
Author

Just to make sure we’re on the same page: because the “hard crash” part of <ol type> and <ul type> was indeed fixed by Drupal updating to https://www.npmjs.com/package/@ckeditor/ckeditor5-list at 34.0.1 (which you released 3 days ago), #11615 (this issue) is not critical.

IOW: your theory was correct, once we switched to DocumentList, supporting <ul type> and <ol type> through GHS works fine 👍 We still want #11615 (this issue) to be able to offer your vastly superior native UX though!

For that we have https://www.drupal.org/project/drupal/issues/3274635. 🚀

oleq added a commit that referenced this issue May 6, 2022
Feature (list): Added support for the `type` attribute of `<ul>` and `<ol>` elements in addition to the `list-style-type` style. Closes #11615.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label May 6, 2022
@CKEditorBot CKEditorBot added this to the iteration 53 milestone May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:list 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.

5 participants