Cached render elements can have duplicate HTML IDs

This is because drupal_html_id() doesn't and cannot work with cached forms.
http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_h...

Probably needs to be something random instead of incrementing an int.

drupal_html_id() could append a hash or random ID instead, or just produce a unique hash.

A change to this function will almost certainly break core (and contrib) themes, but should we be relying on generated ID's for theming in the first place? (form classes could be used instead.)

Looks like this is depreciated in D8 https://api.drupal.org/api/drupal/core!includes!common.inc/function/drup...

So setting this back to D7.

The problem is still relevant to Drupal 8.

Same to me - we must match a very strong W3C Criteria and we sometimes have duplicate IDs. Mostly "edit-submit".

Running into the same issue with my search form block (which appears on every page) on any contact form in D8. The forms duplicate the id="edit-actions".

You should be able to single out the twig template for form actions and customize it for the site search form. You can use one of these methods: https://www.drupal.org/docs/8/theming-drupal-8/using-attributes-in-templ... to remove the id attribute completely or change it to something unique to avoid the duplicate id issue.

This problem also has an accessibility dimension. It's a WCAG failure at level-A, specifically F77: Failure of Success Criterion 4.1.1 due to duplicate values of type ID.

In practice though, duplicate IDs only cause an accessibility problem if something is pointing to a duplicate ID, such as afor or aria-labelledby attribute. Most browsers seem to use the first element with a matching ID in these cases, which can lead to problems such as the wrong labels being given to assistive tech, or the wrong checkbox bcoming checked when a mouse user clicks the label.

Note: although this is a requirement in the WCAG criteria and a HTML conformance requirement, it causes no harm accessibility wise unless the id value is being referenced by a relationship attribute such as for or headers or aria-labelledby etc.

Source: WCAG 2.0 Parsing Criterion is a PITA, Steve Faulkner (emphasis mine)

The place this is most likely to cause an accessibility problem is with generic IDs like edit-submit, which are repeated on multiple forms. Most other controls have IDs based on field names which are specific to one form.

As an example, there's a common design pattern for quick-search forms in site headers, which omits a <label> element for the text field, and instead it is named using an aria-labelledby reference pointing to the submit button. So there is a text input called search, followed by a button called search. This pattern is a perfectly reasonable one to use for accessibility. It isn't used by Drupal core's search block, but custom themes or modules might implement it. If submit buttons from different forms have the same ID, but different text, then a simple form like this could be incorrectly labelled.

Sure enough, this is a real accessibility issue.

A bit more info about the accessibility problems, after chatting with @Chris820 in the #accessibility channel on Drupal Slack. We looked at an D8 website from the Australian Government...

https://ministers.dss.gov.au/search/node (page archived at the wayback machine too, https://web.archive.org/web/*/https://ministers.dss.gov.au/search/node)

We discovered a particular scenario where duplicate IDs cause problems for the accessible name of form elements. Here's what happens:

1. When the core search block is present on the search page (/search/node), there are two text fields with the ID of "edit-keys".
2. The search block is in the header region, so it comes first in the DOM.
3. Both text fields have nearby label elements, using <label for="edit-keys">, but these labels contain different text:
   • The search block's text input has a visually-hidden <label for="edit=keys"> element with the text "Search".
   • The search page's text input has a <label for="edit=keys"> element with the text "Enter your keywords".
4. Using Android 8 + Chrome + Talkback screen reader, these text fields are announced as follows:
   • The search block's text field is announced as "Search, Enter Your Keywords, edit text", i.e. it gets both labels.
   • The text field from the search page gets neither label. It is just announced as an text box, without an accessible name. Without a label, it means nothing to a screen reader user, and a speech-control user may have to jump through some awkward hoops to focus the field.
5. Using a mouse, clicking on the visible label in the main content ("Enter your keywords") causes the text field in the header search block to gain focus. This isn't what you'd expect, going by spatial proximity. The wrong field got focus.This impacts two groups of disabled users:
   • Users with poor motor ability, who experience trouble using a pointer accurately. Focusing a text field by clicking a nearby label is normal behaviour. Users with poor motor control benefit from the extended click area provided by HTML labels, but this scenario behaves enexpectedly.
   • Screen magnifier users. Let's imagine they are zoomed in to view the search text field in the main content. They click on the nearby label, but the search text field in the header gains focus. Quite likely, this means that focus unexpectedly shifted to an input outside their viewport, and they are now lost. This is partly mitigated by the fact that some screen magnifier software can shift the viewport to follow form focus
     (e.g. macOS magnifier, and ZoomText on Windows).
6. Upshot: both labels have been associated with the first instance of <input id="edit-keys"> in the DOM order. The second text field has no associated labels. FOUR different groups of disabled users have a poor experience.

I expect this configuration, with a search block in the header of the search page, is pretty common scenario. So this isn't just a case of upsetting the validator - real human users will be affected too.

It's not clear how many browser + OS + screenreader combinations are affected. So far I've only had test Chrome + Android + Talkback, I'll try more when I get the chance.

In the case of the search forms, perhaps we could disambiguate them by making their Form API element IDs different? However that may be a backwards compatibility break. I presume lots of sites have form alter hooks targeting one or the other. It wouldn't be a solution for the general case of duplicated IDs.

Further Slack #accessibility chat...

I turns out that @chris820 on Drupal Slack is a.k.a. @lambch on Drupal.org. I'm giving them an issue credit here.

This needs an issue summary update. The original report was a bit rambling, but #11 has clear steps-to-reproduce.

Comments #2 and #3 suggest that we could use a random string as the HTML ID.

Comment #3 also points out that a random ID might break backwards compatibility. However the comment is over 5 years old, and we have removed IDs from CSS (and JS?).

Needs framework manager review, front and back, not sure which, probably both...

Can we change Html::getUniqueId() to produce a random URL every time, without breaking backwards compatibility? I note that in cases of isAjax it already produces a random base64 ID. Can't we just do that every time?

The practical accessibility problem in #11 is a must-fix.

I have no idea how the the current framework manager tags work these days. I think these tags are right, trusting the autocomplete.

Re: #8

remove the id attribute completely [ ... ] to avoid the duplicate id issue

No, we need to keep the ID for accessibility, so labels are correctly associated with inputs, using the <label for="IDREF"> mechanism.

Crediting @b_sharpe (a.k.a. @bryan on Drupal Slack) who was also involved in the recent Slack #accessibility chat. Their contribution was actually finding this old issue :-)

@andrewmacpherson If I understand what you are asking, you would like that Html::getUniqueId() always returns a randomized id, even for the first call for a given id and even outside the Ajax context, is that right?

I thought it was already the case but I ran some tests to understand how it works a bit better:

$ drush php
Psy Shell v0.9.6 (PHP 7.0.31-1+0~20180725100316.2+stretch~1.gbp8e8a8b — cli) by Justin Hileman
Drupal\Component\Utility\Html::getUniqueId('test');
=> "test"
Drupal\Component\Utility\Html::getUniqueId('test');
=> "test--2"
Drupal\Component\Utility\Html::getUniqueId('test');
=> "test--3"
Drupal\Component\Utility\Html::getUniqueId('test');
=> "test--4"

Drupal\Component\Utility\Html::setIsAjax(TRUE);
=> null
Drupal\Component\Utility\Html::getUniqueId('other-test');      
=> "other-test--M_iuC_fjLX4"
Drupal\Component\Utility\Html::getUniqueId('other-test');
=> "other-test--gm8z8uRVnkA"
Drupal\Component\Utility\Html::getUniqueId('other-test');
=> "other-test--QVyUh9Mtwtg"
Drupal\Component\Utility\Html::getUniqueId('other-test');
=> "other-test--JhDcxKa3VM0"

I already faced the #11 issue in one of my projects. It was happening when the first accessed page after a cache rebuild was not the search results page. Because when Drupal builds, let's say the home page, it only builds one search form then put it into the cache. Then, when someone goes to the search results page, where the same form is part of the page, the block is retrieved from cache so Html::getUniqueId() is not called for the form in the block. When the form in the page is built, Html::getUniqueId() is called for the first time so we get a duplicated id for all of its fields which lead to accessibility AND usability issues (you can never know where your focus goes when clicking on a label).

My own workaround was to add an url cache context to my block so it was rebuilt on the search results page but it would be really better to randomize the ids all the time.

As the ids are randomized on all ajax calls or can be incremented on all other calls, since Drupal 8.0.0, and as we can rely on data-drupal-selector to get the non-randomized non-incremented id, I can't see how it could be considered as a BC break to randomize in each scenarii.

