Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Slider Scrolling #8321

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
- Measurement tools are now accessible when viewing datasets outside of an annotation. [#8334](https://github.com/scalableminds/webknossos/pull/8334)

### Changed
- Improved the scrolling behaviour of sliders: Sliders must be focused to scroll them. Additionally, parent element scrolling is prevented when using the slider. [#8321](https://github.com/scalableminds/webknossos/pull/8321) [#8321](https://github.com/scalableminds/webknossos/pull/8321)

### Fixed
- Fixed a bug that lead to trees being dropped when merging to trees together. [#8359](https://github.com/scalableminds/webknossos/pull/8359)
Expand Down
89 changes: 61 additions & 28 deletions frontend/javascripts/components/slider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ import { Slider as AntdSlider, type SliderSingleProps } from "antd";
import type { SliderRangeProps } from "antd/lib/slider";
import { clamp } from "libs/utils";
import _ from "lodash";
import type { WheelEventHandler } from "react";
import { useCallback, useEffect, useRef, useState } from "react";

const DEFAULT_WHEEL_FACTOR = 0.02;
const DEFAULT_STEP = 1;

type SliderProps = (SliderSingleProps | SliderRangeProps) & {
wheelFactor?: number;
onWheelDisabled?: boolean;
onResetToDefault?: () => void;
};

const getDiffPerSliderStep = (
Expand All @@ -23,6 +24,8 @@ const getDiffPerSliderStep = (
};

const getWheelStepFromEvent = (step: number, deltaY: number, wheelStep: number) => {
const absDeltaY = Math.abs(deltaY);
if (absDeltaY === 0 || step === 0) return 0;
// Make sure that result is a multiple of step
return step * Math.round((wheelStep * deltaY) / Math.abs(deltaY) / step);
};
Expand All @@ -32,6 +35,7 @@ export function Slider(props: SliderProps) {
min,
max,
onChange,
onResetToDefault,
value,
range,
defaultValue,
Expand All @@ -40,49 +44,78 @@ export function Slider(props: SliderProps) {
step,
disabled,
} = props;
const isFocused = useRef(false);
const sliderRef = useRef<HTMLDivElement>(null);

Comment on lines +47 to +49
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use useState for focus management to prevent race conditions.

Using useRef for focus state could lead to race conditions in concurrent mode. Consider using useState instead.

-  const isFocused = useRef(false);
+  const [isFocused, setIsFocused] = useState(false);
   const sliderRef = useRef<HTMLDivElement>(null);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const isFocused = useRef(false);
const sliderRef = useRef<HTMLDivElement>(null);
const [isFocused, setIsFocused] = useState(false);
const sliderRef = useRef<HTMLDivElement>(null);

const handleWheelEvent = useCallback(
(event: { preventDefault: () => void; deltaY: number }) => {
if (
onWheelDisabled ||
value == null ||
min == null ||
max == null ||
!isFocused.current ||
onChange == null
)
return;
event.preventDefault();
const diff = getWheelStepFromEvent(ensuredStep, event.deltaY, wheelStep);
// differentiate between single value and range slider
if (range === false || range == null) {
const newValue = value - diff;
const clampedNewValue = clamp(min, newValue, max);
onChange(clampedNewValue);
} else if (range === true || typeof range === "object") {
const newLowerValue = Math.round(value[0] + diff);
const newUpperValue = Math.round(value[1] - diff);
const clampedNewLowerValue = clamp(min, newLowerValue, Math.min(newUpperValue, max));
const clampedNewUpperValue = clamp(newLowerValue, newUpperValue, max);
onChange([clampedNewLowerValue, clampedNewUpperValue]);
}
},
[value, min, max, onChange, range, onWheelDisabled],
);

// Reacts onWheel is passive by default, this means that it can't preventDefault.
// Thus we need to add the event listener manually.
// (See https://github.com/facebook/react/pull/19654)
useEffect(() => {
const sliderElement = sliderRef.current;
if (sliderElement) {
sliderElement.addEventListener("wheel", handleWheelEvent, { passive: false });
return () => {
sliderElement.removeEventListener("wheel", handleWheelEvent);
};
}
}, [handleWheelEvent]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was a little surprised about this solution.
In my view the dependency that makes sense would be sliderRef.current. I read that components are not rerendered if refs change, which makes sense because the concepts behind refs is to reference objects that are not needed for rendering (according to react.dev. I thought that hooks would still be updated, but this must be the wrong assumption.


if (min == null || max == null || onChange == null || value == null || disabled)
return <AntdSlider {...props} />;
const sliderRange = max - min;
const ensuredStep = step || DEFAULT_STEP;
let handleWheelEvent: WheelEventHandler<HTMLDivElement> = _.noop;
let handleDoubleClick: React.MouseEventHandler<HTMLDivElement> = _.noop;

const wheelStep = getDiffPerSliderStep(sliderRange, wheelFactor, ensuredStep);

handleDoubleClick = (event) => {
const handleDoubleClick: React.MouseEventHandler<HTMLDivElement> = (event) => {
if (
event.target instanceof HTMLElement &&
event.target.className.includes("ant-slider-handle") &&
defaultValue != null
)
if (onResetToDefault != null) onResetToDefault();
// @ts-ignore Argument of type 'number | number[]' is not assignable to parameter of type 'number'.
//TypeScript doesn't understand that onChange always takes the type of defaultValue.
onChange(defaultValue);
else onChange(defaultValue);
};
Comment on lines +99 to 109
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix else branch placement.

The else branch is incorrectly placed outside the if block, which could lead to unexpected behavior.

   const handleDoubleClick: React.MouseEventHandler<HTMLDivElement> = (event) => {
     if (
       event.target instanceof HTMLElement &&
       event.target.className.includes("ant-slider-handle") &&
       defaultValue != null
-    )
-      if (onResetToDefault != null) onResetToDefault();
+    ) {
+      if (onResetToDefault != null) {
+        onResetToDefault();
+      } else {
       // @ts-ignore Argument of type 'number | number[]' is not assignable to parameter of type 'number'.
       //TypeScript doesn't understand that onChange always takes the type of defaultValue.
-      else onChange(defaultValue);
+        onChange(defaultValue);
+      }
+    }
   };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const handleDoubleClick: React.MouseEventHandler<HTMLDivElement> = (event) => {
if (
event.target instanceof HTMLElement &&
event.target.className.includes("ant-slider-handle") &&
defaultValue != null
)
if (onResetToDefault != null) onResetToDefault();
// @ts-ignore Argument of type 'number | number[]' is not assignable to parameter of type 'number'.
//TypeScript doesn't understand that onChange always takes the type of defaultValue.
onChange(defaultValue);
else onChange(defaultValue);
};
const handleDoubleClick: React.MouseEventHandler<HTMLDivElement> = (event) => {
if (
event.target instanceof HTMLElement &&
event.target.className.includes("ant-slider-handle") &&
defaultValue != null
) {
if (onResetToDefault != null) {
onResetToDefault();
} else {
// @ts-ignore Argument of type 'number | number[]' is not assignable to parameter of type 'number'.
//TypeScript doesn't understand that onChange always takes the type of defaultValue.
onChange(defaultValue);
}
}
};


// differentiate between single value and range slider
if (range === false || range == null) {
if (!onWheelDisabled) {
handleWheelEvent = (event) => {
const newValue = value - getWheelStepFromEvent(ensuredStep, event.deltaY, wheelStep);
const clampedNewValue = clamp(min, newValue, max);
onChange(clampedNewValue);
};
}
} else if (range === true || typeof range === "object") {
if (!onWheelDisabled) {
handleWheelEvent = (event) => {
const diff = getWheelStepFromEvent(ensuredStep, event.deltaY, wheelStep);
const newLowerValue = Math.round(value[0] + diff);
const newUpperValue = Math.round(value[1] - diff);
const clampedNewLowerValue = clamp(min, newLowerValue, Math.min(newUpperValue, max));
const clampedNewUpperValue = clamp(newLowerValue, newUpperValue, max);
onChange([clampedNewLowerValue, clampedNewUpperValue]);
};
}
}

return (
<div onWheel={handleWheelEvent} onDoubleClick={handleDoubleClick} style={{ flexGrow: 1 }}>
<div
ref={sliderRef}
onDoubleClick={handleDoubleClick}
style={{ flexGrow: 1, touchAction: "none" }}
onFocus={() => (isFocused.current = true)}
onBlur={() => (isFocused.current = false)}
>
<AntdSlider {...props} />
</div>
);
Comment on lines 111 to 121
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update focus handlers to use setState.

Update the focus handlers to use setState for consistency with the earlier suggestion to use useState.

   return (
     <div
       ref={sliderRef}
       onDoubleClick={handleDoubleClick}
       style={{ flexGrow: 1, touchAction: "none" }}
-      onFocus={() => (isFocused.current = true)}
-      onBlur={() => (isFocused.current = false)}
+      onFocus={() => setIsFocused(true)}
+      onBlur={() => setIsFocused(false)}
     >
       <AntdSlider {...props} />
     </div>
   );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return (
<div onWheel={handleWheelEvent} onDoubleClick={handleDoubleClick} style={{ flexGrow: 1 }}>
<div
ref={sliderRef}
onDoubleClick={handleDoubleClick}
style={{ flexGrow: 1, touchAction: "none" }}
onFocus={() => (isFocused.current = true)}
onBlur={() => (isFocused.current = false)}
>
<AntdSlider {...props} />
</div>
);
return (
<div
ref={sliderRef}
onDoubleClick={handleDoubleClick}
style={{ flexGrow: 1, touchAction: "none" }}
onFocus={() => setIsFocused(true)}
onBlur={() => setIsFocused(false)}
>
<AntdSlider {...props} />
</div>
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,11 @@ export class LogSliderSetting extends React.PureComponent<LogSliderSettingProps>
return Math.round(scaleValue);
};

resetToDefaultValue = () => {
if (this.props.defaultValue == null) return;
this.onChangeInput(this.props.defaultValue);
};

render() {
const { label, roundTo, value, min, max, disabled, defaultValue } = this.props;
return (
Expand All @@ -210,6 +215,7 @@ export class LogSliderSetting extends React.PureComponent<LogSliderSettingProps>
value={this.getSliderValue()}
disabled={disabled}
defaultValue={defaultValue}
onResetToDefault={this.resetToDefaultValue}
/>
</Col>
<Col span={this.props.spans[2]}>
Expand Down