-
Notifications
You must be signed in to change notification settings - Fork 31
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
Presets #82
Presets #82
Conversation
@@ -134,6 +134,11 @@ handle additional, application-specific use cases. | |||
DocumentFragment sanitize(SanitizerInput input); | |||
DOMString sanitizeToString(SanitizerInput input); | |||
|
|||
static readonly attribute Sanitizer default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer we expose this as configuration objects. That would expose some constant-defined JS objects from the Sanitizer object, e.g. Sanitizer.CONFIG_DEFAULT would return a dictionary.
That way, we wouldn't have to initialize and keep various Sanitizer isntances in every window.
@@ -134,6 +134,11 @@ handle additional, application-specific use cases. | |||
DocumentFragment sanitize(SanitizerInput input); | |||
DOMString sanitizeToString(SanitizerInput input); | |||
|
|||
static readonly attribute Sanitizer default; | |||
static readonly attribute Sanitizer nofetch; | |||
static readonly attribute Sanitizer nonavigate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I may, I'll punt on the discussion of the nofetch & nonavigate use case for now, maybe even for all of v1. I fully agree that we will have to consider this at some point though :)
```js | ||
// Preset "rich text" allows only formatting: | ||
// <p class="blubb"><b>text</b></p> | ||
Sanitizer.richtext.sanitize("<p id=bla class=blubb><a href=https://example.org><b>text</b></a>"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With my suggestion above someone would still have to initialize it if they need it.
e.g., myCommonMarkSanitizer = new Sanitizer(Sanitizer.CONFIG_COMMONMARK)
static readonly attribute Sanitizer default; | ||
static readonly attribute Sanitizer nofetch; | ||
static readonly attribute Sanitizer nonavigate; | ||
static readonly attribute Sanitizer richtext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be great if we could adopt a list of reasonable HTML elements from "somewhere else", which would allow us to inherit a widely-used set of HTML elements and thus satisfy very common use cases.
https://commonmark.org/ comes to mind, it should be somewhat easy to extract the list of html elements from their spec and provide a Sanitizer.CONFIG_COMMONMARK.
Closing outdated PR. |
This is more a request for comments, than an actual pull request. This isn't ready to actually go in. In fact, I'm not even sure this should be in v1 of the API.
This addresses issues #57 and #71, and specs a way for different "presets" (besides the default), by making suitable
Sanitizer
instances available as static members. Since we now have the.config()
method, it's super easy to access & inspect their configuration, which is why I thought just offering Sanitizers ready-to-go might be a nice idea.This CL punts on the much harder questions, namely what those preset configs should actually be like. It only looks at the mechanics of how to support those presets, once we've agreed on what they might be.