Similar to #3191692: Have secondary menus close on blur, the search form dropdown should close on when focus is removed from the search form.

This can problem reduce or eliminate issues such as #3209871: Olivero's: The header is showing odd behavior when the search bar is opened.

Testing instructions

  1. Load the patch or visit the tugboat link: https://3226785-wide-search-blur-2-u5r32gkuptbfrviscfbruwee2wditkcl.tugb...
  2. Load the page at wide widths
  3. Open the search form (can be either click or via keyboard)
  4. Tab outside of the search form - verify the form closes after you tab away.
  5. Shift-tab to before the search form. Verify the form will close after either the form or the form's disclosure button loses focus.

Comments

mherchel created an issue. See original summary.

mherchel’s picture

Status: Active » Needs review
StatusFileSize
new1.9 KB

I had to use the focusout event, but this patch is working great.

gauravvvv’s picture

StatusFileSize
new1.88 KB
new1.24 KB

Re-rolled patch 2, Remove if condition from else statement. Please review.

mherchel’s picture

Looks good to me! Still needs review from someone else as I wrote the majority of the code.

Thanks for the fix @Gauravmahlawat

mherchel’s picture

Issue summary: View changes
schillerm’s picture

Hi, went through testing instructions on the tugboat link and on a local D9 site. Patch #3 worked as expected for me, no errors.

mherchel’s picture

Issue tags: +JavaScript
StatusFileSize
new5.13 KB
new2.13 KB
new3.97 KB

Added tests. Also, we had a click event listener on the document that was used to close the search form when something other than the search form was clicked. Since we're now closing the search form on focusout. I refactored this to put the event listener on the search disclosure button, and have that simply toggle the form visibility.

Note I'm also applying these changes to the Tugboat preview: https://3226785-wide-search-blur-ndgukawgpielewnzaznw3cxtmoia97gi.tugboa...

mherchel’s picture

StatusFileSize
new5.2 KB
new2.2 KB
new772 bytes
mherchel’s picture

StatusFileSize
new5.78 KB
new2.78 KB
new2.9 KB

Updated tests.

thejimbirch’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new1.58 MB

Tested on the tugboat link: https://3226785-wide-search-blur-ndgukawgpielewnzaznw3cxtmoia97gi.tugboa...
Tabbed to the search form.
Clicked return to open it.
Tabbed into then out of it.
Form closed.
Shift-tabbed to before the search form.
Form closed after losing focus.

Screen recording:
Screen recording of validation steps

Marking as RTBC

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
mherchel’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new6.12 KB
andy-blum’s picture

Status: Needs review » Needs work

A couple notes:

  1. We're declaring the variable searchWideButton twice. They're in different scopes so they don't interfere, and it passes lint:core-js-passing, but it might be better to not declare two consts with the same name
  2. There's a 500ms delay on focusout, but the closing action isn't cancelled if we focusin again before that delay is up. It's minor, but we should probably either add a clearTimeout or remove the delay.

Beyond those two items, #12 corrects the issue and should otherwise be ready for RTBC.

mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new6.14 KB
new1.62 KB

The searchWideButton isn't being added by this patch, so lets defer that.

I'm just going to remove the timeout. It's not needed.

andy-blum’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

mherchel’s picture

Status: Reviewed & tested by the community » Needs work

Tests are failing :(

[0;32m✔[0m Testing if attribute [0;33m'aria-expanded'[0m of element [0;33m<button.block-search-wide__button>[0m equals [0;33m'false'[0m [0;90m(14ms)[0m
 [1;31mError while running .clickElement() protocol action: An element command could not be completed because the element is not visible on the page. – element not visible
[0m
[0;31m✖[0m Timed out while waiting for element <#edit-keys--2> to be visible for 5000 milliseconds. - expected [0;32m"visible"[0m but got: [0;31m"not visible"[0m [0;90m(5119ms)[0m
[0;90m    at Object.search wide form is accessible and altered (/var/www/html/core/tests/Drupal/Nightwatch/Tests/Olivero/oliveroSearchFormTest.js:69:8)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)[0m 


[0;31mFAILED:[0m [0;31m1[0m assertions failed, [0;31m1[0m errors and  [0;32m10[0m passed (6.314s)
mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new6.25 KB
new1.09 KB

Fixed tests (hopefully)!

mherchel’s picture

Still failing

[0;32m✔[0m Element <.block-search-wide__wrapper> was not visible after 536 milliseconds.
[0;32m✔[0m Testing if attribute [0;33m'aria-expanded'[0m of element [0;33m<button.block-search-wide__button>[0m equals [0;33m'false'[0m [0;90m(14ms)[0m
 [1;31mError while running .clickElement() protocol action: An element command could not be completed because the element is not visible on the page. – element not visible
[0m
[0;31m✖[0m Timed out while waiting for element <#edit-keys--2> to be visible for 5000 milliseconds. - expected [0;32m"visible"[0m but got: [0;31m"not visible"[0m [0;90m(5109ms)[0m
[0;90m    at Object.search wide form is accessible and altered (/var/www/html/core/tests/Drupal/Nightwatch/Tests/Olivero/oliveroSearchFormTest.js:70:8)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)[0m 


[0;31mFAILED:[0m [0;31m1[0m assertions failed, [0;31m1[0m errors and  [0;32m10[0m passed (6.691s)

[0;36m[Olivero/Olivero Sticky Header Toggle Test] Test Suite[0m
[0;35m======================================================[0m
mherchel’s picture

Status: Needs review » Needs work
mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new6.27 KB
new667 bytes

These tests pass on my local, so it's hard to debug.

I think we're running into another CSS transition animation issue. Adding delay.

mherchel’s picture

mherchel’s picture

StatusFileSize
new6.32 KB
new2.5 KB

🤞🤞🤞🤞🤞🤞🤞

mherchel’s picture

StatusFileSize
new6.32 KB
new3.25 KB

Yay!

Re-uploading the same patch along with the test by itself (so committers can see the failure).

Tugboat preview at https://3226785-wide-search-blur-2-u5r32gkuptbfrviscfbruwee2wditkcl.tugb...

gauravvvv’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new2.77 MB

.

gauravvvv’s picture

Status: Reviewed & tested by the community » Needs review
mherchel’s picture

Issue summary: View changes

Updating tugboat link in summary.

gauravvvv’s picture

StatusFileSize
new3.07 MB

Tested on live preview
LGTM. Moving to RTBC. Added an after patch screen recording for reference.

gauravvvv’s picture

Status: Needs review » Reviewed & tested by the community

  • lauriii committed 5c01c20 on 9.3.x
    Issue #3226785 by mherchel, Gauravmahlawat, thejimbirch, andy-blum,...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5c01c20 and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

attisan’s picture

Category: Task » Bug report

... "but what if" the block (search-block-form-2) isn't being used as in "removed" or "not placed" 😬. This patch / commit currently results in JS errors:
Uncaught TypeError: Cannot read properties of null (reading 'addEventListener') at search.js?v=9.3.4:58:73