Fixes #366 cursor position jumping to the end

[MuzafferDede]

Thanks to @SimoTod for his suggestion to apply the fix in the right file. Fixes #366

[ryangjchandler]

@MuzafferDede looks like the tests are failing, could you take a look when you get a second please :) I'd suggest double check that they're passing locally first by running 'npm run test model' which is where they are failing (model.spec.js).

[MuzafferDede]

test('x-model casts value to number if number modifier is present', async () => {
    document.body.innerHTML = `
        <div x-data="{ foo: null }">
            <input type="number" x-model.number="foo"></input>
        </div>
    `

    Alpine.start()

    fireEvent.input(document.querySelector('input'), { target: { value: '123' }})

    await wait(() => { expect(document.querySelector('[x-data]').__x.$data.foo).toEqual(123) })
})

Is it because "text" and "number" inputs uses same test?

[SimoTod]

I think you need to set those properties back only if they are defined. Changes can come from the linked property being updated by something else.
In that case, your element doesn't have a cursor so when you read selectionStart and selectionEnd they return something like null or undefined.
If you try to set them back, you are going to get the error because a cursor position has to be a number.

[HugoDF]

I think you need to set those properties back only if they are defined. Changes can come from the linked property being updated by something else.

Maybe "only if they are different"?

[SimoTod]

Maybe "only if they are different"?

I would say defined, they are often going to be different in safari due to the apple bug but it doesn't mean that the value received is valid, I think.

It seems to happen only in the test framework due to a partial implementation of jsdom any way.

However, in safari, it results in the input acquiring the focus when it didn't have it so it needs some additional fixes. (See https://codepen.io/SimoTod/pen/oNjXbmy)

[SimoTod]

@MuzafferDede You might want to check if your current element matches document.activeElement before setting those values back.

It means that you won't change those properties if the input field doesn't have the focus.

That should prevent the other bug in the codepen and it should also fix the tests.

The property is supported by all major browser (https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot/activeElement) so it's safe to use.

[MuzafferDede]

Yes, @SimoTod i did check if current element is active element, then apply the cursor positions. Tests are green now.

[ryangjchandler]

Can we not just use el.setSelectionRange(start, end) instead of the individual setters?

[SimoTod]

I think it's the same, right?

[ryangjchandler]

I think it's the same, right?

Yeah, just thinking one less line. No worries, just me being pedantic

[MuzafferDede]

I was thinking that as well but if you notice, cursorPosition is selectionStart and applied for both selectionStart and selectionEnd. I see some weird behave in safari if i store both and apply both. Since i was thinking this is a safari bug so maybe because of that it's behave weird. So i use start position only. We could still do something like this for sake of better code.

const cursorPosition = {
   start: el.selectionStart,
   end: el.selectionEnd,
}

el.value = value

el === document.activeElement ?: restoreCursor(el,cursorPosition)
...
function restoreCursor(el,cursorPosition) {
   el.selectionStart = cursorPosition.start
   el.selectionEnd = cursorPosition.end
}

[ryangjchandler]

I think what you done is fine. I was only suggesting consolidating it into one single line instead of the 2 setters.

[calebporzio]

This looks great. Thanks!