drupal_html_id does not work correctly in AJAX context with multiple forms on page

This happens in D7 as well still. http://api.drupal.org/api/function/drupal_html_id/7 like form_clean_id before it uses a static cache. A static cache is preserved only across the time it takes to generate a page. An AHAH/AJAX callback is a separate page therefore it starts with an empty static cache.

So when caching the form based on form_build_id, we cached along with it all the $seen_ids, it would allow drupal_html_id to start with those ids.

In ajax_get_form

  // Get the form from the cache.
  $form = form_get_cache($form_build_id, $form_state);
  if (!$form) {
    // If $form cannot be loaded from the cache, the form_build_id in $_POST
    // must be invalid, which means that someone performed a POST request onto
    // system/ajax without actually viewing the concerned form in the browser.
    // This is likely a hacking attempt as it never happens under normal
    // circumstances, so we just do nothing.
    watchdog('ajax', 'Invalid form POST data.', array(), WATCHDOG_WARNING);
    drupal_exit();
  }

  // Set drupal_static('drupal_html_id') from the cache here.
  $seen_ids = &drupal_static('drupal_html_id');
  $seen_ids = $array_from_form_cache;

Just have to figure out how to write and pull from the cache. Sortof build up a 'page id'.

related #684568: drupal_html_id() should use $drupal_static_fast pattern deals with using the drupal_fast_cache pattern.

Exotic idea, do hook_page_alter add in a JS setting that puts the unique cache_id (user session . REQUEST_TIME?) into the js settings and writes the data to the cache table.

edit: Decided I didn't like that idea, that was a little strange.

Subscribing for now. Will contribute ideas as I take time with this, but I'm also interested in Scott's and other ideas. I like the idea of a page build id, if we can come up with a core-worthy way of doing it. Such a thing would also help #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework as it would allow tracking of js/css already on the page, so only new js/css would need to be sent back. It's a concept I explored in a contrib module for D6: http://drupal.org/project/ahah_page_storage, but I'm not too happy with that particular solution; I think we can do better for D7 core.

Yes, badly needed. Also required for Dialog module.

Silly idea: see attached patch :)

Clever. I like it. I'd still like to work on a "page storage" idea for D7, so that only a single page build id needs to be transferred over the network rather than a potentially big string of all html ids, but that can be a future performance optimization issue, whereas #6 allows the bug to be fixed, so we should proceed with that.

Ideas for improvement of #6:
- add same code to drupalPostAjax(), so a test can be written.
- add a test.
- drupal_process_form() calls drupal_static_reset('drupal_html_id'). Ideally it should reset to $_POST['ajax_html_ids']. If we had confidence that system_init() occurred before the first time drupal_html_id() was called, it could just call drupal_static('drupal_html_id', $_POST['ajax_html_ids']);, and that would give us what we want. But is that assumption too brittle?

[EDIT: or, um, how about removing the code in system_init() and just making drupal_html_id() use $_POST['ajax_html_ids'] as the default value in the drupal_static() call?]

Also, I wonder if there should be an opt-out or opt-in mechanism. Consider a FieldAPI "add more" button. It's not going to render most of the form, but it does form_builder() the entire form. Do we really want all the element #ids to have a suffix? Also, what it does render will replace what is on that part of the page, so even for the part that's rendered, do we want a suffix on ids?

re EDIT in #7: yeah, just figured that, too :)

re #8: I have no idea what HTML ids are good for these days (other than JS behaviors attaching to dynamic ids). Thus, no clue. I could live without opt-in/out.

Actually, yeah, default value makes even more sense. :)

+++ misc/ajax.js	20 Mar 2010 17:44:43 -0000
@@ -204,6 +204,14 @@ Drupal.ajax.prototype.beforeSubmit = fun
+  // @see system_init()

s/system_init()/drupal_html_id()/

148 critical left. Go review some!

