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.
dydave’s picture

dydave commented

Thanks a lot Jeremy (@jmcerda) for creating this issue and contributing a solution with the suggested code changes in a merge request.
It's greatly appreciated! 🙏

István (@csakiistvan), thanks for the initial review. 👍

Could anyone maybe help also take a look at the related issue with the two search fields displayed on Gin?
#3571169: Two search inputs display when using Gin theme
It has maybe a slightly different approach to fix the issues with the keyboard shortcut (Alt + a).

I'm not saying at all the changes in this issue are not needed.
It's just that these two issues might have some points in common, on which I would appreciate your feedbacks.

Thanks in advance! 😊

csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented

@dydave Took a look at #3571169 together with this one.

✅ Tested and works — merged MR !222 and MR !217 on top of 3.x, rebuilt caches and loaded /admin/content with Gin: no console errors on page load, and Alt + a now focuses the search field in the toolbar both with the search tab removed by the test module and with it present. The two MRs complement each other: !222 fixes the focus problem from my previous review (duplicate IDs), !217 covers the missing search tab.

Without !222, !217 alone still leaves focus on body after Alt + a, as in my previous review.

dydave’s picture

dydave commented

Thank you very much István (@csakiistvan) for the speedy reply and great help with the testing and reviews, it's greatly appreciated! 🙏

Sounds great! Glad the merge requests complement each other and that they address the issues accurately. 👍

Let's not complicate things more than they should be:
I'll start by merging #3571169: Two search inputs display when using Gin theme (with MR !222), then come back to this issue and get it merged in as well. 👌

Thanks again very much for all your great help! 😊

  • dydave committed b6ffbf05 on 3.x authored by jmcerda 
    Issue #3624439 by jmcerda, dydave: Guarded the Admin Toolbar Search...
dydave’s picture

dydave commented
Status: Needs work » Fixed

Following the confirmation from István (@csakiistvan) at #12
✅ The changes from MR !222 were merged in fixed issue #3571169: Two search inputs display when using Gin theme.
✅ Rebased MR !217, fixed conflicts and added a commit to fix broken tests following the rebase.
✅ Tested the changes successfully once again locally.

All the changes in this merge request are limited to the admin_toolbar_search module, so the potential impacts should be a bit "safer".
Since all the tests and jobs were still passing 🟢, I went ahead and merged the changes above at #14. 🥳

At this point I don't really see any follow-up to this issue and since all the work initially described in the issue summary should have been completed, marking it as Fixed for now.

If more bugs related with the Gin theme appear in the future they should be addressed in new specific issues and we will fix any new problems as we go. But for the time being, this merge request should already provide some basic "safety-net" for the Admin Toolbar Search keyboard shortcut JS code.

Feel free to let us know if you have any questions or concerns on any aspects of the recent code changes or this issue in general, we would certainly be happy to hear your feedback.
Thanks in advance! 🙏

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented

Thanks @dydave for merging both MRs and for the quick turnaround, and thanks @jmcerda for the fix! 🙏

Quickly re-checked 3.x after the merge on Gin: no console errors on page load, and Alt + a focuses the toolbar search field both with and without the search tab. 👍