alpine: Regression in handleAttributeBindingDirective: el.setSelectionRange is applied naively
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!
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?
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Comments: 35 (16 by maintainers)
Commits related to this issue
- Fix for issue #401. Preserve text selection on applicable inputs. Test is currently failing. — committed to 03juan/alpine by deleted user 4 years ago
- Merge pull request #403 from 03juan/fix-cursor-selection-when-binding-value Fixes #401. Preserve text selection on applicable inputs — committed to alpinejs/alpine by calebporzio 4 years ago
@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. 👍
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.
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.
Probably more of a chain-of-review issue, where all PRs should be proven by tests before merging? Either way always a learning experience 😌
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.
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 😃
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.Yes! @03juan appreciated for the bug hunt. 😃
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)
Since initially we are storing
selectionStarton cursorPosition, wouldn’t it be easier if we just check as:if(el === document.activeElement && cursorPosition !== null) {Great, thanks for the comments. I will give this PR a shot.
That looks like a good fix, would you mind submitting a PR (ideally with a test for this case)?