Problem/Motivation

First encountered this some years ago, but finally got around to getting to the root cause.
From jQuery 3.3.0 onwards there is a mostly undocumented "bugfix" that breaks .position().
This affects autocomplete fields that are in tables.

See

Steps to reproduce

  1. Enable modules Entity reference and Multifield table (and their dependencies)
  2. Add a multifield field to a content type with the table widget
  3. To the multifield add an entity reference field with the autocomplete widget
  4. Try to create a new node of previously used content type, ensure multifield is displaying as a table, type into entity reference autocomplete field
  5. Observe autocomplete dropdown appears in the correct place (under the input box)
  6. Enable jQuery 3.3+
  7. Repeat step 4
  8. Observe autocomplete dropdown appears in the wrong place (somewhere near the top of the page)

Proposed resolution

The workaround is to use .offset() instead.

Remaining tasks

Fix it.

User interface changes

Autocomplete dropdown appears in the correct place again instead of at the top of the page.

API changes

None.

Data model changes

None.

Release notes snippet

None.

Comments

MustangGB created an issue. See original summary.

mustanggb’s picture

StatusFileSize
new1020 bytes

And a patch.

mustanggb’s picture

Status: Active » Needs review
mcdruid’s picture

Thanks! Can we address this in jquery_update instead?

Apart from anything else, the lack of any JS testing in D7 means it'd be preferable not to change D7 core to cater for the small (?) number of sites running up-to-date jQuery 3.x

mustanggb’s picture

Project: Drupal core » jQuery Update
Version: 7.x-dev » 7.x-4.x-dev
Component: ajax system » Code
Status: Needs review » Needs work

Sure, would you suggest doing something similar to jquery_browser.js?

mustanggb’s picture

Status: Needs work » Needs review
StatusFileSize
new3.04 KB

New patch against jquery_update.

Status: Needs review » Needs work

The last submitted patch, 6: jquery_update-3325927-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mustanggb’s picture

Unrelated test failure?

If not need guidance.

mcdruid’s picture

Erm, yeah I'm not sure why that's happened.

A quick way to try and see what's going on is something like this (with and without the patch applied):

$ drush -y si && drush -y en jquery_update && drush -y vset jquery_update_jquery_version '2.2'

$ drush ev "print_r(drupal_get_library('system')['jquery']);"
Array
(
    [title] => jQuery
    [website] => http://jquery.com
    [version] => 2.2.4    // <== this is what we're testing, IIUC
    [js] => Array
        (
            [misc/jquery.js] => Array
                (
                    [group] => -100
                    [weight] => -20
                    [data] => sites/all/modules/contrib/jquery_update/replace/jquery/2.2/jquery.min.js
                    [version] => 2.2.4
                )

            [misc/jquery-extend-3.4.0.js] => Array
                (
                    [group] => -100
                    [weight] => -19
                    [version] => 2.2.4
                )

            [misc/jquery-html-prefilter-3.5.0-backport.js] => Array
                (
                    [group] => -100
                    [weight] => -19
                    [version] => 2.2.4
                )

        )

    [dependencies] => Array
        (
            [0] => Array
                (
                    [0] => jquery_update
                    [1] => jquery_update.browser.fix
                )

        )

    [css] => Array
        (
        )

)

$ drush ev "print_r(drupal_get_library('system')['drupal.ajax']['js']);"
Array
(
    [misc/ajax.js] => Array
        (
            [group] => -100
            [weight] => 2
            [version] => 7.94-dev
        )

    [0] => Array
        (
            [data] => Array
                (
                    [ajaxPageState] => Array
                        (
                            [jquery_version] => 2.2  // <== this is what we're testing, IIUC 
                            [jquery_version_token] => _cQ*****token******t0
                        )

                )

            [type] => setting
            [version] => 7.94-dev
        )

)

I can imagine the patch moving things around in $libraries a little, but don't see how it'd cause these specific assertions to fail.

Generally the patch looks like the right sort of idea, but I've not looked too closely yet.

I've queued the tests to re-run. If the result's the same we may need to look closely using an actual drupalci instance.

mcdruid’s picture

Status: Needs work » Needs review
StatusFileSize
new1.07 KB
new4.18 KB

I do have a local drupalci I can start testing this with (see https://www.drupal.org/drupalorg/docs/drupal-ci/running-drupalci-locally ).

Here's a quick tweak to the patch just to see if the main drupalci will give us a clue about what's going wrong though...

Status: Needs review » Needs work

The last submitted patch, 10: 3325927-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mcdruid’s picture

fail: [Other] Line 405 of sites/all/modules/jquery_update/tests/jquery_update.test:
Failed jquery version: 1.12.4

fail: [Other] Line 407 of sites/all/modules/jquery_update/tests/jquery_update.test:
Failed ajax version: 1.12

Okay, so looks like the admin form submission to set the initial version for the test isn't working for some reason... not sure why that would be, but it gives us somewhere to start.

mcdruid’s picture

Status: Needs work » Needs review
StatusFileSize
new390 bytes

I think this is happening without any of the changes from this issue.

Here's a noop patch to check that.

If so, I don't understand why ... but it'd be good to know.

Status: Needs review » Needs work

The last submitted patch, 13: 3325927-13_noop.patch, failed testing. View results

mustanggb’s picture

@mcdruid
Unrelated test failure aside, is there anything that needs changing with the actual patch?

mcdruid’s picture

Filed #3327465: Fix broken tests to fix the broken tests. I don't want to commit / release anything until that's resolved.

If this patch fixes the issue with autocomplete that you initially noticed, that's great. Guessing it wouldn't be an easy thing to test in D7?

Will review this properly once tests are passing again; sorry for the delay.

mcdruid’s picture

Status: Needs work » Needs review

Re-testing #6 now that #3327465: Fix broken tests is fixed.

mcdruid’s picture

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.

mustanggb’s picture

Issue summary: View changes

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.

mustanggb’s picture

Issue summary: View changes
mcdruid’s picture

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".

mustanggb’s picture

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 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.

I'd not considered jquery_update.autocomplete_position.fix, because as previously mentioned I wanted to keep the door open to other fixes relating to position() 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.

mcdruid’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Fixed

Thank you!

Will get a new point release out soon (before marking the old 7.x-2.x release as unsupported).

Status: Fixed » Closed (fixed)

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