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

Remove format does not remove styles applied by style dropdown plugin #11580

Closed
Tracked by #11574
FilipTokarski opened this issue Apr 7, 2022 · 10 comments · Fixed by #13949
Closed
Tracked by #11574

Remove format does not remove styles applied by style dropdown plugin #11580

FilipTokarski opened this issue Apr 7, 2022 · 10 comments · Fixed by #13949
Assignees
Labels
package:remove-format package:style squad:core Issue to be handled by the Core team. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@FilipTokarski
Copy link
Member

📝 Provide detailed reproduction steps (if any)

  1. Open style plugin manual test
  2. Add some inline and block styles to the content
  3. Try using Remove format button on various styled content

✔️ Expected result

Remove format button works at least on inline styles.

❌ Actual result

Remove format does nothing or is inactive.


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

@FilipTokarski FilipTokarski added type:bug This issue reports a buggy (incorrect) behavior. package:remove-format squad:core Issue to be handled by the Core team. release:potential-blocker This issue potentially blocks the upcoming release (should be checked). package:style labels Apr 7, 2022
@mlewand mlewand removed the release:potential-blocker This issue potentially blocks the upcoming release (should be checked). label Apr 8, 2022
@caspervoogt
Copy link

caspervoogt commented Nov 21, 2022

According to https://ckeditor.com/docs/ckeditor5/latest/features/remove-format.html, "The remove format feature allows you to quickly remove any text formatting applied using inline HTML elements and CSS styles, like basic text styles (bold, italic, etc.), font family, size, and color and similar."

That excludes block level elements (though I don't understand why we wouldn't want to also be able to remove styles from block level elements, but that's another issue).

In my testing (which you can view in a screencast at https://loom.com/share/5a0add14f4bf443fae07dbbd15e7db34), styles do not get removed from <span> elements in certain scenarios, which are inline elements, and should have their styles removed by this plugin. It isn't working as I believe it should, based on the plugin description.

@caspervoogt
Copy link

I added a screencast showing this issue; https://loom.com/share/5a0add14f4bf443fae07dbbd15e7db34

@wimleers
Copy link

Also reported by @caspervoogt on drupal.org: https://www.drupal.org/project/drupal/issues/3321254

@njsoftdev
Copy link

Short workaround might help if use GHS plugin

config.htmlSupport = {
    disallow: [
        {
            styles: false
        }
    ]
};

@aaronbauman
Copy link

Chiming in to add that, after pasting formatted text, clicking "remove formatting" does nothing - doesn't remove styles, links, any text decorations, etc.

This is a regression from CK4, where "remove formatting" didn't work great but removed at least some formatting.

@Witoso
Copy link
Member

Witoso commented Apr 3, 2023

After some investigation, we noticed a simple solution that would allow us to fix it quickly but it comes with some quirks.

When we implement the change, Remove Format will remove all attributes set on the elements it has selected. Classes, id's, data-*, styles. So it will result in data loss which may not be related to the format. But it will not differ from the behavior of CKEditor 4 which does exactly the same, as tested here.

Potential Scope: Set isFormatting: true on all inline GHS elements to make Remove Format work.

@Reinmar, thoughts?

@Reinmar
Copy link
Member

Reinmar commented Apr 3, 2023

Testing this on CKE4 is tricky because there are many aspects that distort the results.

First of all, as you mentioned, this needs to be tested with ACF off. The "Disabling ACF" sample in https://ckeditor.com/docs/ckeditor4/latest/examples/acf.html is ok.

Second, different types of elements may be treated differently.

Finally, selection handling isn't always... intuitive.

Handling blocks

Let's start with:


<p class="bar" data-foo="1" style="color: blue">aaa</p>
<p class="bar" data-foo="1" style="color: blue">aaa</p>
<p class="bar" data-foo="1" style="color: blue">aaa</p>

Press Ctrl+A and Remove format. Result:


<p class="bar" data-foo="1" style="color: blue">aaa</p>

<p data-foo="1">aaa</p>

<p data-foo="1">aaa</p>

The first block wasn't discovered by CKE4 as "fully selected" and hence it was skipped. In other blocks, as you can see, the data attribute was retained. And this, to me, makes a lot of sense, because this isn't a formatting attribute.

Handling inline content

Input:

<p>foo <span data-foo=1>xxx</span> bar</p>
<p>foo <strong data-foo=1>xxx</strong> bar</p>
<p>foo <a href="#" data-foo=1 class=bar style="color:red">xxx</a> bar</p>
<p>foo <img src="x" data-foo=1 class=bar style="color:red"> bar</p>

Output:

<p>foo xxx bar</p>

<p>foo xxx bar</p>

<p>foo <a data-foo="1" href="#">xxx</a> bar</p>

<p>foo <img data-foo="1" src="x" /> bar</p>

Some more inspiration

See https://github.com/ckeditor/ckeditor4/blob/master/plugins/removeformat/plugin.js#L166-L182

I don't think that CKE4's remove format worked perfectly, but it definitely needs to be a bit smarter than it seems.

Remove format vs GHS

Theoretically, when in doubt, Remove format may treat HTML content enabled via GHS as unwanted. However, this might lead to removing too much and breaking some content. So this is a tricky decision.

I'd avoid going "all in" too quickly. I'd try with this:

  • Remove classes and styles added via GHS.
  • Remove all inline formatting enabled via GHS (inline elements that we store as text attributes).

And then let's see how it works in real life.

@Witoso
Copy link
Member

Witoso commented Apr 4, 2023

Valid points, my proposal would be to push it quickly for inline elements first as a step forward in compatibility with CKE4, and then push the removal of the block styles to the later iterations as a topic that requires much more analysis and refinement.

@Witoso
Copy link
Member

Witoso commented Apr 4, 2023

Scope v1:

  • Add isFormatting: true to all inline elements in GHS' schemadefinitions.ts, except a and except all $inlineObject.
  • Edit docs to mention that Remove Format will remove all HTML attributes of inline elements if they appear in GHS integration.
  • Create a new issue for gathering interest in Remove Format for blocks.

@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 5, 2023
@filipsobol filipsobol self-assigned this Apr 20, 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 Apr 20, 2023
arkflpc added a commit that referenced this issue Apr 25, 2023
…remove-inline-styles

Fix (style): Remove Format feature should also remove styles. Closes #11580.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Apr 25, 2023
@CKEditorBot CKEditorBot added this to the iteration 63 milestone Apr 25, 2023
@Witoso
Copy link
Member

Witoso commented Apr 26, 2023

The remove format action for blocks will be tracked in #13983. Vote if it's something you're looking forward to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:remove-format package:style 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.

10 participants