Add hx-swap-oob modifier support

[MichaelWest22]

Description

This Change re-implements hx-swap-oob swapping to use the full htmx swap function and all of its various modifiers and adds two additional modifiers for additional oob functionality. The recent refactors of the swap function to support history, delay and transitions have now enabled us to reuse swap() much easier than before!

Core to this change is the full backwards compatibility with existing hx-swap-oob use cases where the attribute value can be a <swapStyle>:<selector> format. To allow proper support for all the swap modifers it now checks if the value is a valid full swap specifier with more than just a single thing in it and if so it uses the new full complex swap specification. Otherwise it falls back to the old style handling.

target: modifier

To support a custom target selector I've added a target: modifier that only works with oob swaps and allows parsing of multiple words after target: to be used as a complex extended selector.

strip: modifier

The oob tags are set to be stripped off by default in all but outerHTML swap styles to make oob behaviour simple for inner style swaps but it is helpful sometimes to override this default to allow various other kinds of swap behaviors like doing an outerHTML style swap of an oob tags inner contents to place multiple items easily over top of a single target element. So I've added a strip:true and strip:false options that allows you to easily override it with this modifier.

template handling

This change also includes support for oob tags to be template tags themselves instead of just wrapping template tags around them which simplifies tag choice when dealing with tables and other element types. This just required using fragment.content if the oob element happened to be a template element

hx-select/hx-select-oob

hx-select tested working with strip:true and hx-select-oob updated to be fully backwards compatible with its old undocumented id only format and now supports css selectors properly as this was documented but not working properly and it now supports full hx-swap-oob syntax as well. hx-select-oob now supports querying multiple response elements now that it supports more than just an id only selector so you can do "[id]" for example to swap in all id'ed

idiomroph advanced oob swap support

This change now allows hx-oob-swap to support : in the swap style which allows morph:innerHTML swap styles that were not usable before. Note that to support this users will have to add modifers like target: to make it render with the new method as it now has code to detect if there are valid swap modifiers and if there are use the new format which supports : in the swap style. But if it detects invalid modifiers then it falls back to just splitting on the first : for backwards compatibility. It also now detects and warns users when it detects swap styles that contain spaces which happens when an invalid value is provided in error and can lead to default swap style fallback.

Corresponding issue:
#2308
#3316

Testing

Performed some manual testing of various hx-swap-oob actions and added many tests for the various core functions.

Checklist

• I have read the contribution guidelines
• I have targeted this PR against the correct branch (master for website changes, dev for
  source changes)
• This is either a bugfix, a documentation update, or a new feature that has been explicitly
  approved via an issue
• I ran the test suite locally (npm run test) and verified that it succeeded

[scrhartley]

idiomroph (sic) advanced oob swap support
This change now allows hx-oob-swap to support : in the swap style which allows morph:innerHTML swap styles that were not usable before.

I can't spot a test for this.

[MichaelWest22]

idiomroph (sic) advanced oob swap support
This change now allows hx-oob-swap to support : in the swap style which allows morph:innerHTML swap styles that were not usable before.

I can't spot a test for this.

There are no tests for idiomorph in htmx for me to add tests for this. all the tests are inside the idiomorph repo which I can't test till the htmx change ships and is made available there.

Edit: the test for this I had already added in idiomorph a while ago in here https://github.com/bigskysoftware/idiomorph/blob/main/test/htmx-integration.js but it is disabled as it does not work right now.

[scrhartley]

I more just meant a more general test that proves the parsing works, rather than an end-to-end test that's specific to idiomorph, even if that's the scenario driving the change.

[scrhartley]

Most of the other modifiers for getSwapSpecification have a config default associated with them somewhere in the code base.
Would it make sense to have something for the strip: modifier?
swap: -> defaultSwapDelay
settle: -> defaultSettleDelay
transition: -> globalViewTransitions
ignoreTitle: -> ignoreTitle
show: -> scrollIntoViewOnBoost
focus-scroll: ->defaultFocusScroll

