Clarify condition by splitting it up into explicitly named bools.#385
Clarify condition by splitting it up into explicitly named bools.#385otherdaniel wants to merge 4 commits intoWICG:mainfrom
Conversation
|
I have been thinking it might be cleaner to add a new algorithm like is attribute allowed. This might also be useful for #387, because we could use it like "is attribute allowed |
Done. Please take another look. |
| 1. [=/remove an attribute|Remove=] |attribute|. | ||
|
|
||
| 1. If [=is attribute allowed=] for |attribute| given |configuration| | ||
| and |child| is `remove`, |
There was a problem hiding this comment.
I would suggest using a boolean or a pair of singletons: https://infra.spec.whatwg.org/#singleton
There was a problem hiding this comment.
Done. Using [=/blocked=] and [=/allowed=]
| [=Attr/local name=] and [=Attr/namespace=]. | ||
| 1. Let |elementName| be a {{SanitizerElementNamespace}} with |current element|'s | ||
| [=Element/local name=] and [=Element/namespace=]. | ||
| 1. Let |elementWithLocalAttributes| be « [] ». |
There was a problem hiding this comment.
| 1. Let |elementWithLocalAttributes| be « [] ». | |
| 1. Let |elementWithLocalAttributes| be « ». |
There was a problem hiding this comment.
It was supposed to be an empty ordered map, per https://infra.spec.whatwg.org/#maps. Now I just use the phrase "empty ordered map", which is used elsewhere.
| |configuration|["{{SanitizerConfig/elements}}"] [=SanitizerConfig/contains=] | ||
| |elementName|: | ||
| 1. Set |elementWithLocalAttributes| to | ||
| |configuration|["{{SanitizerConfig/elements}}"][|elementName|]. |
There was a problem hiding this comment.
It was a list but now it's an object?
There was a problem hiding this comment.
:) It's now an empty ordered map or an IDL dictionary. I think I can use both with square brackets operator.
| 1. If |elementWithLocalAttributes|["{{SanitizerElementNamespaceWithAttributes/removeAttributes}}"] [=map/with default=] « » | ||
| [=SanitizerConfig/contains=] |attrName|: | ||
| 1. Return `remove`. | ||
| 1. Otherwise, if |configuration|["{{SanitizerConfig/attributes}}"] [=map/exists=]: |
There was a problem hiding this comment.
Do we need to say "Otherwise" here? We're using early returns after all.
|
Should we move the |
Done. |
| |elementName|: | ||
| 1. Set |elementWithLocalAttributes| to the |item| in | ||
| |configuration|["{{SanitizerConfig/elements}}"] where | ||
| |elementName|[{{SanitizerElementNamespace/name}}] [=string/is=] |
There was a problem hiding this comment.
Shouldn't this (and below) be |elementName|["{{SanitizerElementNamespace/name}}"] (with quotes)?
| the [=built-in navigating URL attributes list=], and if |attribute| | ||
| [=contains a javascript: URL=], then return [=/blocked=]. | ||
| 1. If |current element|'s [=Element/namespace=] [=string/is=] the | ||
| [=MathML Namespace=] and |attr|'s [=Attr/local name=] [=string/is=] |
There was a problem hiding this comment.
| [=MathML Namespace=] and |attr|'s [=Attr/local name=] [=string/is=] | |
| [=MathML Namespace=] and |attribute|'s [=Attr/local name=] [=string/is=] |
(and more)
|
Sorry, I just realized that this won't really work for my idea of using this for implementing a manual |
| 1. If |elementWithLocalAttributes|["{{SanitizerElementNamespaceWithAttributes/attributes}}"] | ||
| [=map/exists=] and |elementWithLocalAttributes|["{{SanitizerElementNamespaceWithAttributes/attributes}}"] does not [=SanitizerConfig/contain=] |attrName|: | ||
| 1. Return [=/blocked=]. | ||
| 1. If |handleJavascriptNavigationUrls|: |
There was a problem hiding this comment.
Sorry for flip-flopping on this, but please remove this again ... For #396.
3 participants
Split the dreaded multi-line condition into several subclauses, assign each to an appropriately named boolean, and then test those bools.
That's more verbose than I usually go for, but it's surely more readable than the old version.
Fix: #380
Preview | Diff