Skip to content
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

Automatically focus "send message" when the user is typing #1416

Closed
wants to merge 1 commit into from

Conversation

vrde
Copy link

@vrde vrde commented Aug 30, 2017

First time contributor checklist

Contributor checklist

  • My contribution is fully baked and ready to be merged as is
  • My changes are rebased on the latest master branch
  • My commits are in nice logical chunks
  • I have followed the best practices in my commit messages
  • I have made the choice whether I want the BitHub reward or not by omitting or adding the word FREEBIE in my Git commit messages
  • I have tested my contribution on these platforms:
  • Ubuntu 17.04, Chromium 60.0.3112.78
  • My changes pass all the local tests 100%
  • I have considered whether my changes need additional tests, and in the case they do, I have written them

Description

Clicking on the discussion container (highlighted in red in the screenshot) now focus on the message box.
I've added an event listener for keydown events that focuses the message box if nothing else has focus. Why? Because I don't have always the patience to go to the message box and click there 😃.

PS: I have tried to sign the Contributor Licence Agreement but it doesn't work.

@scottnonnenberg
Copy link
Contributor

It looks like something weird happened with your commit - it includes all the changes I made in this commit: 46b64e3 I see that I'm also (strangely) one of the authors on your change. Did you perhaps amend my original commit?

I'm testing the change now, but we won't be able to get this approved and merged until the changes are cleaned up. Please author a new commit with just your changes in conversation_view.js.

@scottnonnenberg
Copy link
Contributor

Okay, I just played with it a bit, and it works pretty well. However, it does fail in a couple scenarios:

  • I can no longer double- or triple-click to select the contents of a message.
  • When I click on an image to make it bigger, the whole overlay is shifted up and partially out of the window.

Maybe the right solution is to limit the click-handling to outside the message bubbles?

@vrde vrde force-pushed the focus-on-conversation-area-click branch 2 times, most recently from 0196f0e to 04aad58 Compare September 2, 2017 10:47
@vrde
Copy link
Author

vrde commented Sep 2, 2017

It looks like something weird happened with your commit - it includes all the changes I made in this commit: 46b64e3 I see that I'm also (strangely) one of the authors on your change. Did you perhaps amend my original commit?

I just noticed that, dunno what went wrong, anyway I rebased with master and committed again :)

Okay, I just played with it a bit, and it works pretty well. However, it does fail in a couple scenarios:

I can no longer double- or triple-click to select the contents of a message.
When I click on an image to make it bigger, the whole overlay is shifted up and partially out of the window.

OK, give me a bit more time 💃

@cyrus-and
Copy link

I actually miss this feature in Signal Desktop but it would probably be more useful focusing the input field as one starts typing as it happens in WhatsApp Web. What do you think?

@vrde
Copy link
Author

vrde commented Sep 12, 2017

I actually miss this feature in Signal Desktop but it would probably be more useful focusing the input field as one starts typing as it happens in WhatsApp Web. What do you think?

@cyrus-and definitely yes! I thought the same right after opening this PR and writing to a friend on WhatsApp 👯‍♀️

I need to be extra careful though, adding a global event listener might be quite invasive. I'll try to push some code soon and have an early feedback to validate my approach.

@vrde vrde force-pushed the focus-on-conversation-area-click branch from c7cc06f to 866a77a Compare September 16, 2017 15:37
@vrde
Copy link
Author

vrde commented Sep 16, 2017

@scottnonnenberg: I changed the approach, I'm using keyboard events now (the behavior is similar to WhatsApp Web). I've put the event listener in chromium.js, I don't know this code–base much so please tell me if it's the right place.

@cyrus-and: ✨ check it out ✨

@vrde vrde changed the title Focus "send message" when clicking on conversation Automatically focus "send message" when the user is typing Sep 16, 2017
@vrde vrde force-pushed the focus-on-conversation-area-click branch from 866a77a to 781840c Compare September 16, 2017 15:44
js/chromium.js Outdated
var lightbox = document.querySelector('.lightbox');
var element = document.querySelector('textarea.send-message');

if (tagName == 'INPUT' || tagName == 'TEXTAREA' || lightbox) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use triple equals here.

@cyrus-and
Copy link

@vrde awesome, works great so far!

@cyrus-and
Copy link

@vrde you may want to restrict this to the typeable events? For example, tab doesn't work as intended anymore.

@vrde
Copy link
Author

vrde commented Sep 16, 2017

