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

Improve API of insertAdjacent*() methods #10122

Open
LeaVerou opened this issue Feb 3, 2024 · 2 comments
Open

Improve API of insertAdjacent*() methods #10122

LeaVerou opened this issue Feb 3, 2024 · 2 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest

Comments

@LeaVerou
Copy link

LeaVerou commented Feb 3, 2024

What problem are you trying to solve?

insertAdjacentHTML() is fantastic for templating. It bridged an old usability cliff where authors had to either give up working with HTML strings, or do a lot of painful wrangling with innerHTML if they only wanted to insert an HTML fragment inside or relative to another element, rather than replace the entire contents of an element.

However, due to its origins, the API is …let’s say suboptimal and clumsy.
The position arguments are excessively long and inconsistent with related DOM methods (e.g. child.before() but "beforebegin"), and there’s not even a default for the first argument, so authors are forced to specify them on every call.

Furthermore, we also have insertAdjacentElement() and insertAdjacentText() methods, which are simply worse APIs for before() / after() / append() / prepend(), presumably included for compat.

How would you solve it?

Option 1: Overloading insertAdjacent*() methods

  1. Deprecate existing position strings, and instead move to:
    • "before" instead of "beforebegin"
    • "after" instead of "afterend"
    • "start" (or "prepend"?) instead of "afterbegin"
    • "end" (or "append"?) instead of "beforeend"
  2. Default position to "end"

Option 1b: Instead of overloading the signature with a one argument signature to default position, introduce a dictionary argument with position and html keys (or text or node for the other two).

Pros:

  • Path of least resistance, less substantial change than the other ideas
  • Better layering, as 1 and 2 can be shipped independently

Cons:

  • Is the improvement significant enough to warrant deprecating the existing position keywords?
  • 1a: We tend to avoid making non-final arguments optional

Option 2: Introducing new <position>HTML() methods

This would introduce new HTML methods like appendHTML(), prependHTML(), beforeHTML(), afterHTML()

Not a huge fan of this approach, as it increases the API surface significantly, and having different methods that do related but different things is an antipattern. However, it's the only one that doesn't involve overloading existing methods.

Option 3: Overload element.append(), element.prepend(), node.before(), node.after()

Since these already handle strings as text nodes, overloading wouldn't work. However, a dictionary overload with an html key still could. We probably want to be able to combine HTML strings with elements and text nodes, so this would still accept multiple arguments, each of which can be a dictionary.

Pros:

  • All functionality to append/prepend/insert before/after lives under the same methods.
  • It allows us to insert before/after text nodes, whereas insertAdjacent* is not available there.
  • Dictionary allows future extension
  • More flexibility: it's the only solution that allows us to insert snippets of HTML, elements, and text nodes in a single call.

Cons:

  • More verbose than 2, though still less than 1 (compare: element.insertAdjacentHTML("before", foo) vs element.beforeHTML(foo) vs element.before({html: foo}))

Option 4: Single node.insert(...content) method to rule them all

This would basically encompass all insertAdjacent* PLUS functionality in a single method.
content could be either a string (which would create a text node), a node, or a dictionary with the following structure:

{
	html?: string,
	text?: string
	node?: string,
	[position = "end"]: "before" | "after" | "start" | "end"
}

The method would ideally be available on Node and would error if start | end are used on non-elements.

Pros:

  • Basically what insertAdjacent* should have been
  • Nicely readable
  • Extensible

Cons:

  • More verbose than some of the other solutions
  • Design creates some error conditions that do not exist in the other designs: What happens if more than one of html, text, node is specified? Do we get all of them or do we throw?
@LeaVerou LeaVerou added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest labels Feb 3, 2024
@annevk annevk transferred this issue from whatwg/dom Feb 4, 2024
@annevk
Copy link
Member

annevk commented Feb 4, 2024

HTML defines the parser APIs so moving this issue there. WICG/sanitizer-api#184 (comment) still seems the most reasonable to me (your option 2) although now we'd have to name these beforeHTMLUnsafe(), etc. It's quite a few additional methods, but adding methods doesn't have a significant cost. And it's often clearer to have several well-named methods than an overloaded one.

@zcorpan
Copy link
Member

zcorpan commented Feb 12, 2024

The legacy HTML fragment parser APIs don't support DSD and don't have Sanitizer API support. They also parse as HTML or XML depending on the "HTML document" bit on the document, and setHTML() and setHTMLUnsafe() always parse as HTML. (If we want XML variants in the future, we can add setXMLUnsafe() etc.) So possible axes are:

  • position
  • sanitize or not
  • HTML/XML

I think option 1 is not a sufficient improvement to be worthwhile, it would still be a clunky API and it might be hard to add Sanitizer API in a consistent way.

Option 3 would make existing methods XSS sinks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest
Development

No branches or pull requests

4 participants
@LeaVerou @zcorpan @annevk and others