Disable Shift key behavior? #20

Open
iansinnott opened this Issue Oct 14, 2016 · 21 comments

Comments

10 participants
@iansinnott

Is there a best-practices way to disable the shift key behavior?

In my app the brush is enabled only when the user holds shift. I.e. they hold shift and then drag to select nodes in a force layout. This means that event.shiftKey is always true during the brush drag. I understand that this is a feature of the brush, but if possible I would like to disable it for this use case.

I scanned through the code and saw that if I change this line to the following everything works fine:

shifting = false, // Disable `shifting` entirely

But I wasn't sure how I might hook into this functionality without modifying the source. Any suggestions would be much appreciated. Thanks!

@mbostock

This comment has been minimized.

Show comment
Hide comment
@mbostock

mbostock Oct 14, 2016

Member

Hmm. You can register a brush start event listener that removes the brush’s keydown and keyup event listeners, thereby preventing it from receiving these events:

brush.on("start.nokey", function() {
  d3.select(window).on("keydown.brush keyup.brush", null);
});

But, it’ll still see event.shiftKey, event.metaKey and event.altKey if they are pressed during mousedown or touchstart, so that won’t really disable the behavior completely.

I think the simplest thing would probably be a new feature that lets you disable the event.shiftKey and other key-driven behaviors. Possibly it should let you remap them to different modifier keys, or null to remove the functionality entirely.

Member

mbostock commented Oct 14, 2016

Hmm. You can register a brush start event listener that removes the brush’s keydown and keyup event listeners, thereby preventing it from receiving these events:

brush.on("start.nokey", function() {
  d3.select(window).on("keydown.brush keyup.brush", null);
});

But, it’ll still see event.shiftKey, event.metaKey and event.altKey if they are pressed during mousedown or touchstart, so that won’t really disable the behavior completely.

I think the simplest thing would probably be a new feature that lets you disable the event.shiftKey and other key-driven behaviors. Possibly it should let you remap them to different modifier keys, or null to remove the functionality entirely.

@iansinnott

This comment has been minimized.

Show comment
Hide comment
@iansinnott

iansinnott Oct 14, 2016

Thanks for the quick reply. As you said the mousedown and touchstart events are still coming through and the event.shiftKey is true when that event fires.

A remapping solution would be great 👍 since these are still useful features to have, it's just a bit inflexible currently.

I'll likely fork the lib and remove this functionality in the meantime but the customizable solution would be great.

Thanks for the quick reply. As you said the mousedown and touchstart events are still coming through and the event.shiftKey is true when that event fires.

A remapping solution would be great 👍 since these are still useful features to have, it's just a bit inflexible currently.

I'll likely fork the lib and remove this functionality in the meantime but the customizable solution would be great.

@iansinnott

This comment has been minimized.

Show comment
Hide comment
@iansinnott

iansinnott Nov 9, 2016

If anyone stumbles upon this issue and is interested in my solution I did end up forking the lib and removing shifting functionality. As the project developed I also ended up tweaking a few other things about the brush. Namely, I found myself wanting to hook into events which were being blocked by the brush so I ended up removing nopropagation and noevent in some places.

More customizability in the way the brush binds to keyboard/mouse events would be really cool, but forking the lib is a viable solution for anyone else who runs into similar issues.

If anyone stumbles upon this issue and is interested in my solution I did end up forking the lib and removing shifting functionality. As the project developed I also ended up tweaking a few other things about the brush. Namely, I found myself wanting to hook into events which were being blocked by the brush so I ended up removing nopropagation and noevent in some places.

More customizability in the way the brush binds to keyboard/mouse events would be really cool, but forking the lib is a viable solution for anyone else who runs into similar issues.

@geekplux

This comment has been minimized.

Show comment
Hide comment
@geekplux

geekplux Dec 2, 2016

these are still useful features to have, it's just a bit inflexible currently

totally agree...maybe it could be improved to a optional feature?

geekplux commented Dec 2, 2016

these are still useful features to have, it's just a bit inflexible currently

totally agree...maybe it could be improved to a optional feature?

@yandongCoder

This comment has been minimized.

Show comment
Hide comment
@yandongCoder

yandongCoder Dec 28, 2016