[MichaelWest22]

Most of the other modifiers for getSwapSpecification have a config default associated with them somewhere in the code base. Would it make sense to have something for the strip: modifier? swap: -> defaultSwapDelay settle: -> defaultSettleDelay transition: -> globalViewTransitions ignoreTitle: -> ignoreTitle show: -> scrollIntoViewOnBoost focus-scroll: ->defaultFocusScroll

I feel like there are already maybe too many config items in htmx and so I would not recommend adding more. right now it defaults to strip on for oob inner swaps and off for all normal swaps and oob outerHTML swaps which matches existing htmx behaviour. To me turning strip on by default would over complicate the feature as it is something you only need for very specific situations and the user should be aware of when they are using it to override the default documented behaviour. and how would the default even work here as in theory each swap situation could have a different strip default like a user will want all their oob afterend ones not strip but may expect to default their oob innerHTML to still strip. And defaulting all normal swaps for all swap styles would cause a lot of confusion and issues and bugs for the user.

If we did it we would have to have a oobSwapStyleNoStrip that defaults to ['outerHTML'] and a SwapStyleStrip that defaults to []

Edit: looking at the code it seems complex to handle this as we already have a isInlineSwap function that does the defaulting for oob swaps already and this is used by extensions as well making it messy to implement.

And another note is that most of the defaults above you list i explicitly had to exclude them from applying to oob swaps as it would cause a major change in behaviour and do things the users wouldn't expect. The only one i left in place for oob was focus-scroll as this kind of made sense.

[MichaelWest22]

I more just meant a more general test that proves the parsing works, rather than an end-to-end test that's specific to idiomorph, even if that's the scenario driving the change.

Added some tests for the new : handling in custom extension swap styles. Note that what doesn't work if you do hx-swap-oob="morph:innerHTML" as this has to be treated as morph with target selector innerHTML. But it is designed to work with "morph:innerHTML:#foo" which is the format i'm testing and also "morph:innerHTML target:#foo strip:false" etc

[scrhartley]

