Avoid bubbling form submissions that target a new window/tab

[martinzamuner]

Resolves #754

Avoid bubbling <form> element submissions when they target a new window/tab. This occurs when:

• a <form> declares a target of _blank
• a <form> element's submitting <button> element declares a formtarget of _blank
• a <form> element's submitting <input type="submit"> element declares a formtarget of _blank

Based on #389

[seanpdoyle]

@martinzamuner thank you for proposing this change!

When I'd proposed the submissionDoesNotTargetIFrame method, I'd forgotten about _blank:

turbo/src/observers/form_submit_observer.ts

Lines 59 to 67 in 732db00

	function submissionDoesNotTargetIFrame(form: HTMLFormElement, submitter?: HTMLElement): boolean {
	const target = submitter?.getAttribute("formtarget") || form.target
	for (const element of document.getElementsByName(target)) {
	if (element instanceof HTMLIFrameElement) return false
	}
	return true
	}

Instead of adding a new method, what do you think about combining submissionDoesNotTargetIFrame and submissionDoesNotTargetBlankWindow so that it checks for the presence of any [target] on the <form> or [formtarget] on the submitter?

Similarly, could you add coverage for the same scenario for <a> elements and make similar changes to the LinkClickObserver?

turbo/src/observers/link_click_observer.ts

Lines 74 to 80 in 732db00

	function doesNotTargetIFrame(anchor: HTMLAnchorElement): boolean {
	for (const element of document.getElementsByName(anchor.target)) {
	if (element instanceof HTMLIFrameElement) return false
	}
	return true
	}

?

[seanpdoyle]

If we're going to have a special case handler for _self, could we also add test coverage to guarantee that behavior and guard against regressions?

[martinzamuner]

Done! BTW, I had to remove the check on target from LinkClickObserver#findLinkFromClickTarget to avoid a full-page load in the case of [target=_self].

[martinzamuner]

@seanpdoyle Sure thing! I've added a special case for _self, which I think should behave the same as having no target. Let me know if you think this is unnecessary.

There was already a test for this behavior with anchors: https://github.com/hotwired/turbo/blob/main/src/tests/functional/navigation_tests.ts#L208-L213.

[martinzamuner]

@dhh Thanks for running the tests! There were a couple failures related to anchors inside SVGs behaving kind of weird. Their target is implemented through an SVGAnimatedString, which I didn't know. Using the getAttribute function fixed those.

Aside from that, those same tests were now broken because I added some elements before the SVGs in the fixture, and they were not above the fold anymore. That means that page.click would have to scroll to the anchors inside, but was failing to do so. I'm not exactly sure why (maybe something to do with this)? My solution was to scroll to the next element before attempting the click. It's not very elegant, though. Suggestions are welcomed.

[martinzamuner]

Fixed by #835.