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.

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();

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);
   });

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}}

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"

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>

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

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;

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;
       }

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;

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%;
+  }
 }

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!
 

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) }

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

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

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

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