I am unsure about this logic and if it impacts existing code.
Here are some CSS selectors I've been playing around with:
morph:outerHTML::has(> #b1) > :first-child (or without the spaces morph:outerHTML::has(>#b1)>:first-child)
innerHTML:#b1:first-child
innerHTML:#b1::before
innerHTML:#b1:not(:empty)
innerHTML:#b1 > :first-child
Do we want to special case :: or make sure we're only allowing max one colon in the swap style?
Do we need a test proving that existing selectors will continue to work?
From docs: beforeend:#table tbody (with a space) and so I would want confidence that morph:outerHTML:#table tbody also works. This is such a large change to parsing that it needs tests beyond the positive cases.

[MichaelWest22]

I am unsure about this logic and if it impacts existing code. Here are some CSS selectors I've been playing around with: morph:outerHTML::has(> #b1) > :first-child (or without the spaces morph:outerHTML::has(>#b1)>:first-child) innerHTML:#b1:first-child innerHTML:#b1::before innerHTML:#b1:not(:empty) innerHTML:#b1 > :first-child Do we want to special case :: or make sure we're only allowing max one colon in the swap style? Do we need a test proving that existing selectors will continue to work? From docs: beforeend:#table tbody (with a space) and so I would want confidence that morph:outerHTML:#table tbody also works. This is such a large change to parsing that it needs tests beyond the positive cases.

Yeah it is a pain with :. I've found a way to update it now to always first parse it the new way and if this fails fall back to the old legacy style:selector format handling so it is now 100% backwards bug compatible with the old version. So all the edge cases now work as expected.

However you are no longer able to do things like morph:outerHTML::has(> #b1) > :first-child as this is now split on the first : as it always has breaking things like it did before. instead for idiomorph you would need to do morph:outerHTML target::has(> #b1) > :first-child where you are explicit in setting a target and this will now parse fine.

The only other issue is that if you mis spell or mis type one of the modifiers and create an invalid swap specification like outerHTML swwap:1s it will no longer throw an error log and instead treat it as swap style="outerHTML swwap" and selector "1s" which will fail silently. But I can't think of a practical way to detect this. This only impacts users trying to do new modifier based oob swaps though and this may be the cost of full backwards compatibility :/

[MichaelWest22]

Managed to add simple error logging that detects when the swap style is invalid as in situations where modifers fail the swap style will contain whitespace which we can check for. Found it falls back to a innerHTML swap if the invalid swap spec has no : but if it contains a : it fails with a invalid css selector error.

[1cg]

@scrhartley how do you feel about this change? Comfortable that it is backwards compatible?

[scrhartley]

@scrhartley how do you feel about this change? Comfortable that it is backwards compatible?

I want to be convinced, but it's a significant change and so I'll try to spend time at the weekend to really understand its implications and that the parsing doesn't cause any surprises.

[scrhartley]

Some initial findings for hx-select-oob:

Bug
In the added unit tests, an example of an extended selector was added hx-select-oob="span div", but this doesn't work in common cases e.g. with a class selector it will throw a DOMException due to invalid CSS syntax:

SyntaxError: Failed to execute 'querySelector' on 'DocumentFragment': '#.my-div' is not a valid selector.

<div class="my-div"></div>
<button hx-get="/foo" hx-swap="none" hx-select-oob=".my-div">Button</button>

Regression
A trailing colon for hx-select-oob is no longer ignored and results in an accidental innerHTML swap:

<div id="my-div2"></div>
<button hx-get="/foo" hx-swap="none" hx-select-oob="#my-div2:"
        hx-on::oob-after-swap="console.log(event.detail.swapSpec.swapStyle)">
    Button
</button>

Docs (or perhaps implementation)
Although it now supports more selectors, hx-select-oob still uses querySelector so each comma separated item can only apply to the first match.

[MichaelWest22]

Some initial findings for hx-select-oob:

Bug In the added unit tests, an example of an extended selector was added hx-select-oob="span div", but this doesn't work in common cases e.g. with a class selector it will throw a DOMException due to invalid CSS syntax:

SyntaxError: Failed to execute 'querySelector' on 'DocumentFragment': '#.my-div' is not a valid selector.

<div class="my-div"></div>
<button hx-get="/foo" hx-swap="none" hx-select-oob=".my-div">Button</button>

Regression A trailing colon for hx-select-oob is no longer ignored and results in an accidental innerHTML swap:

<div id="my-div2"></div>
<button hx-get="/foo" hx-swap="none" hx-select-oob="#my-div2:"
        hx-on::oob-after-swap="console.log(event.detail.swapSpec.swapStyle)">
    Button
</button>

Docs (or perhaps implementation) Although it now supports more selectors, hx-select-oob still uses querySelector so each comma separated item can only apply to the first match.

Thanks @scrhartley!

Yeah the bug existed in the existing implementation as well as it always forced you to use # in frount and threw selector errors on any non id value before. But I was trying to improve it to make it more generic as the documentation does not point out this existing implementation detail. But my design had issues here so i've pushed a change to address the two things you found and added a note about the limitation of hx-select-oob being only the first item.

I could revert my hx-select-oob improvements so it just only supports id based selectors only like before maybe. But it would be nice to have proper CSS selector support if we can as the old limitation was never documented.

Adding support for querySelectorAll may be possible but it could cause a minor change in behavior if the user had two of the same id in the response and it now swaps both but this edge case seems unlikely. But I don't know if we really need the added complexity of this feature.

I had to use a simple regex to identify valid ID's that are safe to add # to like the old implementation. This does mean that someone could have had an invalidly formated ID with weird characters and found that they had to escape the ID and also found that they could remove the # from this complex id selector and it would still work and this would not catch this edge case and be a regression. But This seems too unlikely to cause issues as all users should have a # in such cases and won't have any issues.

[scrhartley]

it('invalid swap modifers without ":" will fall back to innerHTML swap and log error', function() {

I don't understand this test. Since it's hx-swap-oob, shouldn't it fall back to outerHTML (i.e. true)?

[MichaelWest22]

invalid swap modifers without ":" will fall back to innerHTML swap and log error

yeah that is a confusing one sorry. When I was testing broken and bad selector handling if you miss type something like hx-swap-oob="outerHTML swapp:10s" it will parse it as "outerHTML swapp" and selector "10s" because it does not detect valid modifiers. This will throw a invalid selector error to the user because 10s is not valid CSS. I also added user error logging for this case when the selector contains a space because if they miss key a modifer with no : or the value after the : happens to be a valid selector like "true" then we need to let them know its broken so i log an error. But I found it can sometimes still follow though and do a swap in this situation and because it has a manual swap specifier it does not use the true/outerHTML fallback and instead falls back to innerHTML.

The only reason I added this test was because without it the error logging was never being fired properly so i needed a test just for code coverage but it seems the test above covers this anyway so probable easier just to remove this edge case test as it is not something we something we are trying to enforce

[MichaelWest22]

@scrhartley I've re-looked at the querySelectorAll missing feature for hx-select-oob. I've just added support for this as it was not as hard as I thought to add. it could impact users with duplicate id's and now swap both but they would just overwrite the same one anyway so I think this is not really a problem we need to worry about. There are some good use cases for hx-select-oob=".myclass" so that if you had multiple elements in your response and you don't know their unique id's but just some class or other property you could hx-select-oob them all in one go which is cool. and using hx-select-oob="[id]" would find every single id element in the response and swap these all in which is cool. One issue with this is that if you combine it with a normal or hx-select swap it is possible to cause problems if you hx-select-oob all id'ed elements and your main or select swap contains these same id's as they will be removed from the response data during oobSwap() and then when the main swap hits it removes them from the DOM at the end. But this is already an existing issue you could cause anyway so no real regression here. It would be nice to take out the remove element call at the end of oobSwap() and move it to the findAndSwapOobElements() function instead as then hx-select-oob would never destroy the response data like it does now. But that may be a feature for another day if it was a real issue maybe.

Edit also note the multi selector feature does have a limitation that if the elements you are selecting don't have unique id's then it is going to be hard to target them anywhere. You could probably do fancy things with ".myclass:beforeend target:#myclassdiv strip:false" to place them all one by one at the end of a div but its getty pretty complex!

[scrhartley]

Regression

<div id="my.div3"></div>
<button hx-get="/foo" hx-swap="none" hx-select-oob="my\.div3">
    Button 1
</button>

(I assume you need something like /^[a-z\-_](\\\.|[a-z0-9\-_])*$/i)

[scrhartley]

Testing
Given the way the code works, it may be prudent to cover the target modifier not being the last one while also having spaces in it.

[MichaelWest22]

Testing Given the way the code works, it may be prudent to cover the target modifier not being the last one while also having spaces in it.

The target modifier is designed so that once it finds it all remaining words that don't match exactly with another modifier are included in the target selector. this means you can do any target modifier including spaces after "target:" and still have "swap:10s" or whatever valid modifier after it.

So this works for example:

<div hx-swap-oob='outerHTML target:#table tbody swap:10s'/>

Regression

<div id="my.div3"></div>
<button hx-get="/foo" hx-swap="none" hx-select-oob="my\.div3">
    Button 1
</button>

(I assume you need something like /^[a-z\-_](\\\.|[a-z0-9\-_])*$/i)

Yeah id's can contain . in the html4 spec so maybe handling this case could make some sense. I've added your regex and this works now. from the docs most users should be including a # at the start of the selector which would avoid the issue if the user had a wierd id that needed an escapped css selector so it is very unlikely that this change will ever happen in practice. With HTML5 there are other random characters you can include in a id (even though you should never do this!) and these could cause an issue but this seems far too much of an edge case to care about to me. If we did care about these edge cases we could pull the hx-select-oob improvments and just leave it as just an id only maybe.

[MichaelWest22]

Also just fixed up swapping class handling so oob swaps add and remove swapping class in case a user wants to add a settleDelay for an animation trigger. Just moving the add swapping class point is an easy fix here. and this same fix was actually already done in #2845 that was merged but then I reverted this when I redid swap feature.

[scrhartley]

One thing I haven't tested yet is how stripping works with template elements for troublesome tables. Not sure if hx-swap-oob vs. hx-select-oob would make a difference. But I'm too tired for that, so I'm off for bed.

[scrhartley]

Shower thought 1:
I do wonder if we should split out most of getSwapSpecification into an internal function.
That way we don't expose to extensions that it has a weird extra parameter, we can remove some checking for which mode we're in and it's only internal that we may return undefined for some OOB specific parsing logic.

Shower thought 2:
Do to a lack of stacked PRs as a GitHub feature, it's difficult for Carson to say no to individual pieces. Does more need to be done to describe what's been done (plus associated limitations), starting with updating the description to better cover the current implementation?

[MichaelWest22]

thanks scrhartley! just tested quickly with the oob swap supports table row in fragment along other oob swap elements with config tests

<!-- starting table content -->
       <tbody id="tbody">
          <tr id="r1">
           <td>foo</td>
          </tr>
          <tr>
            <td id="td1">Bar</td>
          </tr>
        </tbody>

      <template> <!-- with this template -->
        <tr hx-swap-oob="outerHTML strip:true" id="r1"><td>bar</td></tr>
      </template>

<!--This replaces tr[r1] with the inner td as i would expect fine. its not valid table but you get what you asked for.  the strip:false version worked like normal when i tested-->
         <tbody id="tbody">
          <td class="">bar</td>
          <tr>
            <td id="td1">hey</td>
          </tr>
        </tbody>

     <template> <!-- with this template -->
        <tr hx-swap-oob="innerHTML strip:false" id="r1"><td>bar</td></tr>
      </template>

<!--This places tr[r1] inside the old tr[r1] as i would expect fine.  the strip:true version worked like normal-->
        <tbody id="tbody">
          <tr id="r1" class=""><tr id="r1"><td>bar</td></tr></tr>
          <tr>
            <td id="td1">hey</td>
          </tr>
        </tbody>

As for hx-swap-oob vs hx-select-oob the strip function seems to work the same from the tests I've added and tried.

Shower thought 1: I do wonder if we should split out most of getSwapSpecification into an internal function. That way we don't expose to extensions that it has a weird extra parameter, we can remove some checking for which mode we're in and it's only internal that we may return undefined for some OOB specific parsing logic.

I want to keep the changes we need to make to a minimum to reduce the minified size of the library as much as possible. getSwapApecifications is really already a pretty internal function and there is no clear documentation on the arguments the extensions should use for functions like this and I think adding additional optional parameters is fine and I have seen several examples in other internalApi functions where optional parameters have been added before. It just has a new defaults param that extension users could now use to pass in their own defaults to set settle/swap delays they need and the extra function to skip boosted show defaults and not report errors by default I think are fine. should be zero behaviour change for existing extension users otherwise

Shower thought 2: Do to a lack of stacked PRs as a GitHub feature, it's difficult for Carson to say no to individual pieces. Does more need to be done to describe what's been done (plus associated limitations), starting with updating the description to better cover the current implementation?

I think we have a good running list of the changes documented in this 30 comment thread now and Carson can ask for us to revert any individual changes he is worried about any of them. I've updated the PR description with the main changes we have made.