Yeah, I guess what I'm worried about is whether always using a random ID will:

• Break anything in core. I know that Ajax already uses a random ID, and we've removed most IDs from core CSS and JS. But are all of our <label for> and aria-* references robust enough, in twig templates, etc? They must rely on the ID attribute, they can't make use of data-drupal-selector like JS.
• Be a risk to existing sites. There are probably plenty of IDs that never conflict, so they never get a suffix number in practice. So existing sites can probably get away with a selector like #edit-my-field-with-a-really-specific-name and never encounter a problem. Enforcing random IDs all the time could break their custom themes and behaviours.
• Likewise for contrib. We can reasonably expect maintainers to follow core change records, but site-owners won't necessarily update a module unless it's a security release.

My own workaround was to add an url cache context to my block so it was rebuilt on the search results page [...]

OK, that might be something we could do for the search block (and others?), if it's still OK for performance.

Meanwhile, we could try a simple patch to Html::getUniqueID(), and see if any existing core tests fail. That's a start, but I don't think we have much test coverage for our aria-* references.

We could put explicit aria-label on to some elements at the Form API stage, duplicating the existing label text. This would
address the problem of accessible names in #11.4, because aria-label would trump the <label for> in the accessible name computation. It wouldn't fix the label click problem in #11.5 though. Using ARIA when there's already a pure-HTML mechanism is a code smell too.

According to the ARIA Spec:

If the label text is visible on screen, authors SHOULD use aria-labelledby and SHOULD NOT use aria-label.

...but at least it's a SHOULD, and not a MUST. So not completely ruled out.

If patching getUniqueID to append randomBytesBase64 to all IDs, the likelihood of it being unique is pretty much assured. and may negate the purpose of getUniqueID in the first place - could deprecate a whole bunch of stuff in \Drupal\Component\Utility\Html

I do like the idea of adding a url cache context with hook_block_build_alter() as a workaround. Thanks DuaelFr. But as mentioned, is only a workaround, doesn't solve the larger issue.

This came up again tiday in the #accessibility channel on Slack.

The scenario this time was a pair if search inputs. One site-wide searchbox in the main navigation, and one on the search page itself. Both came from the same View which uses Solr.

I think the scenario of two search inputs like this is going to be a common one, whether it's core search, or search_api views.

@andrewmacpherson

This came up again tiday in the #accessibility channel on Slack.

...and what was the outcome of the Slack conversation? I'm noticing the same issue.

@zanematthew - The outcome of #26 is that we learned that this isn't isolated to one specific module. It's just as likely to happen whether you use the core search module or the Search API module from contrib.

There are several proposals for how to fix it in the comments here.

The person who mentioned it in Slack at the start of February hid their header search block on the main search page, if I recall correctly.

Fixing tag

Maybe this needs to be a thing where the current behavior of Html::getUniqueId() in Drupal 8 is deprecated, and there's a $setting that'll make it always append a random string to all IDs, so contrib and custom themes and stuff can get ready, and then in Drupal 9, that function is changed permanently to always append a random string (ie. the code for the deprecated behavior is removed) and the $setting is ignored.

I am experiencing this with Drupal 7.66, two forms on every page, search form and mailchimp subscribe block. I could not find any similar thread for D7 if anyone can direct me to one or suggest a solution other than hiding one form. Thanks.

@ahimsauzi There's workaround solutions. On one site, we implemented some preprocess hooks to add a random string to certain IDs that we know got duplicated. That should work in D7 or D8, but ideally, this should get fixed in core so that Drupal is accessible by default!

Our workaround is to ensure that all blocks are cached per page.

Having a new cached instance of a form (in a block) created for that page should ensure IDs are unique on that page. Basically not to use a pre-cached version... where it may or may not have the chance of a dupe ID.

In D8:

function THEME_block_build_alter(&$build, $block) {
  $build['#cache']['contexts'][] = 'url';
}

In D7, something similar should be possible using HOOK_block_info_alter() to set the ['cache'] = DRUPAL_CACHE_PER_PAGE;
That's just off the top of my head. Hopefully points you in the right direction.

Thanks @dsnopek in my case it seems that it is a search form in a block and a Mailchimp signup that co-exist on each page. Adding a random string can get slippery if there is some other process that might be depended on the original id no?

