From 367030ebfe1e5809901495eee494047144559c5a Mon Sep 17 00:00:00 2001 From: evykassirer Date: Tue, 20 Jun 2023 18:42:08 -0700 Subject: [PATCH] search: Stop disabling and enabling the search close button. This is logic from 10 years ago (dbc4798594c4aa859013e46fcbe94977fd43cdf4) that is no longer relevant. It seems like we used to show the search bar open all the time and only showed the buttons when there was focus in the search bar. Now we close the search bar when it's not being used, and no longer need to update button visibility or disable the search close button. --- web/src/hashchange.js | 2 -- web/src/narrow.js | 2 -- web/src/search.js | 48 ----------------------------- web/styles/search.css | 9 ++---- web/tests/hashchange.test.js | 3 -- web/tests/narrow_activate.test.js | 3 -- web/tests/search.test.js | 50 +------------------------------ 7 files changed, 3 insertions(+), 114 deletions(-) diff --git a/web/src/hashchange.js b/web/src/hashchange.js index 71aa91b114..4c8b4d6b8f 100644 --- a/web/src/hashchange.js +++ b/web/src/hashchange.js @@ -18,7 +18,6 @@ import * as popovers from "./popovers"; import * as recent_topics_ui from "./recent_topics_ui"; import * as recent_topics_util from "./recent_topics_util"; import * as scheduled_messages_overlay_ui from "./scheduled_messages_overlay_ui"; -import * as search from "./search"; import * as settings from "./settings"; import * as settings_panel_menu from "./settings_panel_menu"; import * as settings_toggle from "./settings_toggle"; @@ -112,7 +111,6 @@ function show_all_message_view() { const coming_from_recent_topics = maybe_hide_recent_topics(); narrow.deactivate(coming_from_recent_topics); top_left_corner.handle_narrow_deactivated(); - search.update_button_visibility(); // We need to maybe scroll to the selected message // once we have the proper viewport set up setTimeout(navigate.maybe_scroll_to_selected, 0); diff --git a/web/src/narrow.js b/web/src/narrow.js index 333058b153..783a3742c7 100644 --- a/web/src/narrow.js +++ b/web/src/narrow.js @@ -539,8 +539,6 @@ export function activate(raw_operators, opts) { } compose_closed_ui.update_reply_recipient_label(); - search.update_button_visibility(); - compose_actions.on_narrow(opts); const current_filter = narrow_state.filter(); diff --git a/web/src/search.js b/web/src/search.js index 488c792fe6..8a0ad03cfe 100644 --- a/web/src/search.js +++ b/web/src/search.js @@ -6,7 +6,6 @@ import {Filter} from "./filter"; import * as keydown_util from "./keydown_util"; import * as message_view_header from "./message_view_header"; import * as narrow from "./narrow"; -import * as narrow_state from "./narrow_state"; import * as search_suggestion from "./search_suggestion"; // Exported for unit testing @@ -34,20 +33,6 @@ export function narrow_or_search_for_term(search_string) { return $search_query_box.val(); } -function update_buttons_with_focus(focused) { - const $search_query_box = $("#search_query"); - - // Show buttons iff the search input is focused, or has non-empty contents, - // or we are narrowed. - if (focused || $search_query_box.val() || narrow_state.active()) { - $(".search_close_button").prop("disabled", false); - } -} - -export function update_button_visibility() { - update_buttons_with_focus($("#search_query").is(":focus")); -} - export function initialize() { const $search_query_box = $("#search_query"); const $searchbox_form = $("#searchbox_form"); @@ -102,7 +87,6 @@ export function initialize() { $searchbox_form .on("keydown", (e) => { - update_button_visibility(); if (keydown_util.is_enter_event(e) && $search_query_box.is(":focus")) { // Don't submit the form so that the typeahead can instead // handle our Enter keypress. Any searching that needs @@ -127,39 +111,8 @@ export function initialize() { // Pill is already added during keydown event of input pills. narrow_or_search_for_term($search_query_box.val()); $search_query_box.trigger("blur"); - update_buttons_with_focus(false); } }); - - // Some of these functions don't actually need to be exported, - // but the code was moved here from elsewhere, and it would be - // more work to re-order everything and make them private. - - $search_query_box.on("focus", focus_search); - $search_query_box.on("blur", () => { - // The search query box is a visual cue as to - // whether search or narrowing is active. If - // the user blurs the search box, then we should - // update the search string to reflect the current - // narrow (or lack of narrow). - // - // But we can't do this right away, because - // selecting something in the typeahead menu causes - // the box to lose focus a moment before. - // - // The workaround is to check 100ms later -- long - // enough for the search to have gone through, but - // short enough that the user won't notice (though - // really it would be OK if they did). - setTimeout(() => { - update_button_visibility(); - }, 100); - }); -} - -export function focus_search() { - // The search bar is not focused yet, but will be. - update_buttons_with_focus(true); } export function initiate_search() { @@ -170,5 +123,4 @@ export function initiate_search() { export function clear_search_form() { $("#search_query").val(""); $("#search_query").trigger("blur"); - $(".search_close_button").prop("disabled", true); } diff --git a/web/styles/search.css b/web/styles/search.css index 2296c28a70..c1a8651e77 100644 --- a/web/styles/search.css +++ b/web/styles/search.css @@ -87,10 +87,9 @@ box-shadow: inset 0 0 0 2px hsl(204deg 20% 74%); } - .search_close_button, - .search_close_button:disabled:hover { + .search_close_button { position: absolute; - right: 35px; + right: 0; top: 6px; background: none; border-radius: 0; @@ -104,10 +103,6 @@ box-shadow: none; text-shadow: none; z-index: 5; - } - - .search_close_button { - right: 0; &:hover { opacity: 1; diff --git a/web/tests/hashchange.test.js b/web/tests/hashchange.test.js index db992dfc30..991a9fcbcc 100644 --- a/web/tests/hashchange.test.js +++ b/web/tests/hashchange.test.js @@ -11,9 +11,6 @@ const {user_settings} = require("./lib/zpage_params"); let $window_stub; set_global("to_$", () => $window_stub); -mock_esm("../src/search", { - update_button_visibility() {}, -}); set_global("document", "document-stub"); const history = set_global("history", {}); diff --git a/web/tests/narrow_activate.test.js b/web/tests/narrow_activate.test.js index add322e10c..d3e3fe1384 100644 --- a/web/tests/narrow_activate.test.js +++ b/web/tests/narrow_activate.test.js @@ -28,7 +28,6 @@ const message_feed_top_notices = mock_esm("../src/message_feed_top_notices"); const message_feed_loading = mock_esm("../src/message_feed_loading"); const message_view_header = mock_esm("../src/message_view_header"); const notifications = mock_esm("../src/notifications"); -const search = mock_esm("../src/search"); const stream_list = mock_esm("../src/stream_list"); const top_left_corner = mock_esm("../src/top_left_corner"); const typing_events = mock_esm("../src/typing_events"); @@ -88,7 +87,6 @@ function test_helper() { stub(message_feed_loading, "show_loading_older"); stub(message_feed_top_notices, "hide_top_of_narrow_notices"); stub(notifications, "redraw_title"); - stub(search, "update_button_visibility"); stub(stream_list, "handle_narrow_activated"); stub(message_view_header, "render_title_area"); stub(top_left_corner, "handle_narrow_activated"); @@ -193,7 +191,6 @@ run_test("basics", () => { [hashchange, "save_narrow"], [compose_closed_ui, "update_buttons_for_stream"], [compose_closed_ui, "update_reply_recipient_label"], - [search, "update_button_visibility"], [compose_actions, "on_narrow"], [top_left_corner, "handle_narrow_activated"], [stream_list, "handle_narrow_activated"], diff --git a/web/tests/search.test.js b/web/tests/search.test.js index b03ca6654a..c94e5cf7f5 100644 --- a/web/tests/search.test.js +++ b/web/tests/search.test.js @@ -2,7 +2,7 @@ const {strict: assert} = require("assert"); -const {mock_esm, set_global, zrequire} = require("./lib/namespace"); +const {mock_esm, zrequire} = require("./lib/namespace"); const {run_test} = require("./lib/test"); const $ = require("./lib/zjquery"); @@ -16,59 +16,21 @@ mock_esm("../src/filter", { Filter, }); -set_global("setTimeout", (func) => func()); - const search = zrequire("search"); run_test("clear_search_form", () => { $("#search_query").val("noise"); $("#search_query").trigger("focus"); - $(".search_close_button").prop("disabled", false); search.clear_search_form(); assert.equal($("#search_query").is_focused(), false); assert.equal($("#search_query").val(), ""); - assert.equal($(".search_close_button").prop("disabled"), true); -}); - -run_test("update_button_visibility", () => { - const $search_query = $("#search_query"); - const $search_button = $(".search_close_button"); - - $search_query.is = () => false; - $search_query.val(""); - narrow_state.active = () => false; - $search_button.prop("disabled", true); - search.update_button_visibility(); - assert.ok($search_button.prop("disabled")); - - $search_query.is = () => true; - $search_query.val(""); - delete narrow_state.active; - $search_button.prop("disabled", true); - search.update_button_visibility(); - assert.ok(!$search_button.prop("disabled")); - - $search_query.is = () => false; - $search_query.val("Test search term"); - delete narrow_state.active; - $search_button.prop("disabled", true); - search.update_button_visibility(); - assert.ok(!$search_button.prop("disabled")); - - $search_query.is = () => false; - $search_query.val(""); - narrow_state.active = () => true; - $search_button.prop("disabled", true); - search.update_button_visibility(); - assert.ok(!$search_button.prop("disabled")); }); run_test("initialize", ({mock_template}) => { const $search_query_box = $("#search_query"); const $searchbox_form = $("#searchbox_form"); - const $search_button = $(".search_close_button"); mock_template("search_list_item.hbs", true, (data, html) => { assert.equal(typeof data.description_html, "string"); @@ -261,10 +223,6 @@ run_test("initialize", ({mock_template}) => { search.initialize(); - $search_button.prop("disabled", true); - $search_query_box.trigger("focus"); - assert.ok(!$search_button.prop("disabled")); - $search_query_box.val("test string"); narrow_state.search_string = () => "ver"; $search_query_box.trigger("blur"); @@ -301,7 +259,6 @@ run_test("initialize", ({mock_template}) => { }; let operators; let is_blurred; - narrow_state.active = () => false; $search_query_box.off("blur"); $search_query_box.on("blur", () => { is_blurred = true; @@ -309,7 +266,6 @@ run_test("initialize", ({mock_template}) => { const _setup = (search_box_val) => { is_blurred = false; - $search_button.prop("disabled", false); $search_query_box.val(search_box_val); Filter.parse = (search_string) => { assert.equal(search_string, search_box_val); @@ -336,14 +292,12 @@ run_test("initialize", ({mock_template}) => { $searchbox_form.trigger(ev); assert.ok(!is_blurred); - assert.ok(!$search_button.prop("disabled")); ev.key = "Enter"; $search_query_box.is = () => false; $searchbox_form.trigger(ev); assert.ok(!is_blurred); - assert.ok(!$search_button.prop("disabled")); ev.key = "Enter"; $search_query_box.is = () => true; @@ -355,14 +309,12 @@ run_test("initialize", ({mock_template}) => { $searchbox_form.trigger(ev); // No change on Enter keyup event when using input tool assert.ok(!is_blurred); - assert.ok(!$search_button.prop("disabled")); _setup("ver"); ev.key = "Enter"; $search_query_box.is = () => true; $searchbox_form.trigger(ev); assert.ok(is_blurred); - assert.ok(!$search_button.prop("disabled")); }); run_test("initiate_search", () => {