fix: fix controlled slider component can still trigger value change b… (#768)

* fix: fix controlled slider component can still trigger value change by click the track

* fix: fix cypress electron left 0.008px diff problem

* fix: avoid prop value re-assign

cypress/integration/slider.spec.js

@@ -0,0 +1,144 @@
+describe('slider', () => {
+    let cyWin;
+
+    before(() => {
+        cy.visit('http://127.0.0.1:6006/iframe.html?id=slider--controlled-slider-demo&args=&viewMode=story', {
+            onBeforeLoad(win) {
+                cyWin = win;
+            },
+        });
+    });
+
+    beforeEach(() => {
+        cy.stub(cyWin.console, 'log').as('consoleLog');
+    });
+
+    // afterEach(() => {
+    //     cy.reload();
+    // });
+
+    it('controlled slider', () => {
+        const parentSelector = '[data-cy=horizontalNoChangeSlider]';
+        const sliderTrackSelector = `${parentSelector} .semi-slider-rail`;
+        const sliderHandleSelector = `${parentSelector} .semi-slider-handle`;
+        
+        // test track click
+        let handleInitialPos;
+        cy.get(sliderHandleSelector).then(($handle) => {
+            handleInitialPos = $handle.position();
+        });
+
+        cy.get(sliderTrackSelector).trigger('click', 'right');
+        cy.get('@consoleLog').should('be.calledWith', 'value改变了100');
+        
+        cy.get(sliderHandleSelector).should(($button) => {
+            expect($button.position()).deep.equal(handleInitialPos);
+        });
+
+        // test knob slide
+        cy.get(sliderHandleSelector)
+            .trigger('mousedown')
+            .trigger('mousemove', { pageX: 600, pageY:0 })
+            .trigger('mouseup', { force: true });
+        
+        cy.get(sliderHandleSelector).should(($button) => {
+            expect($button.position()).deep.equal(handleInitialPos);
+        });
+    });
+
+    it('controlled slider with onChange', () => {
+        const parentSelector = `[data-cy=horizontalOnChangeSlider]`;
+        const sliderTrackSelector = `${parentSelector} .semi-slider-rail`;
+        const sliderHandleSelector = `${parentSelector} .semi-slider-handle`;
+
+        // test track click
+        cy.get(sliderTrackSelector).trigger('click', 'right');
+        cy.get('@consoleLog').should('be.calledWith', 'value改变了100');
+        cy.get(sliderHandleSelector).should('have.css', 'left', '774px');
+
+        // test knob slide (pageX 300 = 32%)
+        cy.get(sliderHandleSelector)
+            .trigger('mousedown')
+            .trigger('mousemove', { pageX: 300, pageY:0 })
+            .trigger('mouseup', { force: true });
+        
+        // left 32% = 247.68px;
+        // cy.get(sliderHandleSelector).should('have.css', 'left', '247.68px');
+        cy.window().then(window => {
+            const style = window.getComputedStyle(window.document.querySelector(sliderHandleSelector));
+            expect(Math.ceil(parseFloat(style.left))).eq(248);
+        });
+    });
+
+    it('controlled range slider', () => {
+        const parentSelector = `[data-cy=horizontalNoChangeRangeSlider]`;
+        const sliderTrackSelector = `${parentSelector} .semi-slider-rail`;
+        const sliderHandleSelector = `${parentSelector} .semi-slider-handle`;
+
+        let leftKnobPos, rightKnobPos;
+        cy.get(sliderHandleSelector).then((handles) => {
+            leftKnobPos = handles.eq(0).position();
+            rightKnobPos = handles.eq(1).position();
+        });
+
+        // test track right click
+        cy.get(sliderTrackSelector).trigger('click', 'right');
+        cy.get('@consoleLog').should('be.calledWith', 'value改变了10,100');
+
+        // test track left click
+        cy.get(sliderTrackSelector).trigger('click', 'left');
+        cy.get('@consoleLog').should('be.calledWith', 'value改变了0,30');
+
+        // check final knob position
+        cy.get(sliderHandleSelector).should((handles) => {
+            expect(handles.eq(0).position()).deep.equal(leftKnobPos);
+            expect(handles.eq(1).position()).deep.equal(rightKnobPos);
+        });
+
+        // test left knob slide
+        cy.get(sliderHandleSelector).eq(0)
+            .trigger('mousedown')
+            .trigger('mousemove', { pageX: 100, pageY:0 })
+            .trigger('mouseup', { force: true });
+
+        // test right knob slide
+        cy.get(sliderHandleSelector).eq(1)
+            .trigger('mousedown')
+            .trigger('mousemove', { pageX: 600, pageY:0 })
+            .trigger('mouseup', { force: true });
+
+        cy.get(sliderHandleSelector).should((handles) => {
+            expect(handles.eq(0).position()).deep.equal(leftKnobPos);
+            expect(handles.eq(1).position()).deep.equal(rightKnobPos);
+        });
+    });
+
+    it('controlled vertical slider', () => {
+        const parentSelector = '[data-cy=horizontalNoChangeVerticalSlider]';
+        const sliderTrackSelector = `${parentSelector} .semi-slider-rail`;
+        const sliderHandleSelector = `${parentSelector} .semi-slider-handle`;
+
+        let handleInitialPos;
+        cy.get(sliderHandleSelector).then(($handle) => {
+            handleInitialPos = $handle.position();
+        });
+
+        // test track click
+        cy.get(sliderTrackSelector).trigger('click', 'bottom');
+        cy.get('@consoleLog').should('be.calledWith', 'value改变了100');
+
+        cy.get(sliderHandleSelector).should(($button) => {
+            expect($button.position()).deep.equal(handleInitialPos);
+        });
+
+        // test knob slide
+        cy.get(sliderHandleSelector)
+            .trigger('mousedown')
+            .trigger('mousemove', { pageX: 0, pageY:600 })
+            .trigger('mouseup', { force: true });
+        
+        cy.get(sliderHandleSelector).should(($button) => {
+            expect($button.position()).deep.equal(handleInitialPos);
+        });
+    });
+});

packages/semi-foundation/slider/foundation.ts

@@ -83,7 +83,7 @@ export interface SliderAdapter extends DefaultAdapter<SliderProps, SliderState>{
     getMinHandleEl: () => { current: HTMLElement };
     getMaxHandleEl: () => { current: HTMLElement };
     onHandleDown: (e: any) => any;
-    onHandleMove: (mousePos: number, isMin: boolean, stateChangeCallback?: () => void, clickTrack?: boolean) => boolean | void;
+    onHandleMove: (mousePos: number, isMin: boolean, stateChangeCallback?: () => void, clickTrack?: boolean, outPutValue?: number | number[]) => boolean | void;
     setEventDefault: (e: any) => void;
     setStateVal: (state: keyof SliderState, value: any) => void;
     onHandleEnter: (position: SliderState['focusPos']) => void;
@@ -406,6 +406,14 @@ export default class SliderFoundation extends BaseFoundation<SliderAdapter> {
 
     checkAndUpdateIsInRenderTreeState = () => this._adapter.checkAndUpdateIsInRenderTreeState();
 
+    calculateOutputValue = (position: number, isMin: boolean): undefined | number | number[] => {
+        const moveValue = this.transPosToValue(position, isMin);
+        if (moveValue === false) {
+            return undefined;
+        }
+        return this.outPutValue(moveValue);
+    }
+
     /**
      *
      *
@@ -487,7 +495,16 @@ export default class SliderFoundation extends BaseFoundation<SliderAdapter> {
         let pagePos = vertical ? mousePos.y : mousePos.x;
         pagePos = pagePos - this._dragOffset;
         if ((chooseMovePos === 'min' && dragging[0]) || (chooseMovePos === 'max' && dragging[1])) {
-            this._adapter.onHandleMove(pagePos, chooseMovePos === 'min');
+            const outPutValue = this.calculateOutputValue(pagePos, chooseMovePos === 'min' );
+            
+            if (outPutValue === undefined) {
+                return false;
+            }
+            
+            this._adapter.notifyChange(outPutValue);
+
+            // allow drag for controlled component, so no _isControlledComponent check
+            this._adapter.onHandleMove(pagePos, chooseMovePos === 'min', undefined, false, outPutValue);
         }
         return true;
     };
@@ -565,7 +582,22 @@ export default class SliderFoundation extends BaseFoundation<SliderAdapter> {
         const mousePos = this.handleMousePos(e.pageX, e.pageY);
         const position = vertical ? mousePos.y : mousePos.x;
         const isMin = this.checkWhichHandle(position);
-        this.setHandlePos(position, isMin, true);
+
+        const outPutValue = this.calculateOutputValue(position, isMin);
+        if (outPutValue === undefined) {
+            return;
+        }
+
+        this._adapter.notifyChange(outPutValue);
+
+        // check if is controlled component
+        if (this._isControlledComponent()) {
+            // only perform callback ops, skip UI update
+            return;
+        }
+
+        // trigger UI state update
+        this.setHandlePos(position, isMin, true, outPutValue);
     };
 
     /**
@@ -573,8 +605,8 @@ export default class SliderFoundation extends BaseFoundation<SliderAdapter> {
      *
      * @memberof SliderFoundation
      */
-    setHandlePos = (position: number, isMin: boolean, clickTrack = false) => {
-        this._adapter.onHandleMove(position, isMin, () => this._adapter.onHandleUpAfter(), clickTrack);
+    setHandlePos = (position: number, isMin: boolean, clickTrack = false, outPutValue: number | number[]) => {
+        this._adapter.onHandleMove(position, isMin, () => this._adapter.onHandleUpAfter(), clickTrack, outPutValue);
     };
 
     /**

packages/semi-ui/slider/_story/slider.stories.js

@@ -254,14 +254,15 @@ SliderInScrollContainer.story = {
 
 class ControlledSlider extends Component {
   state = {
-    value: 50,
-    rangeValue: undefined,
+    value: 30,
+    rangeValue: [10, 30],
   };
   changeValue = () => {
     this.setState({ value: this.state.value + 10 });
+    const [start, end] = this.state.rangeValue;
+    this.setState({ rangeValue: [start - 10, end + 10]})
   };
   changeRangeValue = rangeValue => {
-    console.log('rangeValue' + rangeValue);
     this.setState({ rangeValue: rangeValue });
   };
   render() {
@@ -269,21 +270,42 @@ class ControlledSlider extends Component {
     return (
       <div>
         <Button onClick={() => this.changeValue()}>点击改变value</Button>
-        <div style={{ width: 800, marginLeft: 20, marginTop: 40 }}>
+        <div data-cy="horizontalNoChangeSlider" style={{ width: 800, marginLeft: 20, marginTop: 40 }}>
+          <Slider
+            value={30}
+            onChange={value => {
+              console.log('value改变了' + value);
+            }}
+          ></Slider>
+        </div>
+        <div data-cy="horizontalOnChangeSlider" style={{ width: 800, marginLeft: 20, marginTop: 40 }}>
           <Slider
             value={value}
             onChange={value => {
               console.log('value改变了' + value);
+              this.setState({value: value});
             }}
           ></Slider>
         </div>
-        <div style={{ width: 800, marginLeft: 20, marginTop: 40 }}>
+        <div data-cy="horizontalNoChangeRangeSlider" style={{ width: 800, marginLeft: 20, marginTop: 40 }}>
           <Slider
             value={rangeValue}
-            onChange={rangeValue => this.changeRangeValue(rangeValue)}
+            onChange={rangeValue => {
+              console.log('value改变了' + rangeValue);
+            }}
             range
           ></Slider>
         </div>
+        <div data-cy="horizontalNoChangeVerticalSlider" style={{ width: 800, marginLeft: 20, marginTop: 40 }}>
+          <Slider
+            value={40}
+            vertical
+            onChange={value => {
+              console.log('value改变了' + value);
+            }}
+            style={{height: '300px'}}
+          ></Slider>
+        </div>
       </div>
     );
   }

packages/semi-ui/slider/index.tsx

@@ -195,29 +195,33 @@ export default class Slider extends BaseComponent<SliderProps, SliderState> {
                 this._addEventListener(document.body, 'mouseup', this.foundation.onHandleUp, false);
                 this._addEventListener(document.body, 'touchmove', this.foundation.onHandleTouchMove, false);
             },
-            onHandleMove: (mousePos: number, isMin: boolean, stateChangeCallback = noop, clickTrack = false): boolean | void => {
+            onHandleMove: (mousePos: number, isMin: boolean, stateChangeCallback = noop, clickTrack = false, outPutValue): boolean | void => {
 
                 const sliderDOMIsInRenderTree = this.foundation.checkAndUpdateIsInRenderTreeState();
                 if (!sliderDOMIsInRenderTree) {
                     return;
                 }
 
-                const { value, onChange } = this.props;
-                const moveValue = this.foundation.transPosToValue(mousePos, isMin);
-                if (moveValue === false) {
-                    return;
+                const { value } = this.props;
+                
+
+                let finalOutPutValue = outPutValue;
+                if (finalOutPutValue === undefined) {
+                    const moveValue = this.foundation.transPosToValue(mousePos, isMin);
+                    if (moveValue === false) {
+                        return;
+                    }
+                    finalOutPutValue = this.foundation.outPutValue(moveValue);
                 }
 
-                const outPutValue = this.foundation.outPutValue(moveValue);
                 const { currentValue } = this.state;
-                if (!isEqual(this.foundation.outPutValue(currentValue), outPutValue)) {
-                    onChange(outPutValue);
+                if (!isEqual(this.foundation.outPutValue(currentValue), finalOutPutValue)) {
                     if (!clickTrack && this.foundation.valueFormatIsCorrect(value)) {
                         // still require afterChangeCallback when click on the track directly, need skip here
                         return false;
                     }
                     this.setState({
-                        currentValue: outPutValue,
+                        currentValue: finalOutPutValue,
                     }, stateChangeCallback);
                 }
             },
@@ -300,14 +304,14 @@ export default class Slider extends BaseComponent<SliderProps, SliderState> {
         const maxClass = cls(cssClasses.HANDLE, {
             [`${cssClasses.HANDLE}-clicked`]: chooseMovePos === 'max' && isDrag,
         });
-        const {min, max, currentValue} = this.state;
+        const { min, max, currentValue } = this.state;
 
         const commonAria = {
             'aria-label': ariaLabel,
             'aria-labelledby': ariaLabelledby,
             'aria-disabled': disabled
         };
-        vertical && Object.assign(commonAria, {'aria-orientation': 'vertical'});
+        vertical && Object.assign(commonAria, { 'aria-orientation': 'vertical' });
 
         const handleContents = !range ? (
             <Tooltip
@@ -494,6 +498,7 @@ export default class Slider extends BaseComponent<SliderProps, SliderState> {
                         });
                         const markPercent = (Number(mark) - min) / (max - min);
                         return activeResult ? (
+                            // eslint-disable-next-line jsx-a11y/no-static-element-interactions
                             <span
                                 key={mark}
                                 onClick={e => this.foundation.handleWrapClick(e)}