I am going to @lambch suggestion (thanks @lambach) and report back.

Adding a random string can get slippery if there is some other process that might be depended on the original id no?

Sure, it could be if you did it to all IDs. We're doing it for select IDs. However, if changing the cache contexts works for you, that means that the IDs will be changed (by adding an incrementing integer), so, in your case then it would seem that changing the ID is safe. :-)

Thanks for clarifying @dsnopek. I was about to try your suggestion since changing the cache contexts seems to work only on specific block leaving the possibility for block not included in the HOOK_block_info_alter() to be the duplicated one. can you share which preprocess/hook you are using? In my case edit-submit with its --# iterations is the culprit id. I was planing on using hook_form_alter() and target specific forms, but I'd love to know if there is a better preprocess to use.

Here's what we did on the project where we most recently ran into this issue:

/**
 * Implements hook_preprocess_container().
 */
function THEME_preprocess_container(&$variables) {
  if (isset($variables['element']['#id'])) {
    $id = $variables['element']['#id'];
    if (in_array($id, ['edit-actions'])) {
      $id .= '--' . Crypt::randomBytesBase64(8);
    }
    $variables['element']['#id'] = $id;
    $variables['attributes']['id'] = $id;
  }
}

We're just targeting the specific thing that was getting duplicated for this particular site (the 'edit-actions' ID on a container), to avoid breaking anything that might have targeted some other ID. So, you might have to implement a different preprocess, and certainly update the ID it's looking for.

I'm hoping that this can get fixed upstream in Drupal core at some point, though, even if that's Drupal 9 in order to deal with the problem of maybe breaking some existing sites.

Thanks for sharing @dsnopek, I'll try it in my D8 sites. For my D7 site I used hook_form_alter and had to to alter both field and submit action:

function HOOK_form_alter(&$form, &$form_state, $form_id) {
     if ($form_id == 'search_form') {
  		$data = strtolower(drupal_random_key($byte_count = 1));
  		$form['#id'] = $form_id . '--' . $data;
      $form['#submit'][0] = '_search_form_submit_function';
    }

function _search_form_submit_function(&$form, &$form_state) {
	$str = strtolower(drupal_random_key($byte_count = 1));
	$form['basic']['submit']['#id'] = $form['form_id']['#id'] . '--' . $str;
}

The above could probably be refactor some more, but it works with this following caveat:

With RWD where a form exists twice on the page, hidden for mobile devices, shown for desktop and vice versa, the accessibility scan still picks up the markup despite using aria-hidden="true" on one of the forms. The only way I can think of solving that is by creating a custom form, or using javascript.

I sure hope this issue will be solve for D8 and not get pushed to D9 since there are some law firms out there that having a field day with this.

I'd say the relevant component here is the form system, not cache?

Writing to confirm that we've run into this issue. The comment from #37 helped with many of the accessibility issues.

In the ajaxdependencies project, we created a formstate-stored unique form id and concatenated the array parents.

I've run into this issue. The comment from #37 helped, but it is just a workaround...

You may also run into this when you have a search block in the site header and are on the search page or are using an exposed filter in views that uses the search field. In these instances you'll have multiple inputs on the page with the id "edit-keys". Like the example above where you use template_preprocess_container to randomize the edit-actions container you can use template_preprocess_input to randomize this id as well:

/**
 * Implements template_preprocess_input().
 */
function my_theme_preprocess_input(&$variables) {
  if (isset($variables['element']['#id'])) {
    $id = $variables['element']['#id'];
    if (in_array($id, ['edit-keys'])) {
      $id .= '--' . Crypt::randomBytesBase64(8);
    }
    $variables['element']['#id'] = $id;
    $variables['attributes']['id'] = $id;
  }
}

There is one additional problem in this case, these fields have labels with a for="edit-keys" attribute and that will not be changed to match. To address this I added the following in my_theme.js:

(function ($) {
  Drupal.behaviors.search_keys = {
    attach: function (context, settings) {
      $("input[id^='edit-keys--']").each(function(index) {
        var $inputid = $(this).attr('id');
        $(this).prev('label').attr("for", $inputid);
      });
    }
  }
})(jQuery);

It feels a little dirty to make such a change in JS but it works for now. I do hope we can get this fixed in core soon.

Closing #1548976: Multiple forms on same page, duplicate HTML id after validation error as a duplicate of this one.

Transferred an issue credit for David Stosik who provided detailed steps to reproduce, on that issue.

In Drupal\Component\Utility\Html::getUniqueId() there's a comment which says that we're intending to go to unique IDs only. The linked issue is #1090592: [meta] Use HTML5 data-drupal-* attributes instead of #ID selectors in Drupal.settings which only concerns itself with JS behaviours.

I'm not clear if there are any blockers to using random IDs everywhere.

Closing #3029563: [DOM] Found 2 elements with non-unique id #edit-submit in favour of this one.

Transferring issue credit for @abhishekkumar (steps to reproduce) and @cilefen (lots of triage).

Closing #2938842: Duplicate ID on the login page in favour of this one. That issue was also about clash between the search block and login form. The search block seems to be the most widely reported scenario.

Transferring issue credits for @martin107 and @digitaldonkey. That issue got as far as committing a patch, but reverting it because it still didn't guarantee unique IDs.

A useful new test method was added recently - #3000762: Add assertNoDuplicateIds to a functional test trait. This lets tests ensure there are no duplicate IDs on the page.

It doesn't help us fix this issue, but it will help us write tests for this issue.

Adding the bug smash tag, because this has a ton of "needs..." tags. It's a serious accessibility issue (comment #11) but there's no consensus on the next steps.

Hi!

I think we are running into this too. In our case, we have:

• Header: Search block form, overridden to use a custom form with autocomplete functionality.
• Content: A view with a simple custom filter form (2 select elements and 1 group of checkboxes).

Both have #edit-submit on the submit button.

It may sound minor, but it's a major issue, as it is associating the search autocomplete functionality in the header with the views filters elements in the content. The end result is that JAWS thinks a regular select element in the views filters, is a combobox, which it is not, so it's pretty confusing to users.

We resolved this by hard-coding an id on the submit elements.

The worlds most sophisticated test.

Basically the steps from @Jacine in #62, place the search block on all pages and go to the search page and test for duplicate ID's => fail

Edit: the method for testing duplicate ID's doesn't exist in 8.9, so queued a test for 9.1

Hi @Lendude! Well played :)
Your patch only contains the code of the test, not the block configuration that you are talking about, though. Is this block already positioned on every page in this test context?

