-
Notifications
You must be signed in to change notification settings - Fork 294
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
Axe d3.event? #191
Comments
Sounds like a great proposal! It always felt a bit odd using that temporarily set global. |
also related to #170 |
This change would affect a few methods that currently rely (implicitly) on the d3.event global, namely:
For example, it’s fairly common to have code like this: d3.select(context.canvas).on("mousemove", function() {
const [x, y] = d3.mouse(this);
context.fillRect(x, y, 2, 2);
}); One option would be to require the use of d3.clientPoint: d3.select(context.canvas).on("mousemove", function(event) {
const [x, y] = d3.clientPoint(this, event);
context.fillRect(x, y, 2, 2);
}); Or avoiding this: d3.select(context.canvas).on("mousemove", event => {
const [x, y] = d3.clientPoint(context.canvas, event);
context.fillRect(x, y, 2, 2);
}); Or with event.target (which works well with Canvas but not SVG): d3.select(context.canvas).on("mousemove", event => {
const [x, y] = d3.clientPoint(event.target, event);
context.fillRect(x, y, 2, 2);
}); Or avoiding d3.clientPoint entirely, though note that event.offsetX and event.offsetY are different from d3.clientPoint in the context of SVG when there’s a transform, since the latter uses getScreenCTM (which is sometimes buggy #67 #72 #81) to compute the position in the local (transformed) coordinate space: d3.select(context.canvas).on("mousemove", event => {
const x = event.offsetX;
const y = event.offsetY;
context.fillRect(x, y, 2, 2);
}); Another wrinkle is that d3.mouse automatically extracted the first changed touch from touch events (and automatically crawled up the event.sourceEvent chain for custom events, such as generated by d3-drag, d3-zoom, and d3-brush). So to use d3.clientPoint and support touch events, you’d need to say something like this: d3.select(context.canvas).on("mousemove touchmove", function(event) {
const point = d3.event.changedTouches ? d3.event.changedTouches[0] : d3.event;
const [x, y] = d3.clientPoint(this, point);
context.fillRect(x, y, 2, 2);
}); Annoyingly though, event.offsetX and event.offsetY also aren’t available on touches (see w3c/touch-events). The ancient event.layerX and event.layerY are available on the touch event, giving this code: d3.select(context.canvas).on("mousemove touchmove", event => {
const x = "offsetX" in event ? event.offsetX : event.layerX;
const y = "offsetY" in event ? event.offsetY : event.layerY;
context.fillRect(x, y, 2, 2);
}); But of course sometimes you want to know the position of the individual touches, and so event.layerX and event.layerY won’t work to find the coordinates of a touch with a given identifier. Also, no one supports the GeometryUtils interface yet. Safari does support the nonstandard webkitConvertPointFromPageToNode. These problems are also fixed with the newer Pointer Events specification, but of course that’s not supported on iOS Safari! All in all, quite a mess for what should be a truly basic feature of the web. |
I think we can just use event.currentTarget for this, and it’ll work great for both SVG and canvas since event.currentTarget always refers to the element on which the listener was registered. For example: d3.select(context.canvas).on("mousemove", event => {
context.fillRect(...d3.clientPoint(event), 2, 2);
}); |
…handle d3-selection axing event (d3/d3-selection#191) and typescript syntax issues in SearchBar.tsx component
Related #189, maybe it’s time to consider a new major version that removes d3.event. Instead of temporarily setting a global, how about event listeners simply receive the current event as an argument, as is the standard behavior outside of D3? To further access the bound data (and index/group), the data could be passed as the second argument. And we could drop the additional index and group arguments, and require you to use selection.each if you want to capture that state.
TODO
The text was updated successfully, but these errors were encountered: