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

Action Descriptor Syntax: Support Outlet listeners #621

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

seanpdoyle
Copy link
Contributor

The original idea for this change was outlined in a comment on hotwired/stimulus#576.

The problem

Prior to this commit, any Outlet-powered references would need to manage event listeners from within the Stimulus Controller JavaScript. For example, consider the following HTML:

<dialog id="dialog" data-controller="element">
  <span>This dialog is managed through a disclosure button powered by an Outlet.</span>

  <form method="dialog">
    <button>Close</button>
  </form>
</dialog>

<button type="button"
  data-controller="disclosure"
  data-disclosure-element-outlet="#dialog"
  data-action="click->disclosure#expand">
  Open dialog
</button>

Clicking the button[type="button"] opens the dialog element by calling its HTMLDialogElement.showModal() method. Consider the following element and disclosure controller implementations:

// element_controller.js
import { Controller } from "@hotwired/stimulus"

export default class extends Controller {
  showModal() {
    this.element.showModal()
  }
}

// disclosure_controller.js
import { Controller } from "@hotwired/stimulus"

export default class extends Controller {
  static outlets = ["element"]

  elementOutletConnected(controller, element) {
    this.element.setAttribute("aria-controls", element.id)
    this.element.setAttribute("aria-expanded", element.open)
    element.addEventListener("close", this.collapse)
  }

  elementOutletDisconnected() {
    element.removeEventListener("close", this.collapse)
    this.element.removeAttribute("aria-controls")
    this.element.removeAttribute("aria-expanded")
  }

  collapse = () => {
    this.element.setAttribute("aria-expanded", false)
    this.element.focus()
  }

  expand() {
    for (const elementOutlet of this.elementOutlets) {
      elementOutlet.showModal()
      this.element.setAttribute("aria-expanded", elementOutlet.element.open)
    }
  }
}

Note the mirrored calls to add and remove close event listeners. Whenever the dialog element closes, it'll dispatch a close event, which the disclosure controller will want to respond to.

Attaching and removing event listeners whenever an element connects or disconnects is one of Stimulus's core capabilities, and declaring event listeners as part of [data-action] is idiomatic. In spite of those facts, the disclosure controller is responsible for the tedium of managing its own event listeners.

The proposal

To push those declarations out of the JavaScript and back into the HTML, this commit extends the Action Descriptor syntax to support declaring actions with @-prefixed controller identifiers, in the same way that window and document are special-cased.

With that support, the HTML changes:

 <dialog id="dialog" data-controller="element">
   <span>This dialog is managed through a disclosure button powered by an Outlet.</span>

   <form method="dialog">
     <button>Close</button>
   </form>
 </dialog>

 <button type="button"
   data-controller="disclosure"
   data-disclosure-element-outlet="#dialog"
-  data-action="click->disclosure#expand">
+  data-action="click->disclosure#expand close@element->disclosure#collapse">
   Open dialog
 </button>

And our disclosure controller has fewer responsibilities, and doesn't need to special-case the collapse function's binding:

  elementOutletConnected(controller, element) {
    this.element.setAttribute("aria-controls", element.id)
    this.element.setAttribute("aria-expanded", element.open)
-   element.addEventListener("close", this.collapse)
  }

  elementOutletDisconnected() {
-   element.removeEventListener("close", this.collapse)
    this.element.removeAttribute("aria-controls")
    this.element.removeAttribute("aria-expanded")
  }