@DuaelFr yup, its inside the test for the search block, that already places that block on all pages

Adding the standard template to the IS, hoping it helps to identify what needs to be done.

Re-roll test only patch for 9.2.x

Linking open issues from the CivicActions Accessibility - VPAT.

Just ran into this, and did my best to update the issue summary after reading through all the comments.
The only viable idea presented so far is to randomize all IDs.
The only concern is about backwards compatibility.

Can we alleviate this concern, as @dsnopek suggested, by:

• implement a deprecation / api change notice
• change defaults for new installs
• make the new defaults opt-in for existing installs

?

This is general approach for backend API changes, and would be the most conservative.
IMO, one could argue that this change should fall under the frontend BC policy, and therefore we could implement this change in a minor release, skipping the deprecation and opt-in setting pieces. But that would be more aggressive.

All that said, I don't know what this means for feasibility of getting this backported into D7.

I'm not too happy about the idea of randomness, it's hard to test and then reproduce if there's a problem.
Why the IDs are not collected and bubbled up till the root (like the tags) and when a new ID needs to be generated it checks the already existing (collected and bubbled IDs) that are cached on the page? The only complication that I see here is that it may don't know all the cached items when a new item is generated, But it can simply prevent ID matches if we regenerate the matching block that has this issue.

What I noticed in our case was that the main content area was generated for us first then the header. While we had header cached already... So there's a good chance that sometimes the current code may not know that the feature will have something the same.

I just ran into this locally in 9.4.6.

If cache is locally disabled, 9.4.6 specifically works fine, generated incremented ids. If cache is enabled, duplicate ids.

This was happening for me with a custom module, plugin/block, extending BlockBase.

This effects block ids, as well as 'edit-actions' and 'edit-submit' ids.

This issue can also show up intermittently, depends on when the blocks are cached. If they are cached on a page with multiple ids, they will be incremented, however if the block is cached on a page where its id is unique, it will be cached with that id. Then potentially collide with other cached items.

It'd likely be better to just have ids generated as is, then tag the form id on the end- assuming the form id is unique per instance. If not... then form id and timestamp?

Adding tag for WCAG 4.1.2

I think this is a very serious issue. Especially if you are using modules like Fivestar or Rate where there may be a lot of forms on pages like Taxonomy pages where many nodes are shown with rating widgets.

This happened to me on Drupal 10.2.3 website and it makes the website search and rating system not usable for most users.

I haven't looked into whether this is viable yet, but we might be able to take a similar approach to #2463567: Push CSRF tokens for forms to placeholders + #lazy_builder - i.e. move the #id set in FormBuilder to lazily built placeholders.

They'd have to be uncacheable and rely on a static cache of IDs that have already been built, but we already rely on that in the current logic so it's not making it worse.

The case that this wouldn't cover is IDs added by AJAX, so is it really worth going to the trouble of a placeholder if it doesn't fix every case.

I think we should just switch to a random suffix here. Yes it's possible that people are relying on hard-coded IDs, but that code is already buggy since those IDs could have a counter appended, so it shouldn't stop us from fixing the core bug.

I just submitted the most basic MR implementing this idea, because I was running into the exact scenario of the duplicate search form. I did include the test patch, but nothing in the way of deprecation code yet. I did ponder it for a bit, but it is not evident to me how to deprecate this properly. Do we include a whole new function that people should call instead? If so, should this issue cover all core use of the old function?

Since I am having difficulty getting the issue reproduced (and the bug report in our project is a little old), submitting a branch with only the test changes to see if it is still broken.

With the latest patch from the MR, creating new ECA models is broken.

https://git.drupalcode.org/project/drupal/-/merge_requests/12595.patch

Steps to reproduce:

1. Install ECA with BPMNio modeler
2. Add model, add something so you can spot the output difference
3. Save model > you get redirected to the add-form again, nothing is saved

Surroundings:

• Drupal 10.5.8
• PHP 8.3
• ECA 2.1.17
• BPMN IO 2.0.11

Are ECA and BPMNio contrib modules?
I don't recognize those names as part of Drupal core.

There's already a lot going on in this very old issue.
Perhaps it's worth waiting until there's a "Needs Review" MR before introducing new tangents to the discussion.

@catch

I think we should just switch to a random suffix here. Yes it's possible that people are relying on hard-coded IDs, but that code is already buggy since those IDs could have a counter appended, so it shouldn't stop us from fixing the core bug.

Do you mean similar to what was done for #3570505: Navigation's top level menu items for workspaces and help have the same ID when caching enabled ?

Disregard my previous question. I see what Html::getUniqueId is currently doing for AJAX.

I'll see if I can put a new MR up as a test to see what breaks.

Ended up just rebasing !12595 onto main.

Updated MR looks really good to me, only thing I would maybe add is a comment (replacing the long current comment that's being removed) to explain that it always appends a random string. Something like 'Sanitizes the ID and also appends a random string so that IDs loaded via AJAX or render caching can't conflict with others on the page.'?

One of the failing tests is directly related.

I'll change it and look at the others.

Don't think it needs backport to D7 anymore.

I'm starting to question whether switching to a random suffix for all IDs really is the best approach. There are hundreds of test failures.

Not sure why I didn't see them right after the rebase. I checked a couple of them locally with a git bisect, and they go back to the original commit on the MR.

Is it possible to add a url cache context from within Html::getUniqueId, so that any element that uses it automatically gets the context? Something like mentioned in #76?

As noted in the IS, this issue goes beyond forms. It's happening on basic menu items in #3570505: Navigation's top level menu items for workspaces and help have the same ID when caching enabled .

Didn't mean to change the tags.

@kentr:

Thanks for working on this issue. We (i.e., Curtis) noticed the problem at work, and I am starting to look at it.

From Comment #99:

Is it possible to add a url cache context from within Html::getUniqueId, so that any element that uses it automatically gets the context?

I do not think so. The Html class is in the Drupal\Component\Utility namespace, and all the caching infrastructure is in the Drupal\Core\Cache namespace. Core classes can use Component classes, but not the other way around.

It would not be a small change, but one option would be to add an Html class in the Cache namespace (or somewhere under Core ... maybe Drupal\Core\Render?) that extends the Component class and overrides getUniqueId(). I think it would have to take a second object: the cacheable metadata, so that the new function could add a context to it.

On the related issue, someone pointed out that adding a random string makes it hard to test. An alternative is to add a timestamp: instead of the current search-form--2, we could use something like search-form--2--0.36384700-1772030125, using the output of microtime().

If it makes more sense to use the timestamp of the request, we could use the output of Drupal\Component\Datetime\Time::getRequestMicroTime(). That would give us something like search-form--2--1772030347.0322. I think the different Components are supposed to be independent, so Html::getUniqueId() cannot call that directly (and it cannot call Drupal::service() to get the time service). Instead, we could add a second, optional parameter to Html::getUniqueId() and use that as a suffix. If the parameter is not used, then default to microtime(). Code that calls Html::getUniqueId() could pass the result of getRequestMicroTime(). It could even pass something based on the pid of the PHP process.

I also notice this comment in Html::getUniqueId():

    // @todo Remove all that code once we switch over to random IDs only,
    // see https://www.drupal.org/node/1090592.

#1090592: [meta] Use HTML5 data-drupal-* attributes instead of #ID selectors in Drupal.settings looks like it stalled a long time ago, but I am adding it as a related issue.

One last point: we already use random string for Ajax requests:

  public static function getUniqueId($id) {
    // If this is an Ajax request, then content returned by this page request
    // will be merged with content already on the base page. The HTML IDs must
    // be unique for the fully merged content. Therefore use unique IDs.
    if (static::$isAjax) {
      return static::getId($id) . '--' . Crypt::randomBytesBase64(8);
    }
    // ...

@benjifisher

Thanks for the ideas.

The current MR removes the conditions that distinguish Ajax and returns the random string suffix for all requests.

The pipeline is showing 816 failed tests and 74 errors, though.

I fixed a handful of them locally and looked at some others, so I can say that some are directly related. I also re-ran the previous pipeline that had a lot of failures. At the moment, I looks to me like a pretty big change.

I agree that random IDs are hard to test. @golddragon007 also mentioned the difficulty with testing here (#76).

AFAIK, the XPath version that we use for PHPUnit doesn't have a great way to find these IDs with a random suffix (someone should confirm). Some IDs have another suffix after the random value like some-id--<random>-description, which I found more difficult. I found some methods, but they're verbose and / or imprecise. I think they could fail in some cases.

To me, random IDs are also hard to use in CSS and JS. I can imagine that switching to always-random would break some contrib.

Wouldn't appended timestamps also have similar problems?

Wouldn't appended timestamps also have similar problems?

It probably has some of the same problems, but it is easier to test.

Suppose that a typical call to getUniqueId() looks like this:

      $id = Html::getUniqueId('module-' . $key);

If we add an optional parameter, as I suggested, then we can inject the time service and change that line to

      $id = Html::getUniqueId('module-' . $key, (string) $time->getRequestMicroTime());

Then test code can inject a mocked version of the time service that returns something predictable.

Simpler:

We do not have to change Html::getUniqueId() at all. Just inject the time service and replace the call

      $id = Html::getUniqueId('module-' . $key);

with

      $id = Html::getUniqueId('module-' . $key . '--' . (string) $time->getRequestMicroTime());

Well, that is kind of awkward. So we want some service (which one? a new one?) that injects the time service and provides a new function (also called getUniqueId()?) that appends (string) $time->getRequestMicroTime()) and passes the result to Html::getUniqueId(). Then we can update existing code to inject the service and replace Html::getUniqueId(...) with $mystery_service->getUniqueId(...).

So we want some service (which one? a new one?) that injects the time service and provides a new function (also called getUniqueId()?) that appends (string) $time->getRequestMicroTime()) and passes the result to Html::getUniqueId(). Then we can update existing code to inject the service and replace Html::getUniqueId(...) with $mystery_service->getUniqueId(...).

I'm curious, is this so that it can be mocked in tests? Would it work with purely frontend testing frameworks (Nightwatch or Playwright)?

Can we mock Drupal\Component\Utility\Crypt::randomBytesBase64() in tests?

IIRC, some tests were failing with the new code b/c they're looking for an unaltered ID.

It seems as though the current code returns the original ID on the first run. So, my guess is that replicating that somehow in tests could make a lot of them pass.

@benjifisher:
Can you clarify WHAT kind of test do you have in mind? Sounds like it's functional tests.

Remember that we've got data-drupal-selector, and can leverage that.
For the BrowserTest classes, this is encapsuled in SelectionHandler class and surfaced by ::getElementById and friends.

I had tons of tests that broke when I moved a big project to random IDs. Swapping out SelectionHandler so that ::getElementById targets data-drupal-selector fixed them all.

Would a solution like this be sufficient for your concern?

Yeah I don't think using the timestamp will help functional/js tests at all and those will also be the ones with the problem.

And just FTR, IF we need more determinism, we can always delegate ::getUniqueId to a swappable service. No kittens harmed.

Related: #2346799: Replace #ID selectors for data-drupal-selector attribute

I think that if we change to always-random IDs, we should consider changing selectors from #ID to data-drupal-selector across-the-board in CSS, JS, and tests.

And then make it a coding standard.

That will be a big lift, so maybe a long-term goal.

Remember that we've got data-drupal-selector, and can leverage that.
For the BrowserTest classes, this is encapsuled in SelectionHandler class and surfaced by ::getElementById and friends.

I had tons of tests that broke when I moved a big project to random IDs. Swapping out SelectionHandler so that ::getElementById targets data-drupal-selector fixed them all.

Probably could do something similar for document.getElementById() in JavaScript tests, and override whatever handles this under the hood for \Drupal\Tests\UiHelperTrait::cssSelect().

FTR: FormBuilder::prepareForm adds data-drupal-selector (always, not only on ajax) since this issue.

Thought: We CAN easily add data-drupal-selector auto-filling to
- Renderer (for render arrays)
- Attribute helper class (for Twig)

And yes, this undertaking really deserves a PLAN issue now.

It looks like #2346799 (Comment #112) is a child of #1090592 (Comment #103). I have not looked closely, but I think those issues already describe part of what was suggested in Comment #109.

From #109:

Can you clarify WHAT kind of test do you have in mind? Sounds like it's functional tests.

I was thinking of PHPUnit tests: Kernel, Functional, and FunctionalJavascript. In all of those tests, you can override the time service so that getRequestMicroTime() returns something predictable.

From #107:

I'm curious, is this so that it can be mocked in tests? Would it work with purely frontend testing frameworks (Nightwatch or Playwright)?

Can we mock Drupal\Component\Utility\Crypt::randomBytesBase64() in tests?

I do not know whether the time service can be mocked in those frameworks.

I do not see "Crypt" in core.services.yml, so I do not think there is a service based on that class. (By contrast, the datetime.time service, which is what I mean by "the time service", is based on Drupal\Component\Datetime\Time.) I suppose that we could provide a service in a testing module and use that.

From #110:

Yeah I don't think using the timestamp will help functional/js tests at all and those will also be the ones with the problem.

As in my previous point, I think a timestamp is easier to mock than a random string. But maybe I have to look more closely at the FunctionalJavascript tests.

If you mean that it is hard to get JS (or CSS) selectors to target the right thing, then I agree.

Edit: strike out the suggestion about a test-only service.

From #115:

It looks like #2346799 (Comment #112) is a child of #1090592 (Comment #103).

I agree.

Completely different approach, but instead of randomly-generated IDs, can't we use lazy builders?

You can get a lazy builder to work on any string in the text, so we could output something like HtmlUnique-ID_STRING and then get the lazy builder system to replace those strings.

EDIT: @catch already mentioned this in #82. Sorry!

So it looks like I turned myself around in #82 and gave up on the lazy builder idea immediately after suggesting it, but now re-reading the past couple of dozen comments I'm wondering if it could work.

If we use a lazy builder, that would fix the cached HTML case - we'd never cache the suffixes, so we'd always check for duplicates and append a random string when necessary.

As #82 points out this won't work for AJAX requests, but we already append random strings on AJAX requests anyway, so no change there.

We could then open a follow-up plan issue to update core tests and etc. not to rely on IDs, and try to remove the random string handling again later on once that is done. But at least in the interim we'd have solved the duplicate IDs in a less intrusive way.

It will be important to have access to the form elements IDs in order to associate validation error messages with the invalid element in the final markup: #3083103: Programmatically associate error messages with inputs.

Is that possible with a lazy builder?