It's not working to me, also remove keydown.brush and keyup.brush event, but shifting's init value has been true, and always true when brush.

It's not working to me, also remove keydown.brush and keyup.brush event, but shifting's init value has been true, and always true when brush.

@GordonSmith

This comment has been minimized.

Show comment
Hide comment
@GordonSmith

GordonSmith Mar 28, 2017

I just got hit by this as well - FWIW I am trying to develop a view which has zoom, pan and brush - but I only wanted the brush enabled when the "shift" key was pressed.

I just got hit by this as well - FWIW I am trying to develop a view which has zoom, pan and brush - but I only wanted the brush enabled when the "shift" key was pressed.

@Krzysztof-FF

This comment has been minimized.

Show comment
Hide comment
@Krzysztof-FF

Krzysztof-FF Apr 3, 2017

Grabbing Shift or Alt key behavior with no possibility to reconfigure it is unsuitable.
In my application, I'm using Brush behavior to select several geometrical entities, I'm using Ctrl key to activate Brush behavior over standard Zoom-Pan behavior. Successive Brush selects new set of entities. With e.g. Shift or Alt pressed, I would like to activate other logical operations between successive selections, e.g. by adding or subtracting them.
Built-in behavior of Shift and Alt are in my application of minor significance and I would like to have them removable / reconfigurable.

Grabbing Shift or Alt key behavior with no possibility to reconfigure it is unsuitable.
In my application, I'm using Brush behavior to select several geometrical entities, I'm using Ctrl key to activate Brush behavior over standard Zoom-Pan behavior. Successive Brush selects new set of entities. With e.g. Shift or Alt pressed, I would like to activate other logical operations between successive selections, e.g. by adding or subtracting them.
Built-in behavior of Shift and Alt are in my application of minor significance and I would like to have them removable / reconfigurable.

@pkerpedjiev

This comment has been minimized.

Show comment
Hide comment
@pkerpedjiev

pkerpedjiev Apr 24, 2017

@iansinnott would you mind sharing your fork? The brush behavior is fantastic, but the fact that it takes over the shift and alt keys really makes it difficult to integrate with other functionality like @GordonSmith's example of zooming and brushing.

Trying to tie it to another keydown event is tricky because it looks like other keys repeatedly fire the 'keydown' event while held down. The shift and alt keydown events only fire once when the key is held down continuously.

@iansinnott would you mind sharing your fork? The brush behavior is fantastic, but the fact that it takes over the shift and alt keys really makes it difficult to integrate with other functionality like @GordonSmith's example of zooming and brushing.

Trying to tie it to another keydown event is tricky because it looks like other keys repeatedly fire the 'keydown' event while held down. The shift and alt keydown events only fire once when the key is held down continuously.

@iansinnott

This comment has been minimized.

Show comment
Hide comment
@iansinnott

iansinnott Apr 26, 2017

@pkerpedjiev I would be happy to but it's currently an internal fork so it's not publicly accessible.

I am indeed using it for zoom/pan/shift+brush, so what is suggested in this thread is quite possible and didn't take too much work. It was mostly a matter of code removal. I believe I removed everything relating to shifting (source) as well as noevent and nopropagation. The shifting behavior is simply undesirable if you want to use shift to activate the brush, and the event related stuff allows DOM events to bubble up and be caught by your app code instead of the brush internal handlers.

@pkerpedjiev I would be happy to but it's currently an internal fork so it's not publicly accessible.

I am indeed using it for zoom/pan/shift+brush, so what is suggested in this thread is quite possible and didn't take too much work. It was mostly a matter of code removal. I believe I removed everything relating to shifting (source) as well as noevent and nopropagation. The shifting behavior is simply undesirable if you want to use shift to activate the brush, and the event related stuff allows DOM events to bubble up and be caught by your app code instead of the brush internal handlers.

@mbostock

This comment has been minimized.

Show comment
Hide comment
@mbostock

mbostock Apr 27, 2017

Member

It seems like the main thing blocking this issue is not how to implement it but what the desired API is, both in terms of functionality and name.