- collapse = () => {
+ collapse() {
    this.element.setAttribute("aria-expanded", false)
    this.element.focus()
  }

Risks

Changing the action descriptor syntax has more long-term maintenance risks that other implementation changes. If we "spend" the syntax on this type of support, we're pretty stuck with it.

Similarly, existing support for window and document as special symbols means that we'd need to make special considerations (or warnings) to support applications with window- and document-identified controllers.

@seanpdoyle seanpdoyle force-pushed the outlet-action-event-target branch 2 times, most recently from cc81eb0 to 4bbcafe Compare December 7, 2022 03:21
@marcoroth
Copy link
Member

Thanks for exploring this idea @seanpdoyle!

I really like the esthetics of your proposal. It also makes me wonder if this could be useful for regular Stimulus Targets

@seanpdoyle
Copy link
Contributor Author

I really like the esthetics of your proposal.

Thank you! I'm having some issues with the test suite, and am curious about how to best time the event listener binding code when outlets are connecting and disconnecting. I might ping you as I troubleshoot, if that's alright.

It also makes me wonder if this could be useful for regular Stimulus Targets

I had considered this, but the fact that target elements are in the scope of the controlled element as HTML children means that the controller can listen for events with the traditional syntax. The fact that outlets are elsewhere in the document is what requires expanding the syntax support.

@marcoroth
Copy link
Member

marcoroth commented Dec 11, 2022

I had considered this, but the fact that target elements are in the scope of the controlled element as HTML children means that the controller can listen for events with the traditional syntax.

Yeah, this is true since events bubble up, but still. If you want to get the "original" target element you always needed to write some code to get it back again.

But agreed, it's a lot more helpful for outlets.

@seanpdoyle seanpdoyle force-pushed the outlet-action-event-target branch 6 times, most recently from 0d4132f to df8f748 Compare December 11, 2022 20:55
@seb-jean
Copy link
Contributor

This PR will really help me for my project!

@seb-jean
Copy link
Contributor

How could we load dynamic content into the <dialog> element with this example? For example content that comes from a database.

The original [idea][] for this change was outlined in a comment on
[hotwired#576].

The problem
---

Prior to this commit, any Outlet-powered references would need to manage
event listeners from within the Stimulus Controller JavaScript. For
example, consider the following HTML:

```html
<dialog id="dialog" data-controller="element">
  <span>This dialog is managed through a disclosure button powered by an Outlet.</span>

  <form method="dialog">
    <button>Close</button>
  </form>
</dialog>

<button type="button"
  data-controller="disclosure"
  data-disclosure-element-outlet="#dialog"
  data-action="click->disclosure#expand">
  Open dialog
</button>
```

Clicking the `button[type="button"]` opens the `dialog` element by
calling its [HTMLDialogElement.showModal()][] method. Consider the following `element` and
`disclosure` controller implementations:

```js
// element_controller.js
import { Controller } from "@hotwired/stimulus"

export default class extends Controller {
  showModal() {
    this.element.showModal()
  }
}

// disclosure_controller.js
import { Controller } from "@hotwired/stimulus"

export default class extends Controller {
  static outlets = ["element"]

  elementOutletConnected(controller, element) {
    this.element.setAttribute("aria-controls", element.id)
    this.element.setAttribute("aria-expanded", element.open)
    element.addEventListener("close", this.collapse)
  }

  elementOutletDisconnected() {
    element.removeEventListener("close", this.collapse)
    this.element.removeAttribute("aria-controls")
    this.element.removeAttribute("aria-expanded")
  }

  collapse = () => {
    this.element.setAttribute("aria-expanded", false)
    this.element.focus()
  }

  expand() {
    for (const elementOutlet of this.elementOutlets) {
      elementOutlet.showModal()
      this.element.setAttribute("aria-expanded", elementOutlet.element.open)
    }
  }
}
```

Note the mirrored calls to add and remove [close][] event listeners.
Whenever the `dialog` element closes, it'll dispatch a `close` event,
which the `disclosure` controller will want to respond to.

Attaching and removing event listeners whenever an element connects or
disconnects is one of Stimulus's core capabilities, and declaring event
listeners as part of `[data-action]` is idiomatic. In spite of those
facts, the `disclosure` controller is responsible for the tedium of
managing its own event listeners.

The proposal
---

To push those declarations out of the JavaScript and back into the HTML,
this commit extends the Action Descriptor syntax to support declaring
actions with `@`-prefixed controller identifiers, in the same way that
`window` and `document` are special-cased.

With that support, the HTML changes:

```diff
 <dialog id="dialog" data-controller="element">
   <span>This dialog is managed through a disclosure button powered by an Outlet.</span>

   <form method="dialog">
     <button>Close</button>
   </form>
 </dialog>

 <button type="button"
   data-controller="disclosure"
   data-disclosure-element-outlet="#dialog"
-  data-action="click->disclosure#expand">
+  data-action="click->disclosure#expand close@element->disclosure#collapse">
   Open dialog
 </button>
```

And our `disclosure` controller has fewer responsibilities, and doesn't
need to special-case the `collapse` function's binding:

```diff
  elementOutletConnected(controller, element) {
    this.element.setAttribute("aria-controls", element.id)
    this.element.setAttribute("aria-expanded", element.open)
-   element.addEventListener("close", this.collapse)
  }

  elementOutletDisconnected() {
-   element.removeEventListener("close", this.collapse)
    this.element.removeAttribute("aria-controls")
    this.element.removeAttribute("aria-expanded")
  }

- collapse = () => {
+ collapse() {
    this.element.setAttribute("aria-expanded", false)
    this.element.focus()
  }
```

Risks
---

Changing the action descriptor syntax has more long-term maintenance
risks that other implementation changes. If we "spend" the syntax on
this type of support, we're pretty stuck with it.

Similarly, existing support for `window` and `document` as special
symbols means that we'd need to make special considerations (or
warnings) to support applications with `window`- and
`document`-identified controllers.

[hotwired#576]: hotwired#576
[idea]: hotwired#576 (comment)
[HTMLDialogElement.showModal()]: https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/showModal
[close]: https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/close_event
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants