Focus on composition area when pressing a key #4998
Conversation
|
Currently (1.40.0-beta.4), closing the reply dialogue loses the focus on composition area, would this prevent that too? |
|
@Xashyar Well, the focus is still lost. However, you can still continue to type and the composition area gains focus automatically (also no characters are lost). So, whether it fixes it depends on your definition since the composition area still loses focus but it does not impact your typing. |
|
I think this PR would be really beneficial. Currently, I often have to click inside the text input area to be able to type, and it is getting slightly annoying. I'm not sure what causes the text input area to lose focus. I often just start typing and sometimes nothing appears, because focus is lost. |
|
@EvanHahn-Signal is there any plan on merging this? I guess quite a lot of people are unsatisfied with the current typing behavior. |
|
@n-gao sorry we marked this as "needs review" to keep stale bot away when we added stale bot. Not sure if we actually want to do this though, I mean it seems fine on the surface but:
We're also worried about the implementation, there will now be a global keydown listener and that'll need to be maintained, seems like it could be a recipe for potential disaster if any of the other pieces that it needs to short-circuit on change and this code isn't updated to reflect that. I'm happy to hear counterpoints to this -- I sometimes experience this issue and start typing but the composition area isn't in focus. I usually quickly realize and just focus it with cmd+shift+t |
|
cmd+shift+t seems undocumented to me. I wasn't aware it existed, and I cannot find the shortcut in the menu bar or in preferences. Now that I know of it: neat! I will test if this is sufficient for me. I sometimes find it surprising that the focus is not in the text input field, not sure why it is not sometimes. |
I wasn't aware of it either, but it does appear to be documented in both the ⌘+/ popup and the respective support article under the name "Focus composer". |
|
@josh-signal The comments by @u32i64 and @larstobi illustrate the difficulty of having to rely on hotkeys. Most people (including me) are not aware of these and for them, it is mostly an annoyance having to click on the composing box. Imho, hotkeys are always heavy user-oriented. Is having Signal outside of your screen and the composer focused an actual issue? Right now, I could also have been typing and then moving the window outside of my screen (not sure why anyone would do that) and the focus on the composer would stay. If the window is minimized it automatically loses focus (at least on windows but I would be surprised if other OS's keep the focus if you minimized a window) and you can't type. Furthermore, other chat clients like WhatsApp or Telegram decided it not being a real issue and include this feature. Regarding the global observer, I see your concerns. This is why it's not a global observer (like hotkeys) but restricted to the composition area. I think that the composing area is a good compromise since it's easy to hit without affecting parts of the application where it definitely should not trigger. If the focus is on the contact pane the focus obviously should not switch to the composer. |
|
Yeah the shortcuts aren't intuitive which is why this PR exists. edit: But glad ya'll are aware of them now 😄
In most cases I'm guessing no, but it is a possibility so I raised it as a concern.
It's adding the event listener to |
| } | ||
|
|
||
| // We don't want to switch focus if another panel is up | ||
| const panels = document.querySelectorAll('.conversation .panel'); |
There was a problem hiding this comment.
Also these kind of class lookups are brittle, if we ever change conversation | panel this would fall apart. I think ideally redux would know about panel state and we can just query that selector if there are panels active or not.
There was a problem hiding this comment.
Is this information already saved in a Redux store somewhere? Or are you implying that it should be added? (And if you're suggesting it be added, where would be your suggestion for where to add this?)
There was a problem hiding this comment.
Also, seems like this already has precedent in the codebase:
Signal-Desktop/ts/components/stickers/StickerButton.tsx
Lines 193 to 196 in 6700d2f
Maybe refactoring this could be done in a different PR?
|
@n-gao do you think that you will be able to get back to this sometime soon? This is a feature that every other desktop messenger client has, so it's a big UX drawback that this doesn't already exist. |
| return; | ||
| } | ||
|
|
||
| // We don't want to take focus away of input fields |
There was a problem hiding this comment.
Clarity:
| // We don't want to take focus away of input fields | |
| // Avoid stealing focus from focused input fields |
| const { activeElement } = document; | ||
| if (activeElement?.nodeName.toLowerCase() === 'input') { |
There was a problem hiding this comment.
Style: Avoid extra identifier in scope
| const { activeElement } = document; | |
| if (activeElement?.nodeName.toLowerCase() === 'input') { | |
| if (document.activeElement?.nodeName.toLowerCase() === 'input') { |
| @@ -357,6 +357,37 @@ export const CompositionArea = ({ | |||
| }; | |||
| }, [setLarge]); | |||
|
|
|||
| // Listen to any key in the conversation panel to | |||
There was a problem hiding this comment.
| // Listen to any key in the conversation panel to | |
| // Focus message composition input when key is pressed |
| return; | ||
| } | ||
|
|
||
| inputApiRef.current?.focus(); |
There was a problem hiding this comment.
Should this use the focusInput callback instead?
If yes, then:
| inputApiRef.current?.focus(); | |
| focusInput(); |
| document.addEventListener('keydown', handler); | ||
| return () => { | ||
| document.removeEventListener('keydown', handler); | ||
| }; |
There was a problem hiding this comment.
Surfacing @josh-signal's comment here in a conversation:
It's adding the event listener to
documentso yeah it's grabbing all input. Perhaps abettersafer approach would be to figure out what node is focused and listen tokeypresson that node? I think we can test the "in focus" issue using something like macOS' spaces.
There was a problem hiding this comment.
@josh-signal to be clear, you're suggesting this change?
| document.addEventListener('keydown', handler); | |
| return () => { | |
| document.removeEventListener('keydown', handler); | |
| }; | |
| document.activeElement.addEventListener('keypress', handler); | |
| return () => { | |
| document.activeElement.removeEventListener('keypress', handler); | |
| }; |
Not sure if this will work, but at least it's a starting place for the conversation.
| @@ -357,6 +357,37 @@ export const CompositionArea = ({ | |||
| }; | |||
| }, [setLarge]); | |||
|
|
|||
| // Listen to any key in the conversation panel to | |||
| React.useEffect(() => { | |||
There was a problem hiding this comment.
useEffect has now been imported as a named import
| React.useEffect(() => { | |
| useEffect(() => { |
|
For anyone else who doesn't want to wait for this to be accepted, I've created a patcher for the Signal desktop Electron app (macOS only for now): https://github.com/karlhorky/electron-app-patcher Just clone, install the dependencies and run Edit: Upgraded my patch for Signal 6.0.0: karlhorky/electron-app-patcher@6fbdffe |
|
@josh-signal or @jamiebuilds-signal would it be possible to get another review on this? Would love to get a version of this missing UX feature in the app finally. |
|
why is this no merged already?? |
|
I updated my patch for Signal 6.3.0, which was released recently: |
|
No other patch changes have been necessary for the new versions up to Signal 6.9.0, although it is annoying to have to update every time an update comes out:
|
|
@indutny-signal any news on what it would take to get this merged? Or a timeline? |
|
My patch broke again with one of the recent versions - I'll take a look this weekend at releasing a new version to fix the new versions 😫 |
|
Ohh in the new versions now everything is minified, this makes patching it more difficult 😣 // Listen for cmd/ctrl-shift-x to toggle large composition mode
useEffect(() => {
const handler = (e: KeyboardEvent) => {
const { shiftKey, ctrlKey, metaKey } = e;
const key = KeyboardLayout.lookup(e);
// When using the ctrl key, `key` is `'K'`. When using the cmd key, `key` is `'k'`
const targetKey = key === 'k' || key === 'K';
const commandKey = platform === 'darwin' && metaKey;
const controlKey = platform !== 'darwin' && ctrlKey;
const commandOrCtrl = commandKey || controlKey;
// cmd/ctrl-shift-k
if (targetKey && shiftKey && commandOrCtrl) {
e.preventDefault();
setLarge(x => !x);
}
};
document.addEventListener('keydown', handler);
return () => {
document.removeEventListener('keydown', handler);
};
}, [platform, setLarge]);
const handleRecordingBeforeSend = useCallback(() => {
emojiButtonRef.current?.close();
}, [emojiButtonRef]);Now becomes this in (0,sr.useEffect)(()=>{let Nn=a(aa=>{let{shiftKey:Qd,ctrlKey:qf,metaKey:ly}=aa,jm=pl(aa),tD=jm==="k"||jm==="K",j_=(0,KVe.get)(window,"platform")==="darwin"&&ly,rD=(0,KVe.get)(window,"platform")!=="darwin"&&qf;tD&&Qd&&(j_||rD)&&(aa.preventDefault(),Ir(sN=>!sN))},"handler");return document.addEventListener("keydown",Nn),()=>{document.removeEventListener("keydown",Nn)}},[Ir]);let Ui=(0,sr.useCallback)(()=>{Uo.current?.close()},[Uo])Going to be a fun regex to write :( |
Signal Patch ChangelogVersion changelog for all of the fixes needed with the new versions of Signal:
Hopefully we can get this missing UX feature sometime soon, so that I don't need to keep maintaining this. |
|
@scottnonnenberg-signal @indutny-signal @josh-signal friendly ping - would love to be able to get this missing feature in Signal. Could someone from the Signal team respond here? |
|
what's the major factor blocking this pr |
First time contributor checklist:
Contributor checklist:
developmentbranchyarn readyrun passes successfully (more about tests here)Description
At the moment, a user has to specifically focus on the composition field to write a message. However, it is often more convenient to just click somewhere into the application and start typing with the application automatically moving the focus.
Other chat applications do this too.
So, this PR enables that. The focus is only moved for printable characters and not for control characters and does not move the focus if another input field is currently selected.
Demo:
