- 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Re-testing #6 now that 🐛 Fix broken tests Fixed is fixed.
- 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Tests look good for #6 now.
@MustangGB can you please provide simple steps for how to review the fix manually?
I tried adding search_autocomplete to a vanilla D7 site with jquery_update bringing in the latest jQuery 3.6.x but it wasn't apparent that there was a problem with positioning in the UI.
- 🇬🇧United Kingdom mustanggb Coventry, United Kingdom
Simple? Not sure about that.
Are you sure you tested with the autocomplete in a table?
Nevertheless, added some more in-depth repro steps to the description.
- 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Thanks for the detailed steps - took me quite a bit of fiddling around to get the multifield table widget working, but I was eventually able to reproduce the problem with autocomplete.
The fix looks good.
Sorry to be pedantic, but there is one thing I'd like to tweak though; I was sure we'd made some changes fairly recently relating to jQuery's position() and eventually remembered that I was thinking of SA-CORE-2022-001:
https://git.drupalcode.org/project/drupal/-/commit/c05bb17ce3a697cc36221...
We used quite a specific filename for that backport, and it bothers me slightly that the patch here is specifically addressing an issue with autocomplete (even more specifically
Drupal.jsAC.prototype.populatePopup
) but we're just calling it "position fix":+ 'title' => 'jQuery Update Position Fix', + 'js' => array( + drupal_get_path('module', 'jquery_update') . '/js/jquery_position.js' => array(
The other similar shim that jQuery Update recently added is called jquery_browser.js but that does actually provide
jquery.browser
if it's no longer provided by jQuery.Can we please name this shim / fix more specifically based on what it's actually doing?
Maybe more like "jQuery Update Autocomplete Position fix".
- 🇬🇧United Kingdom mustanggb Coventry, United Kingdom
I originally considered
jquery_update.autocomplete.fix
, meaning all fixes related to autocomplete.But as the thing that changed was the functionality of
position()
thought it made more sense to go withjquery_update.position.fix
, meaning all fixes related to position, it just so happens that the only place we've found to fix so far is in autocomplete, but in theory fixes for other elements could also go in this file.I'd not considered
jquery_update.autocomplete_position.fix
, because as previously mentioned I wanted to keep the door open to other fixes relating toposition()
to have a place to go.If you're still sure you'd like to change it I will update the patch, please let me know.
- Status changed to RTBC
almost 2 years ago 1:58pm 24 January 2023 - 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
... as the thing that changed was the functionality of position() thought it made ... sense to go with jquery_update.position.fix, meaning all fixes related to position, it just so happens that the only place we've found to fix so far is in autocomplete, but in theory fixes for other elements could also go in this file.
Yup, okay that makes sense. Happy to leave it as it is.
Will do a final review and commit this ASAP. Thanks!
-
mcdruid →
committed 0541afe8 on 7.x-4.x authored by
MustangGB →
Issue #3325927 by mcdruid, MustangGB: jQuery 3.3+ breaks autocomplete
-
mcdruid →
committed 0541afe8 on 7.x-4.x authored by
MustangGB →
- Status changed to Fixed
almost 2 years ago 4:59pm 24 January 2023 - 🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
Thank you!
Will get a new point release out soon (before marking the old 7.x-2.x release as unsupported).
Automatically closed - issue fixed for 2 weeks with no activity.