Justin-ZS commented on code in PR #21588:
URL: https://github.com/apache/echarts/pull/21588#discussion_r3145305581
##########
src/component/helper/RoamController.ts:
##########
@@ -362,43 +392,43 @@ class RoamController extends
Eventful<RoamEventDefinition> {
const shouldZoom = isAvailableBehavior('zoomOnMouseWheel', e,
this._opt);
const shouldMove = isAvailableBehavior('moveOnMouseWheel', e,
this._opt);
const wheelDelta = e.wheelDelta;
- const absWheelDeltaDelta = Math.abs(wheelDelta);
- const originX = e.offsetX;
- const originY = e.offsetY;
-
// wheelDelta maybe -0 in chrome mac.
if (wheelDelta === 0 || (!shouldZoom && !shouldMove)) {
return;
}
- // If both `shouldZoom` and `shouldMove` is true, trigger
- // their event both, and the final behavior is determined
- // by event listener themselves.
+ // Reach through to the native WheelEvent for per-axis deltas;
+ // zrender's `wheelDelta` collapses the two axes into a single scalar.
+ // Pre-2013 IE has no `deltaY`; the axis helpers fall back to the
+ // passed default in that case.
+ const nativeEvent = (e.event as unknown as WheelEvent) || null;
+ const originX = e.offsetX;
+ const originY = e.offsetY;
if (shouldZoom) {
- // Convenience:
- // Mac and VM Windows on Mac: scroll up: zoom out.
- // Windows: scroll up: zoom in.
-
- // FIXME: Should do more test in different environment.
- // wheelDelta is too complicated in difference nvironment
- //
(https://developer.mozilla.org/en-US/docs/Web/Events/mousewheel),
- // although it has been normallized by zrender.
- // wheelDelta of mouse wheel is bigger than touch pad.
- const factor = absWheelDeltaDelta > 3 ? 1.4 : absWheelDeltaDelta >
1 ? 1.2 : 1.1;
- const scale = wheelDelta > 0 ? factor : 1 / factor;
+ // `scaleY` defaults to `scale` on the IE fallback so the single
+ // scalar still drives zoom; `scaleX` defaults to identity so
+ // `zoomOnMouseWheelAxis: 'horizontal'` becomes a safe no-op.
+ const scale = ladderZoomScale(wheelDelta);
this._checkTriggerMoveZoom(this, 'zoom', 'zoomOnMouseWheel', e, {
- scale: scale, originX: originX, originY: originY,
isAvailableBehavior: null
+ scale: scale,
+ scaleX: axisZoomScale(nativeEvent, 'horizontal', 1),
Review Comment:
Thanks, the main direction in `80bda33` looks right to me. Hoisting the axis
check into `RoamController._mousewheelHandler` should fix the original
fall-through issue for the simple one-dataZoom case, and Panel E is exactly the
kind of manual check I had in mind.
I noticed one remaining edge case in `mergeControllerParams`: the merged
`zoomOnMouseWheelAxis` / `moveOnMouseWheelAxis` currently considers every
inside dataZoom, even ones where the corresponding behavior is disabled. That
can weaken the controller-level axis guard unnecessarily.
For example, if one inside dataZoom has `zoomOnMouseWheel: true,
zoomOnMouseWheelAxis: 'horizontal'`, and another has `zoomOnMouseWheel: false`
with no `zoomOnMouseWheelAxis`, the merged `zoomOnMouseWheelAxis` becomes
`undefined`. Then a plain vertical wheel can still reach the zoom path and get
consumed at the controller level, even though the only active zoom dataZoom
would be a no-op. The same shape applies to `moveOnMouseWheelAxis` with
`moveOnMouseWheel: false`.
Could we merge each axis option only across dataZooms where the matching
behavior is active? So `zoomOnMouseWheelAxis` only looks at dataZooms with
`zoomOnMouseWheel !== false`, and `moveOnMouseWheelAxis` only looks at
dataZooms with `moveOnMouseWheel !== false`. I think that would keep the fix
precise without changing the public API shape.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
