Closed (fixed)
Project:
Drupal core
Version:
9.3.x-dev
Component:
Olivero theme
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
4 Aug 2021 at 19:12 UTC
Updated:
4 Feb 2022 at 08:59 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
mherchelI had to use the
focusoutevent, but this patch is working great.Comment #3
gauravvvv commentedRe-rolled patch 2, Remove if condition from else statement. Please review.
Comment #4
mherchelLooks good to me! Still needs review from someone else as I wrote the majority of the code.
Thanks for the fix @Gauravmahlawat
Comment #5
mherchelAdding testing instructions, and tugboat link: https://3226785-wide-search-blur-ndgukawgpielewnzaznw3cxtmoia97gi.tugboa...
Comment #6
schillerm commentedHi, went through testing instructions on the tugboat link and on a local D9 site. Patch #3 worked as expected for me, no errors.
Comment #7
mherchelAdded tests. Also, we had a
clickevent 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 onfocusout. 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...
Comment #8
mherchelComment #9
mherchelUpdated tests.
Comment #10
thejimbirch commentedTested 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:

Marking as RTBC
Comment #11
lauriiiComment #12
mherchelComment #13
andy-blumA couple notes:
searchWideButtontwice. They're in different scopes so they don't interfere, and it passeslint:core-js-passing, but it might be better to not declare two consts with the same namefocusout, but the closing action isn't cancelled if wefocusinagain before that delay is up. It's minor, but we should probably either add aclearTimeoutor remove the delay.Beyond those two items, #12 corrects the issue and should otherwise be ready for RTBC.
Comment #14
mherchelThe
searchWideButtonisn't being added by this patch, so lets defer that.I'm just going to remove the timeout. It's not needed.
Comment #15
andy-blumLooks great!
Comment #16
mherchelTests are failing :(
Comment #17
mherchelFixed tests (hopefully)!
Comment #18
mherchelStill failing
Comment #19
mherchelComment #20
mherchelThese tests pass on my local, so it's hard to debug.
I think we're running into another CSS transition animation issue. Adding delay.
Comment #21
mherchelComment #22
mherchel🤞🤞🤞🤞🤞🤞🤞
Comment #23
mherchelYay!
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...
Comment #24
gauravvvv commented.
Comment #25
gauravvvv commentedComment #26
mherchelUpdating tugboat link in summary.
Comment #27
gauravvvv commentedTested on live preview
LGTM. Moving to RTBC. Added an after patch screen recording for reference.
Comment #28
gauravvvv commentedComment #30
lauriiiCommitted 5c01c20 and pushed to 9.3.x. Thanks!
Comment #32
attisan... "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