As context, there are four modes of manipulating the brush. These modes are not exposed in the public API (currently), but here are the internal names that are used:

  • MODE_DRAG - Translates the brush selection, maintaining its current size. This typically happens when you click and drag in the main area of an existing brush selection.

  • MODE_SPACE - Like MODE_DRAG, translates the brush selection, maintaining its current size. The only difference is that this is enabled temporarily by holding down the SPACE key when in another mode, such as MODE_HANDLE.

  • MODE_HANDLE - Resizes one or two edges of the brush selection, maintaining the position of the other edges. This typically happens when you click and drag on an edge (or a corner) of an existing brush selection, or when you click outside the existing brush selection and drag to create a new brush selection.

  • MODE_CENTER - Like MODE_HANDLE, except it resizes two or four edges of the brush selection, maintaining the current center of the brush selection. This mode is temporarily enabled from MODE_HANDLE by holding down the ALT key. Also note that the SPACE key takes priority, so holding down ALT and SPACE will use MODE_SPACE.

Separate from the modes, there is also a flag:

  • shifting - Allow the brush selection to change in only one dimension. The dimension is determined by the first pointer movement after SHIFT is depressed. For example, if you hold down SHIFT and move the pointer to the right, the y-position of the brush selection becomes locked at its current values until the SHIFT key is released.

Some possible options:

  1. The simplest API would be to disable all key modifiers. This would allow you to use MODE_DRAG and MODE_HANDLE, depending on where the initial click landed, but would not allow you to change modes during a brush gesture, and would effectively disable MODE_SPACE, MODE_CENTER, and the shifting flag.

  2. An alternative API might allow disabling of specific modes. If you disabled MODE_SPACE, for instance, it would not listen for the SPACE key, but it might still listen for the ALT key to enable MODE_CENTER. Likewise you could disable MODE_CENTER to avoid listening for ALT. On the other hand since the shifting flag is orthogonal to the modes, I don’t see how this could disable listening for SHIFT, which is the original motivation for this issue.

  3. Another alternative would be to continue to observe key modifiers, but disable key listeners. This would allow you to use a key modifier at the start of a gesture; for example, holding down SHIFT on brush start could still lock the selection position on one dimension, but there would be no keydown and keyup listener, so if the SHIFT key were released while brushing, it would not affect the current brush gesture. Thus the brush would not interfere with external event listeners that want to do something with these key events.

Member

mbostock commented Apr 27, 2017

It seems like the main thing blocking this issue is not how to implement it but what the desired API is, both in terms of functionality and name.

As context, there are four modes of manipulating the brush. These modes are not exposed in the public API (currently), but here are the internal names that are used:

  • MODE_DRAG - Translates the brush selection, maintaining its current size. This typically happens when you click and drag in the main area of an existing brush selection.

  • MODE_SPACE - Like MODE_DRAG, translates the brush selection, maintaining its current size. The only difference is that this is enabled temporarily by holding down the SPACE key when in another mode, such as MODE_HANDLE.

  • MODE_HANDLE - Resizes one or two edges of the brush selection, maintaining the position of the other edges. This typically happens when you click and drag on an edge (or a corner) of an existing brush selection, or when you click outside the existing brush selection and drag to create a new brush selection.

  • MODE_CENTER - Like MODE_HANDLE, except it resizes two or four edges of the brush selection, maintaining the current center of the brush selection. This mode is temporarily enabled from MODE_HANDLE by holding down the ALT key. Also note that the SPACE key takes priority, so holding down ALT and SPACE will use MODE_SPACE.

Separate from the modes, there is also a flag:

  • shifting - Allow the brush selection to change in only one dimension. The dimension is determined by the first pointer movement after SHIFT is depressed. For example, if you hold down SHIFT and move the pointer to the right, the y-position of the brush selection becomes locked at its current values until the SHIFT key is released.