yay, this even works :-D (didn't actually test before)

of course, only stuff that's using drupal_html_id() gets the magic.

This still needs a test added, but how's this so far? A little more complicated than #12, but I think it's clear why.

I think I've read the new code + comments up to 3 times now, and can only slowly get behind the reason for that new parsing ;)

+++ includes/common.inc	21 Mar 2010 19:11:52 -0000
@@ -3332,14 +3371,13 @@ function drupal_html_id($id) {
+  $id = preg_replace('/\-+/', '-', $id);

I do not understand why we're munging the originally passed id here.... or perhaps, just now I understand, duplicate hyphens would break the explode() during initialization, so we need to prevent that here.

Overall, we can probably move most of the high-level docs contained as inline comments within this function into the function docblock, and rather replace those inline comments with explanations/reasonings for why stuff is done (and not what is done). :)

148 critical left. Go review some!

Made tests pass, and tried to improve comments as per #15. I don't think the comments are perfect yet, so suggestions welcome :)

+++ modules/simpletest/drupal_web_test_case.php	21 Mar 2010 21:09:06 -0000
@@ -1642,8 +1642,15 @@ class DrupalWebTestCase extends DrupalTe
+              foreach ($this->xpath('//*[@id]') as $element) {
+                $post['ajax_html_ids[]'] = $element['id'];
+              }

ajax.js puts the element ids into keys, i.e. an associative array. This here looks like it'd create a indexed array?

Powered by Dreditor.

ajax.js puts the element ids into keys, i.e. an associative array.

Nope. #12 did but I changed that in #14. Because the associative array technique from #12 doesn't really reconstruct $seen_ids correctly. Parsing is needed, and if that's the case, makes more sense to pass in a "plain" array.

Also, see #750008: Add test for HTML ID uniqueness. Quite a few ID uniqueness failures in HEAD. Still lots of code that does not use drupal_html_id(). For just one example, see #710172: HTML ids for blocks should not contain underscores.

Hmmm, interesting. Could this potentially result in a large amount of IDs being passed back and forth after a while?

The ids are only passed "forth" and not back, which is the fundamental idea. :) So this will result in potentially large $_POST arrays in AJAX requests, but the only limitation here is PHP's post_max_size, which defaults to 8MB, if I'm not mistaken.

My original idea was to store the ID's in a cache table. This has the advantage of feeling less hacky and could provide a mechanism for knowing what scripts and CSS have already been added to the page. Take this issue as an example: #745468: ctools_ajax_command_scripts() in ctools_ajax_render() causes unwanted scripts to be loaded (and potential js conflicts).

I know its off topic of this patch and I know Im adding some noise, but I think we are passing on an opportunity.

@Scott: I agree with you (see #7), but not a cache table, and instead a page_state table, just like we need to move $form_state away from a cache table: #512026: Move $form_state storage from cache to new state/key-value system. I think the place to refine the idea of a persisted $page_state for AJAX is #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework, and once we have a working solution of that, we can replace the use of $_POST['ajax_html_ids'] with it pretty easily. But in the meantime, proceeding with $_POST['ajax_html_ids'] lets us work on the other issues here, like the inconsistencies of when drupal_html_id() is called (#9/#19).

This looks RTBC to me.

+++ modules/simpletest/tests/common.test	23 Mar 2010 20:00:20 -0000
+++ modules/simpletest/tests/common.test	23 Mar 2010 20:00:20 -0000
@@ -742,15 +742,15 @@ class DrupalHTMLIdentifierTestCase exten

ah, we still need to amend this test case with a new test using drupalPostAJAX()

140 critical left. Go review some!

My thought for the test is to enhance drupalPostAJAX() to process the returned 'insert' commands and modify $this->content as ajax.js would. This would enhance our ability to do other AJAX testing too. Then, to add a assertNoDuplicateIds() method to DrupalWebTestCase, which can have similar code as in #750008: Add test for HTML ID uniqueness. Then, a specific test of a page callback that returns a page array containing 2 instances of the same form, where that form includes a simple AJAX-enabled button, and to call assertNoDuplicateIds() after a drupalPostAJAX() of each button: that's basically the test for the OD of this issue. Anyway, I was hoping to have that all done by now, but I haven't gotten to it yet. I plan on getting to it within the next few days, but just wanted to share these thoughts in the meantime. If someone has another idea for a test, by all means go for it, and I can add mine later, either as part of this issue or another one.

Here's my work so far. Part of why it's big is because it includes the patch from #656782: ajax_process_form() results in settings being returned for elements that aren't re-rendered as part of the AJAX request inside it: a necessary step for making the test in this patch work.

The basic test is form_test_two_instances(), which returns a page with two 'page_node_form' forms on it. Simple concept. Surfaces many issues. Seems to be working in manual testing, but the simpletest still needs troubleshooting. I'll pick this up again when I have a chance. Not sure yet how quickly that will be.

Chasing HEAD. This will fail, but it should at least apply.

Careless typo. This one should only have the new test fail. That's the one that still needs troubleshooting. As I said in #27, it works manually, so something in the new code that adds round-trip AJAX testing to the test framework is still buggy.

#656782: ajax_process_form() results in settings being returned for elements that aren't re-rendered as part of the AJAX request landed.

yep, working on a re-roll, cleanup, and trying to troubleshoot the remaining test failure now.

Still struggling with PHP's DOMDocument, but here's where I am so far.

Another re-roll for HEAD changes. Only the new test should be failing.

subscribe, drupal_html_id() was used in simpletest itself but now we removing this in #601398: Simpletest does not allow assigning actions to triggers

Finally figured out how to work around DOMDocument troubles: this one should pass!

Bumping to critical since waiting until Drupal 8 before we're allowed to have multiple node edit forms on a page seems like a really bad idea. We want a contrib module that implements a good AJAXy UI for nodereference fields. Other use-cases too plus HTML validation issues.

wow. :)

+++ modules/filter/filter.module	27 Mar 2010 00:18:34 -0000
@@ -1076,7 +1076,9 @@ function theme_filter_guidelines($variab
+  // @todo Fix filter.js to not rely on explicit ID.
+  $id = drupal_html_id('filter-guidelines-' . $format->format);

Let's create a follow-up issue for that and link to it from here.

+++ modules/simpletest/drupal_web_test_case.php	27 Mar 2010 00:18:35 -0000
@@ -637,6 +637,13 @@ class DrupalWebTestCase extends DrupalTe
+   * The DOMDocument object of the page currently loaded in the internal browser.
+   *
+   * @var string
+   */
+  protected $dom;

I think this should be @var DOMDocument

+++ modules/simpletest/drupal_web_test_case.php	27 Mar 2010 00:18:35 -0000
@@ -1609,7 +1631,7 @@ class DrupalWebTestCase extends DrupalTe
       if (!empty($form_id)) {
-        $xpath .= "[@id='" . drupal_html_id($form_id) . "']";
+        $xpath .= "[@id='" . $form_id . "']";
       }

Looks like we should get that other issue in ASAP.

+++ modules/simpletest/drupal_web_test_case.php	27 Mar 2010 00:18:35 -0000
@@ -2733,6 +2827,29 @@ class DrupalWebTestCase extends DrupalTe
+  protected function assertNoDuplicateIds($message = '', $group = 'Other') {

+++ modules/simpletest/tests/ajax.test	27 Mar 2010 00:18:35 -0000
@@ -204,6 +208,87 @@ class AJAXFormValuesTestCase extends AJA
+    $this->assertNoDuplicateIds(t('Initial page contains unique IDs'));

I think this method should be moved into the AJAXTestCase helper.

+++ modules/simpletest/tests/form_test.module	27 Mar 2010 00:18:36 -0000
@@ -1035,3 +1047,20 @@ function form_test_user_register_form_re
+  $node2 = clone($node1);

If I'm not mistaken, then we do not use parenthesis for clone.

139 critical left. Go review some!

Already fixed in #601398: Simpletest does not allow assigning actions to triggers

+++ modules/simpletest/drupal_web_test_case.php	27 Mar 2010 00:18:35 -0000
@@ -1609,7 +1631,7 @@ class DrupalWebTestCase extends DrupalTe
       if (!empty($form_id)) {
-        $xpath .= "[@id='" . drupal_html_id($form_id) . "']";
+        $xpath .= "[@id='" . $form_id . "']";
       }

Cleanup for HEAD changes. Also, once I figured out the DOMDocument::getElementById() problem in #40, I realized I could remove the changes to drupalSetContent() from #40, so this patch has less changes to DrupalWebTestCase. This covers most of #42.

From #42 (referring to new DrupalWebTestCase::assertNoDuplicateIds() method):

I think this method should be moved into the AJAXTestCase helper.

I disagree. We'll want tests other than AJAX to assert this. For example: #710172: HTML ids for blocks should not contain underscores. IMO, ideally we should assert this after every drupalGet() as in #750008: Add test for HTML ID uniqueness.

priority change was unintentional.

What's still needed here before it's RTBC?

I tested this and I think it good to go. Any other issues could be solved with follow ups

Good to go. Although it fishes stuff out of POST straight but then those are treated with a ++ so it's casted to int. No security weakness AFAIK.

Reviewing the patch without reading up on the issue, it is not at all clear what is going on, or why. We still need better code comments.

+++ includes/common.inc	28 Mar 2010 20:56:27 -0000
@@ -3318,13 +3318,62 @@ function drupal_html_class($class) {
+  // Upon first invocation, prepopulate HTML ids for AJAX requests.

Can we add a code comment describing why we do this?

+++ includes/common.inc	28 Mar 2010 20:56:27 -0000
@@ -3318,13 +3318,62 @@ function drupal_html_class($class) {
+    // $_POST['ajax_html_ids'] is a list of all HTML ids on the page. Direct use
+    // of $_POST is normally not recommended, but this usage cannot lead to
+    // security issues.

Can we add a comment describing why we add the IDs to the POST environment?

+++ includes/common.inc	28 Mar 2010 20:56:27 -0000
@@ -3318,13 +3318,62 @@ function drupal_html_class($class) {
+      // Each passed id needs to be parsed back into the original id and the
+      // optional counter that this function appended.

Can we add a code comment describing why we do this?

The comment additions requested by #49 only. Straight to RTBC for Dries to review.

I think we're really close. One more thing:

+++ includes/common.inc	6 Apr 2010 17:10:46 -0000
@@ -3321,13 +3321,73 @@ function drupal_html_class($class) {
+  // If this is an AJAX request, then HTML ids must not only be unique across
+  // this page request, but also not duplicate what is already on the base page.

I understand IDs need to be unique on any given page. However, the difference between "must not only be unique across this page request" and "not duplicate what is already on the base page" is not clear. What confuses me is "across this page request". What does it mean for something to be "across a single request"? I'm not native English so this might be a stupid language issue ... If so, sorry about that.

Well done. Not sure whether there's more we could explain. Even more high-level docs should rather go into a bird-level explanation of the AJAX framework functionalities on a d.o handbook page, I guess.

über-cross-post.

// If this is an AJAX request, then HTML ids must be unique for this request, but also take into account what is already on the base page.

...or similar...

This has the comment tweak for #51. It also includes a change to drupal_rebuild_form() to make the test from #50 pass. And while I was in testPreserveFormActionAfterAJAX() I saw a @todo linking to this issue (that I added from another issue) and decided to address it.

Bah. #55's change to drupal_rebuild_form() makes no sense. This removes that hunk and instead changes the test and adds a comment explaining it. The only difference this has vs. #50 is comments and modification to FormsRebuildTestCase::testPreserveFormActionAfterAJAX(), but leaving it as "needs review" so that the test change can be reviewed.

+++ modules/simpletest/tests/form.test	6 Apr 2010 19:17:48 -0000
@@ -810,35 +810,30 @@ class FormsRebuildTestCase extends Drupa
-    // Ensure that the form's action is correct.
-    $this->assertFieldByXPath('//form[@id="page-node-form" and @action="' . url('node/add/page') . '"]', NULL, t('Re-rendered form contains the correct action value.'));
+    // Ensure that the form's action is correct. The HTML id of the form has a
+    // "--2" appended, because the earlier AJAX submission generated new ids and
+    // the form's new generated HTML id got cached in the form cache and reused
+    // on the new page.
+    $this->assertFieldByXPath('//form[@id="page-node-form--2" and @action="' . url('node/add/page') . '"]', NULL, t('Re-rendered form contains the correct action value.'));

This patch cleans up this convoluted issue with the id by using the class instead. We just need a way of targeting the node form and not some other possible form on the page, so using the class is fine.

Powered by Dreditor.

#57: drupal.ajax-ids.57.patch queued for re-testing.

Back green.

Sidenote: If we would have had this idea some weeks/months ago, then Overlay most probably wouldn't be in core today the way it is. Among other minor issues, this was the major, #1 reason for the decision to go with Modal Frame API as base idea/approach for Overlay. This patch allows to turn pure awesomeness into reality via http://drupal.org/project/dialog, which also affects http://drupal.org/project/wysiwyg. With this patch, I can see real (as in: real) UX improvements drive Drupal's future.

I agree that this is ready. Committed to CVS HEAD.

Re #60: I hadn't started working on overlay yet when the iframe decision was made, but in my experience, loading independent page content into a div inside a page that isn't expecting it causes more problems than a simple id mismatch. We've run into this with the Drupal Gardens Themebuilder, which creates an admin interface at the bottom of the page. It's really, really difficult to keep the base page's CSS from affecting the Themebuilder interface. We eventually decided to put the admin interface into a div -- a decision I was happy about, because I hate iframes with a hot, hot hate -- but there's no way we could have done it if we didn't control every div and span and line of CSS in that admin interface. Placing an entire Seven-themed page inside a div in a Garland-themed page is a recipe for a total theming nightmare. And that's just Garland, which we do control. I can't even imagine doing it with a contrib theme as the base page.

I'm enthusiastic about the dialog project, and I bet wysiwyg will do fabulous things with it. But the overlay is more than a dialog box, and I believe iframes will continue to be the right solution for the problem overlay is solving -- even if I do hate iframes almost as much as blink tags.

Excellent find! Separate issue: #766146: Support multiple forms with same $form_id on the same page

Several follow-up issues related to the question of when drupal_html_id() should be used.
#684568: drupal_html_id() should use $drupal_static_fast pattern
#710172: HTML ids for blocks should not contain underscores
#755566: Use of ID within theme_filter_guidelines() breaks HTML validation with more than 1 field item on a page

Regression with file/image ajax uploads #770880: Files and images are not deleted after node saved

Essentially, it means that HTML IDs are unique on a page, but because they're unpredictable, they can never be used for *anything*.

No, they can't be used for hard-coded CSS or JS. IMO, IDs (except ones in page.tpl.php) should never be used for CSS, that's what classes are for, but I'm willing to hear arguments from a CSS expert as to why I'm wrong on that. IDs in JS make conceptual sense, and it's a shame code needs to be updated to not rely on hard-coded IDs, but I think that's the price we pay to use a system like Drupal that supports very dynamic output in conjunction with the HTML requirement that IDs be unique within a document. But there are things that still use IDs, like Drupal.settings, which is pretty major, and in conjunction with behaviors, is the recommended way to write JS to handle the dynamic nature of Drupal, so I don't think it's fair to say we lost them for *everything*.

why should the ID of a table change just because more items are added to it (as with the node-add form when you're using the "add-more". )

It's a shame that the implementation of the field.module "add more" button is to re-render the full field, rather than just to render and return the new item. Perhaps for D8, or in D7 contrib, we can improve this. But given that that's the implementation, then what's returned to the page is a *new* table to replace the *old* table, and therefore, it makes conceptual sense to me that it gets a new ID.