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

positioned counterparts of setHTML? #184

Closed
mozfreddyb opened this issue Jan 11, 2023 · 11 comments
Closed

positioned counterparts of setHTML? #184

mozfreddyb opened this issue Jan 11, 2023 · 11 comments

Comments

@mozfreddyb
Copy link
Collaborator

Hi friends,

CC: @otherdaniel @annevk @evilpie

@annevk raised the question whether we want to have different insertion mechanisms than replacing children (as setHTML() currently does - like innerHTML assignments).

The existing pattern he wants to address exists in the first parameter to insertAdjacentHTML() (beforeend, afterend, beforebegin and afterbegin) or their prepend(), append() and replaceChildren in the DOM (though the DOM functions do not parse).

Anne suggested we might want to look at providing similarly-named functions e.g., prependHTML(), appendHTML() etc. that work just like setHTML() but with a different insertion point (and maybe different parsing context, if you do the equivalent of outerHTML).

In counter, I highlighted that the name setHTML does side-step the position of the insertion completely.
Would this mean that the existing setHTML options-bag could simply take a position: key with values like afterend, beforebegin or prepend or whatever color you'd prefer the bikes shed to be🚲 🏠

The benefit of specific, positioned insertion is that you wouldn't re-parse the whole subtree, as people currently do with innerHTML+=.

P.S: Let's keep this discussion solely on the point of whether we want positioned insertion at all and if so, which of these two options we would prefer: Another option or more additional functions.

@annevk
Copy link
Collaborator

annevk commented Jan 11, 2023

DOM has:

  • prepend()
  • append()
  • replaceChildren()
  • before()
  • after()
  • replaceWith()

It seems kinda attractive to give these (safe) HTML counterparts:

  • prependHTML()
  • appendHTML()
  • setHTML() (I think this is a better name than replaceChildrenHTML(), in large part because it's shorter than innerHTML or equal in length if you count the parenthesis)
  • beforeHTML()
  • afterHTML()
  • replaceWithHTML()

Giving them dedicated method names matches the precedent we set in DOM when we added all those convenience methods.

cc @rniwa @domenic @mfreed7 @smaug----

@otherdaniel
Copy link
Collaborator

Can you explain a bit more about the problems this is trying to solve?

The feedback I've gotten so far is that sanitizer users would prefer APIs that return a sanitized value, rather than one that combines sanitization and tree insertion. Asking for more, different ways to sanitize + insert has literally never come up for me.

@annevk
Copy link
Collaborator

annevk commented Jan 12, 2023

Providing a safe replacement for all HTML insertion APIs in a way that is consistent with the DOM library to date.

@otherdaniel
Copy link
Collaborator

I'm happy to read about the "safe" part; particularly in light of #185.

The replacement aspect is new to me, and would come with some new requirements.

  • Would these new methods (and setHTML) require a safe context? (Rationale for requring a secure context #122)
  • Current Sanitizer is off-by-default for some content, like HTML comments, custom elements, or "unknown" elements. Would this remain, or would those be preserved to be consistent with the current DOM?
  • insertAdjacentHTML & friends have some weird XSS corner cases. E.g. insertAdjacentText/HTML from a child node of a script. The new versions would disallow this?

@annevk
Copy link
Collaborator

annevk commented Jan 25, 2023

In order of appearance:

  1. I'm not sure how additional entry points changes this question. Are a safe and secure context the same thing to you?
  2. The APIs should behave the same way we decide setHTML() to behave. So if comments, custom elements, etc. are removed by default we should continue to do so. I don't see why we would change that?
  3. Can you elaborate on these corner cases?

@mozfreddyb
Copy link
Collaborator Author

Re 3: This came up in our spec meeting earlier today. The main issue is that if you insert e.g., beforebegin or afterend your context node is your parent and you need to make sure that all checks and handling happens on the parent. The funky example was a weird DOM tree created by script where an element p has a parent of script and then calling p.insertAdjacentHTML('afterend', someTextNode). Suddenly, the text node would be script because of the parent script element.

@annevk
Copy link
Collaborator

annevk commented Jan 25, 2023

For that case the combination of how context is set in https://w3c.github.io/DOM-Parsing/#dom-element-insertadjacenthtml and is handled at https://wicg.github.io/sanitizer-api/#sanitizeandset (it would be the "this" equivalent) we'd be in the clear I think. You'd get an exception and that's that.

@pygy
Copy link

pygy commented Feb 8, 2023

Edit: see whatwg/html#8759 (comment) sorry for the noise.

@otherdaniel
Copy link
Collaborator

I think a "safe replacement for all HTML insertion APIs" is a question of how to evolve the DOM APIs, and is substantially out of scope for the Sanitizer API WICG. I think this discussion should be moved to the HTML WG.

I'm rather thankful for this comment, which provides much needed clarity, and also moves the discussion to the right place.

@annevk
Copy link
Collaborator

annevk commented Apr 3, 2024

I suggest we close this in favor of whatwg/html#10122. This is not really an enhancement that needs to be tracked here. (Though if we do decide to add these we obviously need to involve this community to ensure safety.)

@mozfreddyb
Copy link
Collaborator Author

Agreed. Let's move this to HTML.

@mozfreddyb mozfreddyb closed this as not planned Won't fix, can't repro, duplicate, stale Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants