From 4fbf91c135cf6267217ebfdc679e95650e722b66 Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Mon, 17 Mar 2025 17:56:01 +0530 Subject: [PATCH] compose: Fix buggy tooltip on validation. Compose send tooltip was not correctly displayed when the compose box was in an invalid state. We significanlty improve the logic for displaying the tooltip here to provide a more robust experience to the user. --- web/src/compose_actions.ts | 1 + web/src/compose_recipient.ts | 27 ++--------- web/src/compose_setup.js | 9 +--- web/src/compose_tooltips.ts | 48 ++++++++++++-------- web/src/compose_validate.ts | 70 ++++++++++++++++++++--------- web/tests/compose.test.cjs | 3 ++ web/tests/compose_actions.test.cjs | 4 ++ web/tests/compose_validate.test.cjs | 3 +- web/tests/upload.test.cjs | 7 +-- 9 files changed, 97 insertions(+), 75 deletions(-) diff --git a/web/src/compose_actions.ts b/web/src/compose_actions.ts index 30c2964247..f4943d4269 100644 --- a/web/src/compose_actions.ts +++ b/web/src/compose_actions.ts @@ -206,6 +206,7 @@ export let complete_starting_tasks = (opts: ComposeActionsOpts): void => { compose_notifications.maybe_show_one_time_interleaved_view_messages_fading_banner(); } compose_ui.maybe_show_scrolling_formatting_buttons("#message-formatting-controls-container"); + compose_validate.validate_and_update_send_button_status(); }; export function rewire_complete_starting_tasks(value: typeof complete_starting_tasks): void { diff --git a/web/src/compose_recipient.ts b/web/src/compose_recipient.ts index c3c8e3357c..80d6e9409a 100644 --- a/web/src/compose_recipient.ts +++ b/web/src/compose_recipient.ts @@ -1,7 +1,7 @@ /* Compose box module responsible for the message's recipient */ import $ from "jquery"; -import _, {isNumber} from "lodash"; +import _ from "lodash"; import assert from "minimalistic-assert"; import type * as tippy from "tippy.js"; @@ -21,7 +21,6 @@ import {$t} from "./i18n.ts"; import * as narrow_state from "./narrow_state.ts"; import {realm} from "./state_data.ts"; import * as stream_data from "./stream_data.ts"; -import * as sub_store from "./sub_store.ts"; import * as ui_util from "./ui_util.ts"; import * as user_groups from "./user_groups.ts"; import * as util from "./util.ts"; @@ -114,37 +113,17 @@ export function update_on_recipient_change(): void { compose_validate.warn_if_guest_in_dm_recipient(); drafts.update_compose_draft_count(); check_posting_policy_for_compose_box(); -} - -export function get_posting_policy_error_message(): string { - if (compose_state.selected_recipient_id === "direct") { - const recipients = compose_pm_pill.get_user_ids_string(); - return compose_validate.check_dm_permissions_and_get_error_string(recipients); - } - - if (!isNumber(compose_state.selected_recipient_id)) { - return ""; - } - - const stream = sub_store.get(compose_state.selected_recipient_id); - if (stream && !stream_data.can_post_messages_in_stream(stream)) { - return $t({ - defaultMessage: "You do not have permission to post in this channel.", - }); - } - return ""; + compose_validate.validate_and_update_send_button_status(); } export let check_posting_policy_for_compose_box = (): void => { - const banner_text = get_posting_policy_error_message(); + const banner_text = compose_validate.get_posting_policy_error_message(); if (banner_text === "") { - compose_validate.set_recipient_disallowed(false); compose_banner.clear_errors(); return; } let banner_classname = compose_banner.CLASSNAMES.no_post_permissions; - compose_validate.set_recipient_disallowed(true); if (compose_state.selected_recipient_id === "direct") { banner_classname = compose_banner.CLASSNAMES.cannot_send_direct_message; compose_banner.cannot_send_direct_message_error(banner_text); diff --git a/web/src/compose_setup.js b/web/src/compose_setup.js index 1fa42ed46b..e8cc8dd8cb 100644 --- a/web/src/compose_setup.js +++ b/web/src/compose_setup.js @@ -71,14 +71,6 @@ export function initialize() { compose_ui.handle_keyup(event, $("textarea#compose-textarea").expectOne()); }); - $("#compose-send-button").on("mouseenter", () => { - compose_validate.validate(false, false); - compose_validate.update_send_button_status(); - }); - $("#compose-send-button").on("mouseleave", () => { - $("#compose-send-button").removeClass("disabled-message-send-controls"); - }); - $("textarea#compose-textarea").on("input propertychange", () => { if ($("#compose").hasClass("preview_mode")) { compose.render_preview_area(); @@ -606,6 +598,7 @@ export function initialize() { const $input = $("input#stream_message_recipient_topic"); $input.val(""); $input.trigger("focus"); + compose_validate.validate_and_update_send_button_status(); }); $("input#stream_message_recipient_topic").on("focus", () => { diff --git a/web/src/compose_tooltips.ts b/web/src/compose_tooltips.ts index c90da15e86..707d2f826e 100644 --- a/web/src/compose_tooltips.ts +++ b/web/src/compose_tooltips.ts @@ -6,7 +6,7 @@ import * as tippy from "tippy.js"; import render_drafts_tooltip from "../templates/drafts_tooltip.hbs"; import render_narrow_to_compose_recipients_tooltip from "../templates/narrow_to_compose_recipients_tooltip.hbs"; -import * as compose_recipient from "./compose_recipient.ts"; +import * as blueslip from "./blueslip.ts"; import * as compose_state from "./compose_state.ts"; import * as compose_validate from "./compose_validate.ts"; import {$t} from "./i18n.ts"; @@ -209,18 +209,44 @@ export function initialize(): void { }); tippy.delegate("body", { - target: "#compose-send-button:not(.disabled-message-send-controls)", - delay: EXTRA_LONG_HOVER_DELAY, + target: "#compose-send-button", + // 350px at 14px/1em + maxWidth: "25em", // By default, tippyjs uses a trigger value of "mouseenter focus", // but by specifying "mouseenter", this will prevent showing the // Send tooltip when tabbing to the Send button. trigger: "mouseenter", appendTo: () => document.body, + onTrigger(instance) { + if (instance.reference.classList.contains("disabled-message-send-controls")) { + instance.setProps({ + delay: 0, + }); + } else { + instance.setProps({ + delay: EXTRA_LONG_HOVER_DELAY, + }); + } + }, onShow(instance) { // Don't show send-area tooltips if the popover is displayed. if (popover_menus.is_scheduled_messages_popover_displayed()) { return false; } + + if (instance.reference.classList.contains("disabled-message-send-controls")) { + const error_message = compose_validate.get_disabled_send_tooltip(); + instance.setContent(error_message); + + if (!error_message) { + blueslip.error("Compose send button incorrectly disabled."); + // We don't return but show normal tooltip to user. + instance.reference.classList.remove("disabled-message-send-controls"); + } else { + return undefined; + } + } + if (user_settings.enter_sends) { instance.setContent(parse_html($("#send-enter-tooltip-template").html())); } else { @@ -230,22 +256,6 @@ export function initialize(): void { }, }); - tippy.delegate("body", { - target: "#compose-send-button.disabled-message-send-controls", - // 350px at 14px/1em - maxWidth: "25em", - onShow(instance) { - instance.setContent( - compose_recipient.get_posting_policy_error_message() || - compose_validate.get_disabled_send_tooltip(), - ); - }, - appendTo: () => document.body, - onHidden(instance) { - instance.destroy(); - }, - }); - tippy.delegate("body", { target: ".narrow_to_compose_recipients", delay: LONG_HOVER_DELAY, diff --git a/web/src/compose_validate.ts b/web/src/compose_validate.ts index 9aacb2bedc..22663cbf50 100644 --- a/web/src/compose_validate.ts +++ b/web/src/compose_validate.ts @@ -1,5 +1,5 @@ import $ from "jquery"; -import _ from "lodash"; +import _, {isNumber} from "lodash"; import * as resolved_topic from "../shared/src/resolved_topic.ts"; import render_compose_banner from "../templates/compose_banner/compose_banner.hbs"; @@ -41,7 +41,6 @@ let missing_topic = false; let no_private_recipient = true; let no_message_content = false; let message_too_long = false; -let recipient_disallowed = false; export const NO_PRIVATE_RECIPIENT_ERROR_MESSAGE = $t({ defaultMessage: "Please add a valid recipient.", @@ -67,7 +66,7 @@ export let wildcard_mention_threshold = 15; export function set_upload_in_progress(status: boolean): void { upload_in_progress = status; - update_send_button_status(); + validate_and_update_send_button_status(); } function set_no_channel_selected(status: boolean): void { @@ -101,25 +100,31 @@ function set_message_too_long_for_edit(status: boolean, $container: JQuery): voi $message_edit_save_container.toggleClass("disabled-message-edit-save", save_is_disabled); } -export function set_recipient_disallowed(status: boolean): void { - recipient_disallowed = status; -} +export function get_posting_policy_error_message(): string { + if (compose_state.selected_recipient_id === "direct") { + const recipients = compose_pm_pill.get_user_ids_string(); + return check_dm_permissions_and_get_error_string(recipients); + } -export function update_send_button_status(): void { - const recipient_type = compose_state.get_message_type(); - $("#compose-send-button").toggleClass( - "disabled-message-send-controls", - upload_in_progress || - no_channel_selected || - (missing_topic && recipient_type === "stream") || - (no_private_recipient && recipient_type === "private") || - message_too_long || - recipient_disallowed || - no_message_content, - ); + if (!isNumber(compose_state.selected_recipient_id)) { + return ""; + } + + const stream = sub_store.get(compose_state.selected_recipient_id); + if (stream && !stream_data.can_post_messages_in_stream(stream)) { + return $t({ + defaultMessage: "You do not have permission to post in this channel.", + }); + } + return ""; } export function get_disabled_send_tooltip(): string { + const posting_policy_error = get_posting_policy_error_message(); + if (posting_policy_error !== "") { + return posting_policy_error; + } + const recipient_type = compose_state.get_message_type(); if (no_channel_selected && recipient_type === "stream") { return NO_CHANNEL_SELECTED_ERROR_MESSAGE; @@ -887,6 +892,8 @@ export function check_overflow_text($container: JQuery): number { const $indicator = $container.find(".message-limit-indicator"); const is_edit_container = $textarea.closest(".message_row").length > 0; + const old_message_too_long = message_too_long; + if (text.length > max_length) { $indicator.removeClass("textarea-approaching-limit"); $textarea.removeClass("textarea-approaching-limit"); @@ -928,10 +935,28 @@ export function check_overflow_text($container: JQuery): number { set_message_too_long_for_compose(false); } } - + if (!is_edit_container && message_too_long !== old_message_too_long) { + // If this keystroke changed the truth status for whether + // we're too long, then we need to refresh the send button + // status. This is expensive, but naturally debounced by the + // fact that crossing the MAX_MESSAGE_LENGTH threshold is + // rare. + validate_and_update_send_button_status(); + } return text.length; } +export let validate_and_update_send_button_status = function (): void { + const is_valid = validate(false, false); + const $send_button = $("#compose-send-button"); + $send_button.toggleClass("disabled-message-send-controls", !is_valid); +}; + +export function rewire_validate_and_update_send_button_status( + value: typeof validate_and_update_send_button_status, +): void { + validate_and_update_send_button_status = value; +} export function validate_message_length($container: JQuery, trigger_flash = true): boolean { const $textarea = $container.find(".message-textarea"); // Match the behavior of compose_state.message_content of trimming trailing whitespace @@ -968,7 +993,8 @@ function report_validation_error( compose_banner.show_error_message(message, classname, $container, $bad_input); } } -export function validate(scheduling_message: boolean, show_banner = true): boolean { + +export let validate = (scheduling_message: boolean, show_banner = true): boolean => { const message_content = compose_state.message_content(); // The validation checks in this function are in a specific priority order. Don't // change their order unless you want to change which priority they're shown in. @@ -1016,6 +1042,10 @@ export function validate(scheduling_message: boolean, show_banner = true): boole } return true; +}; + +export function rewire_validate(value: typeof validate): void { + validate = value; } export function convert_mentions_to_silent_in_direct_messages( diff --git a/web/tests/compose.test.cjs b/web/tests/compose.test.cjs index c13ce21139..c033e3d233 100644 --- a/web/tests/compose.test.cjs +++ b/web/tests/compose.test.cjs @@ -66,6 +66,7 @@ const echo = zrequire("echo"); const people = zrequire("people"); const {set_current_user, set_realm} = zrequire("state_data"); const stream_data = zrequire("stream_data"); +const compose_validate = zrequire("compose_validate"); const {initialize_user_settings} = zrequire("user_settings"); const realm = {}; @@ -306,6 +307,7 @@ test_ui("send_message", ({override, override_rewire, mock_template}) => { const server_message_id = 127; override(markdown, "render", noop); + override_rewire(compose_validate, "validate_and_update_send_button_status", noop); override_rewire(echo, "try_deliver_locally", (message_request) => { const local_id_float = 123.04; return echo.insert_local_message(message_request, local_id_float, (messages) => { @@ -637,6 +639,7 @@ test_ui("update_fade", ({override, override_rewire}) => { override_rewire(compose_recipient, "update_narrow_to_recipient_visibility", () => { update_narrow_to_recipient_visibility_called = true; }); + override_rewire(compose_validate, "validate_and_update_send_button_status", noop); override_rewire(drafts, "update_compose_draft_count", noop); override(compose_pm_pill, "get_user_ids", () => []); diff --git a/web/tests/compose_actions.test.cjs b/web/tests/compose_actions.test.cjs index ff91105d43..1cc334d0ef 100644 --- a/web/tests/compose_actions.test.cjs +++ b/web/tests/compose_actions.test.cjs @@ -8,6 +8,7 @@ const {run_test, noop} = require("./lib/test.cjs"); const blueslip = require("./lib/zblueslip.cjs"); const $ = require("./lib/zjquery.cjs"); +const compose_validate = zrequire("compose_validate"); const user_groups = zrequire("user_groups"); const nobody = { @@ -153,6 +154,7 @@ test("start", ({override, override_rewire, mock_template}) => { override_rewire(compose_actions, "complete_starting_tasks", noop); override_rewire(compose_actions, "blur_compose_inputs", noop); override_rewire(compose_actions, "clear_textarea", noop); + override_rewire(compose_validate, "validate_and_update_send_button_status", noop); const $elem = $("#send_message_form"); const $textarea = $("textarea#compose-textarea"); const $indicator = $("#compose-limit-indicator"); @@ -302,6 +304,7 @@ test("respond_to_message", ({override, override_rewire, mock_template}) => { mock_banners(); override_rewire(compose_actions, "complete_starting_tasks", noop); override_rewire(compose_actions, "clear_textarea", noop); + override_rewire(compose_validate, "validate_and_update_send_button_status", noop); const $elem = $("#send_message_form"); const $textarea = $("textarea#compose-textarea"); const $indicator = $("#compose-limit-indicator"); @@ -446,6 +449,7 @@ test("quote_message", ({disallow, override, override_rewire}) => { override_rewire(compose_actions, "complete_starting_tasks", noop); override_rewire(compose_actions, "clear_textarea", noop); + override_rewire(compose_validate, "validate_and_update_send_button_status", noop); override_private_message_recipient({override}); let selected_message; diff --git a/web/tests/compose_validate.test.cjs b/web/tests/compose_validate.test.cjs index afe9373c50..6d48de4cf2 100644 --- a/web/tests/compose_validate.test.cjs +++ b/web/tests/compose_validate.test.cjs @@ -531,9 +531,10 @@ test_ui("test_stream_posting_permission", ({mock_template, override}) => { assert.ok(!banner_rendered); }); -test_ui("test_check_overflow_text", ({override}) => { +test_ui("test_check_overflow_text", ({override, override_rewire}) => { const fake_compose_box = new FakeComposeBox(); + override_rewire(compose_validate, "validate_and_update_send_button_status", noop); override(realm, "max_message_length", 10000); // RED diff --git a/web/tests/upload.test.cjs b/web/tests/upload.test.cjs index f765ba98cc..2c8fd6b783 100644 --- a/web/tests/upload.test.cjs +++ b/web/tests/upload.test.cjs @@ -34,6 +34,7 @@ const compose_ui = zrequire("compose_ui"); const upload = zrequire("upload"); const message_lists = mock_esm("../src/message_lists"); const {set_realm} = zrequire("state_data"); +const compose_validate = zrequire("compose_validate"); const realm = {}; set_realm(realm); @@ -248,7 +249,7 @@ test("upload_files", async ({mock_template, override, override_rewire}) => { banner_shown = true; return ""; }); - override(compose_state, "get_message_type", () => "stream"); + override_rewire(compose_validate, "validate", () => false); await upload.upload_files(uppy, config, files); assert.ok($("#compose-send-button").hasClass("disabled-message-send-controls")); assert.ok(banner_shown); @@ -456,10 +457,11 @@ test("copy_paste", ({override, override_rewire}) => { assert.equal(upload_files_called, false); }); -test("uppy_events", ({override, override_rewire, mock_template}) => { +test("uppy_events", ({override_rewire, mock_template}) => { $("#compose_banners .upload_banner .moving_bar").css = noop; $("#compose_banners .upload_banner").length = 0; override_rewire(compose_ui, "smart_insert_inline", noop); + override_rewire(compose_validate, "validate_and_update_send_button_status", noop); const callbacks = {}; let state = {}; @@ -518,7 +520,6 @@ test("uppy_events", ({override, override_rewire, mock_template}) => { override_rewire(compose_ui, "autosize_textarea", () => { compose_ui_autosize_textarea_called = true; }); - override(compose_state, "get_message_type", () => "stream"); on_upload_success_callback(file, response); assert.ok(compose_ui_replace_syntax_called);