Problem/Motivation

After replacing "checkboxes" form element via AJAX, it's behaviours are not correctly attached to the checkboxes.

Steps to reproduce

1. $ mkdir drupal
2. $ cd drupal/
3. $ curl -sSL https://www.drupal.org/download-latest/tar.gz | tar -xz --strip-components=1
4. $ php core/scripts/drupal quick-start standard --site-name BugDemo --host localhost --port 8080
5. Install the attached 'bugmodule' module.
6. Goto http://localhost:8080/bug.
7. Confirm that "checkboxes" element doesn't attach it's behaviours correclty.

Proposed resolution

The problem lies in randomized id attributes of the checkbox elements. See: https://www.drupal.org/node/2503277.
With the single checkbox the id is not randomized so the behaviours attach loop catches it correctly.
With multiple checkboxes element all the ids of individual checkboxes are randomized. This leads to the problem.

See the problem

1. Disable JS aggregation and put a breakpoint here in the browsers js debugger. Breakpoint: /core/misc/ajax.js - line 27
2. Reload the page and check the 'settings.ajax' value in the debugger.

The selector for attaching behaviours is an id based on the key of this array. These keys are not updated when the id's are randomized so the selector doesn't match the element ids anymore.

Solution

We are moving away from HTML ids to use data-drupal-selector data attribute (see: https://www.drupal.org/node/2503277). For this reason we should also move to use it here. Still removing the id selector could break something and cause regression bugs. A simple way to avoid this is to just add the data attribute selector to the list. jQuery still selects the element only once. This allows a gradual shifting from using the ids for AJAX references and fixes this problem.

Objections

"We should fix the randomization of checkbox ids instead."

Answer: Why? The current solution fixes the bug and we don't know why the randomization is done in the first place. Removing it could cause regression bugs. Also note that this fix is generally beneficial. It could fix similar bugs elsewhere. If the randomization is unnecessary it could be a good idea to fix it. If the reader feels motivated to do it, go ahead. In any case the current fix should be done as it's in line with the goal to stop using HTML ids in AJAX references.

"jQuery selector performance will get worse."

Answer: This is a valid point. I ran a performance benchmark on https://jsbench.me/ with jQuery 3.5.1. Results compared to jQuery('#the-element'):
1. jQuery('[data-drupal-selector='the-element'], #the-element') was about 90%-100% percent slower.
2. jQuery('[data-drupal-selector='the-element']') was also about 90%-100% percent slower.
So yes, the performance is worse. But there's no way avoiding it when using data attribute selectors and Drupal is aiming for that. Having two selectors in the query doesn't seem to have noticeable performance impact. This could still affect the end user experience if the DOM is really big or there's a lot of ajax commands registered on the page. Some real world testing with heavy sites would be great. If you are worried, review it on your sites.
3. document.querySelector('[data-drupal-selector="the-element"]') is little bit faster. So there's at least some room for optimization.

"This causes unpredictable regression bugs."

Answer: At least some tests failed. This gives weight to the idea of fixing the bug on the checkboxes element. If there really are nasty regression bugs, maybe I need to try that route when I have time. Before that is hard to say which solution causes more problems.

Remaining tasks

- Review the solution.
- Write release notes.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

Issue fork drupal-3207786

Command icon 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:

Comments

jiv_e created an issue. See original summary.

jiv_e’s picture

StatusFileSize
new2.07 KB

jiv_e’s picture

jiv_e’s picture

jiv_e’s picture

Issue summary: View changes
jiv_e’s picture

Issue summary: View changes
jiv_e’s picture

Assigned: jiv_e » Unassigned
Status: Active » Needs review
jiv_e’s picture

Title: AJAX behaviours are not correclty attached to checkboxes form element » AJAX behaviours are not correctly attached to checkboxes form element

Status: Needs review » Needs work
jiv_e’s picture

Surprisingly (really?) adding the data selector caused some failing tests. The fix wasn't as unproblematic I thought. Need to add one more objection. :)

jiv_e’s picture

Issue summary: View changes

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

iancawthorne’s picture

+1 for this. I've got a hook form alter changing a numberfield to checkboxes and couldn't for the life of me work out why it only worked when the render cache was disabled. With this patch in place, it looks to be working spot on.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jiv_e’s picture

Just confirmed that this is still an issue in Drupal 9.3.13.

jiv_e’s picture

The latest patch also fixes the issue in Drupal 9.3.13, but it causes a regression bug in other Ajax elements. For example repeating text field (multivalue field) doesn't work correctly anymore. You can add one item after the first, but second addition fails.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

iancawthorne’s picture

I'm still finding this is an issue in Drupal 10. Attached is a re-rolled patch against 10.1.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.