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.

Sign up for GitHub

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

Jump to bottom

compose (misc): Fix send message on enter in topic input. #32975

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

compose (misc): Fix send message on enter in topic input. #32975

wants to merge 1 commit into from
+5 −1

Conversation

Harshbansal8705
Copy link
Collaborator

This PR prevents sending the message when pressing enter from within a topic.

This is done by adding event.preventDefault() to the topic input as the input element is directly submitting the form without propagating it to form element, and thus event.stopPropagation() doesn't work.

Fixes: #32596

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@alya
Copy link
Contributor

alya commented Jan 9, 2025

@amanagr Are you up for reviewing this one? I haven't tested.

@alya alya added the maintainer review PR is ready for review by Zulip maintainers. label Jan 9, 2025
@amanagr
Copy link
Member

amanagr commented Jan 9, 2025

This is the wrong way to fix it. We should avoid returning false in hotkey.js so that the form is not submitted by the browser.

    if (processing_text()) {
        if (stream_list.searching()) {
            // This is sort of funny behavior, but I think
            // the intention is that we want it super easy
            // to close stream search.
            stream_list.clear_and_hide_search();
            return true;
        }

        return false;
    }

Let's add a condition here which returns true when user is focused on the topic input box.

@Harshbansal8705
Copy link
Collaborator Author

As per the code style, should I create a function in compose.js to check if topic is focused and import it to hotkey.js?

@amanagr
Copy link
Member

amanagr commented Jan 10, 2025

Sounds good to me. Also, check if there are any existing function that do so. (Could extract one from other functions too)

@Harshbansal8705 Harshbansal8705 force-pushed the topic-input-enter-send-fix branch from 00908c1 to f70bfd9 Compare January 10, 2025 11:40
@zulipbot zulipbot added size: S and removed size: XS labels Jan 10, 2025
@Harshbansal8705
compose (misc): Fix send message on enter in topic input.
f887f48 
This commit prevents sending the message when pressing
enter while on focus at topic input box.

Fixes: zulip#32596
Co-Authored-By: amanagr
@Harshbansal8705 Harshbansal8705 force-pushed the topic-input-enter-send-fix branch from f70bfd9 to f887f48 Compare January 10, 2025 11:46
@Harshbansal8705
Copy link
Collaborator Author

@amanagr Can you have a look at the updated code?

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

@amanagr amanagr

Labels
area: compose (misc) bug maintainer review PR is ready for review by Zulip maintainers. priority: high size: S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enter always sends from topic box
4 participants
@Harshbansal8705 @alya @amanagr @zulipbot