fix(Slider,RangeSlider): fix onChangeCommitted not triggered when the… (

#3472)

* fix(Slider,RangeSlider): fix onChangeCommitted not triggered when the slider is clicked

* test: add tests for disabled and readOnly

* test: changed from assert syntax to expect

* test: changed from assert syntax to expect

src/RangeSlider/RangeSlider.tsx

@@ -44,6 +44,7 @@ const RangeSlider = React.forwardRef((props: RangeSliderProps, ref) => {
     progress = true,
     vertical,
     disabled,
+    readOnly,
     classPrefix = 'slider',
     min = 0,
     max: maxProp = 100,
@@ -195,6 +196,10 @@ const RangeSlider = React.forwardRef((props: RangeSliderProps, ref) => {
    * Callback function that is fired when the mousemove is triggered
    */
   const handleDragMove = useEventCallback((event: React.MouseEvent, dataset: HandleDataset) => {
+    if (disabled || readOnly) {
+      return;
+    }
+
     const nextValue = getNextValue(event, dataset);
 
     if (isRangeMatchingConstraint(nextValue)) {
@@ -208,14 +213,18 @@ const RangeSlider = React.forwardRef((props: RangeSliderProps, ref) => {
    */
   const handleChangeCommitted = useCallback(
     (event: React.MouseEvent, dataset?: DOMStringMap) => {
+      if (disabled || readOnly) {
+        return;
+      }
+
       const nextValue = getNextValue(event, dataset as HandleDataset);
 
       if (isRangeMatchingConstraint(nextValue)) {
         setValue(nextValue);
         onChangeCommitted?.(nextValue, event);
       }
     },
-    [getNextValue, onChangeCommitted, isRangeMatchingConstraint, setValue]
+    [disabled, readOnly, getNextValue, isRangeMatchingConstraint, setValue, onChangeCommitted]
   );
 
   const handleKeyDown = useCallback(
@@ -264,9 +273,9 @@ const RangeSlider = React.forwardRef((props: RangeSliderProps, ref) => {
     [max, min, onChange, rtl, isRangeMatchingConstraint, setValue, step, value]
   );
 
-  const handleClick = useCallback(
+  const handleBarClick = useCallback(
     (event: React.MouseEvent) => {
-      if (disabled) {
+      if (disabled || readOnly) {
         return;
       }
 
@@ -285,14 +294,17 @@ const RangeSlider = React.forwardRef((props: RangeSliderProps, ref) => {
       if (isRangeMatchingConstraint(nextValue)) {
         setValue(nextValue);
         onChange?.(nextValue, event);
+        onChangeCommitted?.(nextValue, event);
       }
     },
     [
       disabled,
+      readOnly,
       getValidValue,
       getValueByPosition,
       isRangeMatchingConstraint,
       onChange,
+      onChangeCommitted,
       setValue,
       value
     ]
@@ -340,7 +352,7 @@ const RangeSlider = React.forwardRef((props: RangeSliderProps, ref) => {
 
   return (
     <Component {...rest} ref={ref} className={classes}>
-      <div className={merge(barClassName, prefix('bar'))} ref={barRef} onClick={handleClick}>
+      <div className={merge(barClassName, prefix('bar'))} ref={barRef} onClick={handleBarClick}>
         {progress && (
           <ProgressBar
             rtl={rtl}

src/RangeSlider/test/RangeSliderSpec.tsx

@@ -13,50 +13,80 @@ describe('RangeSlider', () => {
 
   it('Should render a RangeSlider', () => {
     const instance = getDOMNode(<RangeSlider />);
-    assert.equal(instance.className, 'rs-slider');
+    expect(instance).to.have.class('rs-slider');
   });
 
   it('Should have a progress ', () => {
     const instance = getDOMNode(<RangeSlider defaultValue={[10, 50]} />);
-    assert.equal(
+
+    expect(
       // eslint-disable-next-line testing-library/no-node-access
-      (instance.querySelector('.rs-slider-progress-bar') as HTMLHtmlElement).style.width,
-      '40%'
-    );
-    assert.equal(
+      (instance.querySelector('.rs-slider-progress-bar') as HTMLHtmlElement).style.width
+    ).to.equal('40%');
+
+    expect(
       // eslint-disable-next-line testing-library/no-node-access
-      (instance.querySelector('.rs-slider-progress-bar') as HTMLElement).style.left,
-      '10%'
-    );
+      (instance.querySelector('.rs-slider-progress-bar') as HTMLElement).style.left
+    ).to.equal('10%');
   });
 
   it('Should render 2 handles ', () => {
-    const instance = getDOMNode(<RangeSlider value={[10, 50]} />);
-    // eslint-disable-next-line testing-library/no-node-access
-    assert.equal(instance.querySelectorAll('.rs-slider-handle').length, 2);
+    render(<RangeSlider value={[10, 50]} />);
+    expect(screen.getAllByRole('slider')).to.have.length(2);
   });
 
   it('Should output the scale', () => {
     const instance = getDOMNode(<RangeSlider step={10} max={100} graduated />);
     const instance2 = getDOMNode(<RangeSlider min={10} step={10} max={100} graduated />);
     // eslint-disable-next-line testing-library/no-node-access
-    assert.equal(instance.querySelectorAll('li').length, 10);
+    expect(instance.querySelectorAll('li')).to.have.length(10);
     // eslint-disable-next-line testing-library/no-node-access
-    assert.equal(instance2.querySelectorAll('li').length, 9);
+    expect(instance2.querySelectorAll('li')).to.have.length(9);
   });
 
   it('Should be displayed vertically', () => {
     const instance = getDOMNode(<RangeSlider vertical />);
-    assert.include(instance.className, 'rs-slider-vertical');
+
+    expect(instance).to.have.class('rs-slider-vertical');
 
     screen.getAllByRole('slider').forEach(slider => {
       expect(slider).to.have.attr('aria-orientation', 'vertical');
     });
   });
 
   it('Should be disabled', () => {
-    const instance = getDOMNode(<RangeSlider disabled />);
-    assert.include(instance.className, 'rs-slider-disabled');
+    const onChange = sinon.spy();
+    const onChangeCommitted = sinon.spy();
+
+    const instance = getDOMNode(
+      <RangeSlider disabled onChange={onChange} onChangeCommitted={onChangeCommitted} />
+    );
+    expect(instance).to.have.class('rs-slider-disabled');
+    expect(screen.queryAllByRole('slider')[0]).to.have.attr('aria-disabled', 'true');
+    expect(screen.queryAllByRole('slider')[1]).to.have.attr('aria-disabled', 'true');
+
+    // eslint-disable-next-line testing-library/no-node-access
+    fireEvent.click(instance.querySelector('.rs-slider-bar') as HTMLElement);
+
+    expect(onChange).to.have.not.been.called;
+    expect(onChangeCommitted).to.have.not.been.called;
+  });
+
+  it('Should be readOnly', () => {
+    const onChange = sinon.spy();
+    const onChangeCommitted = sinon.spy();
+
+    const instance = getDOMNode(
+      <RangeSlider readOnly onChange={onChange} onChangeCommitted={onChangeCommitted} />
+    );
+    expect(screen.queryAllByRole('slider')[0]).to.have.attr('readonly');
+    expect(screen.queryAllByRole('slider')[1]).to.have.attr('readonly');
+
+    // eslint-disable-next-line testing-library/no-node-access
+    fireEvent.click(instance.querySelector('.rs-slider-bar') as HTMLElement);
+
+    expect(onChange).to.have.not.been.called;
+    expect(onChangeCommitted).to.have.not.been.called;
   });
 
   it('Should call onChange callback', () => {
@@ -65,8 +95,7 @@ describe('RangeSlider', () => {
     // eslint-disable-next-line testing-library/no-node-access
     fireEvent.click(instance.querySelector('.rs-slider-progress-bar') as HTMLElement);
 
-    assert.equal(onChangeSpy.firstCall.firstArg[0], 0);
-    assert.equal(onChangeSpy.firstCall.firstArg[1], 50);
+    expect(onChangeSpy).to.have.been.calledWith([0, 50]);
   });
 
   it('Should respond to keyboard event', async () => {
@@ -107,7 +136,7 @@ describe('RangeSlider', () => {
   it('Should render custom title', () => {
     const instance = getDOMNode(<RangeSlider tooltip={false} handleTitle={'test'} />);
     // eslint-disable-next-line testing-library/no-node-access
-    assert.equal((instance.querySelector('.rs-slider-handle') as HTMLElement).textContent, 'test');
+    expect(instance.querySelector('.rs-slider-handle')).to.have.text('test');
   });
 
   it('Should handle keyboard operations', () => {
@@ -116,25 +145,32 @@ describe('RangeSlider', () => {
     const handle = instance.querySelector('.rs-slider-handle') as HTMLElement;
     const input = screen.getAllByRole('slider')[0] as HTMLInputElement;
 
-    assert.equal(input.value, '10');
+    expect(input).to.value('10');
+    expect(input).to.have.attr('aria-valuenow', '10');
 
     fireEvent.keyDown(handle, { key: 'ArrowUp' });
-    assert.equal(input.value, '11');
+    expect(input).to.value('11');
+    expect(input).to.have.attr('aria-valuenow', '11');
 
     fireEvent.keyDown(handle, { key: 'ArrowRight' });
-    assert.equal(input.value, '12');
+    expect(input).to.value('12');
+    expect(input).to.have.attr('aria-valuenow', '12');
 
     fireEvent.keyDown(handle, { key: 'ArrowDown' });
-    assert.equal(input.value, '11');
+    expect(input).to.value('11');
+    expect(input).to.have.attr('aria-valuenow', '11');
 
     fireEvent.keyDown(handle, { key: 'ArrowLeft' });
-    assert.equal(input.value, '10');
+    expect(input).to.value('10');
+    expect(input).to.have.attr('aria-valuenow', '10');
 
     fireEvent.keyDown(handle, { key: 'Home' });
-    assert.equal(input.value, '0');
+    expect(input).to.value('0');
+    expect(input).to.have.attr('aria-valuenow', '0');
 
     fireEvent.keyDown(handle, { key: 'End' });
-    assert.equal(input.value, '100');
+    expect(input).to.value('100');
+    expect(input).to.have.attr('aria-valuenow', '100');
   });
 
   it('Should call `onChangeCommitted` callback', async () => {
@@ -148,29 +184,40 @@ describe('RangeSlider', () => {
     fireEvent.mouseDown(handle);
     handle.dispatchEvent(mousemoveEvent);
 
-    assert.include(handle.className, 'active');
+    expect(handle).to.have.class('active');
 
     handle.dispatchEvent(mouseupEvent);
     expect(onChangeCommitted).to.have.been.calledOnce;
   });
 
+  it('Should call `onChangeCommitted` callback when click bar', () => {
+    const onChangeCommitted = sinon.spy();
+    const instance = getDOMNode(
+      <RangeSlider defaultValue={[10, 50]} onChangeCommitted={onChangeCommitted} />
+    );
+    // eslint-disable-next-line testing-library/no-node-access
+    fireEvent.click(instance.querySelector('.rs-slider-bar') as HTMLElement);
+
+    expect(onChangeCommitted).to.have.been.calledWith([0, 50]);
+  });
+
   it('Should output an `input` stored value', () => {
     const instance = getDOMNode(<RangeSlider min={10} max={100} value={[20, 50]} />);
 
     // eslint-disable-next-line testing-library/no-node-access
     const input = instance.querySelectorAll('input[type="range"]') as NodeListOf<HTMLInputElement>;
 
-    assert.equal(input[0].value, '20');
-    assert.equal(input[0].getAttribute('aria-valuenow'), '20');
-    assert.equal(input[0].getAttribute('aria-valuemax'), '100');
-    assert.equal(input[0].getAttribute('aria-valuemin'), '10');
-    assert.equal(input[0].getAttribute('aria-orientation'), 'horizontal');
-
-    assert.equal(input[1].value, '50');
-    assert.equal(input[1].getAttribute('aria-valuenow'), '50');
-    assert.equal(input[1].getAttribute('aria-valuemax'), '100');
-    assert.equal(input[1].getAttribute('aria-valuemin'), '10');
-    assert.equal(input[1].getAttribute('aria-orientation'), 'horizontal');
+    expect(input[0]).to.value('20');
+    expect(input[0]).to.have.attr('aria-valuenow', '20');
+    expect(input[0]).to.have.attr('aria-valuemax', '100');
+    expect(input[0]).to.have.attr('aria-valuemin', '10');
+    expect(input[0]).to.have.attr('aria-orientation', 'horizontal');
+
+    expect(input[1]).to.value('50');
+    expect(input[1]).to.have.attr('aria-valuenow', '50');
+    expect(input[1]).to.have.attr('aria-valuemax', '100');
+    expect(input[1]).to.have.attr('aria-valuemin', '10');
+    expect(input[1]).to.have.attr('aria-orientation', 'horizontal');
   });
 
   it('Should be reversed start and end values', () => {
@@ -191,16 +238,19 @@ describe('RangeSlider', () => {
       Simulate.click(sliderBar, { pageX: 0, pageY: 80 });
     });
 
+    expect(onChangeSpy).to.have.been.calledWith([20, 50]);
+
     act(() => {
       Simulate.click(sliderBar, { pageX: 0, pageY: 0 });
     });
 
-    assert.deepEqual(onChangeSpy.firstCall.firstArg, [20, 50]);
-
     /**
      * fix: https://github.com/rsuite/rsuite/issues/2425
      * Error thrown before fix: expected [ 100, 20 ] to deeply equal [ 20, 100 ]
      */
-    assert.deepEqual(onChangeSpy.secondCall.firstArg, [20, 100]);
+
+    expect(onChangeSpy).to.have.been.calledWith([20, 100]);
+
+    expect(onChangeSpy).to.have.been.calledTwice;
   });
 });

src/Slider/Slider.tsx

@@ -142,7 +142,7 @@ const Slider = React.forwardRef((props: SliderProps, ref) => {
 
   const classes = merge(
     className,
-    withClassPrefix({ vertical, disabled, readOnly, graduated, 'with-mark': renderMark })
+    withClassPrefix({ vertical, disabled, graduated, 'with-mark': renderMark, readonly: readOnly })
   );
 
   const max = useMemo(
@@ -238,6 +238,14 @@ const Slider = React.forwardRef((props: SliderProps, ref) => {
     [disabled, getValidValue, getValueByPosition, onChangeCommitted, readOnly]
   );
 
+  const handleClickBar = useCallback(
+    (event: React.MouseEvent) => {
+      handleChangeValue(event);
+      handleChangeCommitted(event);
+    },
+    [handleChangeCommitted, handleChangeValue]
+  );
+
   const handleKeyDown = useCallback(
     (event: React.KeyboardEvent) => {
       let nextValue;
@@ -283,7 +291,7 @@ const Slider = React.forwardRef((props: SliderProps, ref) => {
 
   return (
     <Componnet {...rest} ref={ref} className={classes} role="presentation">
-      <div ref={barRef} className={merge(barClassName, prefix('bar'))} onClick={handleChangeValue}>
+      <div ref={barRef} className={merge(barClassName, prefix('bar'))} onClick={handleClickBar}>
         {progress && (
           <ProgressBar
             rtl={rtl}