Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Opened by @bronzehedwick in Olivero project queue
Problem/Motivation
When the search menu is open, clicking on the "X" should close/collapse the search menu. Instead, the display flickers, and the menu stays open. Clicking anywhere outside the menu collapses the menu as normal.
Tested on a vanilla Drupal instance.
Steps to reproduce
- Click the search icon
- Click the "X" now exposed
- Notice the search menu stays open
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | 3303112-after_patch-MR_2627.mov | 2.42 MB | Abhijith S |
| #18 | 3303112-before_patch.mov | 1.45 MB | Abhijith S |
| #17 | 3303112-17-10.1.x.patch | 8.83 KB | mherchel |
| |||
| #14 | reroll_diff.txt | 4.27 KB | ravi.shankar |
| #14 | 3303112-14.patch | 8.65 KB | ravi.shankar |
Issue fork drupal-3303112
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:
- 3303112-olivero-when-in
changes, plain diff MR !2627
1 hidden branch
Comments
Comment #2
mherchelI'm unable to reproduce this. We also have test coverage for this interaction.
Are there any additional steps to reproduce? (browser, etc?)
Comment #3
andy-blumThis issue is only in Safari - assuming it's related to the issues with webkit not giving buttons focus on click. Adding a related issue.
Comment #4
andy-blumAlso adding a link to the webkit bug tracker for this: https://bugs.webkit.org/show_bug.cgi?id=229895
Comment #5
mherchelAhh yeah, I see the issue in Safari
Comment #7
andy-blumComment #8
andy-blumTugboat: https://mr2627-tltrnjbvvxdtxk54b7jxbnzjaphcxkkc.tugboatqa.com/
Comment #9
bronzehedwickTested @andy-blum patch against a plain Drupal 9 site, and it fixed the issue in Safari for me. Code looks good to me as well. Thanks!
Comment #10
bronzehedwickComment #11
mherchelThis is great! I spent some time on this yesterday and had to give up as I had to move onto something else. Solving this is more difficult than I thought it would be.
Leaving as RTBC, but I added some comments and questions.
Comment #12
andy-blumComment #13
mherchelSetting back to needs work because of an IE11 incompatibility with
element.closest().All of the rest of the code works great, and I tested with Safari, Chrome, and Firefox.
We also need a D10 patch.
Comment #14
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded a patch for Drupal 10.1.x and added reroll diff between MR and patch.
Still needs work for #13.
Comment #15
andy-blum#13 shouldn't be an issue, as core polyfills Element.prototype.closest(). Do we need to do anything specific to make sure that's included or does Olivero already include core's polyfills?
Comment #16
mherchelLooks like Olivero already includes it as a dependency of our global styling. That being said, we really need to be explicit in our library definitions and include it as a dependency of the
olivero/search-widelibrary.I'd personally be okay with adding it as is, but I don't believe that it'll get by through the commit review without this change.
Comment #17
mherchelReplaced the usage of
Element.closest()withElement.matches()since the whole point was to see if the focused element is within the search bar.Also created a 10.1.x patch based off of the MR.
Comment #18
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedApplied MR 2627 on 9.5.x.The search menu is collapsing on clicking the X button, after applying this patch.Attaching screen recordings for reference.
Comment #19
mherchelSetting to RTBC based on ☝️
Comment #23
lauriiiCommitted 45132e0 and pushed to 10.1.x. Also cherry-picked to 10.0.x, 9.5.x and 9.4.x. Thanks!