Regression in handleAttributeBindingDirective: el.setSelectionRange is applied naively

[03juan]

Please bear with me as this is the first issue I've ever opened...

Unfortunately the fix from PR #367 has introduced a bug when binding a value to a subset of input types.

According to the MDN page on setSelectionRange():

Note that accordingly to the WHATWG forms spec selectionStart, selectionEnd properties and setSelectionRange method apply only to inputs of types text, search, URL, tel and password. Chrome, starting from version 33, throws an exception while accessing those properties and method on the rest of input types. For example, on input of type number: "Failed to read the 'selectionStart' property from 'HTMLInputElement': The input element's type ('number') does not support selection".

I was playing with some code on Firefox, updated to v2.3.0, and then started getting their version of the above Chrome error:

InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable

which is just ridiculously cryptic and took me a while to realize the problem!

alpine/src/directives/bind.js

Lines 45 to 51 in 49de05c

	} else {
	// Cursor position should be restored back to origin due to a safari bug
	const cursorPosition = el.selectionStart
	el.value = value
	if(el === document.activeElement) {
	el.setSelectionRange(cursorPosition, cursorPosition)
	}

At the very least L49 should check that the type is not any of those types before attempting to set the selection.

Something like

(el === document.activeElement && ['text', 'search', 'url', 'password'].includes(el.type)) 

should fix it, but I don't know if the developers would prefer to hoist this type check to the parent if/else block and only declare const cursorPosition for those types, and not also for everything else. Or at least move that const declaration inside the L49 if to optimize memory a bit.

Thoughts?

[HugoDF]

That looks like a good fix, would you mind submitting a PR (ideally with a test for this case)?

[ryangjchandler]

Agreed, it is quite a naive solution. When submitting a PR, it might be worth updating the current test (if there is one, there should be). If you've got any questions, just ask on here and one of us will get back to you! :)

[03juan]

Great, thanks for the comments. I will give this PR a shot.

[SimoTod]

Good finding. it definitely needs fixing. 👍
I assume that el.selectionStart returns null or undefined for other field.
I would consider the initial suggestion in that PR about setting it only if it's defined instead of looping on an array but, given the size of that list, it probably doesn't matter that much.

[MuzafferDede]

Since initially we are storing selectionStart on cursorPosition, wouldn't it be easier if we just check as:

if(el === document.activeElement && cursorPosition !== null) {

[ryangjchandler]

Since initially we are storing selectionStart on cursorPosition, wouldn't it be easier if we just check as:

if(el === document.activeElement && cursorPosition !== null) {

Well it depends on what cursorPosition is for those other fields. It could be undefined or null, JavaScript isn't very consistent.

[03juan]

There is another unintended consequence where setting the start and end of the range to the same value clobbers any previously-made selection on the text itself.

I don't know how likely the situation would be but if:

1. we're binding the text input value with x-model
2. the user highlights a section of text
3. something other than the user changes the model

then we lose the selection range

I'm thinking we actually need to do something like:

const selectionStart = el.selectionStart
const selectionEnd = el.selectionEnd
const selectionDirection = el.selectionDirection

el.value = value

if(el === document.activeElement && ['text', 'search', 'url', 'password'].includes(el.type)) {
    el.setSelectionRange(selectionStart, selectionEnd, selectionDirection)
}

[03juan]

Good finding. it definitely need fixing. 👍
I assume that el.selectionStart returns null or undefined for other field.
I would consider the initial suggestion in that PR about setting it only if it's defined instead of looping on an array but, given the size of that list, it probably doesn't matter that much.

I agree that looping is unnecessary, but the most likely candidate would be 'text', and incidentally I don't see much sense in worrying about selecting text on 'password', but might as well make it complete.

[MuzafferDede]

@03juan That what i was going to do first, also mentioned here. After the new value, the original selection doesn't make sense sine it will be changed anyway. So, original position wont be same as in the end result. We only need start position to replace selected text with new value

[03juan]

@03juan That what i was going to do first, also mentioned here. After the new value, the original selection doesn't make sense sine it will be changed anyway. So, original position wont be same as in the end result. We only need start position to replace selected text with new value

I think @MuzafferDede is right with

Since initially we are storing selectionStart on cursorPosition, wouldn't it be easier if we just check as:

if(el === document.activeElement && cursorPosition !== null) {

as long as we can consistently get null back, which is what the spec says should happen

The selectionStart attribute's getter must run the following steps:

1. If this element is an input element, and selectionStart does not apply to this element, return null.
2. If there is no selection, return the code unit offset within the relevant value to the character that immediately follows the text entry cursor.
3. Return the code unit offset within the relevant value to the character that immediately follows the start of the selection.

edit: Oh, and if we don't care about preserving selection if x-model changes the value

[MuzafferDede]

i just tried this:

            const selectionStart = el.selectionStart
            const selectionEnd = el.selectionEnd
            const selectionDirection = el.selectionDirection

            el.value = value

            if(el === document.activeElement && selectionStart !== null) {
                el.setSelectionRange(selectionStart, selectionEnd, selectionDirection)
            }

it's seems fine. I think i missed the sectionDirection last time so it didn't work. I just feel we are re-inventing the wheel :). Tests are green but we might need new tests for these cases.

Update

This is the output of console.log(selectionStart,selectionEnd,selectionDirection)

Start and End always same. Not sure if any matter of direction

[SimoTod]

It's good enough for me. A comment mentioning that it's due to a Safari issue when setting the value via code would probably help in the future so we remember why we are reinventing the wheel. :)
(and it will help Caleb when he's reviewing the PR)

[MuzafferDede]

Yes! @03juan appreciated for the bug hunt. :)

[03juan]

Happy to let @MuzafferDede update his code with a new PR, the null test is a more efficient solution than instantiating and looping through an array on every bound value that hits the else.

[MuzafferDede]

@SimoTod
Update: I tried quickly and adding the fix in bind.js seems to work without using timeout.
I'm happy for you to send a PR for this so you get recognised as contributor since you provided the initial solution.

This is how i made my first contribution by @SimoTod 's invite. I think i will be more happy if you send your first PR :)

[SimoTod]

This is how i made my first contribution by @SimoTod 's invite. I think i will be more happy if you send your first PR :)

@03juan Exactly, you found the issue and coded most of the solution (including the suggestion about selctionEnd and selectionDirection to preserve the selection). Muzaffer is more then happy to leave this with you if you have time to work on it so you can get the "inestimable" contributor badge.
Welcome to the club. 👍

[03juan]

Thanks for the kind comments. I am having an issue getting my test to pass, though. I'll post the PR as a draft and would appreciate your input on what I might be doing wrong.

[MuzafferDede]

Not sure if this what you intent to do.

[03juan]

Not sure if this what you intent to do.

Almost. I wanted to show that the selection values were preserved when the value changes. Thanks!

[MuzafferDede]

Very nice! 👍 👍 great success!

[SimoTod]

Well done. 👍

if you have time i would consider adding a test for the original bug (javascript error when using x-model on an input type that doesn't support the setSelectionRange).

It shouldn't be too hard, you can use an input range with x-model, set the focus, trigger an input with value = 10 and check that both the input and the variable in the model update correctly.
If you apply the same test to the master branch, you should see a failure (it's important to get the expected failure first so you make sure that you are correctly testing the bug fix).

[03juan]

Well done. 👍

if you have time i would consider adding a test for the original bug (javascript error when using x-model on an input type that doesn't support the setSelectionRange).

It shouldn't be too hard, you can use an input range with x-model, set the focus, trigger an input with value = 10 and check that both the input and the variable in the model update correctly.
If you apply the same test to the master branch, you should see a failure (it's important to get the expected failure first so you make sure that you are correctly testing the bug fix).

I considered it and thought that it might not be necessary since we're relying on a browser feature that has been expected to return null since IE9.

However it's probably better to have more tests that confirm everything is good, rather than hoping for the best, so I'll be happy to implement your suggestion tonight.

Thanks!

[SimoTod]

Yep, not a blocker but another reason we add tests is to avoid regressions. Your code works okay now, you saw it and the test won't really add value niw but we'll stop someone else from breaking it again in the future if they refractor that piece of code. For example, if we had had that test before, the regression would have not made into the release. I hope it makes sense.

[MuzafferDede]

For example, if we had had that test before, the regression would have not made into the release. I hope it makes sense.

That's how feel. Not sure if i should feel guilty about it. But this is a very sneaky bug that's only happens in specific condition.

[ryangjchandler]

Whether or not it's sneaky, all edge cases should be tested really. At least we know now going forwards.

[03juan]

For example, if we had had that test before, the regression would have not made into the release. I hope it makes sense.

That's how feel. Not sure if i should feel guilty about it. But this is a very sneaky bug that's only happens in specific condition.

Probably more of a chain-of-review issue, where all PRs should be proven by tests before merging? Either way always a learning experience 😌

[ryangjchandler]

For example, if we had had that test before, the regression would have not made into the release. I hope it makes sense.

That's how feel. Not sure if i should feel guilty about it. But this is a very sneaky bug that's only happens in specific condition.

Probably more of a chain-of-review issue, where all PRs should be proven by tests before merging? Either way always a learning experience 😌

We (me, @HugoDF, @SimoTod) do try and comment on PRs to ensure they're fully tested, this one just slipped past a little since there are inconsistencies between input types / elements. Definitely a good thing to remember going forward, research the solution too :)

[03juan]

We (me, @HugoDF, @SimoTod) do try and comment on PRs to ensure they're fully tested, this one just slipped past a little since there are inconsistencies between input types / elements. Definitely a good thing to remember going forward, research the solution too :)

Sorry, hope it didn't come across as pointing fingers. I really appreciate all of your efforts and the fact alpine exists at all. 🙇

[MuzafferDede]

Whether or not it's sneaky, all edge cases should be tested really. At least we know now going forwards.

@ryangjchandler Yes, i was wondering how up until now there was no any tests for any other inputs then type "text". Which brings me to this question: Should alpine do model bindings for each input types individually for future feature tests? Because as far as i know it there are still missing other inputs than type 'text' and 'hidden' including the current PR fix.

[SimoTod]

To be honest, i did not test it because I've been busy lately. It's probably less edge case than we think. I would expect it to break for checkboxes, radio buttons, etc.
However, it's an open source project so nobody has to feel guilty, regressions do happen sometimes expecially when a project doesn't have a QA phase. Something to improve. Thanks everyone we can leave it with Caleb now, hopefully he'll be able to tag a new release soon.

[ryangjchandler]

We (me, @HugoDF, @SimoTod) do try and comment on PRs to ensure they're fully tested, this one just slipped past a little since there are inconsistencies between input types / elements. Definitely a good thing to remember going forward, research the solution too :)

Sorry, hope it didn't come across as pointing fingers. I really appreciate all of your efforts and the fact alpine exists at all. 🙇

@03juan Didn't think that at all, criticism is a good thing - helps us understand where and how we can improve.

@MuzafferDede RE test coverage - I think in 95% of cases behaviours are consistent and handled correctly by Alpine, but there will be certain cases such as this where one little bug throws other things off / regressions. These things do happen in OSS.

[03juan]

Whether or not it's sneaky, all edge cases should be tested really. At least we know now going forwards.

@ryangjchandler Yes, i was wondering how up until now there was no any tests for any other inputs then type "text". Which brings me to this question: Should alpine do model bindings for each input types individually for future feature tests? Because as far as i know it there are still missing other inputs than type 'text' and 'hidden' including the current PR fix.

That was essentially my intention in the original suggestion of testing for el.type inclusion in the array of strings, that way if it's any other type we only set value unless we have to worry about cursor position. Like @ryangjchandler says, we're catching most edge cases and someone will make new contributions to OSS if there are any new issues.

[MuzafferDede]

@03juan my suggestion to include other input types was considering whole bind and model tests on inputs. Not for this issue only.

[gio00]

Hey, is this issue related to this error i get on Safari?

Unhandled Promise Rejection: TypeError: Type error - alpine.js:561

Because, if i go to that line i see exactly the line of code that mention the Safari Bug. Is there a workaround? I'm on Safari 13.1

[03juan]

@03juan my suggestion to include other input types was considering whole bind and model tests on inputs. Not for this issue only.

Fair enough. A good conversation to carry on in a new thread 👍

Hey, is this issue related to this error i get on Safari?

Unhandled Promise Rejection: TypeError: Type error - alpine.js:561

Because, if i go to that line i see exactly the line of code that mention the Safari Bug. Is there a workaround? I'm on Safari 13.1

@gio00 Quite likely, as per #404. Either revert versions or patch your local copy until the fix is merged? See #403