@vrde you may want to restrict this to the typeable events? For example, tab doesn't work as intended anymore.

Definitely, I have the same problem with arrows and page up/down. I just noticed they don't do anything anyway.

@vrde
Copy link
Author

vrde commented Sep 16, 2017

@vrde you may want to restrict this to the typeable events? For example, tab doesn't work as intended anymore.

I changed the event listener from keydown to keypress. The latter handles only printable events.

@cyrus-and
Copy link

@vrde There is another thing I noticed, the patch should detect whether the conversation is active or not, for example when the settings pane is open or when a user clicks the top left "Signal" title. That is:

if (document.querySelector('.conversation-stack.inactive')) {
    return;
}

It would be nice to bind/unbind the keypress event on conversation activation/deactivation but I didn't look at the rest of the code I don't know if that's possible.

@vrde
Copy link
Author

vrde commented Sep 17, 2017

There is another thing I noticed, the patch should detect whether the conversation is active or not, for example when the settings pane is open or when a user clicks the top left "Signal" title. [...]

Good point. I've changed the lightbox selector to a more generic modal selector, what do you think?

(I can squash the commits once you are satisfied with the PR)

@cyrus-and
Copy link

@vrde it doesn't seem to work with menus and inactive conversations. Checking the conversation status (via .conversation-stack.inactive, I don't know if there's a better way) seems to be more reliable.

@cyrus-and
Copy link

@vrde Oh bummer, the conversation is still active if the user interacts with the conversation menu... :(

This is a possible (ugly) check:

document.querySelectorAll('.menu-list[style="display: block;"]').length

Just to make it clear, I'm not a member of this repo don't feel compelled to address every comment I make, I'm just trying to help :)

@vrde vrde force-pushed the focus-on-conversation-area-click branch from 97096f0 to 6de90c0 Compare September 17, 2017 16:26
@vrde
Copy link
Author

vrde commented Sep 17, 2017

@cyrus-and: I was trying the same and yep, the selector is savage 🐯. IMO it's fine if a keypress goes through when a drop down menu is open (also, you cannot control those drop down menus using the keyboard, but just the mouse). If you have any other remarks please let me know! You helped me making this PR better 👍

@scottnonnenberg: Can you try out the new changes and tell me what you think?

PS: I've cleaned up my commits and did a push -f, you might need to realign your branches.

@vrde vrde force-pushed the focus-on-conversation-area-click branch 2 times, most recently from 4b997d2 to d565321 Compare September 20, 2017 11:49
@vrde
Copy link
Author

vrde commented Sep 22, 2017

image

@scottnonnenberg unfortunately I cannot sign the CLA 😞 is it a blocker?

@scottnonnenberg
Copy link
Contributor

Looks like it did succeed - we just had a problem showing you a proper success page, which we're working on as we speak. You're good to go. :0)

@vrde vrde force-pushed the focus-on-conversation-area-click branch from d565321 to 53748c8 Compare September 27, 2017 15:20
@vrde
Copy link
Author

vrde commented Oct 24, 2017

Hey @scottnonnenberg, are you still interested in merging this?

@scottnonnenberg
Copy link
Contributor

I just took a look at the latest code, and I have a couple concerns:

  1. What happens when multiple conversations have been loaded (and are therefore in the DOM?)
  2. What happens to the first character typed? Is it lost?
  3. What can be done to reduce brittleness on this, potential breaks when changes are made in far-distant parts of the app?

One solution to 1 and 3 is to add it to ConversationView (registering on load, unregistering on unload) and only respond to the event if the conversation was visible with no modals. this.isHidden() tells you about visibility, and this.panels keeps the list of active modals for that conversation.

@vrde
Copy link
Author

vrde commented Nov 13, 2017

@scottnonnenberg thanks for your feedback. I'm quite busy now, will give it a shot next weekend if I find time :)

@scottnonnenberg
Copy link
Contributor

I'm gonna close this until it gets some more attention.

@tosh
Copy link

tosh commented Jan 10, 2018

Solving this UX issue would be amazing. Right now it makes Signal's UX stand out among the other more popular messaging apps that figured out a way to keep/regain focus on the input field.

@scottnonnenberg
Copy link
Contributor

@tosh Seems that we don't have a good design for how it should work. You seem to be familiar with other messaging apps; perhaps you have a recommendation for the right design here? (and when I say that, I mean an idea of how to handle all the corner cases involved)

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

Successfully merging this pull request may close these issues.

4 participants