| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | ajax system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | reviewed & tested by the community |
| Issue tags: | Ajax, frontend performance, JavaScript, jQuery, needs backport to D7, Needs committer feedback, Needs JS testing |
Issue Summary
Currently when form has file element there's no ability to send ajax_html_ids because it's not possible to serialize that array to send with jQuery Form plugin shipped with core.
Also ajax_html_ids has array structure that makes ajax requests take more bandwidth by prefixing each value with
ajax_html_ids[]
.
Proposed solution
Send ajax_html_ids serialized imploded with space because HTML5 standard does not limits allowed characters for HTML IDs except 'white-space'.
The ajax_page_state already has plain structure so this is some kind of standardization for ajax variables.
Also this change help to minimize request size for ajax calls as discussed in #956186: [meta] Allow to use AJAX with GET requests
Testing troubles
Currently simpletest has no ability to test ajax request properly so only manual testing possible.
Steps to reproduce
- download and enable attached module (ajaxdemo)
- visit /ajaxdemo_test_html_ids and press Get
-- bellow will output count and list of used IDs (they are passed as array)
- visit ajaxdemo_test_html_ids/1 and press Get (file element should appear on form)
-- there would be only one element serialized with ',' (in this case core can't extract IDs used on page)
Related #1574560: Update jQuery Form Plugin to the latest in the git repository
Comments
#1
Proposed patch
Also attached a module to test
Screens of module
-- without file element $ids are passed as array
-- with file element $ids are broken
#2
Tagging, suppose this does not break any existing code so could be backported
Also there's a lot of troubles could be fixed with this small change, forums has a lot of posts when ajax calls print garbage on screen.
PS: found when working on #1414510-41: Remove drupal_reset_static for drupal_html_id() when form validation fails
#3
+1 much better. good to go for me.
I'll let the ajax maintainers have a look.
#4
On vanila D8 install data for ajax_html_ids reduced twice
befora patch
ajax_html_ids%5B%5D=overlay-profile-link&ajax_html_ids%5B%5D=overlay-dismiss-message&ajax_html_ids%5B%5D=toolbar&ajax_html_ids%5B%5D=toolbar-home&ajax_html_ids%5B%5D=toolbar-user&ajax_html_ids%5B%5D=toolbar-menu&ajax_html_ids%5B%5D=toolbar-link-admin-dashboard&ajax_html_ids%5B%5D=toolbar-link-admin-content&ajax_html_ids%5B%5D=toolbar-link-admin-structure&ajax_html_ids%5B%5D=toolbar-link-admin-appearance&ajax_html_ids%5B%5D=toolbar-link-admin-people&ajax_html_ids%5B%5D=toolbar-link-admin-modules&ajax_html_ids%5B%5D=toolbar-link-admin-config&ajax_html_ids%5B%5D=toolbar-link-admin-reports&ajax_html_ids%5B%5D=toolbar-link-admin-help&ajax_html_ids%5B%5D=shortcut-toolbar&ajax_html_ids%5B%5D=edit-shortcuts&ajax_html_ids%5B%5D=page-wrapper&ajax_html_ids%5B%5D=page&ajax_html_ids%5B%5D=header&ajax_html_ids%5B%5D=logo&ajax_html_ids%5B%5D=name-and-slogan&ajax_html_ids%5B%5D=site-name&ajax_html_ids%5B%5D=main-menu&ajax_html_ids%5B%5D=main-menu-links&ajax_html_ids%5B%5D=secondary-menu&ajax_html_ids%5B%5D=secondary-menu-links&ajax_html_ids%5B%5D=main-wrapper&ajax_html_ids%5B%5D=main&ajax_html_ids%5B%5D=sidebar-first&ajax_html_ids%5B%5D=block-search-form&ajax_html_ids%5B%5D=search-block-form&ajax_html_ids%5B%5D=edit-search-block-form--2&ajax_html_ids%5B%5D=edit-actions&ajax_html_ids%5B%5D=edit-submit--2&ajax_html_ids%5B%5D=block-system-navigation&ajax_html_ids%5B%5D=content&ajax_html_ids%5B%5D=main-content&ajax_html_ids%5B%5D=page-title&ajax_html_ids%5B%5D=block-system-main&ajax_html_ids%5B%5D=ajaxdemo-html-ids-form&ajax_html_ids%5B%5D=edit-submit&ajax_html_ids%5B%5D=message_area&ajax_html_ids%5B%5D=footer-wrapper&ajax_html_ids%5B%5D=footer&ajax_html_ids%5B%5D=block-system-powered-byafter patch
ajax_html_ids=skip-link+overlay-disable-message+overlay-profile-link+overlay-dismiss-message+toolbar+toolbar-home+toolbar-user+toolbar-menu+toolbar-link-admin-dashboard+toolbar-link-admin-content+toolbar-link-admin-structure+toolbar-link-admin-appearance+toolbar-link-admin-people+toolbar-link-admin-modules+toolbar-link-admin-config+toolbar-link-admin-reports+toolbar-link-admin-help+shortcut-toolbar+edit-shortcuts+page-wrapper+page+header+logo+name-and-slogan+site-name+main-menu+main-menu-links+secondary-menu+secondary-menu-links+main-wrapper+main+sidebar-first+block-search-form+search-block-form+edit-search-block-form--2+edit-actions+edit-submit--2+block-system-navigation+content+main-content+page-title+block-system-main+ajaxdemo-html-ids-form+edit-submit+message_area+footer-wrapper+footer+block-system-powered-by#5
For file uploads, jQuery Form and our Ajax framework are using an iframe containing a textarea element that contains the embedded JSON returned from the server.
See http://api.drupal.org/api/drupal/core!includes!ajax.inc/function/ajax_deliver/8
If the HTML IDs are broken, then other returned JSON values are possibly broken, too. We should check for potential JSON encoding and decoding differences instead.
Updating jquery.form.js to a later version might also resolve the issue.
#6
@sun this issue about frontend and how jQuery Form serializes data before send (see ajax.js# Drupal.ajax.prototype.beforeSend )
As I written
ajax_html_idsdoes not send to Drupal when form hasencoding=multipart/form-dateI leave a performance tag for reasons of optimization POSTed data
There's already #1574560: Update jQuery Form Plugin to the latest in the git repository
EDIT if jQueryForm get updated to latest version (which uses JS strict) then ajax submission form with multipard encoding would fail with error:
An error occurred while attempting to process /system/ajax: Component returned failure code: 0x80460001 (NS_ERROR_CANNOT_CONVERT_DATA) [nsIDOMFormData.append]#7
re-titled
#8
re-roll chasing core
#9
Fixing tags per http://drupal.org/node/1517250
#10
Regarding #6, there's a pull request at https://github.com/malsup/form/pull/201/files
#11
not sure it's possible to backport to D7 as is but we can make a check to make sure that html_ids are passed as array and in cahe of string just decode them back to array
#12
#8: 1575060-htmlid-8.patch queued for re-testing.
#13
Re-roll to chase core.
#14
#13: 1575060-htmlid-13.patch queued for re-testing.
#15
Let's get this commited before feature freeze
#16
Can we reroll using querySelectorAll please?
35% faster is kinda significant since we have so many ids on a page usually.
http://jsperf.com/drupal-id-selector/2
(edit) Turns out there was a problem in my test, andypost fixed it, thanks :) new link added.
#17
Re-roll for qSA a lot of tests could be found in http://jsperf.com/drupal-id-selector/2
#18
tested and works very well :)
I changed the way we access
ajax_html_ids, JSHint was complaining. I wasn't too happy about changing the type ofajax_html_idsso I just used another variable, no functional change, just moving things around :)#19
forgot tag
#20
+++ b/core/misc/ajax.jsundefined@@ -284,10 +284,13 @@ Drupal.ajax.prototype.beforeSerialize = function (element, options) {
+ var ajaxHtmlIds = [];
Great! This should be a bit faster then access to array element
#21
I guess it's ok to go with this for now (since we need to backport it), but we should really try to get the PR in #10 in the upstream library, so that we can remove this workaround.
That is, because this workaround is a one-off solution for particular extra data we're sending, and it's perfectly possible that there are contrib and custom modules affected by this jQuery Form bug, too.
However, one last nit: The variable dump in #1 showed commas as separators, whereas the latest patch uses spaces. The HTML spec does not allow for commas in IDs, and passing a list using commas as delimiter appears much more natural to me (and also avoids URI escaping of spaces), so I wonder whether we could change the delimiter back to commas?
#22
@sun, about the space, thought that as well. Turns out, ",", ":" and some other weird characters are allowed in ids. Only the space is explicitly forbidden by the spec. http://www.w3.org/TR/html-markup/global-attributes.html#common.attrs.id
The nice benefit is how much the request size shrinks too.
#23
drupal_html_id() documents
- http://www.w3.org/TR/html4/types.html#type-name
- http://www.w3.org/TR/CSS21/syndata.html#characters
as the main constraints for HTML IDs. HTML5 is less restrictive, and this use-case is different, so hm, it's probably fine to diverge from drupal_html_id(). (though I wonder whether it wouldn't be helpful for future reference and readers of ajax.js to have an inline comment in there similar to the one in drupal_html_id() that explains the situation)
#24
We talked at length in IRC about large refactoring patches like this, and their likelihood to break things in core. We decided to tag issues like this with "Needs JS testing" and then zendoodles and her Merry Band of Contributors will test these patches manually (for now) and with automated testing (later!).
Tagging so she can find it. :)
#25
#10 just got fixed upstream
#26
New version contains the fix, see http://malsup.github.com/jquery.form.js
#27
Followed instructions in issue summary for manual testing. I'm getting same results with patch applied and not applied:
See screenshots attached.
#28
After checking up on this issue I can confirm that the behaviour described in the issue summary is correct, and that the patch works as intended.
I did additional testing on AJAX requests in Core and couldn't find anything broken by this patch. I couldn't find anything special in core that seems to rely on $_POST['ajax_html_ids'] that would be impacted by this patch.
#29
Thanks for the manual testing. Committed/pushed to 8.x.
#30
For D7 we can't use
document.querySelectorAll()so patch should use fastest approach from testing #17,also we should file a change-notice about browser compatibility and new format of
ajax-html-ids#31
Re-roll for D7
#32
#33
Sorry. Added simpletest to patch.
#34
We need David Rothstein confirmation on new format of data, or we just proceed with tremendous array for D7
#35
Making it official. :)
#36
If this comment from #11 is correct:
then that sounds like a much better way to fix this issue in Drupal 7 to me, rather than changing $_POST['ajax_html_ids'] for everyone.
For example, I just did a grep through a large codebase of an actual Drupal 7 site and found all sorts of modules doing things with $_POST['ajax_html_ids'] directly. These included Panels, Ctools, Views, etc. Although most didn't look like they were doing much with it except forcefully unsetting it (or resetting it to an empty array) in certain conditions, and I didn't look too far in depth into what they were doing, that's enough to give me pause about the idea of changing this from an array to a string in the middle of a stable release, especially if we don't have to.
#37
Just wanna to mention about unset used in contrib - this all caused by 2 bugs in core - this one and #1414510: Remove drupal_reset_static for drupal_html_id() when form validation fails
So contrib modules can't use this data because it could be easily broken (element IDs are changing) so this leads to #1305882: drupal_html_id() considered harmful
For D7 I see only one solution - implement a check for
$_POST['ajax_html_ids']that it is a string or array and in case on string make it array#38
Marked as duplicate #1811100: Impossible to remove when multiple forms with same file field
#39
I came across this bug when building a module that enables you to create entities via ajax. The #33 D7 patch seems to fix the problem for me. Thanks guys.
#40
I can confirm that the patch in #33 works and is badly needed when doing complex #ajax stuff. Let's please get this in 7 core.
#41
I think we can simply check recieved data and convert it into array if we got string.
Patch attached.
#42
The last submitted patch, drupal-ajax_html_ids-are-broken-for-forms-with-file-element-1575060-41.patch, failed testing.
#43
Ops, my bad. New patch attached.
#44
Because the contrib could have own fixes for the issue we should accept #43
While I think the the proper fix is #33 but a bit progressive as David pointed in #36 most of contrib just
unset($_POST['ajax_html_ids'])to recalculate IDs#45
This suits me—as long as those ids get converted to an array I'm happy and our module will be actually usable: Entity Dialog
#46
Closely related issue #799356: _form_set_class() is too aggressive in assigning the 'error' class needs to be commited after this change
+++ b/includes/common.incundefined@@ -3878,7 +3878,15 @@ function drupal_html_id($id) {
+ // For some reason (http://drupal.org/node/1575060) jquery.form.js can send string instead of array.
the line exceeds 80 chars, so here's re-roll
#47
Thanks @andypost!
Re-rolled patch with more clear comment.
#48
+++ b/modules/simpletest/drupal_web_test_case.phpundefined@@ -2154,9 +2154,12 @@ class DrupalWebTestCase extends DrupalTestCase {
- $extra_post .= '&' . urlencode('ajax_html_ids[]') . '=' . urlencode($id);
+ $ajax_html_ids[] = (string) $element['id'];
+ }
+ if (!empty($ajax_html_ids)) {
+ $extra_post .= '&' . urlencode('ajax_html_ids') . '=' . urlencode(implode(',', $ajax_html_ids));
Not sure that tests should work with wrong format. Let's revert this part or add a condition
#49
Agreed, I don't think we should make that kind of change to a fundamental test method like drupalPostAJAX() as part of this patch... that should really use the expected format. So, "needs work"?
(I'm not sure if it's worth a D7 test or not though, but if so we should use a separate method or do it with a conditional parameter as @andypost suggested.)
Also while we're at it:
+ // jquery.form.js may send to the server a string instead of an array+ // because of specific reasons (see http://drupal.org/node/1575060).
+ // In this case we need to convert a received string into an array
+ // in order to keep to the default behavior.
Those "specific reasons" aren't very specific :) How about:
#50
Re-rolled patch with new comments.
#51
+++ b/modules/simpletest/drupal_web_test_case.phpundefined@@ -2154,9 +2154,12 @@ class DrupalWebTestCase extends DrupalTestCase {
+ $ajax_html_ids = array();
foreach ($this->xpath('//*[@id]') as $element) {
- $id = (string) $element['id'];
- $extra_post .= '&' . urlencode('ajax_html_ids[]') . '=' . urlencode($id);
+ $ajax_html_ids[] = (string) $element['id'];
+ }
+ if (!empty($ajax_html_ids)) {
+ $extra_post .= '&' . urlencode('ajax_html_ids') . '=' . urlencode(implode(',', $ajax_html_ids));
suppose this part better to drop for D7.
Then we need to revisit this for D8 once the issue no more reproducible after upgrade of form library
#52
Droped test part.
#53
Just removed simpletest hunk
For D7 we just need to fix bug
Sorry for crosspost
#54
The last submitted patch, drupal-ajax_html_ids-are-broken-for-forms-with-file-element-1575060-52.patch, failed testing.
#55
Magically stopping the bot breaks status
#56
Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/005708c
#57
According #51, #21 we need to update D8 library to fixed version with commit https://github.com/malsup/form/pull/201/files
Also issue needs change notice about D8 ajax_html_id which now a string
Related issue added #1574560: Update jQuery Form Plugin to the latest in the git repository
#58
The fix in #56 did not resolve the issue. When I download and install ajaxdemo from #1 and test it with Drupal 7, I get
count $_POST[ajax_html_ids] = 1$_POST[ajax_html_ids] = array (
0 => 'skip-link,toolbar,toolbar-home,toolbar-user,toolbar-menu,toolbar-link-admin-dashboard,toolbar-link-admin-content,toolbar-link-admin-structure,toolbar-link-admin-appearance,toolbar-link-admin-people,toolbar-link-admin-modules,toolbar-link-admin-config,toolbar-link-admin-reports,toolbar-link-admin-help,edit-shortcuts,page-wrapper,page,header,logo,name-and-slogan,site-name,main-menu,main-menu-links,secondary-menu,secondary-menu-links,main-wrapper,main,breadcrumb,sidebar-first,block-search-form,search-block-form,edit-search-block-form--2,edit-actions,edit-submit--2,block-system-navigation,content,main-content,page-title,block-system-main,ajaxdemo-html-ids-form,edit-file,edit-submit,message_area,footer-wrapper,footer,block-system-powered-by',
)
The fix in #56 only works when
$_POST[ajax_html_ids] = 'skip-link,toolbar, ... 'which is not the caseAttached is a patch that rectify this. Again, this can only be manually tested.
#59
Tested with module #1
+++ b/includes/common.incundefined@@ -3888,14 +3888,14 @@ function drupal_html_id($id) {
+ if (strpos($_POST['ajax_html_ids'][0], ',') !== FALSE) {
=== FALSE - means no comma found
#60
Right, sorry for the mistake on the condition