Morphing deletes element in extremely specific situation involving LastPass extension and no whitespace between elements

[pedantic-git]

Hi developers - thanks for such an amazing library!

This has been discussed extensively on the original issue hotwired/turbo#1291 so I'll not repeat it all here.

There is a combination of conditions involving the Lastpass browser extension (which messes with shadow roots) and a form field with no whitespace between the label and the div containing the field that results in the field itself being removed from the DOM by Idiomorph when the page is re-rendered (the input changes significantly in this use case because it is showing an error state).

All thanks to @4lllex for narrowing down the issue and providing reproduction steps in hotwired/turbo#1291 (comment) . I can work around it by adding whitespace in the specific place where it fails, but I think it's still an idiomorph bug so thought I should report it upstream.

Do you need any more information from me to debug this? And thanks again!

[botandrose]

@pedantic-git Hello, if this is a bug in Idiomorph, I'd be happy to get it resolved! First step is to get a failing test here. That reproduction in Turbo looks like a great place to start. Would you mind taking a stab at reducing it to just Idiomorph?

[4lllex]

@botandrose Looks like it is already fixed in v0.4.0. Turbo is still using v0.3.0.

Here is a repro if you want to double check:

<form id="existingNode" action="/" accept-charset="UTF-8" method="post">
  <!-- note, no spacing                     vv -->
  <label for="user_password">Password</label><div>
    <input type="password" name="user[password]" id="user_password"/>
    <div id="shadowroot"></div>
  </div>
</form>

<form id="newNode" action="/" accept-charset="UTF-8" method="post">
  <div class="field_with_errors">
    <label for="user_password">Password</label>
  </div>
  <div>
    <div class="field_with_errors">
      <input type="password" name="user[password]" id="user_password"/>
    </div>
  </div>
</form>

<script src="https://unpkg.com/idiomorph@0.3.0"></script>
<script>
  const shadow = shadowroot.attachShadow({mode: "open"});
  // observer is not required
  const observer = new MutationObserver((mutationList) => {
    mutationList.forEach((mutation) => {
      mutation.removedNodes.forEach((node) => {
        if (node.id === "user_password") {
          // host is incorrect after morphing
          shadow.host.remove();
        }
      });
    });
  });
  observer.observe(existingNode, {subtree: true, childList: true})

  Idiomorph.morph(existingNode, newNode);
</script>

[botandrose]

Amazing, thank you @4lllex ! To be clear, does one need Lastpass installed to reproduce the issue in v0.3.0, or would this test case break without it?

[botandrose]

As an aside, we're on the cusp of releasing v0.5.0, which will make it into a future Turbo release. v0.4.0 had a showstopper bug WRT Turbo integration.

[4lllex]

Lastpass is not required, the shadowroot is what's causing the issue (lastpass button on the input). I've stripped away the observer, but maybe it actually makes things clearer to have it , I've updated previous post to add it back.

[botandrose]

Wow, this was an extremely tricky bug to track down. Even though its ostensibly fixed in v0.4.0, I wanted to understand what the heck happened, so that I can confirm that the core issue is indeed fixed. Turns out it has nothing to do with the shadow-root... it just makes it easier to visually see the incorrect morph result.

I ultimately tracked it down to a bug in the removeNodesBetween function, where its incorrectly adding the endExclusive node to the deadIds set, as though it was deleting to the end node inclusively, rather than exclusively.
http://github.com/bigskysoftware/idiomorph/blob/v0.3.0/src/idiomorph.js#L558

Interestingly, this faulty line is still present in v0.4.0, so it should be possible to trigger the bug there, but perhaps with a different set-up.
https://github.com/bigskysoftware/idiomorph/blob/v0.4.0/src/idiomorph.js#L884

Anyways, we've since removed that deadIds mechanism completely, so I can confirm that it will be fixed completely in the next release. Thank you very much for helping me nail this one down, @4lllex! I definitely couldn't have done it without your reproduction.