ractivejs / ractive

Next-generation DOM manipulation
http://ractive.js.org
MIT License
5.94k stars 396 forks source link

Wrong syntax for document.createElement() options #3335

Open paulie4 opened 4 years ago

paulie4 commented 4 years ago

The three uses of extend, starting on this line are passing the extend value directly to document.createElement() and document.createElementNS(), but the caller is passing in this.getAttribute('is'), which is just a string so can't be used directly but instead needs to be wrapped in a {is: ...}, see the descriptions of the options parameter in MDN's docs.

Here's my recommended diff:

diff --git a/src/utils/dom.js b/src/utils/dom.js
index 84cb5ded7..e287a84fc 100755
--- a/src/utils/dom.js
+++ b/src/utils/dom.js
@@ -12,15 +12,15 @@ if (!svg) {
       throw "This browser does not support namespaces other than http://www.w3.org/1999/xhtml. The most likely cause of this error is that you're trying to render SVG in an older browser. See http://ractive.js.org/support/#svgs for more information";
     }

-    return extend ? doc.createElement(type, extend) : doc.createElement(type);
+    return extend ? doc.createElement(type, {is: extend}) : doc.createElement(type);
   };
 } else {
   createElement = (type, ns, extend) => {
     if (!ns || ns === html) {
-      return extend ? doc.createElement(type, extend) : doc.createElement(type);
+      return extend ? doc.createElement(type, {is: extend}) : doc.createElement(type);
     }

-    return extend ? doc.createElementNS(ns, type, extend) : doc.createElementNS(ns, type);
+    return extend ? doc.createElementNS(ns, type, {is: extend}) : doc.createElementNS(ns, type);
   };
 }
evs-chris commented 3 years ago

Interestingly, puppeteer fails the existing test when using { is } rather than is, where is is the string from the is attribute. I took a swing at this by deciding which to use based on the presence of document.registerElement, which is present in chrome, where apparently a string is wanted, and not in firefox, where the object seems to be required. Unfortunately, due to some legacy requirements, I can't actually write a test for the new method using customElements.define because it requires using a class, which in this codebase gets transpiled away to a function and prototype, which is not supported, apparently.

All that goes to say that when travis finishes doing its thing, the change will be on edge, and we'll see where it goes from there. I imagine there's probably a better way to use custom elements now, maybe just by throwing them in a tag, and it would probably be best to support that. I haven't personally used any though, as I haven't felt the need, so if anyone has any guidance, it would be appreciated.

paulie4 commented 3 years ago

Here's a quick and dirty example to test a custom element that extends a native element:

customElements.define('div-with-span',
  class extends HTMLDivElement {
    constructor() {
      super();
      this.innerHTML = '<span>testing 3 2 1</span>';
    }
  }, {extends: 'div'});
document.body.append(document.createElement('div', {is: 'div-with-span'}));

Is that what you were looking for? Is there anything else I can help with? Thanks!

paulie4 commented 3 years ago

BTW, Puppeteer OK if you use {is: is} instead of just { is }?

evs-chris commented 3 years ago

I did successfully test the behavior like you suggested, but I can't create a test case due to the way the code is transpiled. That transpilation also handles converting { is } to { is: is }, so I'm going to say no, puppeteer didn't like it.

I was wondering if there are more ergonomic uses of custom elements, like <SomeTwitterThing> that we could possibly detect by customElements.get('SomeTwitterThing') or something like that. I don't suppose it matters that the custom element is created and possibly has its body (slots?) attached later?

paulie4 commented 3 years ago

You can just check to make sure the new element is an instanceof the Custom Element class:

class DivWithSpan extends HTMLDivElement {
  constructor() {
    super();
    this.innerHTML = '<span>testing 3 2 1</span>';
  }
}
customElements.define('div-with-span', DivWithSpan, {extends: 'div'});
const mydiv = document.createElement('div', {is: 'div-with-span'});
console.assert(mydiv instanceof DivWithSpan);