moveBefore() cosmetic concerns

[annevk]

While addressing some more nits in #1307 I realized that moveBefore() returning a node is a pattern we generally don't follow for new APIs: w3ctag/design-principles#286.

I'm not sure if that deviates it enough from insertBefore() to warrant a new name.

I'm also wondering if it should be [Unscopable]. I think we generally agreed to do that for new APIs, but it's been hit or miss as to whether we remember. E.g., getRootNode() does not have this. (This is probably why it should have been an inverted flag from the beginning.)

cc @domfarolino @noamr @domenic @smaug----

[noamr]

I think that treating moveBefore like an "old" API makes some sense here for consistency with the other primitive APIs. When we design higher level APIs for moving we can consider that TAG guideline and have a void return. WDYT?

[domenic]

I'm not sure, but my impression of [Unscopable] was that we'd treat it like [Replaceable]: use it when necessary; no need to apply it in a blanket fashion.

[annevk]

For [Unscopable]: that's how we treated it in practice. I don't really mind either way.

For returning a node: I feel somewhat strongly that we shouldn't introduce new methods that return information already known to the caller. It prevents us from returning new information in the future.

For the name: I can't think of a better name.

[noamr]

For returning a node: I feel somewhat strongly that we shouldn't introduce new methods that return information already known to the caller. It prevents us from returning new information in the future.

I don't feel strongly about it, happy to hear from others.

For the name: I can't think of a better name.

Likewise. I think it's ok to keep the name.

[domfarolino]

Not using [Unscopable] seems fine to me.

I only feel a little strongly about whether we return the node operated on or not. Personally I prefer as much consistency with insertBefore(), and I think it makes the API more ergonomic. That's why I like appendChild() compared to append(), personally: I like seeing const iframe = document.appendChild(document.createElement('iframe')). But if Anne feels strongly, I'm happy to remove the return value.

[domfarolino]

I've made moveBefore() not return the moved node in 90d8339, to be consistent with TAG guidance. Given no strong opinion on [Unscopable], maybe this can be closed now, do you agree @annevk?

[annevk]

We should probably close this issue as part of the commit message of the PR. That makes it easier to find it in the future.

As "pre-move" is now gone, I think "ensure pre-move validity" can be folded into "move". This also reminded me that "ensure pre-insert validity" is not the only algorithm we have that concerns itself with those things. It's just the only one that's abstracted (and it has reason to be abstracted). https://dom.spec.whatwg.org/#concept-node-replace has a similar set of steps. I don't think there's any room for reasonable abstracting between these three sets, so we probably have to live with the duplication.

And separately it seems some notes should be fixed, such as "The above statements differ from the pre-insert algorithm." which wants to point at "ensure pre-insert validity" today.

[domfarolino]

We should probably close this issue as part of the commit message of the PR. That makes it easier to find it in the future.

Done! (by linking to the issue from "Closes" in the PR OP).

As "pre-move" is now gone, I think "ensure pre-move validity" can be folded into "move".

OK. I've folded them into the move algorithm (i.e., not directly into the moveBefore() method steps).

And separately it seems some notes should be fixed, such as "The above statements differ from the pre-insert algorithm." which wants to point at "ensure pre-insert validity" today.

I see. I guess we can fix this up separately, since that's in the replace algorithm and unrelated to the move before PR? Either way, I think we're ready for a re-review!