feat: Ignore touch on more interactive elements

[StarScape]

Fixes #1654. Added in every interactive element I could think of, including contenteditable.

[birtles]

@StarScape Looks great, thank you!

I wonder if we should re-use isContentEditableNode() from src/utils/dom-utils.ts to test for content editable nodes since I think we could get taps on elements whose ancestor is contenteditable and we'd still want to ignore touches in that case.

While we're at it, maybe we should factor out an isInteractiveElement function for this? The dom-utils.ts file has other functions like isEditableNode etc. so it seems like it would fit in there. (As a side note, in that file we tend to test nodes by looking at their nodeType and tagName properties instead of using instanceof simply so that the functions work cross-frame. I don't think we actually need isInteractiveElement to work cross-frame at this point but if we're adding it to dom-utils.ts maybe we should avoid instanceof just to be consistent.)

[StarScape]

@birtles All great points! I've done what you suggested. Note I just modified the previous commit rather than creating another one—this is a simple enough change I don't think it warrants 2 commits in the history.

[birtles]

Thanks! Looks great.

[birtles]

Nit: If we make this take Node | null like many of the other functions in this file the call site will become a lot simpler.

Suggested change

	export function isInteractiveElement(elem: Element) {
	return (
	elem.tagName === 'A' ||
	elem.tagName === 'BUTTON' ||
	elem.tagName === 'INPUT' ||
	elem.tagName === 'TEXTAREA' ||
	elem.tagName === 'SELECT' ||
	elem.tagName === 'DATALIST' ||
	elem.tagName === 'OPTGROUP' ||
	elem.tagName === 'OPTION' ||
	isContentEditableNode(elem)
	);
	export function isInteractiveElement(elem: Node | null) {
	return (
	isElement(elem) &&
	(elem.tagName === 'A' ||
	elem.tagName === 'BUTTON' ||
	elem.tagName === 'INPUT' ||
	elem.tagName === 'TEXTAREA' ||
	elem.tagName === 'SELECT' ||
	elem.tagName === 'DATALIST' ||
	elem.tagName === 'OPTGROUP' ||
	elem.tagName === 'OPTION' ||
	isContentEditableNode(elem))
	);

[StarScape]

Yeah, agreed. Though I think we'll want to move the isContentEditableNode call up to, since, AFAIK, the event target could still be a non-element Node, like a text node, inside of a content editable. I think we'd still want to prevent a tap in those cases.

Though now that I type it out, I'm not sure if the browser will ever have text nodes as event targets?

[birtles]

With the suggestion below, I think this will become just:

Suggested change

	possibleTargetElem != null &&
	possibleTargetElem.nodeType === Node.ELEMENT_NODE &&
	isInteractiveElement(possibleTargetElem)
	isInteractiveElement(possibleTargetElem)

[StarScape]

Yep! One addition I missed previously: I have to do a type guard to with instanceof Node to make the call type check. As far as I can tell that's the only way to refine the EventTarget interface into Node in a type safe way (I could be wrong about that, I'm no TypeScript expert). My previous code with the as assertion wasn't actually safe, whoops 😬 .

The instanceof usage does lose the cross-frame capability, though. Not sure if that matters for particular this listener?

[birtles]

@birtles All great points! I've done what you suggested. Note I just modified the previous commit rather than creating another one—this is a simple enough change I don't think it warrants 2 commits in the history.

Thank you! Yep, I agree.

[birtles]

Perfect! Thank you!