Patch insertAdjacentHTML

[muan]

Fixes #104.

Tests are mostly passing locally. ⭐️ I am getting some flaky tests on Safari 11.0.2 on this branch as well as master so I'm disregarding them (customElements.polyfillWrapFlushCallback is not a function).

cc @bicknellr @notwaldorf ❤️
cc @josh @dgraham
cc @javan didn't find a commit for your test code, but would rebase on top of it if you have it somewhere 🙏

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
• If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
• In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

[javan]

didn't find a commit for your test code

I only pasted a diff in #104. It's all yours. Thanks for fixing! 💝

[bicknellr]

I started working on this in another branch (also called insertAdjacentHTML) yesterday and I have a handful of tests in there, could you pull them into your branch? Also, the one thing that I'm having a hard time working out is if it's possible for something that shouldn't be upgraded by this step to be inside baseElement when patchAndUpgradeTree is called. I think this might be possible during the initial document walk or a cloneNode if some upgraded element's callback calls insertAdjacentHTML but I'm not exactly sure. If it is possible, you might need to keep track of which elements were already in the element where the HTML is inserted as children so that you can avoid attempting to upgrade them here. If not, this approach is definitely better.

[bicknellr]

I think this.parentElement should be this.parentNode since the element might be a child of some non-element thing like a shadow root or document. Following that, the cast above should be !Node and could be moved to surround this.parentNode instead of the whole line.

[bicknellr]

Could you add where = where.toLowerCase(); here, before the comparison below? This string doesn't have to match case-sensitively.

[bicknellr]

Also, you'll need to sign the CLA before I can pull this in and thanks for being consistent with the style!

[muan]

I signed it!

[googlebot]

CLAs look good, thanks!

[bicknellr]

Sorry, I didn't mean that you needed overwrite all your stuff, just to pull in a copy of the tests!

[bicknellr]

Wait, I didn't read that commit correctly, live updates are funky. I'll just worry about coming up with the example of the thing I mentioned before and be back then.

[bicknellr]

Here's an example of the kind of situation I was referring to:

test('cloneNode', function() {
  const log = [];

  class LoggingElement extends HTMLElement {
    constructor() {
      super();
      log.push(this.localName);
    }
  }

  const ceInsertedName = generateLocalName();
  const CEInserted = class extends LoggingElement {};
  customElements.define(ceInsertedName, CEInserted);

  const ce1Name = generateLocalName();
  customElements.define(ce1Name, class extends LoggingElement {
    constructor() {
      super();
      this.insertAdjacentHTML('afterend', `<${ceInsertedName}></${ceInsertedName}>`);
    }
  });

  const ce2Name = generateLocalName();
  customElements.define(ce2Name, class extends LoggingElement {});

  const template = document.createElement('template');
  template.innerHTML = `<div><${ce1Name}><${ce2Name}></${ce2Name}></${ce1Name}></div>`;

  // Insert a clone of the template, so that the elements are contained in a
  // document with a registry.

  const div = template.content.querySelector('div').cloneNode(true);
  document.body.appendChild(div);
  console.log('original:', div.outerHTML);
  console.log('log:', log);

  // Remove the <ce-inserted> created by the initial upgrade, so that the DOM
  // content looks like the HTML from the template.

  const inserted = div.querySelector(ceInsertedName);
  assert(inserted && inserted instanceof CEInserted);
  div.removeChild(inserted);
  console.log('original:', div.outerHTML);

  // Clear the upgrade log and clone the div.

  log.length = 0;
  const clone = div.cloneNode(true);

  console.log('clone:', clone.outerHTML);
  console.log('log:', log);

  // <ce-inserted> should have been upgraded before <ce-2>, even though it comes
  // after <ce-2> in document order.

  assert.deepEqual(log, [ce1Name, ceInsertedName, ce2Name]);
});

[muan]

@bicknellr I see what both of your comments mean now. 🙈 When I merged it in I thought I only see the tests. I didn't see there's a different insertAdjacentHTML patch.

Looks like your patch takes care of this test case 💯. Perhaps I just close this? :)

[muan]

@bicknellr Hope my last comment made sense. Is there anything I can do here?

[bicknellr]

Oh, yeah it does seem like this branch includes both. Your call; if you want to keep going, sgtm, if not, I can pick up that other branch again.