Enforce exact NewLine key combos #6038
Conversation
This also fixes #17215. Signed-off-by: Clemens Zeidler <clemens.zeidler@gmail.com>
This comment has been minimized.
This comment has been minimized.
Only preventDefault but not the event propagation.
There was a problem hiding this comment.
Thanks for taking a look at this! It doesn't seem to be producing the right result though: with the Ctrl+Enter option enabled, the Shift+Enter approach isn't possible when this change is applied. It should still function to add a newline like the plain Enter key would in that case.
When the Ctrl+Enter option disabled, this seems to work fine. This leads me to believe it's the keybinding stuff that is getting in the way, as mentioned on the issue.
|
Thanks for the feedback. This behavior was actually intentionally :-) Could you please give me some background on why you want to have the additional In general I couldn't really think of a good reason why you want to have more than one key combo for a single action (as already mentioned above). However, I'm not really opposed to having another |
Personally, it's mainly muscle memory. But it's actually a bit more complicated. In other programs where I can't set |
|
Plus its just standard. Like this text area on github, shift enter or enter alone are newline. Cmd enter is send/submit |
This is apparently common in other messengers / text input fields
|
OK I added the combo. You should not use other messenger ;-) shift + Enter without Enter makes kind of sense though so that you not get into the habit of accidentally sending something when you use a system that sends on Enter. |
|
|
||
| if (handled) { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| return; | ||
| } | ||
| // Always prevent the default if key === Enter. There are multiple reasons for this: | ||
| // 1) It makes it possible to set the NewLine key binding to a key other than Enter without still triggering a | ||
| // new line when pressing Enter. | ||
| // 2) It stops key combos like shift + Enter or alt + Enter to generate NewLines. | ||
| // 3) It avoids a Chrome 'quirk' which sometimes results in a '\n\n' input on shift+Enter | ||
| // (https://github.com/vector-im/element-web/issues/17215). | ||
| if (event.key === Key.ENTER) { | ||
| event.preventDefault(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
All of this code doesn't appear to be needed anymore, at least not in testing.
There was a problem hiding this comment.
The issue isn't triggered if shift + Enter OR Enter is used as key combo to insert a new line. @SimonBrandner is working on customizable key bindings though. For this to work the check is still needed, e.g. if you put new line on something other than Enter. For this reason I would leave it in. I updated the outdated comment...
There was a problem hiding this comment.
@turt2live any opinion on that? Still think it makes sense to keep it in and the issue documented in the code.
There was a problem hiding this comment.
The PR worked just fine for all combinations when I removed this whole block, so am curious as to what it's trying to do. The comment isn't super revealing into why it's needed specifically, but does accurately transcribe the issue to the code. Our comments generally strive to explain the why over the how, which means describing both the problem and solution, and why the solution is the way it is - clarifying the block with this format might help understand what this block is doing.
There was a problem hiding this comment.
? there are three reasons in the comment that explain the why :-) However, I think I can simplify the comment:
"Swallow the Enter key event in case the NewLine key action is mapped to something different than a key combo containing the Enter key."
Would that be clearer?
There was a problem hiding this comment.
possibly, but I'm still curious as to why the code block doesn't seem to be doing anything.
There was a problem hiding this comment.
The code block is for when the NewLine action is mapped to somethings else than the Enter key (assuming this will be configurable at some point and somebody actually want to change it...). With the default config the code block is never reached because the key combo is handled earlier.
There was a problem hiding this comment.
@czeidler, after some thinking, I think we might actually want to handle the send shortcut a little differently than the other shortcuts, so I would suggest removing this code, for now, we can always get it back if we need to in the future
There was a problem hiding this comment.
which code? not sure what you mean
There was a problem hiding this comment.
The changes in KeyBindingDefaults.ts are sufficient to fix the issue at hand therefore the changes in BasicMessageComposer.tsx aren't necessary. If we want a more general fix in the future we can always get the changes to BasicMessageComposer.tsx from this PR but for now, we don't need them
SimonBrandner
added
the
T-Defect
The main motivation for this PR is to fix element-hq/element-web#17215.
Some background why I think key combos should be exact. Fuzzy key combos that allow combos like "
ctrl+EnterORalt + Enter OREnter`" to trigger the same binding (e.g. new line), increase the probability that a user enters a fuzzy key combo accidentally. This can for example happen when the user is trying a different combo or is just clumsy. Applying a keybinding accidentally is certainly very frustrating (especially when sending a message too early, e.g. because the user thought alt+Enter instead of shift+Enter means new line). On the other hand, if a wrong/fuzzy key combo was entered on purpose and nothing happens the user simply has to re-enter the correct, exact combo. I argue that the exact matching is less frustrating to the user. Furthermore, it keeps things simple.Signed-off-by: Clemens Zeidler clemens.zeidler@gmail.com
Here's what your changelog entry will look like:
🐛 Bug Fixes