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
| Comment | File | Size | Author |
|---|---|---|---|
| #22 | fix_ajax_behaviours_on_checkboxes-10.1-3207786-22.patch | 1015 bytes | iancawthorne |
| #5 | fix_ajax_behaviours_on_checkboxes-8.9-3207786-5.patch | 1.31 KB | jiv_e |
| #4 | fix_ajax_behaviours_on_checkboxes-9.2-3207786-4.patch | 1.25 KB | jiv_e |
| #2 | bugmodule.zip | 2.07 KB | jiv_e |
Issue fork drupal-3207786
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
Comment #2
jiv_e commentedComment #4
jiv_e commentedComment #5
jiv_e commentedComment #6
jiv_e commentedComment #7
jiv_e commentedComment #8
jiv_e commentedComment #9
jiv_e commentedComment #10
jiv_e commentedComment #12
jiv_e commentedSurprisingly (really?) adding the data selector caused some failing tests. The fix wasn't as unproblematic I thought. Need to add one more objection. :)
Comment #13
jiv_e commentedComment #15
iancawthorne commented+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.
Comment #17
jiv_e commentedJust confirmed that this is still an issue in Drupal 9.3.13.
Comment #18
jiv_e commentedThe 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.
Comment #22
iancawthorne commentedI'm still finding this is an issue in Drupal 10. Attached is a re-rolled patch against 10.1.