Mix in Turbo Frames to built-in elements.

[inopinatus]

This PR abstracts the FrameElement class into an anonymous class factory and matching interface type, permitting mixin to (almost) any element, and enabling registration of customized built-in elements as turbo frames in addition to the standard autonomous custom element.

Supports treating elements as a Turbo Frame that cannot otherwise carry one due to their content model, such as <tbody>.

Set up with:

Turbo.defineCustomFrameElement('tbody')

which will register the identifier turbo-frame-tbody as a customized built-in element extending tbody with Turbo Frame characteristics. Then apply the extended custom behaviour using an is attribute, e.g.

<table>
  <tbody is="turbo-frame-tbody" id="body0">
    <tr><td>Table content</td></tr>
  </tbody>
</table>

When navigating, the response frame tag must match by is attribute, in addition to matching on element name and id attributes as usual.

Implements: #48.

(Recommend reviewing with whitespace hidden, since there's an indent change obscuring meaningful diff otherwise.)

[seanpdoyle]

This is exciting, thank you for opening this @inopinatus!

I'm not very familiar with the [is] attribute in practice. Would declaring <tbody is="turbo-frame"> be viable, or is it important that turbo-frame-body and other occurrences have a suffix that differentiates its value from is="turbo-frame"?

[inopinatus]

Unfortunately yes, they have to be differentiating strings in the is attribute. The custom element registry enforces a globally unique name for each defined custom element, rather than uniqueness by the (base element, custom element) tuple which might suit us slightly better. I too would prefer to have been able to write <tbody is=“turbo-frame”>, seeming slightly less awkward. Sadly the API just isn’t built that way, and I've found that browsers are enforcing the constraints. I went with a turbo-frame-suffix pattern to enable the [is^="turbo-frame-"] selector in supporting/controller code.

The same registry also requires uniqueness of element constructor, which is partly motivating the anonymous class factory approach instead of, say, a generic proxy class.

[inopinatus]

Rebased onto current main - I still think this is useful.

[vanthao03596]

@inopinatus can use make it as library. I think this function is useful

[dhh]

@inopinatus Sorry for the delay on this. I'm very keen to see this move forward. Could we not just have it be "turbo-frame-container" or something else that's fixed? So we don't need -tbody or -div?

[dhh]

Ah, understand why we can't have a shared element. Hmm. In that case, we should start by simply registering all the main default container elements one would want to have as a turbo frame. article/main/div/tbody/p/etc.

[seanpdoyle]

Maybe I'm misunderstanding the motivations behind pre-defining "main default container elements", but what is the value in supporting a generic element like <div is="turbo-frame-div"> instead of using a <turbo-frame> in its place?

Is this about semantics? Would <turbo-frame role="region"> be compatible with <section is="turbo-frame-section">?

As I understood the initial version of this PR, it was to add support for <turbo-frame> elements within a <table> element only because browsers hoist <table> elements in a specialized way compared to other elements. Would it be best to keep this pre-definitions constrained to table elements like <thead>, <tbody>, <tr>, <td>, <th>?

[dhh]

Often it's about integration into existing setups that have set expectations about DOM structure. Like right now I'm dealing with a sortable setup in Basecamp where we're converting some div's to turbo frames, but the existing JS expects a certain DOM structure that doesn't work with an additional container inserted.

It's similar to the stimulus approach vs custom elements in general.

[dhh]

A bummer we need to address is that Safari does not support customizable built-in elements. And has said they don't want to do so either. I believe there's a polyfill, though.

[t27duck]

Would it make sense to expose defineCustomFrameElement on the Turbo window object so developers could specify their own containers without needing Turbo to manage a hard-coded list of all possible elements to apply this to?

Maybe Turbo could come included with a small set of elements like div, tbody, and aside and let developers decide if they need more for the existing structure.

[dhh]

Only if there's an actual cost to defining these custom frame elements, which I don't immediately see that there is.

[dhh]

Hmm, wasn't able to make this work in Safari by just dropping in @corpuscule/custom-builtin-elements or @ungap/custom-elements-builtin. If anyone wants to have a look at this, so we can get Safari compatibility going 🙏

[WebReflection]

Try @ungap/custom-elements instead, which has recently been updated to support upgrade too, and let me know if there’s any issue (a minimal example to reproduce the issue would be ace)

[WebReflection]

the is attribute is used by custom elements builtin extends ... if this property overrides the attribute no polyfill can possibly upgrade this element later on. Please avoid setting this attribute completely, use a different name for custom elements builtin extends, or set such attribute only if this.getAttribute('is') is not defined yet.

[inopinatus]

That is intentional, and necessary when the element is programatically created, due to this part of that same section of the HTML standard: "when creating a customized built-in element programmatically, the is attribute will not be present in the DOM, since it was not explicitly set".

To implement its own behaviour, Turbo usually depends on the element name. Since that is no longer possible in this case, it must instead depend instead on the is attribute being present in the DOM, to identify the element for update. Setting any other attribute would be a) DOM pollution, and b) a potential namespace clash. Using any other means would break the general design promise of progressively enhancing the HTML we already have, and amount to a redesign of Turbo's core behaviour which is definitely out of scope for this PR.

However I agree that it only needs setting if not already defined.

I hope to revisit this PR sometime this month, maybe with the revised polyfill it can even work on Safari, thanks!

[WebReflection]

Using CE since 2014 and wrote all polyfills to date that work with built-in extends too, no need to explain me what's the is attribute about ;-) and yes, I do that in vanilla-elements too, but the reason I've mentioned that is that the polyfill also does that so you don't need to care.

Anyway, a simple check if the attribute is not defined yet is all I am pointing out, and shame on me I didn't check what was the isValue value, but if it's the custom element name, as builtin extend, then all good, this shouldn't interfere with the polyfill, I thought it was something else, like Vue 3 abuse it.

Apparently it's not 👍

[WebReflection]

FYI this works without any issue on Safari/WebKit too:

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <title>Document</title>
  <script type="module">
  import 'https://unpkg.com/@ungap/custom-elements';
  const name = 'tbody';
  customElements.define(
    `turbo-frame-${name}`,
    class extends document.createElement(name).constructor {
      connectedCallback() {
        this.querySelector('td').textContent += ' 🥳';
      }
    },
    {extends: name}
  );
  </script>
</head>
<body>
  <table>
    <tbody is="turbo-frame-tbody" id="body0">
      <tr><td>Table content</td></tr>
    </tbody>
  </table>
</body>
</html>

but I think you are causing troubles to polyfills here because the is attribute is reserved by Custom Elements V1 standard when it comes to built-in extend.

You need to use a different attribute for builtins, or use a non reserved word. This shenanigan is sadly present in Vue 3 too.