Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong syntax for document.createElement() options #3335

Open
paulie4 opened this issue Jun 24, 2020 · 5 comments
Open

Wrong syntax for document.createElement() options #3335

paulie4 opened this issue Jun 24, 2020 · 5 comments

Comments

@paulie4
Copy link
Contributor

paulie4 commented Jun 24, 2020

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
Copy link
Contributor

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
Copy link
Contributor Author

paulie4 commented Mar 19, 2021

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
Copy link
Contributor Author

paulie4 commented Mar 19, 2021

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

@evs-chris
Copy link
Contributor

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
Copy link
Contributor Author

paulie4 commented Mar 21, 2021

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);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants