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: Allow AJAX to use 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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

FileSize
18.63 KB
9.08 KB
745 bytes
2.54 KB

Proposed patch

Also attached a module to test

Screens of module
-- without file element $ids are passed as array
without file element

-- with file element $ids are broken
file element breaks

andypost’s picture

Issue tags: +Performance, +jQuery, +JavaScript, +Ajax, +jQuery UI

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

nod_’s picture

Component: javascript » ajax system

+1 much better. good to go for me.

I'll let the ajax maintainers have a look.

andypost’s picture

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

after 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

sun’s picture

Issue tags: -Performance, -jQuery, -jQuery UI

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

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.

andypost’s picture

Issue tags: +Performance, +jQuery

@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_ids does not send to Drupal when form has encoding=multipart/form-date

I 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]

andypost’s picture

Title: drupal_html_id() broken for forms with file element » ajax_html_ids are broken for forms with file element (encoding=multipart/form-data)
Issue tags: -Performance +frontend performance

re-titled

andypost’s picture

FileSize
2.54 KB

re-roll chasing core

cweagans’s picture

attiks’s picture

Regarding #6, there's a pull request at https://github.com/malsup/form/pull/201/files

andypost’s picture

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

andypost’s picture

#8: 1575060-htmlid-8.patch queued for re-testing.

mcjim’s picture

FileSize
2.54 KB

Re-roll to chase core.

andypost’s picture

#13: 1575060-htmlid-13.patch queued for re-testing.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this commited before feature freeze

nod_’s picture

Status: Reviewed & tested by the community » Needs work

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.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
2.64 KB

Re-roll for qSA a lot of tests could be found in http://jsperf.com/drupal-id-selector/2

nod_’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.59 KB

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 of ajax_html_ids so I just used another variable, no functional change, just moving things around :)

nod_’s picture

Issue tags: -Needs manual testing

forgot tag

andypost’s picture

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

sun’s picture

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?

nod_’s picture

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

sun’s picture

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)

webchick’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs JavaScript testing

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

attiks’s picture

#10 just got fixed upstream

attiks’s picture

New version contains the fix, see http://malsup.github.com/jquery.form.js

kbasarab’s picture

Followed instructions in issue summary for manual testing. I'm getting same results with patch applied and not applied:

See screenshots attached.

Kjartan’s picture

Status: Needs review » Reviewed & tested by the community

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.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks for the manual testing. Committed/pushed to 8.x.

andypost’s picture

Status: Patch (to be ported) » Needs work

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

serm’s picture

FileSize
1.51 KB

Re-roll for D7

andypost’s picture

Status: Needs work » Needs review
serm’s picture

FileSize
2.47 KB

Sorry. Added simpletest to patch.

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs committer feedback

We need David Rothstein confirmation on new format of data, or we just proceed with tremendous array for D7

[00:41] andypost: webchick: you have point about BC for http://drupal.org/node/1575060 I think this feature very internal and we can to send a minified version of IDs, are you agree?
[00:42] Druplicon: http://drupal.org/node/1575060 => ajax_html_ids are broken for forms with file element (encoding=multipart/form-data) => Drupal core, ajax system, normal, needs work, 30 comments, 6 IRC mentions
[00:42] webchick: andypost: hm. i trust David_Rothstein on those types of questions

webchick’s picture

Assigned: Unassigned » David_Rothstein

Making it official. :)

David_Rothstein’s picture

Assigned: David_Rothstein » Unassigned
Status: Reviewed & tested by the community » Needs review

If this comment from #11 is correct:

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

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.

andypost’s picture

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; remove ajax_html_ids to use GET (not POST) AJAX requests

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

andypost’s picture

sonictruth’s picture

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.

jeff h’s picture

Status: Needs review » Reviewed & tested by the community

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.

Spleshka’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.03 KB

I think we can simply check recieved data and convert it into array if we got string.
Patch attached.

Status: Needs review » Needs work
Spleshka’s picture

Status: Needs work » Needs review
FileSize
2.03 KB

Ops, my bad. New patch attached.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

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

sonictruth’s picture

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

andypost’s picture

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

Spleshka’s picture

Thanks @andypost!

Re-rolled patch with more clear comment.

andypost’s picture

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

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

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:

jquery.form.js may send the server a comma-separated string instead of an array (see http://drupal.org/node/1575060), so we need to convert it to an array in that case.

Spleshka’s picture

Status: Needs work » Needs review
FileSize
2.04 KB

Re-rolled patch with new comments.

andypost’s picture

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

Spleshka’s picture

andypost’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.08 KB

Just removed simpletest hunk
For D7 we just need to fix bug

Sorry for crosspost

Status: Reviewed & tested by the community » Needs work
andypost’s picture

Status: Needs work » Reviewed & tested by the community

Magically stopping the bot breaks status

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
andypost’s picture

Issue summary: View changes

Added ref to html5 specs

andypost’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

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

SebCorbin’s picture

Status: Fixed » Needs review
FileSize
1.25 KB

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 case

Attached is a patch that rectify this. Again, this can only be manually tested.

andypost’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Reviewed & tested by the community
FileSize
1.29 KB

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

SebCorbin’s picture

Right, sorry for the mistake on the condition

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work

OK, thanks for the extra testing. Hopefully this time fixes it for real. Committed to 7.x (with a small fix to grammar in the code comment) - thanks! http://drupalcode.org/project/drupal.git/commit/d6f960c

Back to Drupal 8, per #57.

andypost’s picture

Status: Needs work » Fixed
Issue tags: -Needs committer feedback, -Needs backport to D7, -Needs JavaScript testing

Thanx, David!!!
We can close the issue in favour of #1574560: Update jQuery Form Plugin to the latest in the git repository and #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests
Last one should file change notice when the route would be chosen

From IRC: <nod_> andypost: is updating the lib fixes the issue, sure I'm ok with it.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.