Remove Html::resetSeenIds() call during form processing

Clarifying issue title based on #2

oh, I just realized I opened a duplicate: #2034631: Remove drupal_reset_static for drupal_html_id() during form processing
Besides some tests would need to be addapted I'm not sure it's a simple fix to roll out for d7 though, as it will change HTML-IDs for some folks.

Here is my post from there:

So right now, form API resets drupal_html_id() IDs each time a form is processed (and there are no validation errors, thanks to #1414510: Remove drupal_reset_static for drupal_html_id() when form validation fails ). However, this is problematic if
- forms a processed during an ajax request. We do a lot of work to add in all past html IDs, increase the payload of ajax requests to have them, and then we reset the html IDs before the form is rendered? That's dumb.
- if multiple forms are used on page and some of them are always processed - this is a supported form API feature and used by exposed filter forms of Views:

 *   - always_process: If TRUE and the method is GET, a form_id is not
 *     necessary. This should only be used on RESTful GET forms that do NOT
 *     write data, as this could lead to security issues. It is useful so that
 *     searches do not need to have a form_id in their query arguments to
 *     trigger the search.

So once this feature is used you have a reset of your html IDs somewhere in the rendering pipeline (where the exposed views form is displayed), what in my case resulted in duplicate IDs and consequently in bizarre JS errors due to wrongly merged AJAX command settings (those are keyed by a now not more unique ID).

    // drupal_html_id() maintains a cache of element IDs it has seen,
    // so it can prevent duplicates. We want to be sure we reset that
    // cache when a form is processed, so scenarios that result in
    // the form being built behind the scenes and again for the
    // browser don't increment all the element IDs needlessly.
    if (!form_get_errors()) {
      // In case of errors, do not break HTML IDs of other forms.
      drupal_static_reset('drupal_html_id');
    }

So this creates troubles, and that "just" for being nice and not unnecessarily incrementing element IDs? Furthermore, unnecessarily incrementing the IDs will only happen if you process forms behind the scenes what is generally not best practice as you should use API functions instead.

So let's remove that troublesome reset.

It's about HTML-ids not $form_id though - so updating title.

So this creates troubles, and that "just" for being nice and not unnecessarily incrementing element IDs?

As tests have shown this is not just for being nice - I guess the comment was not clear to me here. So this patch makes sure we keep the html IDs during rebuilds, but instead of just resetting we better revert them to the previous state.

Patch of #7 was for Drupal 7. This one is for Drupal 8.

Patch of #7 works not as expected.

If we create views with 2 exposed forms, which of that has similar element id with different values (f.g. two node types use two taxonomy vocabularies, each of them consists brands. So field identifier obviosly should be brand_id or similar...).

All time form is submitted user will see 'An illegal choice has been detected. Please contact the site administrator.' message, despite of his selection.

This srting always returns true (form.inc line 1348)
elseif (!isset($options[$elements['#value']])) {
because $options consists elements of another form. But i haven't no idea why.

Any suggestions?

Patch #7 worked for me with Drupal 7.

I had multiple exposed forms on the same page and having the same item in multiple forms was causing problems. 'drupal_html_id' was being reset between each form, so I was not able to use it to create unique item IDs. With this patch, I was able to and my the forms work as expected.

Keeping the status as "Needs work" due to failed testing.

The patch in #7 works for me as well. Attaching a reroll; I'll move back to D8 after a test run.

The changes to autocomplete in Drupal 7.39 surfaced this issue in Drupal Commons. Commons provides a "browsing widget" which is a set of jQuery UI tabs, where each tab contains an AJAX view displaying nodes of a specific content type. Each content type view displays a form in the header which allows users to create a new node of the same type.

Many of the forms contain the same fields (title, tags, associated organic groups) which all receive the same IDs. While this is invalid HTML, with the autocomplete changes, having one or more autocomplete-enabled fields now results in an Uncaught TypeError: Cannot read property 'form' of undefined error which causes all JS to fail.

This is now about the call to Html::resetSeenIds();

Just tracked this issue down after an accessibility review.

Caused by 2 views exposed filters on the same page. During \Drupal\Core\Form\FormBuilder::processForm of the second form the Html::resetSeenIds() call clears the "edit-actions" id used as the container id for the actions element.

Here's a failing test. Let me know if this is completely in the wrong place!

Following what the patch in #9 was doing (moving the reset into rebuildForm) I've whipped up this. It makes the test in #21 pass and the rest of the form builder tests pass but lets see what else breaks.

Fixes test failures

Hey !

I'm currently experimenting the same problem on a project I'm on. The patch seems great but it doesn't pass on 8.3, the version I'm using. I'll try to modify it in order to import your tests too.

Is this is going to be merged? Few people actually seems to care, but this problem is a real accessibility killer…

Manually applied patch #26 onto the 8.3.x branch, recreated a patch that applies on 8.3.x. Please review and fix if necessary!

New patch added, let's see if the tests work this time.

@mvwensen nice! Can you provide an interdiff please?

Here you go!

Adding patch for D7 as a D8 backport.
Patch #14 was working but it was generating some troubles with contrib modules like captcha.

Can anyone review please? thanks!

Not sure but it seems disk was full:

...
11:00:48 Checks display of aggregator items 227 passes, 0 fails, and 0 exceptions
12:43:09 Build timed out (after 110 minutes). Marking the build as aborted.
12:43:09 Build was aborted
12:43:09 ERROR: Step ‘Archive the artifacts’ failed: no workspace for drupal_d7 #45453
12:43:09 Checking console output
...

Trying to enqueue it again.

#EDIT: First try yes, but second try disk was ok. Two errors detected, see https://www.drupal.org/pift-ci-job/800872.

Found some troubles with complex form like having nested paragraphs, here goes new version of the patch moving "drupal_html_id" reset to drupal_rebuild_form function.
Thanks Tunic for that test evaluation.

1. +++ b/includes/form.inc
   @@ -503,6 +503,16 @@ function drupal_rebuild_form($form_id, &$form_state, $old_form = NULL) {
   +  // drupal_html_id() maintains a cache of element IDs it has seen,
   +  // so it can prevent duplicates. We want to be sure we reset that
   +  // cache when a form is processed, so scenarios that result in
   +  // the form being built behind the scenes and again for the
   +  // browser don't increment all the element IDs needlessly.
   

   I know that this commend is just copy pasted, but it should be reflowed to be closer to 80 cols. It should also start with a capital letter, meaning that the first sentence should be different (because the we can't have the function start with a capital).

2. +++ b/includes/form.inc
   @@ -503,6 +503,16 @@ function drupal_rebuild_form($form_id, &$form_state, $old_form = NULL) {
   +  if (!form_get_errors()) {
   +  // In case of errors, do not break HTML IDs of other forms.
   +    drupal_static_reset('drupal_html_id');
   +  }
   

   The indentation here looks off.

borisson_ here goes new patch with the suggested improvements, thanks for the review.
Any other suggestion is welcome.

Awesome, looks good!

PS: Next time, please provide an interdiff as well as a patch to make reviewing easier.

Sure, thanks!

This should have test coverage if at all possible. Looks like there's rough tests already on the issue that didn't make it into the final patch.

Also, the patches since #31 have been for D7.
Hopefully the work done since then can be salvaged and put back into a workable D8 patch.

8.x code lives at https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Util...

There's still #2897377: $form['#attributes']['data-drupal-selector'] is set to a random value so can't be used for its intended purpose and #1090592: [meta] Use HTML5 data-drupal-* attributes instead of #ID selectors in Drupal.settings are both open related to this.

Applied #41 patch on 7.56 and works. As it was moved to "Reviewed & tested by the community" and the moved to "Needs work" for Drupal 8 I think we should include this fix in Drupal 7 and keep going with the Drupal 8 approximation.

re-rolled the patch to 8.5.x

fixed the minor glitch

Looks nice

How can we be sure that there are not unintended side effects? Are we testing some (other) scenarios of form ID names (already)?

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -378,6 +378,16 @@ public function rebuildForm($form_id, FormStateInterface &$form_state, $old_form
   +    ¶
   +    // \Drupal\Component\Utility\Html::getUniqueId() maintains a cache of
   
   +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -483,6 +483,36 @@ public function testUniqueHtmlId() {
   +    $form_arg_2 = $this->getMock('Drupal\Core\Form\FormInterface');  ¶
   +    $form_arg_2->expects($this->exactly(1))
   

   Minor: whitespace issues in these two hunks.

2. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -483,6 +483,36 @@ public function testUniqueHtmlId() {
   +   * Tests that HTML IDs are unique when building 2 forms with the same element.
   

   It is about elements with the same name, not necessarily the same element, right? Eg. they could be entirely different element types, etc.

I'm experiencing this problem now and it's a real accessibility killer. Is there any hotfix that I can use to resolve it? I'm on the current version of Drupal 8. For how long this issue has been open it really makes me think nobody cares about this problem.

@Couttsy there are patches in this issue that you can use to fix it. E.g the patch in #51.

Rerolled, fixed whitespace and comment issues from #53 and fixed deprecated calls to getMock()

Fixed failure in #58, it looks like the ids went back to their previous edit-type id, but given that it's obviously flakey to rely on the id, I've changed it to use the select label.

Looks good

Random media fail, back to RTBC

This is blocking a bug in Views, #2894747: Views hardcodes exposed filter block form ID's which breaks AJAX when the same form is shown multiple times on one page

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -385,6 +385,16 @@ public function rebuildForm($form_id, FormStateInterface &$form_state, $old_form
+    // result in the form being built behind the scenes and again for the
+    // browser don't increment all the element IDs needlessly.
+    if (!FormState::hasAnyErrors()) {
+      // In case of errors, do not break HTML IDs of other forms.
+      Html::resetSeenIds();
+    }
+

Pre-existing issue with the comment, or maybe due to the context change? The condition is that the form doesn't have any errors, but the comment is 'in case of errors'.

@catch I think the comment is correct, if somewhat clumsily worded. This comes from #1414510: Remove drupal_reset_static for drupal_html_id() when form validation fails where there are screenshots showing that the reset of ID's causes a break when done when there are errors.
So the comment should probably be something along the lines of "We only reset HTML ID's when there are no validation errors as this can cause ID collisions with other forms on the page otherwise.", but that should probably be follow-up?

Patch no longer applies. Ideally whoever re-rolls this could also make the comment change suggested in #68, or whoever commits this could do it on commit. Don't think we need a separate issue really but also don't want to hold this up any more.

Rerolled patch for 9.1.x along with the changes suggested in #68, please review.

Back to RTBC

Quick clean up for the comment being over 80 chars, leaving RTBC

Committed/pushed to 9.1.x and cherry-picked through to 8.9.x, thanks!

#3169918: data-drupal-selector isn't stable since 8.9.0
HTML IDs have been changed. Do we need a Change Record?