Fix a Text Selection Exception in Safari

[chrisvanpatten]

Description

Wraps the isTextField logic inside a try/catch block to handle an exception when trying to get selectionStart on a non-text input in Safari.

Also adds an inline comment to explain the reasoning for the try/catch block, for future devs.

Fixes #8254.

(Text Selection Exception is also my new band name.)

How has this been tested?

Manually in Safari

Types of changes

Bug fix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[jorgefilipecosta]

Hi @chrisvanpatten thank you for catching and addressing this issue.
It seems the isTextField can be called on safari many times and on safari calling it on non-text inputs and having to catch this exception may be expected/part of the normal flow. Ideally, exceptions should not be used for expected flows.
Do you think checking for a list of input types (text, email etc...) when the node name is equal to input can be a valid solution?

[chrisvanpatten]

@jorgefilipecosta I had thought about that but was concerned the list of valid input types could change from browser-to-browser and be harder to maintain. That said, I'm totally fine with that too.

Might need help with structuring the function… here's what I'd do if it were just for personal code, but admittedly it feels a bit verbose. Does this look okay?

// Textareas or anything with `contentEditable` pass the test
if ( element.nodeName === 'TEXTAREA' || element.contentEditable === true ) {
	return true;
}

// List of valid input types for selectionStart via WHATWG spec.
// https://html.spec.whatwg.org/multipage/input.html#concept-input-apply
const validInputTypes = [
	'password',
	'search',
	'tel',
	'text',
	'url',
];

// Filter out inputs that don't support selectionStart
if ( element.nodeName === 'INPUT' && ! includes( validInputTypes, element.type ) ) {
	return false;
}

// Return true for inputs that have a selectionStart value
if ( element.selectionStart !== null ) {
	return true;
}

// Return false for everything else
return false;

[jorgefilipecosta]

Sorry @chrisvanpatten I missed your ping. In my option, the logic you propose seems nice and is preferable over using a try catch. Maybe we can make validInputTypes a constant VALID_INPUT_TYPES_SELECTION_START or something similar.

[aduth]

I'm fine with either the revised approach or the original proposal. The try / catch is agreeable only because it seems in this case Safari is not following the standard by throwing, where the standard dictates:

If this element is an input element, and selectionStart does not apply to this element, return null.

https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#dom-textarea/input-selectionstart

...in which case we're capturing an unpredictable (non-standard) behavior.

The revised approach recreates the intent of the standard above, though in which case I'd find the condition of if ( element.selectionStart !== null ) { to be redundant and unnecessary.

[aduth]

I'm fine with either the revised approach or the original proposal. The try / catch is agreeable only because it seems in this case Safari is not following the standard by throwing, where the standard dictates:

If this element is an input element, and selectionStart does not apply to this element, return null.

https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#dom-textarea/input-selectionstart

...in which case we're capturing an unpredictable (non-standard) behavior.

The revised approach recreates the intent of the standard above, though in which case I'd find the condition of if ( element.selectionStart !== null ) { to be redundant and unnecessary.

[chrisvanpatten]

@aduth I agree that because Safari is breaking spec here, a try/catch is better. It's also a simpler, less rigid approach. I'll get this updated hopefully tomorrow.

[chrisvanpatten]

Okay, I've rebased this, expanded my code comment, and pending Travis success I think we're ready for merge. @aduth do you want to re-review?