From 832ed8ce748982311e5ed654a9110fc608a03bf8 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Fri, 21 Mar 2025 09:20:58 +1000
Subject: [PATCH] UX: Fix various search shortcut UX issues (#31903)

Now we have the search input showing in a few
different configurations:

* Welcome banner
* Header field
* Header icon

And we can get to the search with both `/` and
`Ctrl+F` shortcuts. These configurations can
be used together, and we need to focus on the right
search input at the right time.

This commit fixes the shortcuts not working
or showing the wrong thing in some cases,
and adds a comprehensive system spec for all
the variants.
---
 .../discourse/app/components/header.gjs       |   7 +-
 .../app/components/header/header-search.gjs   |  11 +-
 .../discourse/app/components/search-menu.gjs  |   9 +-
 .../components/search-menu/search-term.gjs    |   3 +-
 .../app/components/welcome-banner.gjs         |  33 +++-
 .../discourse/app/services/search.js          |   1 +
 .../stylesheets/common/base/search-menu.scss  |   3 +-
 .../common/components/welcome-banner.scss     |   3 +-
 .../desktop/components/header-search.scss     |   6 +-
 .../mobile/components/welcome-banner.scss     |   4 +
 spec/support/system_helpers.rb                |  15 ++
 spec/system/header_spec.rb                    |  13 --
 spec/system/page_objects/pages/header.rb      |   6 +-
 spec/system/page_objects/pages/search.rb      |  15 +-
 spec/system/search_shortcut_variation_spec.rb | 160 ++++++++++++++++++
 spec/system/welcome_banner_spec.rb            |   4 +-
 16 files changed, 253 insertions(+), 40 deletions(-)
 create mode 100644 spec/system/search_shortcut_variation_spec.rb

diff --git a/app/assets/javascripts/discourse/app/components/header.gjs b/app/assets/javascripts/discourse/app/components/header.gjs
index 076cdf36c01..d69a899ce1e 100644
--- a/app/assets/javascripts/discourse/app/components/header.gjs
+++ b/app/assets/javascripts/discourse/app/components/header.gjs
@@ -129,7 +129,12 @@ export default class GlimmerHeader extends Component {
   headerKeyboardTrigger(msg) {
     switch (msg.type) {
       case "search":
-        this.toggleSearchMenu();
+        // This must be done here because toggleSearchMenu is
+        // also called from the search button, we only want to
+        // stop it using the shortcut.
+        if (!this.search.welcomeBannerSearchInViewport) {
+          this.toggleSearchMenu();
+        }
         break;
       case "user":
         this.toggleUserMenu();
diff --git a/app/assets/javascripts/discourse/app/components/header/header-search.gjs b/app/assets/javascripts/discourse/app/components/header/header-search.gjs
index f8e8a2962f7..8f7c35baa7d 100644
--- a/app/assets/javascripts/discourse/app/components/header/header-search.gjs
+++ b/app/assets/javascripts/discourse/app/components/header/header-search.gjs
@@ -1,6 +1,6 @@
 import Component from "@glimmer/component";
 import { service } from "@ember/service";
-import { modifier as modifierFn } from "ember-modifier";
+import { modifier } from "ember-modifier";
 import DButton from "discourse/components/d-button";
 import SearchMenu, { focusSearchInput } from "discourse/components/search-menu";
 import bodyClass from "discourse/helpers/body-class";
@@ -14,8 +14,13 @@ export default class HeaderSearch extends Component {
 
   advancedSearchButtonHref = "/search?expanded=true";
 
-  handleKeyboardShortcut = modifierFn(() => {
-    const cb = () => focusSearchInput();
+  handleKeyboardShortcut = modifier(() => {
+    const cb = (appEvent) => {
+      if (appEvent.type === "search" || appEvent.type === "page-search") {
+        focusSearchInput();
+        appEvent.event.preventDefault();
+      }
+    };
     this.appEvents.on("header:keyboard-trigger", cb);
     return () => this.appEvents.off("header:keyboard-trigger", cb);
   });
diff --git a/app/assets/javascripts/discourse/app/components/search-menu.gjs b/app/assets/javascripts/discourse/app/components/search-menu.gjs
index 5a1f1faf15c..b23991015b1 100644
--- a/app/assets/javascripts/discourse/app/components/search-menu.gjs
+++ b/app/assets/javascripts/discourse/app/components/search-menu.gjs
@@ -37,8 +37,8 @@ export const SEARCH_INPUT_ID = "search-term";
 export const MODIFIER_REGEXP = /.*(\#|\@|:).*$/gi;
 export const DEFAULT_TYPE_FILTER = "exclude_topics";
 
-export function focusSearchInput() {
-  document.getElementById(SEARCH_INPUT_ID).focus();
+export function focusSearchInput(inputId = SEARCH_INPUT_ID) {
+  document.getElementById(inputId).focus();
 }
 
 export default class SearchMenu extends Component {
@@ -121,6 +121,8 @@ export default class SearchMenu extends Component {
   onKeydown(event) {
     if (event.key === "Escape") {
       this.close();
+      event.preventDefault();
+      event.stopPropagation();
     }
   }
 
@@ -132,7 +134,7 @@ export default class SearchMenu extends Component {
 
     // We want to blur the search input when in stand-alone mode
     // so that when we focus on the search input again, the menu panel pops up
-    document.getElementById(SEARCH_INPUT_ID)?.blur();
+    document.getElementById(this.args.searchInputId || SEARCH_INPUT_ID)?.blur();
     this.menuPanelOpen = false;
   }
 
@@ -430,6 +432,7 @@ export default class SearchMenu extends Component {
           @closeSearchMenu={{this.close}}
           @openSearchMenu={{this.open}}
           @autofocus={{@autofocusInput}}
+          @inputId={{@searchInputId}}
         />
 
         {{#if this.loading}}
diff --git a/app/assets/javascripts/discourse/app/components/search-menu/search-term.gjs b/app/assets/javascripts/discourse/app/components/search-menu/search-term.gjs
index bd6eb1f362e..53d3b1c9178 100644
--- a/app/assets/javascripts/discourse/app/components/search-menu/search-term.gjs
+++ b/app/assets/javascripts/discourse/app/components/search-menu/search-term.gjs
@@ -31,7 +31,7 @@ export default class SearchTerm extends Component {
 
   // make constant available in template
   get inputId() {
-    return SEARCH_INPUT_ID;
+    return this.args.inputId || SEARCH_INPUT_ID;
   }
 
   @action
@@ -121,6 +121,7 @@ export default class SearchTerm extends Component {
   <template>
     <input
       id={{this.inputId}}
+      class="search-term__input"
       type="search"
       autocomplete="off"
       enterkeyhint="search"
diff --git a/app/assets/javascripts/discourse/app/components/welcome-banner.gjs b/app/assets/javascripts/discourse/app/components/welcome-banner.gjs
index 4c4be76efd7..2e86ab500a0 100644
--- a/app/assets/javascripts/discourse/app/components/welcome-banner.gjs
+++ b/app/assets/javascripts/discourse/app/components/welcome-banner.gjs
@@ -5,7 +5,7 @@ import { htmlSafe } from "@ember/template";
 import { modifier } from "ember-modifier";
 import DButton from "discourse/components/d-button";
 import PluginOutlet from "discourse/components/plugin-outlet";
-import SearchMenu from "discourse/components/search-menu";
+import SearchMenu, { focusSearchInput } from "discourse/components/search-menu";
 import bodyClass from "discourse/helpers/body-class";
 import { prioritizeNameFallback } from "discourse/lib/settings";
 import { applyValueTransformer } from "discourse/lib/transformer";
@@ -15,13 +15,15 @@ export default class WelcomeBanner extends Component {
   @service router;
   @service siteSettings;
   @service currentUser;
+  @service appEvents;
+  @service search;
 
   @tracked inViewport = true;
 
   checkViewport = modifier((element) => {
     const observer = new IntersectionObserver(
       ([entry]) => {
-        this.inViewport = entry.isIntersecting;
+        this.search.welcomeBannerSearchInViewport = entry.isIntersecting;
       },
       { threshold: 1.0 }
     );
@@ -31,6 +33,20 @@ export default class WelcomeBanner extends Component {
     return () => observer.disconnect();
   });
 
+  handleKeyboardShortcut = modifier(() => {
+    const cb = (appEvent) => {
+      if (
+        (appEvent.type === "search" || appEvent.type === "page-search") &&
+        this.search.welcomeBannerSearchInViewport
+      ) {
+        focusSearchInput("welcome-banner-search-term");
+        appEvent.event.preventDefault();
+      }
+    };
+    this.appEvents.on("header:keyboard-trigger", cb);
+    return () => this.appEvents.off("header:keyboard-trigger", cb);
+  });
+
   get displayForRoute() {
     return this.siteSettings.top_menu
       .split("|")
@@ -69,11 +85,15 @@ export default class WelcomeBanner extends Component {
 
   <template>
     {{#if this.shouldDisplay}}
-      {{#if this.inViewport}}
+      {{#if this.search.welcomeBannerSearchInViewport}}
         {{bodyClass "welcome-banner--visible"}}
       {{/if}}
 
-      <div class="welcome-banner" {{this.checkViewport}}>
+      <div
+        class="welcome-banner"
+        {{this.checkViewport}}
+        {{this.handleKeyboardShortcut}}
+      >
         <div class="custom-search-banner welcome-banner__inner-wrapper">
           <div class="custom-search-banner-wrap welcome-banner__wrap">
             <h1 class="welcome-banner__title">{{htmlSafe this.headerText}}</h1>
@@ -85,7 +105,10 @@ export default class WelcomeBanner extends Component {
                 @href="/search?expanded=true"
                 class="search-icon"
               />
-              <SearchMenu @location="welcome-banner" />
+              <SearchMenu
+                @location="welcome-banner"
+                @searchInputId="welcome-banner-search-term"
+              />
             </div>
             <PluginOutlet @name="welcome-banner-below-input" />
           </div>
diff --git a/app/assets/javascripts/discourse/app/services/search.js b/app/assets/javascripts/discourse/app/services/search.js
index 5a236a847ba..99ebd13b633 100644
--- a/app/assets/javascripts/discourse/app/services/search.js
+++ b/app/assets/javascripts/discourse/app/services/search.js
@@ -15,6 +15,7 @@ export default class Search extends Service {
   @tracked visible = false;
   @tracked results = {};
   @tracked noResults = false;
+  @tracked welcomeBannerSearchInViewport = false;
 
   // only relative for the widget search menu
   searchContextEnabled = false; // checkbox to scope search
diff --git a/app/assets/stylesheets/common/base/search-menu.scss b/app/assets/stylesheets/common/base/search-menu.scss
index e0de058fe86..9c0f614955e 100644
--- a/app/assets/stylesheets/common/base/search-menu.scss
+++ b/app/assets/stylesheets/common/base/search-menu.scss
@@ -30,7 +30,8 @@ $search-pad-horizontal: 0.5em;
     margin: 1px;
     padding: 0.25rem;
 
-    input#search-term {
+    input#search-term,
+    input.search-term__input {
       background: none;
       border: 0;
       margin-bottom: 0;
diff --git a/app/assets/stylesheets/common/components/welcome-banner.scss b/app/assets/stylesheets/common/components/welcome-banner.scss
index 38c60f43012..7d8416eadd1 100644
--- a/app/assets/stylesheets/common/components/welcome-banner.scss
+++ b/app/assets/stylesheets/common/components/welcome-banner.scss
@@ -59,7 +59,8 @@
     }
 
     .search-input {
-      #search-term {
+      #search-term,
+      .search-term__input {
         min-width: 0;
         flex: 1 1;
       }
diff --git a/app/assets/stylesheets/desktop/components/header-search.scss b/app/assets/stylesheets/desktop/components/header-search.scss
index 9b71f8438d4..10db1fe3645 100644
--- a/app/assets/stylesheets/desktop/components/header-search.scss
+++ b/app/assets/stylesheets/desktop/components/header-search.scss
@@ -59,7 +59,8 @@
             display: none;
           }
 
-          .search-input .search-context ~ #search-term {
+          .search-input .search-context ~ #search-term,
+          .search-input .search-context ~ .search-term__input {
             padding-left: 0.5em;
           }
 
@@ -98,7 +99,8 @@
             padding: 0 0 0 1.5em;
             border-radius: var(--d-input-border-radius);
 
-            #search-term {
+            #search-term,
+            .search-term__input {
               height: 100%;
               width: 100%;
               margin: 0;
diff --git a/app/assets/stylesheets/mobile/components/welcome-banner.scss b/app/assets/stylesheets/mobile/components/welcome-banner.scss
index 3ff1deb4cdb..96289dd60ee 100644
--- a/app/assets/stylesheets/mobile/components/welcome-banner.scss
+++ b/app/assets/stylesheets/mobile/components/welcome-banner.scss
@@ -6,4 +6,8 @@
   .search-menu .search-input input#search-term {
     width: 100%;
   }
+
+  .search-menu .search-input input.search-term__input {
+    width: 100%;
+  }
 }
diff --git a/spec/support/system_helpers.rb b/spec/support/system_helpers.rb
index 7ba73ee189d..5641d213010 100644
--- a/spec/support/system_helpers.rb
+++ b/spec/support/system_helpers.rb
@@ -168,6 +168,21 @@ module SystemHelpers
     page.execute_script(js, selector, start, offset)
   end
 
+  def current_active_element
+    {
+      classes: page.evaluate_script("document.activeElement.className"),
+      id: page.evaluate_script("document.activeElement.id"),
+    }
+  end
+
+  def fake_scroll_down_long(selector_to_make_tall = "#main-outlet")
+    # Trick to give a huge vertical space to scroll
+    page.execute_script(
+      "document.querySelector('#{selector_to_make_tall}').style.height = '10000px'",
+    )
+    page.scroll_to(0, 1000)
+  end
+
   def setup_or_skip_s3_system_test(enable_secure_uploads: false, enable_direct_s3_uploads: true)
     skip_unless_s3_system_specs_enabled!
 
diff --git a/spec/system/header_spec.rb b/spec/system/header_spec.rb
index e0249e2454f..bb46bd67cdd 100644
--- a/spec/system/header_spec.rb
+++ b/spec/system/header_spec.rb
@@ -234,19 +234,6 @@ RSpec.describe "Glimmer Header", type: :system do
     end
   end
 
-  context "when cmd + f keyboard shortcut pressed - when within a topic with 20+ posts" do
-    before { sign_in(current_user) }
-    fab!(:posts) { Fabricate.times(21, :post, topic: topic) }
-
-    it "opens search on first press, and closes on the second" do
-      visit "/t/#{topic.slug}/#{topic.id}"
-      header.search_in_topic_keyboard_shortcut
-      expect(search).to have_search_menu_visible
-      header.search_in_topic_keyboard_shortcut
-      expect(search).to have_no_search_menu_visible
-    end
-  end
-
   describe "mobile topic-info" do
     fab!(:topic)
     fab!(:posts) { Fabricate.times(21, :post, topic: topic) }
diff --git a/spec/system/page_objects/pages/header.rb b/spec/system/page_objects/pages/header.rb
index 3e4252f3ed3..9d6d9520ea2 100644
--- a/spec/system/page_objects/pages/header.rb
+++ b/spec/system/page_objects/pages/header.rb
@@ -14,16 +14,12 @@ module PageObjects
       end
 
       def active_element_id
-        page.evaluate_script("document.activeElement.id")
+        current_active_element[:id]
       end
 
       def click_outside
         find(".d-modal").click(x: 0, y: 0)
       end
-
-      def search_in_topic_keyboard_shortcut
-        page.send_keys([PLATFORM_KEY_MODIFIER, "f"])
-      end
     end
   end
 end
diff --git a/spec/system/page_objects/pages/search.rb b/spec/system/page_objects/pages/search.rb
index 1c0ea2ba8bf..2a492b1c1a4 100644
--- a/spec/system/page_objects/pages/search.rb
+++ b/spec/system/page_objects/pages/search.rb
@@ -43,11 +43,18 @@ module PageObjects
       end
 
       def has_search_menu_visible?
-        page.has_selector?(".search-menu .search-menu-panel", visible: true)
+        page.has_css?(".search-menu .search-menu-panel", visible: true)
+      end
+
+      # This is used for cases like header and welcome banner search,
+      # where we show the search results with a quick tip, but the panel
+      # itself is not technically "visible" in CSS terms.
+      def has_search_menu?
+        page.has_css?(".search-menu .search-menu-panel", visible: false)
       end
 
       def has_no_search_menu_visible?
-        page.has_no_selector?(".search-menu .search-menu-panel")
+        page.has_no_css?(".search-menu .search-menu-panel")
       end
 
       SEARCH_ICON_SELECTOR = "#search-button.btn-icon"
@@ -103,6 +110,10 @@ module PageObjects
       def not_active?
         has_no_css?(SEARCH_PAGE_SELECTOR)
       end
+
+      def browser_search_shortcut
+        page.send_keys([PLATFORM_KEY_MODIFIER, "f"])
+      end
     end
   end
 end
diff --git a/spec/system/search_shortcut_variation_spec.rb b/spec/system/search_shortcut_variation_spec.rb
new file mode 100644
index 00000000000..7c706063a43
--- /dev/null
+++ b/spec/system/search_shortcut_variation_spec.rb
@@ -0,0 +1,160 @@
+# frozen_string_literal: true
+
+describe "Search | Shortcuts for variations of search input", type: :system do
+  fab!(:current_user) { Fabricate(:user) }
+
+  let(:welcome_banner) { PageObjects::Components::WelcomeBanner.new }
+  let(:search_page) { PageObjects::Pages::Search.new }
+
+  before { sign_in(current_user) }
+
+  context "when search_experience is search_field" do
+    before { SiteSetting.search_experience = "search_field" }
+
+    context "when enable_welcome_banner is true" do
+      before { SiteSetting.enable_welcome_banner = true }
+
+      it "displays and focuses welcome banner search when / is pressed and hides it when Escape is pressed" do
+        visit("/")
+        expect(welcome_banner).to be_visible
+        page.send_keys("/")
+        expect(search_page).to have_search_menu
+        expect(current_active_element[:id]).to eq("welcome-banner-search-term")
+        page.send_keys(:escape)
+        expect(search_page).to have_no_search_menu_visible
+      end
+
+      it "displays and focuses welcome banner search when Ctrl+F is pressed and hides it when Escape is pressed" do
+        visit("/")
+        expect(welcome_banner).to be_visible
+        search_page.browser_search_shortcut
+        expect(search_page).to have_search_menu
+        expect(current_active_element[:id]).to eq("welcome-banner-search-term")
+        page.send_keys(:escape)
+        expect(search_page).to have_no_search_menu_visible
+      end
+
+      context "when welcome banner is not in the viewport" do
+        before do
+          visit("/")
+          fake_scroll_down_long
+        end
+
+        it "displays and focuses header search when / is pressed and hides it when Escape is pressed" do
+          expect(welcome_banner).to be_invisible
+          page.send_keys("/")
+          expect(search_page).to have_search_menu
+          expect(current_active_element[:id]).to eq("search-term")
+          page.send_keys(:escape)
+          expect(search_page).to have_no_search_menu_visible
+        end
+
+        it "displays and focuses header search when Ctrl+F is pressed and hides it when Escape is pressed" do
+          expect(welcome_banner).to be_invisible
+          search_page.browser_search_shortcut
+          expect(search_page).to have_search_menu
+          expect(current_active_element[:id]).to eq("search-term")
+          page.send_keys(:escape)
+          expect(search_page).to have_no_search_menu_visible
+        end
+      end
+    end
+
+    context "when enable_welcome_banner is false" do
+      before { SiteSetting.enable_welcome_banner = false }
+
+      it "displays and focuses header search when / is pressed and hides it when Escape is pressed" do
+        visit("/")
+        expect(welcome_banner).to be_hidden
+        page.send_keys("/")
+        expect(search_page).to have_search_menu
+        expect(current_active_element[:id]).to eq("search-term")
+        page.send_keys(:escape)
+        expect(search_page).to have_no_search_menu_visible
+      end
+
+      it "displays and focuses header search when Ctrl+F is pressed and hides it when Escape is pressed" do
+        visit("/")
+        expect(welcome_banner).to be_hidden
+        search_page.browser_search_shortcut
+        expect(search_page).to have_search_menu
+        expect(current_active_element[:id]).to eq("search-term")
+        page.send_keys(:escape)
+        expect(search_page).to have_no_search_menu_visible
+      end
+    end
+  end
+
+  context "when search_experience is search_icon" do
+    before { SiteSetting.search_experience = "search_icon" }
+
+    context "when enable_welcome_banner is true" do
+      before { SiteSetting.enable_welcome_banner = true }
+
+      it "displays and focuses welcome banner search when / is pressed and hides it when Escape is pressed" do
+        visit("/")
+        expect(welcome_banner).to be_visible
+        page.send_keys("/")
+        expect(search_page).to have_search_menu
+        expect(current_active_element[:id]).to eq("welcome-banner-search-term")
+        page.send_keys(:escape)
+        expect(search_page).to have_no_search_menu_visible
+      end
+
+      it "displays and focuses welcome banner search when Ctrl+F is pressed and hides it when Escape is pressed" do
+        visit("/")
+        expect(welcome_banner).to be_visible
+        search_page.browser_search_shortcut
+        expect(search_page).to have_search_menu
+        expect(current_active_element[:id]).to eq("welcome-banner-search-term")
+        page.send_keys(:escape)
+        expect(search_page).to have_no_search_menu_visible
+      end
+
+      context "when welcome banner is not in the viewport" do
+        before do
+          visit("/")
+          fake_scroll_down_long
+        end
+
+        it "displays and focuses search icon search when / is pressed and hides it when Escape is pressed" do
+          expect(welcome_banner).to be_invisible
+          page.send_keys("/")
+          expect(search_page).to have_search_menu
+          expect(current_active_element[:id]).to eq("search-term")
+          page.send_keys(:escape)
+          expect(search_page).to have_no_search_menu_visible
+        end
+      end
+    end
+
+    context "when enable_welcome_banner is false" do
+      before { SiteSetting.enable_welcome_banner = false }
+
+      it "displays and focuses search icon search when / is pressed and hides it when Escape is pressed" do
+        visit("/")
+        expect(welcome_banner).to be_hidden
+        page.send_keys("/")
+        expect(search_page).to have_search_menu
+        expect(current_active_element[:id]).to eq("search-term")
+        page.send_keys(:escape)
+        expect(search_page).to have_no_search_menu_visible
+      end
+
+      # This search menu only shows within a topic, not in other pages on the site,
+      # unlike header search which is always visible.
+      context "when within a topic with 20+ posts" do
+        fab!(:topic)
+        fab!(:posts) { Fabricate.times(21, :post, topic: topic) }
+
+        it "opens search on first press of Ctrl+F, and closes on the second" do
+          visit "/t/#{topic.slug}/#{topic.id}"
+          search_page.browser_search_shortcut
+          expect(search_page).to have_search_menu_visible
+          search_page.browser_search_shortcut
+          expect(search_page).to have_no_search_menu_visible
+        end
+      end
+    end
+  end
+end
diff --git a/spec/system/welcome_banner_spec.rb b/spec/system/welcome_banner_spec.rb
index 6f7f280316e..bdf5b4bec23 100644
--- a/spec/system/welcome_banner_spec.rb
+++ b/spec/system/welcome_banner_spec.rb
@@ -46,9 +46,7 @@ describe "Welcome banner", type: :system do
       expect(banner).to be_visible
       expect(search_page).to have_no_search_field
 
-      # Trick to give a huge vertical space to scroll
-      page.execute_script("document.querySelector('.topic-list').style.height = '10000px'")
-      page.scroll_to(0, 1000)
+      fake_scroll_down_long
 
       expect(banner).to be_invisible
       expect(search_page).to have_search_field