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

Update the spec to match the current group consensus. #208

Merged
merged 11 commits into from
Mar 19, 2024

Conversation

otherdaniel
Copy link
Collaborator

@otherdaniel otherdaniel commented Jan 23, 2024

This updates the current spec to the current group consensus. More work needs to be done, but with this the spec should roughly reflect the current discussion.

This closes out a whole lot of issues: Closes #200. Closes #199. Closes #188. Closes #186. Closes #183. Closes #181. Closes #148. Closes #147. Closes #101. Closes #59. Closes #45.

- Use camel case for variable names.
- Use list syntax. Mark list members as strings.
- Be more explicit about "strictly conform to".
- Remove canonical config.
- Replace removeChild().
- "set and filter" take a "safe" boolean
- Use "set difference" instead of intersection complement.
- Introduce "dataAttributes" for data-* attributes.
- Add special cases for javascript:-URLs and templates ("funky elements") back in
- Minor edits.
index.bs Outdated Show resolved Hide resolved
@mozfreddyb
Copy link
Collaborator

Would be nice if we could close this out and do the rest as follow-ups. Do you want to re-review? @zcorpan, @annevk?

@mozfreddyb mozfreddyb requested a review from zcorpan March 5, 2024 13:29
Copy link
Collaborator

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I have a number of nits doing a more detailed read through, but I wonder if this is the best use of time as we'll likely also get to this once it gets proposed to HTML. What do you think Daniel?

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
Comment on lines 252 to 253
1. If |config| exists:
1. Run [=sanitize=] on |fragment| using |config|.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems fine to inline again, but also unclear if config is actually conditional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Inlined.

Config is conditional, because step 4 of canonicalize can return empty. The idea is that in the "unsafe" usages, one could in theory define a "no-op" config that doesn't do anything. But it seemed clearer to me to just skip the filtering step for "unsafe" ops where the user hasn't requested any filtering.

In terms of mechanism, I think the previous way was a bit confused. So I've now changed canonicalize to return an empty list / ordered map; and this check to check for emptyness.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@mozfreddyb
Copy link
Collaborator

Can you resolve the review comments that are addressed in the latest. commits?

@otherdaniel Did you notice that your latest commit failed syntax validation?

@otherdaniel
Copy link
Collaborator Author

Can you resolve the review comments that are addressed in the latest. commits?

Will do.

@otherdaniel Did you notice that your latest commit failed syntax validation?

I had not. That particular problem is now resolved, but I wonder if there's a way to replicate all the commit hooks locally. I chiefly rely on bikeshed's error messages. But the repo runs additional checks, like this WebIDL check, and it'd be good if I could run the same checks before pushing a new version.

@otherdaniel
Copy link
Collaborator Author

Can you resolve the review comments that are addressed in the latest. commits?

Will do.

All comments should now be resolved, except for one about the potentially-empty config. (Which I don't think is much of an issue.)

My plan would be to land this in its current version, unless I receive any additional comments, so that we can handle the rest in additional PRs.

@mozfreddyb
Copy link
Collaborator

+1, let's land and do the remaining discussion about an empty config as a follow-up. Thank you @otherdaniel!

@otherdaniel otherdaniel merged commit db21b50 into WICG:main Mar 19, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Mar 19, 2024
SHA: db21b50
Reason: push, by otherdaniel

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment