-
Notifications
You must be signed in to change notification settings - Fork 61
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
Using passive event handlers #35
Comments
@ernest-okot listeners added via The problem is that drag calls Until we get a better API for what's there inside DON'T USE THIS TECHNIQUE. IT'S HERE FOR A REFERENCE. SEE COMMENTS BELOW const clone = selection.sort();
// Not sure if it's the best way to clone a selection. I found no other.
// Cloning to prevent exposing patched `on` to the rest of the codebase
clone.on = (() => {
const originalSelectionOn = selection.on.bind(selection);
return (name, fn, options = {passive: true}) =>
originalSelectionOn(name, fn, options);
})();
clone.call(
drag()
.on('start', () => {}, {passive: true})
.on('drag', () => {}, {passive: true})
.on('drag', () => {}, {passive: true})
); This way I got rid of all [Violation] warnings Chrome Canary showed me: both those that I could pass the 3rd argument (drag.on) and those what drag calls internally. |
Passive listeners are not appropriate for the drag behavior as it must cancel scrolling. |
yeah, I noticed that, especially on mobile, it broke some things. @mbostock do all handlers |
You can see the table of events in the README. My guess is that the Violation warnings occur when you have a non-passive event listener that does not call event.preventDefault. Actually, it does occur to me that it might be possible to make the mousedown and touchstart event listeners passive, since due to the convoluted way in which these events need to be handled, the default behavior on these events is not prevented even though mousemove and touchmove are subsequently prevented. |
So, as I understand, you will think about making some of the internally attached handlers passive, but it's not as easy as saying "this one can be passive and that one not". Then my patch snippet definitely does more harm than good. I'll leave a remark in the original comment in case someone finds it later. |
It seems that the Violation appears in the touchstart and touchmove event listeners in Chrome on macOS, even though those event listeners are never invoked (because it’s not a touch device). That seems unfortunate, since it implies that you get a warning even if your event listeners are necessarily non-passive. It might be possible to make the touchstart listener passive, but it’s not possible to make touchmove passive as we need to call event.preventDefault. Also, per #9, I’d rather always call event.preventDefault. It’s just I can’t because it breaks capturing of mouseup outside the window and it breaks click emulation on touch devices. |
Thanks for the briefing |
This might be fixable with Pointer Events (#25) but browser support is still around 60% (behind a flag Firefox, and missing from Safari and iOS). |
Have you already considered using pointer events when they're available with a fallback to mouse events? I'm just thinking out loud, I'm not aware of if it's worth maintaining that or how complex may it become |
This is already some of the most complex and issue-prone code in D3, so I am wary of adding another code path for a subset of browsers. Reading a little more about passive event listeners, it seems that you can have a passive event listener that cancels scroll by using the touch-action style property. So perhaps we can make them passive after all? |
Too bad it's again Safari that does not respect that |
Okay. I think I have a good fix for the warning: don’t register touch event listeners on devices that don’t support touch. |
This avoids a [Violation] warning in Chrome (#35).
This avoids a [Violation] warning in Chrome. Related #99 #103 d3/d3-drag#35.
What about devices like MS Surface which may support touch in general but user has other options? generally you will have let currentMeanOfInteractionIsTouch = false;
window.addEventListener('touchstart', () => {
currentMeanOfInteractionIsTouch = true;
// attach other touch event listeners
}, {passive: true})
window.addEventListener('mousedown', () => {
currentMeanOfInteractionIsTouch = false;
// remove touch event listeners but the one above, attach mouse
}, {passive: true}) I just can't tell up front if attaching events on demand won't be either too late or too expensive. How do you think? |
@jrencz On convertible devices like the Surface you could also try to detect whether it is in tablet mode, e.g. see the discussion at https://stackoverflow.com/questions/32493093, but neither the solutions from there nor your solution belong inside d3 IMHO since it's too use-case specific. On a side note, I came across this and related bug tracker entries while looking for solutions for issues I was having with pan and drag-drop with d3 in Microsoft Edge and IE11 even in demos from @mbostock. There I learned |
@Herst I do own Surface and I barely use tablet mode. Yet I tend to touch the screen a lot in desktop mode. It serves a purpose of lightweight task home laptop, not a tablet at all in my household. I'd bet I'm not alone in this mode of usage. So detecting the mode (btw: how can it be done in JS?) is not the best choice IMO |
Related to d3-selection#113, how can we use passive event handlers for drag events?
The text was updated successfully, but these errors were encountered: