Regression fix for (if feasible) uses of the jQuery trim function to use vanillaJS

Thanks for reporting this issue. We rely on issue reports like this one to resolve bugs and improve Drupal core.

As part of the Bug Smash Initiative, we are triaging issues that are marked "Postponed (maintainer needs more info)". This issue was marked "Postponed (maintainer needs more info)" more than 6 months ago with a request for steps to reproduce and there has been no activity since that time.

Since we need more information to move forward with this issue, I am tagging for Bug Smash Initiative and keeping the status at Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Still needing this patch

This regression is caused by a core change made two years ago
#3239132-11: Refactor (if feasible) uses of the jQuery trim function to use vanillaJS →

I'll try to write an es6 version patch to fix this issue once and for all.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

new patch

This part of a D11 commit fixes my D10 issue.

the D11 commit in question is:

╰❯ $ git show cd9fed87f05 core/misc/form.js

commit cd9fed87f0568d948187e9a0ae6456324c087a5a
Author: Lee Rowlands <lee.rowlands@previousnext.com.au>
Date:   Mon Jun 26 17:01:04 2023 +1000

    Issue #3238849 by mstrelan, hooroomoo, smustgrave, bnjmnm, larowlan: Refactor (if feasible) use of jquery is function to use vanillaJS

diff --git a/core/misc/form.js b/core/misc/form.js
index c02220c099..0e51d833f9 100644
--- a/core/misc/form.js
+++ b/core/misc/form.js
@@ -177,7 +177,7 @@
   Drupal.behaviors.formUpdated = {
     attach(context) {
       const $context = $(context);
-      const contextIsForm = $context.is('form');
+      const contextIsForm = context.tagName === 'FORM';
       const $forms = $(
         once('form-updated', contextIsForm ? $context : $context.find('form')),
       );
@@ -212,7 +212,7 @@
     },
     detach(context, settings, trigger) {
       const $context = $(context);
-      const contextIsForm = $context.is('form');
+      const contextIsForm = context.tagName === 'FORM';
       if (trigger === 'unload') {
         once
           .remove(

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

ok I have the steps to reproduce, it can be reproduced with core alone.

in your content type, allow the "Provide a menu link" option provided by the core menu_ui module.

When editing content that does not have the checkbox checked for "Provide a menu link" the callback is not a valid callback, it is "Not in menu"

The "Not in menu" functionality comes from the core menu_ui module.

The regression is in core, only need core to reproduce, it's not even caused by contrib.

Patch 18 works

the $.trim() function in jQuery does more than the ES6 trim() function and it's encapsulated by jQuery.

Basically the es6 trim function is not as good as the jQuery $.trim() function.

callback(this[0]).trim() would work if callback(this[0]) returned a string since trim() is available on the string prototype.

this.data or jQuery.data() as it is in this context returns an object. I think if we need to specify the exact key we're trying to stringify and trim here.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I can't reproduce this – maybe it's because I'm getting another JavaScript error in the Node edit form:

Uncaught TypeError: Cannot read properties of undefined (reading 'checked')
    at entity-form.js?v=10.2.0-dev:62:38
    at $.fn.drupalGetSummary (form.js?v=10.2.0-dev:34:34)
    at DetailsSummarizedContent.onSummaryUpdated (details-summarized-content.js?v=10.2.0-dev:57:33)
    at HTMLDetailsElement.dispatch (jquery.min.js?v=3.7.0:2:39997)
    at v.handle (jquery.min.js?v=3.7.0:2:37968)
    at Object.trigger (jquery.min.js?v=3.7.0:2:70063)
    at HTMLDetailsElement.<anonymous> (jquery.min.js?v=3.7.0:2:70665)
    at Function.each (jquery.min.js?v=3.7.0:2:3129)
    at ce.fn.init.each (jquery.min.js?v=3.7.0:2:1594)
    at ce.fn.init.trigger (jquery.min.js?v=3.7.0:2:70640)
edit:1 

It doesn't look like #23 applies against 11.x so marking as needs work.

#23 is hidden, please refer to the fix in #18

The fix is to use jQuery trim instead of the ES6 trim, jQuery trim encapsulates doesn't chain like the trim command, and the es6 trim only trims whitespace not newline and tabs

the jQuery trim is vastly more powerful and works differently than the chained ES6 trim.

I think we should try to look for another solution because #18 is not going to pass the custom commands given that it adds the $.trim call back. Can you test if this addresses the problem?

@joseph.olstad confirmed on Slack that this fixes the bug. I'm not sure if the error posted here is related because he explained that the problem is that the callback returns an undefined. This is probably just hiding an underlying bug so not sure what to do that since I don't have steps to reproduce it.

Debugged this together with @joseph.olstad to understand the root cause. The root cause is that entity_translation_unified_form changes some of the form elements and for that reason Drupal.behaviors.nodeDetailsSummaries for .node-form-author fails to find a summary, and returns undefined. I don't think we can fix the broken summary in core but we could add the extra defensiveness proposed in #34 so that at least there isn't a JavaScript error in this case. I think the fact that a core implementation of this API is suspect to the bug is sufficient justification for the defensiveness. I think this would be a good candidate for a bug fix without a test failure since the fix is really simple and it's essentially a contrib bug.

@lauriii, yes, the code from your patch #34 brings us back to the robustness of the jQuery trim behavior, so I believe it's an equivalent or almost equivalent solution that improves the robustness of core to allow the crazy things we're doing in contrib such as putting two or more languages on the node form .

This is exactly what I was trying to accomplish last night but I got stuck. Thanks so much for the fix!

this works and passes tests, simple robustness change, low risk high reward.

hoping this can go into 10.0, 10.1 10.2 and 11.x
and maybe even 9.5

Ok so I wasn't that far off with #29 after all.

+1 to RTBC from reviewing the code, I'm less certain that core committers will allow it to pass without a test though.

Bug fixes go into current stable releases and development branches so I think you'll see it in all of those versions except 10.0 if it's committed before 10.2.0 stable.

@darvanen, thanks for the review.

The solution that @lauriii and I found is a more equivalent refactor of a jQuery trim function. Recent core policy is restricting jQuery so I believe it's the responsibility of core to implement equivalent robustness of trim without requiring JQuery. So therefore I think it's core maintainers responsibility to expedite this fix into core.

The last submitted patch, 34: 3291587-34.patch, failed testing. View results →

retriggered tests, appears to be a testbot hiccup, there was no code change to 11.x since the last success.

@lauriii

FYI, the known scope of this issue:

At least three contrib modules add translations of fields onto the same node form, they were all affected by the jQuery refactor.

https://www.drupal.org/project/entity_translation_unified_form →
Entity Translation Unified Form (AKA ETUF) 2.0.0+
332 installs

This project above is a project I've been maintaining almost exclusively since 2019.

Two other similar projects:

https://www.drupal.org/project/mfd →
Multilanguage Form Display
My colleague is the maintainer of this module, he just published a D10 compatible version of it and I haven't had time to evaluate this yet.

https://www.drupal.org/project/translate_side_by_side →
Translation Side by Side
352 installs

So we're talking over 700 installs of a known issue. There may be other use cases where the jQuery robustness was helpful.

I'd like to see your patch #34 move forward rather than have to do a three ring circus in a bunch of contrib modules for something that was fine when jQuery trim was in play.

Assigning issue credits

Per our proposed better scoping of test 🌱 [policy, no patch] Better scoping for bug fix test coverage RTBC I think we don't need tests here. We're just adding more defensive code. We don't need to be testing for combinations of contrib module form alters that change something that is normally there.

Committed to 11.x and backported to 10.1.x.

There are no more bugfix releases for 10.0 and 9.5 so this won't get backported there @joseph.olstad

Thanks for persisting with this one folks.

Thanks @larowlan and also again thanks to @lauriii

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