How to use v6 modifiers version to not trigger when textarea is focused?
st-h opened this issue · 17 comments
I am using this addon to allow to use the space bar to stop and start playback of an audio file. When an input/textarea/contenteditable is focused, the event should not be handled by ember-keyboard, as every space key would stop or start audio playback.
I thought I was able to migrate to the new version without issue, but I just noticed that I am having issues with textareas.
To sum it up shortly:
This is inside the player component to trigger an action when the space key is pressed:
{{on-key 'Space' this.onKey}}
onKey is a simple action:
@action
onKey(e) {
e.preventDefault();
console.log('key pressed');
}
If I ommit e.preventDefault(), every time I hit space both ember-keyboard and the textarea trigger simultaneously.
If I make use of e.preventDefault(), I can no longer enter spaces into the textarea, but ember-keyboard triggers every time I hit space (no matter where the focus is).
From what I read in the docs, textareas should work out of the box? don't they? Am I missing something?
I have quickly add a repo to reproduce here: https://github.com/st-h/ember-keyboard-repro
I also tried to use e.stopPropagation();, e.stopImmediatePropagation(); and assigning a priority, but neither worked. Thinking I would need to prevent every input event from being propagated to ember-keyboard, but that sounds like a massive change to larger apps, which also could have additional sideeffects.
Isn't it intended that when a textarea is focused, ember-keyboard should not handle the event?
Hi @st-h, I'm happy to look into this and I appreciate the repro repo. However, I cloned the repo and it seems to be a new empty ember app. Maybe you missed committing something?
@lukemelia sorry. Yes, that was the case. Should be fine now.
I also tried to call stopPropagation etc on the keyup event of the textfield, but that seems to be triggered after ember-keyboard has received the event. Probably that could be worked around with the priorities, but at that point I stopped to look into it further - because having to intercept all events of all input elements would be too error prone and would mean too many changes to our app (having to add event handlers where everything would previous work out of the box).
Thanks, @st-h. The repo made it a lot easier to consider. My thoughts:
- In previous versions of ember-keyboard, an initializer was mixed into Ember.Textarea to make it a "firstResponder" when focused. It did not have any effect if you were using regular HTML <textarea></textarea> directly.
- The equivalent of this using the
on-keyboardmodifier with a regular textarea is
The on-key modifier has special behavior on input, textarea, and select where is is only active when the element is focused. With the above syntax, this means that it will become a firstResponder when focused and prevent other responders from receiving events.
- This is not well-documented and that should be improved.
- We can consider making this behavior the default somehow, perhaps via an opt-in configuration, as it could be a breaking change.
@lukemelia Thanks for the explanation. The things that tripped me were, that in a few places it is mentioned that native input and textarea elements should be used and that the initialiser is no longer needed. It made the impression that we can now replace embers input helpers with native elements and ember-keyboard would continue its previous behaviour.
I would really like to see this behaviour work automagically again. Otherwise, each input or Textelement added where one would forget to add the firstResponder would result in odd user behaviour, which is hard to detect in advance. It would be awesome if this could work for elements with contenteditable enabled as well.
@st-h All makes sense. Would you be up for creating a documentation PR to try to improve the clarity for future people in your situation?
I'll give some further thought to how to create an automatic solution that works with native elements. I'm open to ideas from you or others reading this issue, too.
@lukemelia I was looking into this again today, as I was trying to introduce native inputs to our app and quickly noticed that this would cause issues with v5.
I think we can solve this issue by checking the event target - possibly in service::keyboard::_respond(event) (probably you know of a more suitable location).
Now if event.target.tagName == 'INPUT' then we should not process the event, but let the input field handle the input.
Additionally we should check for tagName == 'TEXTAREA and event.target.getAttribute('contenteditable') != null. (contenteditable might be enable if it equals an empty string)
We could put this behind a flag (that is enabled per default), so people can bail out if they need to intercept their input fields or want to add firstResponders manually.
What do you think? If you have some time to quickly look at this, I think I can put some more time aside today to try it out and put together a PR.
@lukemelia I just tested an augmented version of the _respond() method:
@action
_respond(event) {
if (event.target) {
const tag = event.target.tagName;
const isContentEditable = event.target.getAttribute && event.target.getAttribute('contenteditable') != null;
if (isContentEditable || tag === 'TEXTAREA' || tag === 'INPUT') {
return;
}
}
run(() => {
if (this.isPropagationEnabled) {
let { firstResponders, normalResponders } = this;
handleKeyEventWithPropagation(event, { firstResponders, normalResponders });
} else {
let { sortedResponders } = this;
handleKeyEventWithLaxPriorities(event, sortedResponders);
}
});
}
Which seems to work quite fine. I just couldn't figure out why there is no getAttribute method on event.target when running tests.
Does this look like a viable approach to you? If you so, I'll happily submit a PR. What do you think about making it configurable? Probably we could even add a modifier, which can be applied to an input, which would skip the check if the current element is an input (so, one would have to specify which input should be taking into account instead of having to apply firstResponder to all elements that shouldn't)?
@st-h I think this approach has potential, but in cases where an {{on-key ...}} modifier has been placed on the input/textarea/contenteditable, I think we should respect the modifier rather than ignoring events. Real-world example: a user wants to bind the Enter key to an action when the input field is focused.
@lukemelia I think this sounds like the right solution. Do not use events from any input elements, unless there is an {{on-key}} modifier applied to it. Makes perfect sense. I'll try to find some time to add this to my code and submit a PR. I have been using the code above for a few days now and haven't come across any issues so far.
@st-h sounds good. please make this change opt-in as well, so that we can release it without cutting a major version.
@st-h sounds good. please make this change opt-in as well, so that we can release it without cutting a major version.
I think this will cause a lot less confusion if it is opt-out and come with a major release. But is there even a good reason/practical use case to opt out of that behaviour? I can only think of someone wanting to intercept ALL the input fields within their app, which seems quite ridiculous to me (and there are much better ways to achieve something like that). Additionally making it not opt in would be much closer to previous versions, which would handle those things automatically.
@lukemelia sorry, I've been very short on time. Every time I find a bit of time, I find myself looking at the tests trying to figure out a good place where to add a test, so I can implement the missing details (not yet sure how it would be possible to detect if the element has a modifier applied). Could you please suggest which test would be a good one to test that functionality?
I hate to say it, but the current behaviour also is an issue with scrollable elements. Binding space bar to ember-keyboard and having a scrollable element focused leads to both scrolling and ember-keyboard being triggered simultaneously.
@lukemelia I've finally revisited this issue. Turns out, my scrolling issue could be easily resolved by adding event.preventDefault() to the handler of the space key. I have opened a PR with above mentioned changes and a config option called disableOnInputFields. When set to true, ember keyboard events never fire when an input, textarea or a contenteditable element is focused. I'll happily leave it up to you if you want to keep this opt-in or make it opt-out in the future.
I was also running into this. Looking forward when #557 lands. As a workaround for now I tried as suggested applying {{on-key firstResponder=true}} on the native input, but this didn't work for me.
Looking at the modifier's code, I also don't see that this alone would prevent any other responders. Also firstResponder doesn't seem to be actually a supported argument AFAICT, is it?
What worked for me is something like this:
// component.js
ignoreEkEvent(_event: Event, ekEvent: Event): void {
ekEvent.stopImmediatePropagation();
}Is this expected? Or do I miss something?
Until the PR is merged you can use this entry in your package json, which will pull the code from the git branch:
"ember-keyboard": "git+https://github.com/st-h/ember-keyboard.git#auto-first-responder",
Otherwise, I think stopping propagation on the ekEvent is the right way to do this.