Fix form in nested turbo-frame with target=_top

[tleish]

#396

The class LinkInterceptor checks if the target is a valid Element and if the closest turbo-frame matches the current turbo-frame.

    class LinkInterceptor {
        constructor(delegate, element) {
          //...
          if (this.respondsToEventTarget(event.target)) 

          respondsToEventTarget(target) {
              const element = target instanceof Element
                  ? target
                  : target instanceof Node
                      ? target.parentElement
                      : null;
              return element && element.closest("turbo-frame, html") == this.element;
          }

The FormInterceptor checks if the target is a HTMLFormElement, but does not check if the closest turbo-frame matches the current turbo-frame.

    class FormInterceptor {
        constructor(delegate, element) {
            //...
                if (event.target instanceof HTMLFormElement) {

As a result, a nested turbo-frame will bubble up to the parent turbo-frame when the turbo-frame is not disabled or the target == "_top". The parent turbo-frame should not be handling form events nested in a child turbo-frame.

[tleish]

@seanpdoyle - check my logic on this.

[seanpdoyle]

What is the significance of the second html selector?

Also, if the frame that the <form> is nested within matches turbo-frame[disabled], wouldn't we want events to bubble past it to the outer frame?

[seanpdoyle]

According to the commit message, the element.closest("turbo-frame, html") == this.element is lifted directly from the LinkInterceptor. The [disabled] checks happen within the delegates' shouldInterceptLinkClick and shouldInterceptFormSubmission, which on further consideration feels more aligned with the delegator pattern than leaking that concept up into the observer layer.

[tleish]

What is the significance of the second html selector?

I had the same question about the logic in link_interceptor.ts#L58, but figured I'd follow the same pattern to be safe.

Also, if the frame that the

is nested within matches turbo-frame[disabled], wouldn't we want events to bubble past it to the outer frame?

I also wondered this, and do not know the answer. Under what circumstances are a frame disabled?

I saw you were the last one that worked with this code, which is why I pinged you for review.

The [disabled] checks happen within the delegates' shouldInterceptLinkClick and shouldInterceptFormSubmission, which on further consideration feels more aligned with the delegator pattern than leaking that concept up into the observer layer.

I originally placed the code in shouldInterceptFormSubmission, but then went this direction after seeing how LinkInterceptor handled it.

private shouldInterceptNavigation(element: Element, submitter?: Element) {
  const id = submitter?.getAttribute("data-turbo-frame") || element.getAttribute("data-turbo-frame") || this.element.getAttribute("target")

- if (!this.enabled || id == "_top") {
+ if (!this.enabled || id == "_top" || this.element != element.closest("turbo-frame") || id == "_top") {
  }
    return false
  }

It seemed odd to me to handle it one way for links and another for forms when the overall concept is very similar. Almost seems like there's some shared code that should exist here.

FYI, I've seen this happen a couple of times with turbo, where updates are made to link logic, but not forms or the other way around.

[seanpdoyle]

I've opened #402 and #382 to attempt to reduce the surface area of the delegate interfaces, and to attempt to use consistent method naming across similar scenarios.

I don't think they'd address this particular issue entirely, but it's a start.

[boardfish]

I've got a piece of functionality ready to go that relies on the issue being fixed, so this PR getting merged would be helpful to me. Are those two PRs a prerequisite for this one to go through? I should also ask whether there's anything I can do to help progress this further.

[seanpdoyle]

@boardfish they're parallel concerns in my mind, and shouldn't block this one (#399).

[boardfish]

A test for a form with data-turbo-frame="_top" might also be helpful here: https://turbo.hotwired.dev/reference/frames#frame-with-overwritten-navigation-targets

[tleish]

add the additional test