Problem/Motivation

Some sites want just the search field in the toolbar, without the Search tab. You can do that by unsetting $items['administration_search'] in hook_toolbar_alter(). You also have to copy its #attached onto administration_search_field, since that's where the libraries live.

Search still works after that, but two scripts expect the tab to be there and throw:

  • On every page load, admin_toolbar_search.js adds a click listener to the tab:
    TypeError: Cannot read properties of null (reading 'addEventListener')
  • On Alt + a, admin_toolbar_search.keyboard_shortcut.js calls getComputedStyle() on the tab, so the field never gets focus:
    TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not of type 'Element'.

Steps to reproduce

  1. Enable Admin Toolbar Search with "Display menu item" off and the keyboard shortcut on.
  2. Add a hook_toolbar_alter() that copies #attached from administration_search to administration_search_field, then unsets administration_search.
  3. Clear caches and load any admin page on a wide screen. You'll see the first error in the console.
  4. Press Alt + a. You'll see the second one, and the field doesn't get focus.

Proposed resolution

Check that the tab exists before using it, like #3532249: Uncaught TypeError: toolbarElement.querySelector(...) is null did for admin_toolbar.toggle_shortcut.js:

  • Click listener: ?.addEventListener.
  • Alt + a: if there's no tab, focus the search field. If there's neither, do nothing.

Remaining tasks

Review !217.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#5 3624439 after.png172.18 KBcsakiistvan
#5 3624439 before.png1003.55 KBcsakiistvan

Issue fork admin_toolbar-3624439

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jmcerda created an issue. See original summary.

jmcerda opened merge request !217

jmcerda’s picture

jmcerda
Location Boca Raton, FL
commented
Status: Active » Needs review

Merge request !217 is up against 3.x.

The handler now picks the first visible element for each ID instead of the first match. A visible search field is focused directly. Otherwise the visible search tab's tray is toggled and its input focused, as before. When neither exists it returns without calling preventDefault(), so nothing throws and the keystroke is left alone.

One new test, AdminToolbarSearchEventsTest::testAdminToolbarSearchKeyboardShortcutSearchField(), covers the search field in the toolbar, a hidden duplicate placed before it, a page without the search tab, and a page with nothing to focus.

The pipeline is green. The "test-only changes" job fails as expected: on the current code the new test fails at the hidden duplicate step, and the two existing tray tests still pass.

Also checked by hand on a Gin site with the new navigation layout, serving the patched file in place of the released one: Alt + a focuses the visible field with the duplicate present, and still does with the search tab removed.

csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: Unassigned » csakiistvan
csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: csakiistvan » Unassigned
StatusFileSize
new 3624439 before.png1003.55 KB
new 3624439 after.png172.18 KB

@jmcerda: ❓ Needs more info — with MR !217 applied and caches rebuilt, case 1 is fixed: the getComputedStyle TypeError on Alt + a is gone once the search tab is removed in hook_toolbar_alter(). Case 2 could not be reproduced as written — with gin.settings classic_toolbar: new no admin toolbar search element is rendered at all, so there is nothing to focus; in the Gin configuration where the ids are duplicated, Alt + a still focuses the first copy, so it is unclear which Gin setup the report means.

jmcerda opened merge request !220

jmcerda’s picture

jmcerda
Location Boca Raton, FL
commented
Title: Alt + a search shortcut throws when the search tab is removed and focuses a hidden field with Gin » Guard Admin Toolbar Search JavaScript when the search tab is removed
Issue summary: View changes

Thanks for testing this @csakiistvan, and sorry, I made this more confusing than it needed to be.

Some background: on our site we only want the search field in the toolbar, not the Search tab too, so we unset the tab in hook_toolbar_alter(). That's what this issue is really about.

You're right on case 2. I'd only seen it on our site, which also has that alter hook, and I never checked it on a clean install. Since you can't reproduce it, I've dropped it. If I can get it to happen on a fresh install with Gin I'll open a new issue.

Removing the tab also causes a second error, on every page load, not just on Alt + a. admin_toolbar_search.js adds a click listener to the tab without checking that it's there:

TypeError: Cannot read properties of null (reading 'addEventListener')

I put the fix for that in a separate MR (!220). It's a one-line change, so I've moved it into !217 and closed !220. Now there's just one MR.

!217 now:

  • Alt + a: if the tab isn't there, focus the search field. If neither is there, do nothing. The visible-element lookup is gone.
  • Click listener: ?.addEventListener
  • A small test module that removes the tab the same way, plus a test for it.

I updated the summary too. Back to Needs review.

jmcerda’s picture

jmcerda
Location Boca Raton, FL
commented

Changing status to needs review.

csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: Unassigned » csakiistvan
csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: csakiistvan » Unassigned
Status: Needs review » Needs work

❌ Does not fix the issue — applied MR !217, rebuilt caches and loaded /admin/content with the search tab removed by the MR's test module: the page-load addEventListener error and the Alt + a getComputedStyle error are gone, but Alt + a still leaves focus on body, so the search field is never focused with Gin.

Prerequisites
  • Gin admin theme, display_menu_item: false, keyboard shortcut on, admin_toolbar_search_test_no_search_tab from the MR enabled.
  • Core Navigation uninstalled, otherwise no toolbar is rendered.
Actual results
  • Before the MR: both TypeErrors from the summary appear in the console.
  • After the MR: no errors, but document.activeElement is body after Alt + a (checked in Chromium via Playwright and manually in Firefox).
  • The page has two #admin-toolbar-search-field-tab elements: the first, inside #gin-toolbar-bar, has no client rects; the visible one is inside #toolbar-bar. getElementById() returns the hidden one, so focus() does nothing.
Notes
  • This is the duplicate-ID case dropped after the previous review; it reproduces on the no-tab path, and the earlier visible-element lookup in !217 would cover it.