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

[GHS] Allowing custom attribute values using *lists* instead of RegExps (in MatcherPattern), just like CKE5 allows for classes #11155

Closed
wimleers opened this issue Jan 24, 2022 · 5 comments
Labels
package:html-support resolution:expired This issue was closed due to lack of feedback. squad:core Issue to be handled by the Core team. status:stale type:bug This issue reports a buggy (incorrect) behavior.

Comments

@wimleers
Copy link

wimleers commented Jan 24, 2022

📝 Provide detailed reproduction steps (if any)

  1. Enable the following plugins in CKEditor 5 31.1.0: Paragraph, BlockQuote, Heading, Link and GeneralHtmlSupport. Configure the latter like this:
[
  {
    "name": "cite",
    "attributes": {
      "foo": ["bar"]
    }
  },
  {
    "name": "a",
    "attributes": {
      "foo": ["bar"],
      "hreflang": true
    },
    "classes": [
      "button"
    ]
  },
  {
    "name": "blockquote",
    "attributes": {
      "foo": ["bar"],
    }
  },
  {
    "name": "h6",
    "attributes": {
      "foo": ["bar"],
      "id": true
    }
  },
  {
    "name": "p",
    "attributes": {
      "foo": ["bar"]
    }
  }
]
  1. Paste this content:
<p foo="bar">
    <a class="button" foo="bar">sdfsdfasdf</a>
</p>
<blockquote foo="bar">
    <p>
        thoughtful advice here
    </p>
</blockquote>
<cite foo="bar">nobody</cite>
<h6 foo="bar" id="dfsdf">heading 6 with custom attribute thanks to GHS</h6>

Note that <cite> is only possible thanks to GHS, <a> is provided by the complex Link plugin, <blockquote> and <p> are simple plugins yet it won't work there either, <h6> is provided by a pretty simple plugin.

✔️ Expected result

The same as the input, minus whitespace changes. Notably, all foo="bar" occurrences are retained

❌ Actual result

All foo="bar" occurrences are stripped.

❓ Possible solution

I first thought this was related to #9914, #9916, #11000, et cetera (all listed in #9856). But it's happening everywhere.

📃 Other details

  • Browser: N/A
  • OS: N/A
  • First affected CKEditor version: Unknown
  • 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 Jan 24, 2022
@wimleers wimleers changed the title [GHS] Allowing custom attributes with GHS never seems to work: on <a>, <blockquote>, <p>, <h6> [GHS] Allowing custom attributes with GHS never seems to work: on <cite>, <a>, <blockquote>, <p>, <h6> Jan 24, 2022
@wimleers
Copy link
Author

Digging deeper … turns out that https://ckeditor.com/docs/ckeditor5/latest/api/module_engine_view_matcher-MatcherPattern.html actually suggests that listing allowed values is not possible, unlike what https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/features/general-html-support.html#configuration suggests.

https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/features/general-html-support.html#configuration explicitly lists classes: ['foo', 'bar']. But it does not list that for individual attributes … 🤔

And … yep: https://www.drupal.org/project/drupal/issues/3259493#comment-14383513 → converting it to using RegExp syntax instead does the trick:

[
  {
    "name": "cite",
    "attributes": {
      "foo": /^(bar)$/
    }
  },
…

It'd be great if:

  1. docs could be improved
  2. better yet: support for listing allowed values were added

@wimleers wimleers changed the title [GHS] Allowing custom attributes with GHS never seems to work: on <cite>, <a>, <blockquote>, <p>, <h6> [GHS] Allowing custom attribute values using *lists* instead of RegExps, just like CKE5 allows for classes Jan 24, 2022
@wimleers wimleers changed the title [GHS] Allowing custom attribute values using *lists* instead of RegExps, just like CKE5 allows for classes [GHS] Allowing custom attribute values using *lists* instead of RegExps (in MatcherPattern), just like CKE5 allows for classes Jan 24, 2022
@Reinmar
Copy link
Member

Reinmar commented Jan 26, 2022

unlike what https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/features/general-html-support.html#configuration suggests.

Where does it suggest that an object of attrName => arrayOfValues is allowed? The notation there is tricky to read, but I don't think it suggests that such an object is allowed.

But I agree that it would be nice if this format was also supported, for completeness. 

So, as a scope of this ticket I'd consider:

  • Adding support for { attributes: { foo: ARRAY_OF_WHATEVER } }
  • Rewriting this into something more readable:

@Reinmar Reinmar added squad:core Issue to be handled by the Core team. package:html-support labels Jan 26, 2022
@wimleers
Copy link
Author

Where does it suggest that an object of attrName => arrayOfValues is allowed? The notation there is tricky to read, but I don't think it suggests that such an object is allowed.

It does by example:

            classes: [ 'foo', 'bar' ]

… if this works for class, I expect it to work for other attributes too. Yes, even though class is a special case.

The notation docs say:

        // Classes to allow (by name or just all).
        classes: array<string|regexp>|true,

        // Other attributes to allow (by name, name and value or just all).
        attributes: object<string=>true|string|regexp>|array<string>|true,

Both contain array<string>, which also led me to this conclusion. Of course, the first one operates at the level of class attribute values, the second at the level of attribute names. Subtle but important difference.

So yes, you're right, technically it does not say that. But it feels like a very reasonable assumption to make, and there's zero validation on this configuration, meaning that you have to actually debug CKEditor 5 code in order for you to know for sure.

(Validation would be a good idea either way — even if this does not happen.)

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added the resolution:expired This issue was closed due to lack of feedback. label Oct 31, 2023
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:html-support resolution:expired This issue was closed due to lack of feedback. squad:core Issue to be handled by the Core team. status:stale type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

3 participants