Skip to content

Commit

Permalink
fix: fix checkbox event edge case in Firefox
Browse files Browse the repository at this point in the history
  • Loading branch information
@yyx990803
yyx990803 committed Jan 24, 2019
1 parent 8cb2069 commit 1868561
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 31 deletions.
2 changes: 1 addition & 1 deletion src/platforms/web/runtime/modules/dom-props.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ function updateDOMProps (oldVnode: VNodeWithData, vnode: VNodeWithData) {
// In Chrome / Firefox, click event fires before change, thus having this problem.
// In Safari / Edge, the order is opposite.
// Note: in Edge, if you click too fast, only the click event would fire twice.
if (key === 'checked' && !isNotInFocusAndDirty(elm, cur)) {
if (key === 'checked' && cur === oldProps[key]) {
continue
}

Expand Down
76 changes: 46 additions & 30 deletions test/unit/features/directives/model-checkbox.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ describe('Directive v-model checkbox', () => {
},
template: `
<div>
{{ test }}
<input type="checkbox" v-model="test" value="1">
<input type="checkbox" v-model="test" value="2">
</div>
Expand All @@ -65,13 +66,16 @@ describe('Directive v-model checkbox', () => {
expect(vm.$el.children[0].checked).toBe(true)
expect(vm.$el.children[1].checked).toBe(false)
vm.$el.children[0].click()
expect(vm.test.length).toBe(0)
vm.$el.children[1].click()
expect(vm.test).toEqual(['2'])
vm.$el.children[0].click()
expect(vm.test).toEqual(['2', '1'])
vm.test = ['1']
waitForUpdate(() => {
expect(vm.test.length).toBe(0)
vm.$el.children[1].click()
}).then(() => {
expect(vm.test).toEqual(['2'])
vm.$el.children[0].click()
}).then(() => {
expect(vm.test).toEqual(['2', '1'])
vm.test = ['1']
}).then(() => {
expect(vm.$el.children[0].checked).toBe(true)
expect(vm.$el.children[1].checked).toBe(false)
}).then(done)
Expand All @@ -93,13 +97,16 @@ describe('Directive v-model checkbox', () => {
expect(vm.$el.children[0].checked).toBe(true)
expect(vm.$el.children[1].checked).toBe(false)
vm.$el.children[0].click()
expect(vm.test.length).toBe(0)
vm.$el.children[1].click()
expect(vm.test).toEqual(['2'])
vm.$el.children[0].click()
expect(vm.test).toEqual(['2', '1'])
vm.test = ['1']
waitForUpdate(() => {
expect(vm.test.length).toBe(0)
vm.$el.children[1].click()
}).then(() => {
expect(vm.test).toEqual(['2'])
vm.$el.children[0].click()
}).then(() => {
expect(vm.test).toEqual(['2', '1'])
vm.test = ['1']
}).then(() => {
expect(vm.$el.children[0].checked).toBe(true)
expect(vm.$el.children[1].checked).toBe(false)
}).then(done)
Expand All @@ -121,13 +128,16 @@ describe('Directive v-model checkbox', () => {
expect(vm.$el.children[0].checked).toBe(true)
expect(vm.$el.children[1].checked).toBe(false)
vm.$el.children[0].click()
expect(vm.test.length).toBe(0)
vm.$el.children[1].click()
expect(vm.test).toEqual([2])
vm.$el.children[0].click()
expect(vm.test).toEqual([2, 1])
vm.test = [1]
waitForUpdate(() => {
expect(vm.test.length).toBe(0)
vm.$el.children[1].click()
}).then(() => {
expect(vm.test).toEqual([2])
vm.$el.children[0].click()
}).then(() => {
expect(vm.test).toEqual([2, 1])
vm.test = [1]
}).then(() => {
expect(vm.$el.children[0].checked).toBe(true)
expect(vm.$el.children[1].checked).toBe(false)
}).then(done)
Expand All @@ -149,13 +159,16 @@ describe('Directive v-model checkbox', () => {
expect(vm.$el.children[0].checked).toBe(true)
expect(vm.$el.children[1].checked).toBe(false)
vm.$el.children[0].click()
expect(vm.test.length).toBe(0)
vm.$el.children[1].click()
expect(vm.test).toEqual([{ a: 2 }])
vm.$el.children[0].click()
expect(vm.test).toEqual([{ a: 2 }, { a: 1 }])
vm.test = [{ a: 1 }]
waitForUpdate(() => {
expect(vm.test.length).toBe(0)
vm.$el.children[1].click()
}).then(() => {
expect(vm.test).toEqual([{ a: 2 }])
vm.$el.children[0].click()
}).then(() => {
expect(vm.test).toEqual([{ a: 2 }, { a: 1 }])
vm.test = [{ a: 1 }]
}).then(() => {
expect(vm.$el.children[0].checked).toBe(true)
expect(vm.$el.children[1].checked).toBe(false)
}).then(done)
Expand All @@ -177,13 +190,16 @@ describe('Directive v-model checkbox', () => {
expect(vm.$el.children[0].checked).toBe(true)
expect(vm.$el.children[1].checked).toBe(false)
vm.$el.children[0].click()
expect(vm.test.length).toBe(0)
vm.$el.children[1].click()
expect(vm.test).toEqual([[2]])
vm.$el.children[0].click()
expect(vm.test).toEqual([[2], { a: 1 }])
vm.test = [{ a: 1 }]
waitForUpdate(() => {
expect(vm.test.length).toBe(0)
vm.$el.children[1].click()
}).then(() => {
expect(vm.test).toEqual([[2]])
vm.$el.children[0].click()
}).then(() => {
expect(vm.test).toEqual([[2], { a: 1 }])
vm.test = [{ a: 1 }]
}).then(() => {
expect(vm.$el.children[0].checked).toBe(true)
expect(vm.$el.children[1].checked).toBe(false)
}).then(done)
Expand Down

4 comments on commit 1868561

@razvanioan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that this commit broke a special scenario I was using on an app:

  • radio group with checked bound to some vuex object property
  • change event dispatching an action
  • on action handler I was checking some conditions on data level (store) and in some cases I was not updating the property

Until [email protected] worked as expected (radio didn't change)
From [email protected] until latest 2.x version the checked property changes even if the bound store value not

For the moment I'll use [email protected] and won't update further.

I also know that change event it's not cancel-able, so maybe I'd have to use click event in this case (a little bit more complicated as the logic it's on data level and not UI, but doable)

@posva
Copy link
Member

@posva posva commented on 1868561 Aug 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the notes about nextTick changes in the 2.6.0 release should help you understand

@razvanioan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've understood, but in this case the bound property is out of sync: the checked element on the UI doesn't reflect store data

@posva
Copy link
Member

@posva posva commented on 1868561 Aug 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will be able to find similar (closed) issues about checkboxes

Please sign in to comment.