Make Trix compatible with morphing #1227

jorgemanrubia · 2025-03-03T16:22:42Z

Morphing the trix editor will leave it in an unusuable state because morphing will replace the bootstrapping elements without triggering the connection logic.

The solution is based on adding an attribute to flag the editor as initialized and detect when that attribute is modified.

See hotwired/turbo-rails#533 (comment)

Pending:

Tests

jorgemanrubia · 2025-03-03T16:24:33Z

@seanpdoyle I would love your eyes on the approach 🙏

seanpdoyle · 2025-03-03T16:48:46Z

src/trix/elements/trix_editor_element.js

+  // Element callbacks
+
+  attributeChangedCallback(name, oldValue, newValue) {
+    if (name === "initialized" && oldValue && oldValue !== newValue) {


What happens to an editor with the [initialized] attribute that is disconnected from the page, written to a cache (like Turbo's cache), then re-connected with the [initialized] attribute already encoded into the HTML?

Would the [initialized] attribute need to be removed in the custom element's disconnectedCallback()?

seanpdoyle · 2025-03-03T16:49:30Z

src/trix/elements/trix_editor_element.js

@@ -485,6 +497,8 @@ export default class TrixEditorElement extends HTMLElement {
      }
      this.editorController.registerSelectionManager()
      this.#delegate.connectedCallback()
+
+      this.setAttribute("initialized", "true")


Since this is to be treated as a boolean attribute, would Element.toggleAttribute with true as an argument be more appropriate?

Suggested change

this.setAttribute("initialized", "true")

this.toggleAttribute("initialized", true)

We could even base the check simply on whether the attribute is present or not, and it could be blank.

Tweaked it to use toogleAttribute and changed the check accordingly.

seanpdoyle · 2025-03-03T16:51:08Z

src/trix/elements/trix_editor_element.js

+    this.disconnectedCallback()
+    this.connectedCallback()


Does this approach work with an existing <trix-toolbar> element? What about a <trix-editor> element with a [toolbar] attribute that references another element elsewhere in the page? Are the toolbars re-initialized as expected without attaching a duplicate?

Good points, I'll see how to make these cases work. You are right that it will lose bootstrapped toolbars. I'll make sure it works well with both scenarios (existing toolbar and bootstrapped).

I added some logic to (1) flag internal (bootstrapped) trix toolbars and (2) remove those when reconnecting. This works well in my test.s

seanpdoyle · 2025-03-03T16:56:43Z

I'd opened hotwired/turbo-rails#688 on the turbo-rails side to explore a different path toward support. I don't love it, and am skeptical that it's a complete solution. Having said that, sharing it here provides some additional context to the breadth of the problem.

src/trix/elements/trix_editor_element.js

jorgemanrubia · 2025-03-06T09:25:02Z

I'd opened hotwired/turbo-rails#688 on the turbo-rails side to explore a different path toward support. I don't love it, and am skeptical that it's a complete solution. Having said that, sharing it here provides some additional context to the breadth of the problem.

Thanks for the exploration @seanpdoyle. I prefer trying to fix this as the Trix level and keep the turbo Rails gem ignorant about this, as much as possible.

seanpdoyle · 2025-03-06T11:44:40Z

src/trix/elements/trix_editor_element.js

@@ -494,6 +507,18 @@ export default class TrixEditorElement extends HTMLElement {
    this.#delegate.disconnectedCallback()
  }

+  async reconnect() {


What is the significance of defining this function as asynchronous?

Does it still function if omitted?

Suggested change

async reconnect() {

reconnect() {

Oh yes, good catch. That's a remaining bit from some earlier explorations 👍

seanpdoyle · 2025-03-06T16:25:22Z

src/trix/elements/trix_editor_element.js

@@ -410,6 +412,7 @@ export default class TrixEditorElement extends HTMLElement {
    } else if (this.parentNode) {
      const toolbarId = `trix-toolbar-${this.trixId}`
      this.setAttribute("toolbar", toolbarId)
+      this.toggleAttribute("internal-toolbar", true)


This line toggles the [internal-toolbar] attribute on the the <trix-editor> (since this references that element).

However, the removeInternalToolbar() function defined below checks this.toolbarElement (the <trix-toolbar> if the <trix-editor> element is using an "internal" toolbar).

Should this line set the attribute on the toolbar, or should the check be switched from this.toolbarElement to this?

Furthermore, is it important that the attribute be set? Could this same kind of internal state tracking be achieved by a JavaScript object property that isn't represented in the DOM?

Ugh I think I messed that up when switching it to use toogleAttribute. I'll also make sure the tests catch this. Thanks 👍

Morphing the trix editor will leave it in an unusuable state because morphing will replace the bootstrapping elements without triggering the connection logic. The solution is based on adding an attribute to flag the editor as initialized and detect when that attribute is modified. See hotwired/turbo-rails#533 (comment)

jorgemanrubia requested a review from brunoprietog March 3, 2025 16:23

jorgemanrubia marked this pull request as draft March 3, 2025 16:48

seanpdoyle reviewed Mar 3, 2025

View reviewed changes

jorgemanrubia force-pushed the morphing branch from aa115d3 to 4253057 Compare March 3, 2025 16:52

brunoprietog reviewed Mar 4, 2025

View reviewed changes

src/trix/elements/trix_editor_element.js Outdated Show resolved Hide resolved

jorgemanrubia force-pushed the morphing branch 7 times, most recently from 989bf70 to 6ffd337 Compare March 5, 2025 14:31

jorgemanrubia marked this pull request as ready for review March 5, 2025 14:34

jorgemanrubia force-pushed the morphing branch 7 times, most recently from 6c6b54d to 98b4d0c Compare March 6, 2025 16:16

seanpdoyle reviewed Mar 6, 2025

View reviewed changes

jorgemanrubia added 2 commits March 6, 2025 17:32

Attempt to fix solano errors

f0a7d4e

jorgemanrubia force-pushed the morphing branch from 98b4d0c to f0a7d4e Compare March 6, 2025 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Trix compatible with morphing #1227

Make Trix compatible with morphing #1227

jorgemanrubia commented Mar 3, 2025

jorgemanrubia commented Mar 3, 2025 •

edited

Loading

seanpdoyle Mar 3, 2025 •

edited

Loading

seanpdoyle Mar 3, 2025

brunoprietog Mar 4, 2025

jorgemanrubia Mar 5, 2025

seanpdoyle Mar 3, 2025

jorgemanrubia Mar 3, 2025

jorgemanrubia Mar 5, 2025

seanpdoyle commented Mar 3, 2025

jorgemanrubia commented Mar 6, 2025

seanpdoyle Mar 6, 2025

seanpdoyle Mar 6, 2025

jorgemanrubia Mar 6, 2025

seanpdoyle Mar 6, 2025

seanpdoyle Mar 6, 2025

jorgemanrubia Mar 6, 2025

	this.setAttribute("initialized", "true")
	this.toggleAttribute("initialized", true)

Make Trix compatible with morphing #1227

Are you sure you want to change the base?

Make Trix compatible with morphing #1227

Conversation

jorgemanrubia commented Mar 3, 2025

jorgemanrubia commented Mar 3, 2025 • edited Loading

seanpdoyle Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanpdoyle commented Mar 3, 2025

jorgemanrubia commented Mar 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgemanrubia commented Mar 3, 2025 •

edited

Loading

seanpdoyle Mar 3, 2025 •

edited

Loading