Skip to content

Commit

Permalink
fix(lib): wrong thumbs of range Slider
Browse files Browse the repository at this point in the history
- The bug that `Slider` showed thumbs at wrong positions when a range
  slider was used. This bug was caused by circular update among the
  following methods and data,
    - modelValue
    - setValues
    - value1
    - value2
    - onInternalValueUpdate
    - emitValue('update:modelValue')

  The problem was in the `setValues` method. Its intention is to update
  `value1` and `value2`, and then do `onInternalValueUpdate`. But
  because it updated `value1` and `value2` one by one,
  `onInternalValueUpdate` was prematurely called after only `value1`
  was updated. The premature `onInternalVaueUpdate` ended up with
  premature emit of `update:modelValue` event, and the thumb positions
  of slider were messed up. This also caused a "maximum recusion
  exceeded" error when `Slider` component was updated but I have not
  confirmed this happens in a production environment.

  The bug is fixed by introducing a `internal` data that works as a
  buffer to update `value1` and `value2` with a single shot. It is also
  used to stop unnecessary propagation of `value1` and `value2` update.

  I think that this did not happen on Vue 2 because watch methods are
  called in another event loop iteration.
- Minor changes,
    - Events to be emitted by `Slider` and `SliderThumb` are listed in
      `emits`.
    - `$destroy` call in `SliderTick` is removed because it is obsolted
      on Vue 3.
    - Automatic ESLint fix is applied to `SliderTick`.
  • Loading branch information
kikuomax committed Jul 7, 2023
1 parent 95c3d87 commit 8f3b92c
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 11 deletions.
41 changes: 31 additions & 10 deletions src/components/slider/Slider.vue
Original file line number Diff line number Diff line change
Expand Up @@ -151,10 +151,17 @@ export default {
default: false
}
},
emits: ['change', 'dragend', 'dragging', 'dragstart', 'update:modelValue'],
data() {
return {
value1: null,
value2: null,
// internal is used to update value1 and value2 with a single shot.
// internal is also used to stop unnecessary propagation of update.
internal: {
value1: null,
value2: null
},
dragging: false,
isRange: false,
_isSlider: true // Used by Thumb and Tick
Expand Down Expand Up @@ -217,11 +224,19 @@ export default {
modelValue(value) {
this.setValues(value)
},
value1() {
this.onInternalValueUpdate()
internal({ value1, value2 }) {
this.value1 = value1
this.value2 = value2
},
value2() {
this.onInternalValueUpdate()
value1(newValue) {
if (this.internal.value1 !== newValue) {
this.onInternalValueUpdate()
}
},
value2(newValue) {
if (this.internal.value2 !== newValue) {
this.onInternalValueUpdate()
}
},
min() {
this.setValues(this.modelValue)
Expand All @@ -243,14 +258,20 @@ export default {
const largeValue = typeof newValue[1] !== 'number' || isNaN(newValue[1])
? this.max
: bound(newValue[1], this.min, this.max)
this.value1 = this.isThumbReversed ? largeValue : smallValue
this.value2 = this.isThumbReversed ? smallValue : largeValue
// premature update will be triggered and end up with circular
// update, if value1 and value2 are updated one by one
this.internal = {
value1: this.isThumbReversed ? largeValue : smallValue,
value2: this.isThumbReversed ? smallValue : largeValue
}
} else {
this.isRange = false
this.value1 = isNaN(newValue)
? this.min
: bound(newValue, this.min, this.max)
this.value2 = null
this.internal = {
value1: isNaN(newValue)
? this.min
: bound(newValue, this.min, this.max),
value2: null
}
}
},
onInternalValueUpdate() {
Expand Down
1 change: 1 addition & 0 deletions src/components/slider/SliderThumb.vue
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ export default {
default: false
}
},
emits: ['dragend', 'dragstart', 'update:modelValue'],
data() {
return {
isFocused: false,
Expand Down
1 change: 0 additions & 1 deletion src/components/slider/SliderTick.vue
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ export default {
},
created() {
if (!this.$parent.$data._isSlider) {
this.$destroy()
throw new Error('You should wrap bSliderTick on a bSlider')
}
}
Expand Down

0 comments on commit 8f3b92c

Please sign in to comment.