Skip to content

Refactor "is attribute allowed"#385

Merged
evilpie merged 4 commits into
WICG:mainfrom
otherdaniel:380-clarify
May 22, 2026
Merged

Refactor "is attribute allowed"#385
evilpie merged 4 commits into
WICG:mainfrom
otherdaniel:380-clarify

Conversation

@otherdaniel

@otherdaniel otherdaniel commented Mar 23, 2026

Copy link
Copy Markdown
Collaborator

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

@evilpie

evilpie commented Mar 31, 2026

Copy link
Copy Markdown
Collaborator

I have been thinking it might be cleaner to add a new algorithm like is attribute allowed. This might also be useful for whatwg/html#12533, because we could use it like "is attribute allowed is".

@otherdaniel

Copy link
Copy Markdown
Collaborator Author

I have been thinking it might be cleaner to add a new algorithm like is attribute allowed. This might also be useful for whatwg/html#12533, because we could use it like "is attribute allowed is".

Done. Please take another look.

Comment thread index.bs Outdated
Comment thread index.bs Outdated
Comment thread index.bs Outdated
Comment thread index.bs Outdated
@evilpie

evilpie commented Apr 7, 2026

Copy link
Copy Markdown
Collaborator

Should we move the handleJavascriptNavigationUrls branch also into is attribute allowed?

@otherdaniel

Copy link
Copy Markdown
Collaborator Author

Should we move the handleJavascriptNavigationUrls branch also into is attribute allowed?

Done.

Comment thread index.bs Outdated
Comment thread index.bs Outdated
@evilpie

evilpie commented Apr 12, 2026

Copy link
Copy Markdown
Collaborator

Sorry, I just realized that this won't really work for my idea of using this for implementing a manual is check, because in that case we don't have a real Attr attr attribute.

Comment thread index.bs Outdated
@evilpie evilpie mentioned this pull request Apr 23, 2026
6 tasks
@evilpie evilpie changed the title Clarify condition by splitting it up into explicitly named bools. Refactor "is attribute allowed" Apr 29, 2026
@evilpie

evilpie commented May 5, 2026

Copy link
Copy Markdown
Collaborator

@otherdaniel Can I help getting this done?

@otherdaniel

Copy link
Copy Markdown
Collaborator Author

@otherdaniel Can I help getting this done?

Please give it another look. Apologies for being a bit late with this.

Comment thread index.bs Outdated
«[|elementName|, |attrName|]» and |attr|'s
[=get an attribute value|value=] [=string/is=] "`href`" or
"`xlink:href`", then [=/remove an attribute|remove=] |attribute|.
1. Let |attrName| be a {{SanitizerAttributeNamespace}} with |attribute|'s

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this before is attribute allowed, and then call it with |attrName|.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. (Also calling it with |elementName| for consistency.)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error in the preview script seems unrelated to this change. If I've traced it correctly, bikeshed seems to expect boilerplate files in bikeshed/spec-data/boilerplate, but they're actually installed in bikeshed/spec-data/readonly/boilerplate. No idea why...

Comment thread index.bs
1. Otherwise:
1. If |elementWithLocalAttributes|["{{SanitizerElementNamespaceWithAttributes/attributes}}"]
[=map/exists=] and |elementWithLocalAttributes|["{{SanitizerElementNamespaceWithAttributes/attributes}}"] does not [=SanitizerConfig/contain=] |attrName|:
1. Return [=/blocked=].

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem to have dropped the check for SanitizerConfig/removeAttributes?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right! Should now be fixed.

Comment thread index.bs Outdated
@evilpie evilpie merged commit c0f1ca8 into WICG:main May 22, 2026
2 checks passed
@evilpie

evilpie commented May 22, 2026

Copy link
Copy Markdown
Collaborator

@noamr fyi for upstreaming

@noamr

noamr commented May 22, 2026

Copy link
Copy Markdown
Collaborator

Since this is editorial perhaps upstreaming this change can be a follow up? It's getting difficult to keep track of the giant upstreaming PR

lando-worker Bot pushed a commit to mozilla-firefox/firefox that referenced this pull request May 22, 2026
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request May 26, 2026
…milio

Implements WICG/sanitizer-api#385

Differential Revision: https://phabricator.services.mozilla.com/D301303

UltraBlame original commit: 30574364f047793f130880bf86201326e0da72c6
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request May 26, 2026
…milio

Implements WICG/sanitizer-api#385

Differential Revision: https://phabricator.services.mozilla.com/D301303

UltraBlame original commit: 30574364f047793f130880bf86201326e0da72c6
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request May 26, 2026
…milio

Implements WICG/sanitizer-api#385

Differential Revision: https://phabricator.services.mozilla.com/D301303

UltraBlame original commit: 30574364f047793f130880bf86201326e0da72c6
@otherdaniel otherdaniel deleted the 380-clarify branch June 12, 2026 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Attribute allowing conditions seem wrong

4 participants