Some possible options:

  1. The simplest API would be to disable all key modifiers. This would allow you to use MODE_DRAG and MODE_HANDLE, depending on where the initial click landed, but would not allow you to change modes during a brush gesture, and would effectively disable MODE_SPACE, MODE_CENTER, and the shifting flag.

  2. An alternative API might allow disabling of specific modes. If you disabled MODE_SPACE, for instance, it would not listen for the SPACE key, but it might still listen for the ALT key to enable MODE_CENTER. Likewise you could disable MODE_CENTER to avoid listening for ALT. On the other hand since the shifting flag is orthogonal to the modes, I don’t see how this could disable listening for SHIFT, which is the original motivation for this issue.

  3. Another alternative would be to continue to observe key modifiers, but disable key listeners. This would allow you to use a key modifier at the start of a gesture; for example, holding down SHIFT on brush start could still lock the selection position on one dimension, but there would be no keydown and keyup listener, so if the SHIFT key were released while brushing, it would not affect the current brush gesture. Thus the brush would not interfere with external event listeners that want to do something with these key events.

@Frazer

This comment has been minimized.

Show comment
Hide comment
@Frazer

Frazer Apr 27, 2017

Is it possible that all key modifiers start turned off, and we can assign certain keys to the modifiers?

Ie: brush.setModifier("CENTER", alt_key);

(I love how it works now, but I need to use shift for other functions in my app, so I just had to comment it out from my d3.js)

Frazer commented Apr 27, 2017

Is it possible that all key modifiers start turned off, and we can assign certain keys to the modifiers?

Ie: brush.setModifier("CENTER", alt_key);

(I love how it works now, but I need to use shift for other functions in my app, so I just had to comment it out from my d3.js)

@mbostock

This comment has been minimized.

Show comment
Hide comment
@mbostock

mbostock Apr 27, 2017

Member

I wouldn’t change the default behavior, as that would require a major version change.

However I think allowing the key bindings to be reconfigurable could work. We’d need to enumerate the behavioral modifiers, say symbols like this:

  • d3.brushFixSize - defaults to SPACE (32)
  • d3.brushFixCenter - defaults to ALT (18)
  • d3.brushFixSecondary - defaults to SHIFT (16)

It’s a little weird in that we need to know the mapping from key code to key modifier (i.e., keycode 18 means event.altKey). But it’s probably doable? I worry it’s more complex than just an all-on/all-off flag, of course.

Member

mbostock commented Apr 27, 2017

I wouldn’t change the default behavior, as that would require a major version change.

However I think allowing the key bindings to be reconfigurable could work. We’d need to enumerate the behavioral modifiers, say symbols like this:

  • d3.brushFixSize - defaults to SPACE (32)
  • d3.brushFixCenter - defaults to ALT (18)
  • d3.brushFixSecondary - defaults to SHIFT (16)

It’s a little weird in that we need to know the mapping from key code to key modifier (i.e., keycode 18 means event.altKey). But it’s probably doable? I worry it’s more complex than just an all-on/all-off flag, of course.

@pkerpedjiev

This comment has been minimized.

Show comment
Hide comment
@pkerpedjiev

pkerpedjiev Apr 27, 2017

And d3.brushFixSize(null) would disable that behavior?

And d3.brushFixSize(null) would disable that behavior?

@mbostock

This comment has been minimized.

Show comment
Hide comment
@mbostock

mbostock Apr 27, 2017

Member

Probably something like this:

brush.keyModifier(d3.brushFixSize, null)
Member

mbostock commented Apr 27, 2017

Probably something like this:

brush.keyModifier(d3.brushFixSize, null)
@mbostock

This comment has been minimized.

Show comment
Hide comment
@mbostock

mbostock Apr 27, 2017

Member

But as I said, that’s just one proposal, and I don’t know if the added complexity of configurable modifiers is justified in comparison to just being able to turn them all off.

Member

mbostock commented Apr 27, 2017

But as I said, that’s just one proposal, and I don’t know if the added complexity of configurable modifiers is justified in comparison to just being able to turn them all off.

@Frazer

This comment has been minimized.

Show comment
Hide comment
@Frazer

Frazer Apr 28, 2017

Could you also be able to use the metaKey? or to set no key if we want to disable a function?

Frazer commented Apr 28, 2017

Could you also be able to use the metaKey? or to set no key if we want to disable a function?

@kpaxton

This comment has been minimized.

Show comment
Hide comment
@kpaxton

kpaxton May 4, 2017

Having the option to just turn them all off is one thing. Very beneficial for those apps, like mine, that have Ctrl, Alt, and Shift mapped to other things and just want basic brush behaviors. In my case, I want to use Shift to be an additive selection like you had in your Draggable Network II example for v3. So just being able to disable them would work in my scenario.

Remapping the keys to allow for the same behavior is definitely more beneficial for a larger audience I believe. Maybe creating a shortcut function that performs a blanket disable by remapping each to null would be the solution for both issues brush.disableKeyModifiers(). What @mbostock suggested above brush.keyModifier(d3.brushFixSize, null) I think is fine. As long as what brushFixSize, brushFixCenter, and brushFixSecondary are documented well so that everyone knows what they mean. I also think @Frazer's suggestion of just using 'CENTER', 'SIZE', or 'SECONDARY' are viable as well. I would expect almost all users of D3 who are trying to set keydown functions would understand the mapping between keycodes and key modifiers. So I don't necessarily see that as a blocker to moving forward with this.

I've been trying to think of a way you could tie it into either the brush.on() functions or some other type of event override similar to your example above.

brush.on("start.nokey", function() {
  d3.select(window).on("keydown.brush keyup.brush", null);
});

But I can't come up with anything that feels viable.

kpaxton commented May 4, 2017

Having the option to just turn them all off is one thing. Very beneficial for those apps, like mine, that have Ctrl, Alt, and Shift mapped to other things and just want basic brush behaviors. In my case, I want to use Shift to be an additive selection like you had in your Draggable Network II example for v3. So just being able to disable them would work in my scenario.

Remapping the keys to allow for the same behavior is definitely more beneficial for a larger audience I believe. Maybe creating a shortcut function that performs a blanket disable by remapping each to null would be the solution for both issues brush.disableKeyModifiers(). What @mbostock suggested above brush.keyModifier(d3.brushFixSize, null) I think is fine. As long as what brushFixSize, brushFixCenter, and brushFixSecondary are documented well so that everyone knows what they mean. I also think @Frazer's suggestion of just using 'CENTER', 'SIZE', or 'SECONDARY' are viable as well. I would expect almost all users of D3 who are trying to set keydown functions would understand the mapping between keycodes and key modifiers. So I don't necessarily see that as a blocker to moving forward with this.

I've been trying to think of a way you could tie it into either the brush.on() functions or some other type of event override similar to your example above.

brush.on("start.nokey", function() {
  d3.select(window).on("keydown.brush keyup.brush", null);
});

But I can't come up with anything that feels viable.

@pkerpedjiev

This comment has been minimized.

Show comment
Hide comment
@pkerpedjiev

pkerpedjiev May 4, 2017

I'm personally fine with turning off all of these behaviors at once. But that is also possible if each are individually configurable. So I suppose it depends on how much extra effort it takes to implement the latter option to determine whether it's worth it.

@kpaxton If you're interested, I implemented a selectable, draggable, force directed network by forking d3-brush.js and turning off the default shift key behavior. Example is on bl.ocks.org.

I'm personally fine with turning off all of these behaviors at once. But that is also possible if each are individually configurable. So I suppose it depends on how much extra effort it takes to implement the latter option to determine whether it's worth it.

@kpaxton If you're interested, I implemented a selectable, draggable, force directed network by forking d3-brush.js and turning off the default shift key behavior. Example is on bl.ocks.org.

@kpaxton

This comment has been minimized.

Show comment
Hide comment
@kpaxton

kpaxton May 4, 2017

Thansk @pkerpedjiev but I'm in a production situation and don't want to stray from supported packages. I was using your other one initially. http://bl.ocks.org/pkerpedjiev/0389e39fad95e1cf29ce

kpaxton commented May 4, 2017

Thansk @pkerpedjiev but I'm in a production situation and don't want to stray from supported packages. I was using your other one initially. http://bl.ocks.org/pkerpedjiev/0389e39fad95e1cf29ce

@kpaxton

This comment has been minimized.

Show comment
Hide comment
@kpaxton

kpaxton May 26, 2017

Any movement on this yet? I could really use this feature.

kpaxton commented May 26, 2017

Any movement on this yet? I could really use this feature.

@bumbeishvili

This comment has been minimized.

Show comment
Hide comment
@bumbeishvili

bumbeishvili Dec 12, 2017

I got hit by this as well, will be there any update regarding this behavior?

I got hit by this as well, will be there any update regarding this behavior?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment