drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests

There are many side-effects of the mentioned change, and IMO it didn't accomplish some of the most important things it was to do.

Related:
#766146: Support multiple forms with same $form_id on the same page
#831626: ajax reloads break existing HTML ids

Marked #831626: ajax reloads break existing HTML ids as a dup

Another problem with drupal_html_id() is that if you're elsewhere in code and want to get the ID for a form element, you can't call it because that would generate you a new code rather than reproduce the one you're after.

Could say that this one is a duplicate of #956186: Allow AJAX to use GET requests. There are viable solutions to get rid of drupal_html_id() on the other thread, can one of you have a look please?

This also broken for some forms #1575060: ajax_html_ids are broken for forms with file element (encoding=multipart/form-data)

This is indeed a duplicate of #956186: Allow AJAX to use GET requests — however, the very limited scope of HTML IDs is helpful.

Perhaps it would best to turn the other into a meta issue and focus on the individual aspects to resolve bigger problem in separate issues (like this).

So when combining sorta both proposals from the OP, then we'd end up with something like this:

1) Append a request-specific nonce/hash to all IDs.

2) Still ensure that, within a single request, we don't produce duplicate IDs.

I performed some basic manual testing, and Ajax functionality seems to work as expected.

However, the new Views code contains a ton of code that seems to work around the entire Ajax/ID stuff, and I didn't have the passion to investigate what that code is actually doing and whether it can be removed or simplified with this. I added trailing white-space markers to this patch for those.

drupal_html_id() is a solution to a symptom, but it doesn't solve the root problem. The symptom is that there are elements with the same HTML ID on a page, the problem is that there are multiple elements that Drupal has not been given a unique identifier for.

Let's take the example of a default Drupal install (for simplicty, assume block cache is disabled). Drupal shows a login box to anonymous users, or a search box to authenticted users. Both of these have submit buttons with the ID 'edit--submit'. If I am working on the search form, it is very easy for me to use this ID in a CSS or JavaScript selector, not realising that I'm applying the same code to the login form.

Worse, if I make the search form available to anonymous users then they get unique IDs (edit-submit and edit-submit--2), but which is given which depends on the order they are rendered. If I put the login form first then the ID of the search form button will change depending on whether I'm logged in or not!

Since the ID can change so easily I cannot hard code it into a JavaScript or CSS selector. If I want to use it reliably I must pass it with or parse it from the block it is contained in. They may as well be entirely randomly generated.

By making it appear that it is static (e.g. "edit-submit"), we encourage people to write code that depends on it having that value, which then breaks when it gets a different value.

I propose (in this order):

1. Modules are encouraged to either remove the ID, or take their own steps to ensure the uniqueness of the ID (e.g. by allowing it to be user specified, or including the region name in it). This has to be the responsibility of the module, as drupal_html_id() can never have enough information to generate a sensible, consistent and unique HTML ID. If a piece of content can be included multiple times then it should be identified by the class.
2. We deprecate drupal_html_id()
3. When it does get called, it should return an entirely random string (e.g. using uniqid).

• This will solve any bugs about duplicate IDs being output when a page is not generated in a single request (e.g. ajax, block caching).
• It will prevent people from thinking that the ID is fixed when it is not.

Related issues

Problems with AJAX forms:
#1831560: Remove Html::resetSeenIds() call during form processing
#766146: Support multiple forms with same $form_id on the same page
#956186: Allow AJAX to use GET requests
#1575060: ajax_html_ids are broken for forms with file element (encoding=multipart/form-data)

Duplicate IDs:
#1852090: Cached forms can have duplicate HTML IDs, which disrupts accessible form labels
#1548976: Multiple forms on same page, duplicate HTML id after validation error
#1807870: Don't assume $form_ids are strings (function names) when building forms.

Removal of block HTML IDs for similar reasons (i.e. not being able to generate a consistent, unique ID):
#1880646: [meta] Update block HTML IDs now that they're plugins
#1989568: Remove block config ID from being used as an HTML ID or template suggestion

Code attempting to get a block delta from the HTML ID (not reliably possible):
#936798: Dashboard uses unreliable method for identifying blocks.

Code hardcoding output from drupal_html_id:
#1448668: password reset link doesn't work if login button clicked twice
#1268886: #ajax doesn't work if element CSS ID attribute set

#1575060: ajax_html_ids are broken for forms with file element (encoding=multipart/form-data) is fixed for d7 and d8 so now we need update #1574560: Update jQuery Form Plugin to the latest in the git repository and file change notice about what to do with the subject (drupal_html_ids)

Also we need change notice about d8 changes, see #1575060-4: ajax_html_ids are broken for forms with file element (encoding=multipart/form-data)
IDs are passed as string separated by space char, but in d7 they was passed as array.

No change notification tag until the issue is in.

Right....so I've separately tried to fix this issue in D7 and instead of appending a random hash to IDs, I just get the JS to send the highest appended ID on any ID on the page, then the server can seed the array of 'seen' IDs with that as a lowest value.

This means that IDs can get really high, but as they are basically random, that's okay.

I reckon that adding a nonce is probably better though, as the original patch does. I've just added this for reference, sorry for the noise.

Just setting to D7 to get the testbot to test it, I'll set it back after it's done.

Needed a re-roll for 7.23, but I'm going to set back to 8.x.

And...we still need work here.

Added related issue.

Adding related issue again. (Pasting an url didn't work as expected.)

Well, Just tried

function drupal_html_id($id) {
  return uniqid($id);
}

and it works like a charm on D8. No styling problems nor ajax mishap.

Patch a bit too brutal, might need to use the logic from #7.

Ok tried it again taking #7 logic, should be less test failures hopefully.

Bumping to major. Not sure I agree this is a real API change - if there's strong arguments that it is, we should probably bump to critical - the performance improvement is worth doing.

Arg, this is blocked on #1090592: [meta] Use HTML5 data-drupal-* attributes instead of #ID selectors in Drupal.settings we have some scripts relying on form ids still.

Here is a reroll. I had an idea on how to not trash all the JS ever with this change and make #2346799: Replace #ID selectors for data-drupal-selector attribute a little easier. It's to add a new data-drupal-id with the 'unprocessed' id so that we can still use it in the JS by changing selectors only.

Can you explain a bit more what #28 is supposed to do? It's the same as the patches in #18 and #21, but completely different from the preceding patches. It'd be great if you could explain it in the IS.

It's implementing solution 1. from the IS. The new data- thing might be better explained in the related issue about JS, not here.

Ok, then I don't understand why in addition to modifying Html::getUniqueId(), you're also adding data-drupal-id attributes? It's not being used anywhere? If that's for #1090592: [meta] Use HTML5 data-drupal-* attributes instead of #ID selectors in Drupal.settings, then that's out of scope, isn't it?

yup, but this patch is postponed on not breaking all our UI so it'd never get anywhere without a solution to the other issue. It's a lazy way to show there are things we can do.

For this patch you modify Html::getUniqueId and you're done, but everything else (including pretty much all of contrib JS) is broken so it kinda depend what we mean by scope.

Here is the single change, Let's see what breaks.

Breaking more stuff.

AFAICT — see #956186-61: Allow AJAX to use GET requests — this is the last blocker to using GET requests for Drupal's AJAX system instead of POST requests. Any chance we can still do this?

I think all of our js in core has been updated to not rely on this (although this needs to be verified), so it's just a case of removing the support. We need to make sure there's a change notice to point out this is explicitly not supported any more, but in terms of removing the code itself it's not disruptive at all, as can be seen by the patch diffstat.

Unfortunately that's not the case, there is still about a hundred id selector in our JS. Replacing them with data attributes should be fine overall but it'll be a huge deal for contrib.

Using data attributes we switch from "here is something you can use to select the element" to "explicitly add something to select this element". Which is what the data-drupal-id was supposed to be like in previous patches. This is the right issue to decide which solution to adopt for contrib. I don't think it's reasonable to say to contrib they have to add a new data attribute to select their stuff, core should provide one with the caveat that it's not guaranteed to be unique.

There is a jQuery way of selecting the start of an id, but it's really slow compared to a proper CSS query, core can't encourage that.

[…] but it'll be a huge deal for contrib.

Can you clarify why that would be a huge deal for contrib? In what way/to what degree would it be disruptive?

Take any module which declare or make use of a form and attach JS to said form. The JS will break.

Some examples:
http://cgit.drupalcode.org/admin_menu/tree/admin_menu.js#n61 (and probably line 73)
http://cgit.drupalcode.org/webform/tree/js take your pick.
http://cgit.drupalcode.org/google_analytics/tree/google_analytics.admin.js

That's a shame about core usage, quick grep reveals it is indeed still quite a few.

I'm not sure it's as broken as your comment suggests though, for two reasons:

1. Unless the markup gets updated by an AJAX response, the ID remains the same and will continue to work. So a custom form is going to be fine if it's not AJAX-enabled.

2. If an ID is targeted with js and drupal_html_id() changes it to #the_id-1 and then the JS will break anyway?

Discussed in IRC:

13:10 < Fabianx-screen> nod_: okay, so could we do:
13:10 < Fabianx-screen> Generate HTML IDs with -1 -2 -3 as usual for non-AJAX.
13:11 < Fabianx-screen> And random suffix for AJAX?
13:11 < Fabianx-screen> nod_: plus html id attribute, which you need anyway for good DX.
13:11 < Fabianx-screen> Keeps non-AJAX behavior the same, gets rid of html ids key to send for AJAX, has nicety new random behavior for ajax.
13:12 < Fabianx-screen> And data attribute is the correct way anyway, but can add without BC break.
13:12 < nod_> Fabianx-screen: i think the concern was around making sure we're in an ajax request an not a symfony subrequest or whatnot
13:12 < catch> New attribute we can add.
13:12 < nod_> Fabianx-screen: but I don't know the details, I think sun raised it
13:12 < Fabianx-screen> contrib disruption is minimal and AJAX + html IDS had been a nightmare anyway, so only improves it.
13:12 < Fabianx-screen> nod_: Well, we _do_ have a way to check that now.
13:12 < catch> Yeah I like splitting it like this.
13:12 < Fabianx-screen> I mean Wims libraries path will always be set, so that is simple.

The TL;DR is:

- Have a function to know if an AJAX request is active, e.g. pseudo: ajax_is_active(); Need Wims help here

- Split drupal_html_id() in two parts, use old code for !ajax_is_active() and new code with random IDs for ajax_is_active().

- Add a new data attribute: data-html-id='input-html-id' (e.g. without a suffix), this can be used to natively target any form ID

And that should be it.

#45 seems like a good way to keep backwards compatibility for existing half-working js while we get everything transitioning to not using ID selectors at all. Then we can start taking the IDs out of the HTML altogether.

#45 is the way to go to solve the AJAX problem and BC-break is minimal, but we have a way larger problem, because in Drupal 8 relying on a global static is impossible and this is border-line critical I am afraid (though I think we removed most IDs, such fortunately only major!):

- Block showing entity with ID 1 in 'special' view_mode in a side bar (<article id="entity-1">)
- Entity being displayed as part of a list in the main content area with ID 1 (<article id="entity-1">)

==

Depending on what gets cached first, the outcome is totally random.

For the case of the block being cached first (e.g. on another page), it got entity-1 as ID as there is no duplicate

Because the block comes from the cache, the entity-1 is also assigned to the content listing entity => BOOM, we have duplicate IDs.

We have three possibilities here:

a) drupal_html_id() is a "lie", is not working properly anymore and should just be removed and we live with duplicate IDs. (bad for HTML validation)
b) We choose to go all on out and use entity-1--random-suffix, such breaking _all_ of contrib (bad for contrib, thousands of test fails)
c) We use random suffixes only for AJAX / ESI requests and #post_render_cache to store a list of HTML IDs that need to be verified. Then a reg-exp to fix the duplicates with.

As #post_render_cache is run just at the last moment, the HTML IDs should be able to be verified properly.

c) could theoretically also be done without #post_render_cache and just checking all id=".*" attributes instead before rendering the response and having this #post_render_ache always run (so we don;t have to have that in all our arrays) [ This could help to support contrib code like: #id => html_id('x') . '-foo', which would break by storing the actual IDs.]

The long story short however is we need:

- 1. A data-html-id attribute that is non-unique, but exactly like the ID we set, for forms we can do that automatically.
- 2. Getting developers to understand that HTML IDs are neither good for CSS nor JS in a dynamic framework.
- 3. Some way to make HTML IDs unique and a combination of AJAX suffix (supporting the ajax_html_ids case) and a regexp to replace duplicates or #post_render_cache is the way to go.

----

I am gonna focus on #45 for now, but have written this all up to give attention to the larger issue we have here (even though as forms are currently not cacheable and Html IDs hopefully have been removed from entities) some of that is more a mis-use of this API and hence more theoretical than any practical impact.

Another way (that might be way easier) is to just add a magic string to all HTML IDs:

entity-1--html-id-

Then remove --html-id- from the first ID we see on the page (for each prefix) for the MainContentHTMLSubscriber.

That would remove BC break and test failures for the 90% case.

The scope of this issue however is to deal with the ajax problem, not solve everything.

Follow-ups needed, but out of cope here, are:

- Add data-html-id to all forms. (major)
- Somehow fix the problem of cached HTML IDs (postponed until forms are cacheable anyway)

--

Attached patch implements #45 with the minimum amount of code possible:

- ajax_html_ids=a,b,c => ajax=1 to uniquely identify an drupal.ajax ajax request
- check for this attribute
- Fix the unit tests, that test change is not ideal, but better than nothing.

Lets see how many failures this approach gives us for the other tests.

BooleanFieldTest was simple, but something is up with the file test.

There is a duplicate wrapper anyway, which makes this code very hard to understand where what ID comes from where.

Same with Dialog.

I guess both should not use IDs in the first place as e.g. views does.

Maybe someone else can figure this out.

@Fabianx:

Something is up with the file test. There is a duplicate wrapper anyway, which makes this code very hard to understand where what ID comes from where

See #1811100: File field AJAX wrapper always targets first instance on the page : the use of html IDs in the file upload form element is buggy atm (D8 and D7), it uses a drupal_html_id() that is never actually written in the HTML, and thus breaks the regular ajax logic around html IDs.

Reroll

I've come into this issue from a different angle as can be seen in this test...

https://qa.drupal.org/pifr/test/1015673

I wasn't aware of this API when I expanded the attribute handling system adding a class that asserted the uniqueness of the outgoing id's on all pages, Ajax or not. In theory, if all elements pass through the Template engines render method they'll have an Attribute object, which in turn passes all id's through AttributeNamedId. So I wrote that class to have an internal cache of all the id's it's seen. It keeps this cache as a protected static variable.

These issues are either related or redundant. I don't think I'm replicating this API though - AttributeNamedId only asserts uniqueness during development - it doesn't make that check during production. The assertion could perhaps be expanded to a full time system for this API to interface - but I worry about the performance hit of always doing that validation.

Note that assertion also checks the id's insure they are valid CSS identifiers.

Adding performance tag, the static cache showed up on #2488538: Add SafeMarkup::remove() to free memory from marked strings when they're printed as a contributor to high memory usage (all those checkboxes) although a small saving compared to the overall problems there.

Reroll of #58. Same tests should fail.

Fixed the dialog test, in a way…

The file widget stuff is strange. Id is not built the same between the input id and the label for attribute. Fixed the test so that it'll work when the two have actually the same values.

Removed some duplicate lines in a test.

The file widget stuff is strange. Id is not built the same between the input id and the label for attribute. Fixed the test so that it'll work when the two have actually the same values

Might be related to #55 ?
(--> #1811100: File field AJAX wrapper always targets first instance on the page)

Aside from the file upload field (still can't make it work, even with the proposed issue and it's trail) the JS is mostly working. There are issues with views ui since it relies on ids but it's not too involved to fix, the rest of core is mostly fine, what needs to be checked is filter module, ckeditor admin, field ui. Anything that has ajax stuff on the page basically. It's less bad than I remembered so I'm happy, this issue is definitely doable.

Need help on the file widget.

1. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -142,43 +142,14 @@ public static function setAjaxHtmlIds($ajax_html_ids = '') {
   +      return static::getId($id) . '--' . uniqid();
   

   I read how uniqid() works, http://php.net/manual/de/function.uniqid.php#95001 ... Wouldn't it be possible easily,
   there in that order of magnitude of speed, there could be introduced some collisions?

2. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -142,43 +142,14 @@ public static function setAjaxHtmlIds($ajax_html_ids = '') {
   +    // seenIdsInit is only useful for tests anymore.
        if (!isset(static::$seenIdsInit)) {
   

   Well, it sounds like we should kill it then.

The patch is much smaller than I expected, but I guess that's because not everything is working yet. This would be lovely to get done.

Also, @dawehner said in IRC that this is necessary if you want views that use the AJAX pager to still be fast (i.e. render cache hits).

1. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -35,11 +35,11 @@ class Html {
   +   * Contains if the current request was send via AJAX.
   

   s/send/sent/

2. +++ b/core/lib/Drupal/Core/EventSubscriber/AjaxSubscriber.php
   @@ -20,13 +20,15 @@
   +    if ($event->getRequest()->request->get('ajax', '') == 1) {
   

   This is a very strange if-test.

3. +++ b/core/misc/ajax.js
   @@ -394,16 +394,8 @@
   +    // Set that this is an drupal ajax request.
   

   Inform Drupal that this is an AJAX request.

4. +++ b/core/misc/ajax.js
   @@ -394,16 +394,8 @@
   +    options.data.ajax = 1;
   

   But why is this even necessary? Doesn't Drupal already know this because of the Accept header?

#73: Everything except #%^#%#$%#$%#$% file field ajax upload is working. And that is freaking hard to debug, because it is double broken ...

4. No, and we remove accept-header based things anyway, so better to make it explicit ...

So there is a simple API change here for javascript (use the right selectors).

But then likely more complex changes for file uploads.

Also in irc it was pointed out that AJAX views in 7.x work with output caching, but since we disable render caching on POST (to support form submissions), it won't work in 8.x.

And the much smaller AJAX requests (should be a real life improvement on slow connections since that is upstream bandwidth), and ability to cache AJAX responses that aren't form submissions more generally makes this a significant performance improvement once done.

So given all of these, I'm going to go ahead and bump it to critical.

But why is this even necessary? Doesn't Drupal already know this because of the Accept header?

Well that or the special query parameter ... But I agree with fabian, having a explicit parameter is a good idea

Well, it sounds like we should kill it then.

Let's keep it to reduce the amount of changes ...

Mh, at least for me locally, the test doesn't fail, yet.

Renamed 'ajax' to _drupal_ajax, seems to be a more specific term. Also introduced a constant for it.

Now that the form IDs are not deterministic again, its not that trivial anymore to select them.
I'm pretty sure though that you can solve it better.

+++ b/core/misc/ajax.js
@@ -377,6 +377,11 @@
+   DRupal.ajax.AJAX_REQUEST_PARAMETER = '_drupal_ajax';

CaSe IsSuE! ;)

Tricky ...

There are two problems: a) the case described in https://www.drupal.org/node/1305882#comment-9990789 actually happens.
It was just pure implementation detail that \Drupal\Component\Utility\Html::getUniqueId() of the <input> and the <label>
have been the same. I went with using Crypt::randomBytesBase64(8) as alternative but now we have two issues,
let's have a look at the HTML.

 <label for="edit-u9vtzkq1-0--eM4_8PBxUjM-upload">u9vtzkq1</label>
        <div class="file-widget form-managed-file clearfix">
  <div id="edit-u9vtzkq1-0--eM4_8PBxUjM-upload-ajax-wrapper"><input type="file" id="edit-u9vtzkq1-0-upload--AI6aosmQ-As" name="files[u9vtzkq1_0]" size="22" class="form-file">

So you see two things: a) the random suffixes are now different, as you expect In HTML::getUniqueId(), but NOT from an accessibility point of view.
and b) for some reason even the structure of the ID also doesn't match. It seems to be that we prepend '-upload' somehow, after generating the unique HTML ID.

This though works fine in HEAD:

 <label for="edit-es0a0r3e-0-upload">es0a0r3e</label>
        <div class="file-widget form-managed-file clearfix">
  <div id="edit-es0a0r3e-0-upload-ajax-wrapper"><input type="file" id="edit-es0a0r3e-0-upload" name="files[es0a0r3e_0]" 

as they don't have a randomized prefix.

Given that I'm not sure whether the approach actually works perfectly.

This is blocked on #2346799: Replace #ID selectors for data-drupal-selector attribute for the JS.

Alright, let's use some dirty tricks to ensure that we have the same ID all the time.
With that the $element['#id']-ajax-wrapper did not worked anymore, because $element['#id'] is NULL.
Given that nod_ and myself came up with the fact that all what matters here is that the ajax wrapper has a unique ID and its the same between the rendered HTML and the ajax settings.
So we now just generate a more or less random html ID: 'ajax-wrapper-$suffix' and be done.

Can't fault much here, mostly doc and test nitpicks

1. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -35,11 +35,11 @@ class Html {
   +   * Contains if the current request was sent via AJAX.
   

   Instead of contains could we use Stores, Flags or Determines?

2. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -142,43 +142,14 @@ public static function setAjaxHtmlIds($ajax_html_ids = '') {
   +    // seenIdsInit is only useful for tests anymore.
   

   Suggest 'seenIdsInit is now only useful during testing'

3. +++ b/core/lib/Drupal/Core/EventSubscriber/AjaxSubscriber.php
   @@ -20,13 +20,21 @@
   +   * Request parameter to indicate that a request is a drupal ajax request.
   

   drupal » Drupal ?

4. +++ b/core/modules/file/src/Element/ManagedFile.php
   @@ -144,6 +142,9 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
   +    // Generate a wrapper html ID. All we need is that its unique.
   

   its » it's
   html » HTML
   - although the text itself might read better as 'Generate a unique wrapper HTML ID'

5. +++ b/core/modules/file/src/Element/ManagedFile.php
   @@ -259,8 +260,12 @@ public static function processManagedFile(&$element, FormStateInterface $form_st
   +    // Let #id point of the file element, so the field label's 'for' corresponds
   

   %s/of/to

6. +++ b/core/modules/simpletest/src/AssertContentTrait.php
   @@ -822,14 +822,13 @@ protected function assertThemeOutput($callback, array $variables = array(), $exp
   -   * Asserts that a field exists in the current page by the given XPath.
   +   * Asserts that a field exists in the current page with a given xpath result.
   

   Any reason to change from XPath to xpath?

7. +++ b/core/modules/simpletest/src/AssertContentTrait.php
   @@ -822,14 +822,13 @@ protected function assertThemeOutput($callback, array $variables = array(), $exp
   +   *   Value of the field to assert. You may pass in NULL (default) to skip
   

   I think we need to prefix this with (optional)

8. +++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
   @@ -143,19 +143,17 @@ public function providerTestHtmlGetUniqueId() {
   +    $t = explode('--', $id);
   

   Should we use 2 as third param here for strict explode?

9. +++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
   @@ -166,10 +164,8 @@ public function testHtmlGetUniqueIdWithAjaxIds($expected, $source, $reset = FALS
   +      array('test-unique-id1--', 'test-unique-id1'),
   +      array('test-unique-id2--', 'test-unique-id2'),
   

   Should we have a test case that includes -- in the $source too, to see what happens in the edge cases

Any reason to change from XPath to xpath?

Nope

Should we have a test case that includes -- in the $source too, to see what happens in the edge cases

Sure, why not

Thank you @larowlan!!

#84 / ManagedFile : that's more or less what #1811100: File field AJAX wrapper always targets first instance on the page was doing. Don't we need to adjust the logic around FileWidget::process() & processMultiple() likewise, though ?

Interesting point!

#84 / ManagedFile : that's more or less what #1811100: File field AJAX wrapper always targets first instance on the page was doing. Don't we need to adjust the logic around FileWidget::process() & processMultiple() likewise, though ?

I'm not entirely sure. This workaround was needed, because suffixing to construct a different ID is a broken concept with randomness. The only thing which works relyably is to use the same value, using the reference. Using that though leads to $element['#id'] not being set yet. In FileWidget::process()/FileWidget::processMultiple() $element['#id'] already exists and is called through Html::getUniqueId() already.

There we go

Looking more at the JS side of the story it turns out that a subset of JS using ids will break, not everything that is listed in #2346799: Replace #ID selectors for data-drupal-selector attribute.

So I'd say we roll the JS changes in this patch and get the leftovers in the other issue, that way we can commit that one and keep things working. What do you think?

Added the JS changes required to make things work.

Only thing I couldn't find was in views ui, #edit-options-value-all I could not find what that was about. I'm almost convinced it's dead code.

Now everything works here but there are some ids left, a few of those are hardcoded in #prefix so no problems with them beside uglyness, for the others I'm guessing loading the admin pages in a modal would break everything. I'm guessing we will need to fix all the ids stuff to have something like panel work without issues. For the moment stuff is working in core.

I'm pretty sure its not dead code ... but rather broken code., see screenshot

Sorry my bad. I thought my Views UI breaking days were over :)

Improved the overall approach, making it way easier on contrib (and core) to do the reconversion from ids. I couldn't live with the views ui code handling the selectall stuff so I refactored. Still working fine.

All forms and form elements have a data-drupal-selector attribute added, which correspond to the old id (so, no random stuff added to it). Making the change in the frontend as trivial as:

// Changing 
$('#edit-refresh');

// to
$('[data-drupal-selector="edit-refresh"]');

In places where hardcoded ids are used, because they won't get randomized, I did not change the JS in order to minimize diff. We can take care of that in the related issue.

I also changed Drupal.ajax.AJAX_… to Drupal.Ajax.AJAX_… because that's not something to add to the factory. I'll file an issue for Drupal.ajax.WRAPPER_FORMAT afterwards.

Sorry my bad. I thought my Views UI breaking days were over :)

Yeah no idea, I remember that we fixed that at some point. Too bad that we still don't have any JS test coverage.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -629,6 +629,8 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
+      // Add a non-random identifier for use with JavaScript.
+      $form['#attributes']['data-drupal-selector'] = Html::getId($form_id);

I'm curious, are we okay with blowing up the form markup a bit more?

Right I tried that back in #28 but messed it up somehow. This time it seems to be working.

1. +++ b/core/modules/block/js/block.js
   @@ -63,7 +63,7 @@
   -      var table = $('[data-drupal-selector="blocks"]');
   +      var table = $('#blocks');
   

   Is that really what you wanted? So do we want to use selectors just for forms? I mean theoretically everything is embeddable into modals, for example

2. +++ b/core/modules/views_ui/js/views-admin.js
   @@ -950,25 +950,27 @@
      Drupal.behaviors.viewsFilterConfigSelectAll = {
   

   Thank you for fixing this!!

The id is set in #attribute, it won't be random, even when ajaxed. So I just left it there.

The id is set in #attribute, it won't be random, even when ajaxed. So I just left it there.

Fair, I mean I'm fine with using data selectors like that here, even it probably blows up the HTML a bit.
I mean there is no real alternative, what else do we need to move this issue forward?

We've implemented #45 and we agreed on that a few months ago and it looks like we're still agreeing on it but we need to check that the problems around caching outlined in #48 are not a problem anymore.

I'm pretty sure going all random all the time would be fine now that we have the data-drupal-selector attribute if it comes to that.

@effulgentsia and @dawehner talked about that issue and we were wondering, whether we should generate always a random hash ...
given that this removes the problem that people accidentally rely on it. In case you want IDs still, its not a problem in custom themes, because
there you have access to your templates and can do whatever you want, but at least for core and contrib modules it would be helpful to not break
on modals for example.

Does someone sees why we should not always append a random string?

I would make going all random a major follow-up. (and totally fine with that - given how pages are assembled now, it is a major bug indeed.)

The amount of test failures was kinda insane at the time we tried it however and it would be good to keep this patch focused on ajax_html_ids as that is the last thing missing for major front-end performance improvements and bringing back a fast views_pager.

Therefore I would propose:

1. Get this patch in (can we RTBC it, yet?) It looks ready to me.
2. Fix all #ids in core to data-drupal-select selectors (can be crowdsourc'ed to more contributors pretty easily)
3. Deprecate #ids officially, make data-drupal-select the only way and add a random suffix to all ID selectors (if not remove them all-together).

1. Get this patch in (can we RTBC it, yet?) It looks ready to me.

Here is a fix for the broken test.

2. Fix all #ids in core to data-drupal-select selectors (can be crowdsourc'ed to more contributors pretty easily)

Given that they are already potentially broken this sounds sane.

Ups.

The retest passed, so back to NR.

Little things needed, then RTBC.

1. +++ b/core/lib/Drupal/Component/Utility/Html.php
   @@ -142,43 +142,14 @@ public static function setAjaxHtmlIds($ajax_html_ids = '') {
   +    // seenIdsInit is now only useful during testing.
   

   Note: Those tests still exist and are at the moment still useful to test the non-ajax behavior.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/AjaxSubscriber.php
   @@ -20,13 +20,21 @@
   +    // Ensure that ajax=1 is set.
   

   nit - ajax=1 needs to be something like:

   "Pass through information that the request is an ajax request to the Html class"

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -629,6 +629,8 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +      // Add a non-random identifier for use with JavaScript.
   +      $form['#attributes']['data-drupal-selector'] = Html::getId($form_id);
   

   Once / If we remove #id we will have the same amount of markup in the forms again.

4. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -746,7 +748,14 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
   +      $element['#attributes']['data-drupal-selector'] = HTML::getId($unprocessed_id);
   

   nit - HTML vs. Html

5. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -746,7 +748,14 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
   +      // Add a non-random identifier for use with JavaScript.
   +      $element['#attributes']['data-drupal-selector'] = Html::getId($element['#id']);
   

   Should say something like:

   "For convenience reasons add the given ID also as data selector."

6. +++ b/core/modules/block/js/block.admin.js
   @@ -92,7 +92,7 @@
   -        $('#blocks').once('block-highlight').each(function () {
   +        $(context).find('[data-drupal-selector="blocks"]').once('block-highlight').each(function () {
   

   While this is "more correct" this is strictly speaking an unrelated change to take context into account.

   I am okay with it, just saying.

7. +++ b/core/modules/block/js/block.js
   @@ -34,9 +34,9 @@
   -      $('#edit-visibility-request-path').drupalSetSummary(function (context) {
   +      $('[data-drupal-selector="edit-visibility-request-path"]').drupalSetSummary(function (context) {
   

   Unrelated note: We should write a regexp to automate these conversions :).

8. +++ b/core/modules/views/src/Controller/ViewAjaxController.php
   @@ -133,7 +133,7 @@ public function ajaxView(Request $request) {
   -      foreach (array('view_name', 'view_display_id', 'view_args', 'view_path', 'view_dom_id', 'pager_element', 'view_base_path', 'ajax_html_ids') as $key) {
   +      foreach (array('view_name', 'view_display_id', 'view_args', 'view_path', 'view_dom_id', 'pager_element', 'view_base_path', 'ajax') as $key) {
   

   Needs to be _drupal_ajax or better yes the constant from AjaxSubscriber.

9. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -582,7 +582,7 @@ public function renderPreview($display_id, $args = array()) {
   -      foreach (array('view_name', 'view_display_id', 'view_args', 'view_path', 'view_dom_id', 'pager_element', 'view_base_path', 'ajax_html_ids', 'ajax_page_state', 'form_id', 'form_build_id', 'form_token') as $key) {
   +      foreach (array('view_name', 'view_display_id', 'view_args', 'view_path', 'view_dom_id', 'pager_element', 'view_base_path', 'ajax', 'ajax_page_state', 'form_id', 'form_build_id', 'form_token') as $key) {
   

   Needs to use _drupal_ajax or constant here instead of 'ajax'.

10. +++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
    @@ -166,10 +172,11 @@ public function testHtmlGetUniqueIdWithAjaxIds($expected, $source, $reset = FALS
    +      array('test-unique-id1--', 'test-unique-id1'),
    +      // Note, we truncate two hyphens at the end.
    +      // @see \Drupal\Component\Utility\Html::getId()
    +      array('test-unique-id1---', 'test-unique-id1--'),
    +      array('test-unique-id2--', 'test-unique-id2'),
    

    Not sure, but we might want to extend the test coverage here some more.

Note: Those tests still exist and are at the moment still useful to test the non-ajax behavior.

Updated the documentation and replaced it with a pointer to #1090592: [meta] Use HTML5 data-drupal-* attributes instead of #ID selectors in Drupal.settings

Once / If we remove #id we will have the same amount of markup in the forms again.

Well, we can't. We will always need it for the <label> tag: https://developer.mozilla.org/de/docs/Web/HTML/Element/label

While this is "more correct" this is strictly speaking an unrelated change to take context into account.

I am okay with it, just saying.

Well, I think doing the right thing here is okay, but we should actually do the right thing.
It should be 'edit-blocks' as selector, not 'blocks'. Manually tested that.

Not sure, but we might want to extend the test coverage here some more.

We already have some more test coverage for the non JS case, like ensuring that we just return valid IDs. Do you have something additional in mind?

All except #112.9 has been addressed.

#113: Yes, I checked again, the unit test coverage is good enough and better than before, not much too test anyway.

Hmm looking at https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label it's possible to do <label><input></input></label> - I'm wondering if we could use that mechanism by default rather than the for attribute. Obviously it'd still be possible to hard-code an ID and use for with that.

Didn't look into the implications of that though and I'm OK with the extra markup to resolve the critical - but could be worth discussing.

#115: Good point and markup is not frozen, yet, however removing all IDs is out-of-scope here anyway, so lets keep it in mind for the follow-ups.

We need:

- A change record for the ajax case and that its not safe to rely on IDs for that case (it never was, but hey!)
- A quick beta evaluation (summary of what catch wrote in the IS regarding views' pager performance)
- #112.9 fixed

Then RTBC :)

Opened that follow-up: #2503239: Consider using <label><input></input></label> instead of <label for="foo"></label>.

Needs to use _drupal_ajax or constant here instead of 'ajax'.

I'm sorry, the skipping hasn't been on purpose

Added a beta evaulation and a added a change record

@catch re #115 if at all possible it would be nice to have both and currently prefer to use for="id" in most cases.

Wanted to share (though I don't personally use bootstrap and avoid it at all costs), this styling is awesome and requires the for="id" approach:

https://github.com/flatlogic/awesome-bootstrap-checkbox

See the demo: http://flatlogic.github.io/awesome-bootstrap-checkbox/demo/

Tweaked the change record somewhere, looks all good.

=> RTBC - as promised

Disclaimer: I feel like I am able to RTBC this, because even though I worked on parts of the patch and provided initial architectural guidance, the patch has been verified, re-rolled, extended and checked by multiple people in the mean time - including the JS component maintainer - so it has gotten enough reviews.

This breaks the test coverage I'm adding in #2263569: Bypass form caching by default for forms using #ajax..

How are you supposed to use functions like:
drupalPostAjaxForm(..., $$form_html_id)
assertOptionSelected($id, ...)
assertOption($id, ...)

when the ID will have --randomstringhere appended? At least before I could pick the correct number.

Looking at $ grep -nr "assertOption.*--" *
I'm not sure how \Drupal\filter\Tests\FilterFormTest is passing right now

@tim.plunkett
Fair points. So this patch doesn't touch drupalPostAjaxForm because there is not a single usecase in core where something specifies an HTML ID
as part of that function call. Do you need that?

Regarding the other functions I guess none of those are used yet for tests of ajax functions. as well,
but I think we could convert all of them (which might be fun), to the new selector.

I'm not sure how \Drupal\filter\Tests\FilterFormTest is passing right now

It's passing because we don't use an Ajax request here but rather have multiple IDs on a normal request already, which doesn't change with the current patch.

Looks like this needs to either update or remove drupalPostAjaxForm() no?

Just trying something out for now.

For assertOptionSelected ... etc. we could just use the same argument and just rename it. For drupalPostAjaxForm/drupalPostForm the usecase is to be able to select beteween
multiple forms on the page. There data-drupal-selector would not be enough as its not unique. Instead I changed it in allow to an arbitrary xpath to select exactly what you want.

I'd need some time dig into whole Ajax system but looking at patch and #115. It seems the ID are useless. What blocking it to do this job in frontend ? eg:

     var $data = $(response.data);
      $('input', $data).attr('id', function() {
        drupalSettings.ajaxCounter = drupalSettings.ajaxCounter + 1;
        var newId = 'drupal' + drupalSettings.ajaxCounter;
        var id = $(this).attr('id');
        $('[for="'+ id +'"]', $data).attr('for', newId);
        return newId;
      });

I'd need some time dig into whole Ajax system but looking at patch and #115. It seems the ID are useless. What blocking it to do this job in frontend ? eg:

I don't know about accessibility, but I would assume that those systems potentially don't have javascript enabled?

let's fix stuff

let's fix stuff

Failed on one ...

Yeah! Do it on front end! It makes sense that the client keeps a count of the ID's or creates the random strings for them. Great outside the box idea @droplet! Should be considered in my opinion.

I don't know about accessibility, but I would assume that those systems potentially don't have javascript enabled?

Then they won't have AJAX either. Right?

#126 #135: While the idea of doing it on the front-end on ajax receive has some merits, the end result is the same:

- You end up with random IDs depending on which AJAX request fired first.

The whole notion that a CMS can know which IDs have been used and which not is flawed in itself. Unique, random suffixes are the sanest way to deal with that problem.

The whole idea that JS or CSS can rely on IDs is also in the Web 3.0 almost an impossibility:

Example, what happens if your targeted element changes from:

#myselector to #myselector--1

Or if a node that is shown in a list, is then shown also in another list, because it happens that recent articles and popular articles have the same node in there.

Then #article-123 suddenly is no longer valid and your whole styling fails. (seen on real life sites)

The faster we get people to understand that relying on IDs to target elements is a no-go in a highly dynamic system, the better. And the best way to do it is to make IDs change always, so they can't rely on them. :-D

And that is what the patch implements in baby steps by first taking care of the AJAX part of it :).

That's all true! I would not argue for counters. And it's also true that with the random string approach it doesn't really matter where it is inserted.

I'm just thinking that maybe the front end approach would give more control and options. With JavaScript you can do context aware stuff without sending markup information to the backend. Maybe it would be better if someone wants to customise the system later on for their peculiar needs.

Urgs, more fixes have been needed ...

Oh I retested on the qa.drupal.org

Frontend solution is definitely sleek but I'm not a fan, the less work on the frontend the better. I don't think it complicate things more than what's reasonable to let that be handled by the backend.

#138: Would it be possible to get one full interdiff to #121 (last RTBC patch)?

That would be great!

+++ b/core/misc/ajax.js
@@ -580,16 +587,8 @@
+    options.data[Drupal.Ajax.AJAX_REQUEST_PARAMETER] = 1;

Can we make this option (or adding another option) more explicit for random IDs only. I can imagine after few more refactoring, things will be diff than now.

We should not forcing everybody write custom Drupal-Specificed-Adoption-Code for other vendor libs all-time.

EDITED.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -629,6 +629,9 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
     if (!isset($form['#id'])) {
       $form['#id'] = Html::getUniqueId($form_id);
+      // Provide a selector usable by JavaScript. As the ID is unique, its not
+      // possible to rely on it in JavaScript.
+      $form['#attributes']['data-drupal-selector'] = Html::getId($form_id);
     }

@@ -746,7 +749,16 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
     if (!isset($element['#id'])) {
...
+    }
+    else {
+      // Provide a selector usable by JavaScript. As the ID is unique, its not
+      // possible to rely on it in JavaScript.
+      $element['#attributes']['data-drupal-selector'] = Html::getId($element['#id']);
     }

This looks like it sets 'data-drupal-selector' again to use the random #id for forms?

+1 to #143. How about renaming the boolean from Html::isAjax to Html::randomizeIDs, defaulting it to TRUE, and only setting it to FALSE in a subscriber that detects that 1) $request->getFormat() is 'html' and 2) no _wrapper_format is provided? And then in a followup, we remove that subscriber per the proposed resolution of "In a follow-up, consider making all IDs random-suffixed.".

#145: Switching, the order opt-out vs. opt-in, but with that fixing the 99% case?

Count me in! Brilliant suggestion and one parameter less!

+1 to #143. How about renaming the boolean from Html::isAjax to Html::randomizeIDs, defaulting it to TRUE, and only setting it to FALSE in a subscriber that detects that 1) $request->getFormat() is 'html' and 2) no _wrapper_format is provided? And then in a followup, we remove that subscriber per the proposed resolution of "In a follow-up, consider making all IDs random-suffixed.".

I really like this.

This looks like it sets 'data-drupal-selector' again to use the random #id for forms?

Good point, we can get rid of one of them.

We discussed this issue today with @alexpott, @catch, @webchick, @effulgentsia, and myself.

AJAX views pagers being essentially uncacheable means that the AJAX pager feature is pretty much broken, and that is a fairly significant regression for Views. In and of itself, the Views AJAX pager being completely nonfunctional would probably be a major issue for Drupal 8 core.

However, given the JS BC break that's necessary to resolve the issue, and given that there is no automated JS testing that would let sites and modules know that their JS was broken, it would be very disruptive to fix this issue in a patch release. For those reasons, we decided to keep this issue critical for the time being and thus keep resources on it. If it turns out that this issue takes longer to resolve and risks pushing back an RC some weeks from now, at that time we might choose not to block release on it.

Thanks everyone for the quick focus and iteration on this issue so far!

Updating the priority line in the summary with more detail, to clarify why the issue is marked critical.

Let's fix the remaining failures ...

Quick reroll.

Reroll, should fail like #154.

Overall fantastic work!

1. +++ b/core/misc/ajax.js
   @@ -580,16 +580,8 @@
   +    // Inform Drupal that this is an AJAX request.
   +    options.data[Drupal.Ajax.AJAX_REQUEST_PARAMETER] = 1;
   

   This is no longer needed now!

2. +++ b/core/modules/block/src/Tests/BlockViewBuilderTest.php
   @@ -79,6 +79,9 @@ public function testBasicRendering() {
   +    // Ensure to opt out of random HTML IDs.
   +    Html::setRandomIds(FALSE);
   

   Can we get an @todo to when random Html Ids are removed all-together?

3. +++ b/core/modules/block/src/Tests/Views/DisplayBlockTest.php
   @@ -95,10 +95,10 @@ public function testBlockCategory() {
   -    $elements = $this->xpath('//details[@id=:id]//li[contains(@class, :li_class)]/a[contains(@href, :href) and text()=:text]', $arguments);
   +    $elements = $this->xpath('//details[starts-with(@id, :id)]//li[contains(@class, :li_class)]/a[contains(@href, :href) and text()=:text]', $arguments);
   
   @@ -122,7 +122,7 @@ public function testBlockCategory() {
   -    $elements = $this->xpath('//details[@id=:id]//li[contains(@class, :li_class)]/a[contains(@href, :href) and text()=:text]', $arguments);
   +    $elements = $this->xpath('//details[starts-with(@id, :id)]//li[contains(@class, :li_class)]/a[contains(@href, :href) and text()=:text]', $arguments);
   

   Is it better to use data-drupal-selector-here?

4. +++ b/core/modules/field/src/Tests/Boolean/BooleanFieldTest.php
   @@ -171,7 +171,7 @@ function testBooleanField() {
   -      '*//input[@id="edit-fields-' . $field_name . '-settings-edit-form-settings-display-label" and @value="1"]',
   +      '*//input[starts-with(@id, "edit-fields-' . $field_name . '-settings-edit-form-settings-display-label") and @value="1"]',
   

   I wonder if there is a nice xpath expression to use for data-drupal-selector, though starts-with works for me.

   Edit looking further down it seems like all we need is exchange '@id' with '@data-drupal-selector'.

   So maybe better to use this here?

5. +++ b/core/modules/filter/src/Tests/FilterFormTest.php
   @@ -201,20 +201,20 @@ protected function assertNoSelect($id) {
        $passed = $this->assertTrue($select instanceof \SimpleXMLElement, SafeMarkup::format('Field @id exists.', array(
   -      '@id' => $id,
   +      '@id' => $drupal_selector,
   

   nit - use @data_drupal_selector here as well?

6. +++ b/core/modules/simpletest/src/AssertContentTrait.php
   @@ -1080,15 +1105,15 @@ protected function assertNoFieldById($id, $value = '', $message = '', $group = '
   +    return $this->assertTrue(isset($elements[0]) && !empty($elements[0]['checked']), $message ? $message : SafeMarkup::format('Checkbox field @id is checked.', array('@id' => $drupal_selector)), $group);
   
   @@ -1103,15 +1128,15 @@ protected function assertFieldChecked($id, $message = '', $group = 'Browser') {
   +    return $this->assertTrue(isset($elements[0]) && empty($elements[0]['checked']), $message ? $message : SafeMarkup::format('Checkbox field @id is not checked.', array('@id' => $drupal_selector)), $group);
   
   @@ -1128,15 +1153,15 @@ protected function assertNoFieldChecked($id, $message = '', $group = 'Browser')
   +    return $this->assertTrue(isset($options[0]), $message ? $message : SafeMarkup::format('Option @option for field @id exists.', array('@option' => $option, '@id' => $drupal_selector)), $group);
   
   @@ -1153,16 +1178,16 @@ protected function assertOption($id, $option, $message = '', $group = 'Browser')
   +    return $this->assertTrue(isset($selects[0]) && !isset($options[0]), $message ? $message : SafeMarkup::format('Option @option for field @id does not exist.', array('@option' => $option, '@id' => $drupal_selector)), $group);
   
   @@ -1181,15 +1206,15 @@ protected function assertNoOption($id, $option, $message = '', $group = 'Browser
   +    return $this->assertTrue(isset($elements[0]) && !empty($elements[0]['selected']), $message ? $message : SafeMarkup::format('Option @option for field @id is selected.', array('@option' => $option, '@id' => $drupal_selector)), $group);
   
   @@ -1206,16 +1231,16 @@ protected function assertOptionSelected($id, $option, $message = '', $group = 'B
   +    return $this->assertTrue(isset($elements[0]) && empty($elements[0]['selected']), $message ? $message : SafeMarkup::format('Option @option for field @id is not selected.', array('@option' => $option, '@id' => $drupal_selector)), $group);
   

   nit - @id => @data_drupal_selector

7. +++ b/core/modules/system/src/Tests/Ajax/DialogTest.php
   @@ -162,22 +166,23 @@ public function testDialog() {
   -    $form = $this->xpath("//form[@id='ajax-test-form']");
   +    $form = $this->xpath("//form[starts-with(@id, 'ajax-test-form')]");
   
   @@ -193,7 +198,7 @@ public function testDialog() {
   -    $form = $this->xpath("//form[@id='contact-form-add-form']");
   +    $form = $this->xpath("//form[starts-with(@id, 'contact-form-add-form')]");
   
   +++ b/core/modules/system/src/Tests/Ajax/MultiFormTest.php
   @@ -58,10 +58,9 @@ function testMultiForm() {
   +    $form_xpath = '//form[starts-with(@id, "node-page-form")]';
   

   Should we use data-drupal-selector here?

8. +++ b/core/modules/views/src/Controller/ViewAjaxController.php
   @@ -133,7 +135,7 @@ public function ajaxView(Request $request) {
   -      foreach (array('view_name', 'view_display_id', 'view_args', 'view_path', 'view_dom_id', 'pager_element', 'view_base_path', 'ajax_html_ids') as $key) {
   +      foreach (array('view_name', 'view_display_id', 'view_args', 'view_path', 'view_dom_id', 'pager_element', 'view_base_path', MainContentViewSubscriber::WRAPPER_FORMAT) as $key) {
   

   That is cheating ;) - I am okay with that little bug fix being hidden in here though.

9. +++ b/core/modules/views_ui/js/views-admin.js
   @@ -950,25 +950,27 @@
   -      $(context).find('#views-ui-handler-form div.form-item-options-value-all').once('filterConfigSelectAll')
   -        .show()
   -        .find('input[type=checkbox]')
   -        .on('click', function () {
   -          var checked = $(this).is(':checked');
   +      var $context = $(context);
   +
   +      var $selectAll = $context.find('.form-item-options-value-all').once('filterConfigSelectAll');
   +      var $selectAllCheckbox = $selectAll.find('input[type=checkbox]');
   

   While that works and is likely more robust, I wonder if simply replacing #... with form[data-drupal-selector="views-ui-handler-form"] would not be simpler to review?

10. +++ b/core/modules/views_ui/src/ViewUI.php
    @@ -582,7 +584,7 @@ public function renderPreview($display_id, $args = array()) {
    -      foreach (array('view_name', 'view_display_id', 'view_args', 'view_path', 'view_dom_id', 'pager_element', 'view_base_path', 'ajax_html_ids', 'ajax_page_state', 'form_id', 'form_build_id', 'form_token') as $key) {
    +      foreach (array('view_name', 'view_display_id', 'view_args', 'view_path', 'view_dom_id', 'pager_element', 'view_base_path', MainContentViewSubscriber::WRAPPER_FORMAT, 'ajax_page_state', 'form_id', 'form_build_id', 'form_token') as $key) {
    

    :-D

11. +++ b/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php
    @@ -186,6 +196,7 @@ public function providerTestHtmlGetUniqueIdWithAjaxIds() {
    +    Html::setRandomIds(FALSE);
    

    This should no longer be needed, it was my try to reset the value to the old one, but setup takes care of that now.

---

Food for thought:

dawehner had concerns that this patch gets really big and the test changes are necessary but partially unrelated to the changes here.

Proposal:

- Split out creation of data-drupal-selector and conversion of tests to use it to another issue as its (besides the introduction of data-drupal-selector, which can be justified based on this issue) test-only changes and hence unfrozen changes.

Then this issue will get easy to review. We also ensure breakage is at a minimum.

Given this is critical triage deferred, I think that would be a good way to get this in.

Then this issue will get easy to review. We also ensure breakage is at a minimum.

Ha, actually its the opposite.

- Split out creation of data-drupal-selector and conversion of tests to use it to another issue as its (besides the introduction of data-drupal-selector, which can be justified based on this issue) test-only changes and hence unfrozen changes.

To be clear, why we need so many changes in tests is that we potentially generate random IDs now. With the introduction of #145 a lot of test code now generate random IDs,
so everything since #147 for example can't be removed.

I'd argue that 80K is still in a reviewable size, especially because many changes are just dump.

Review points 3 - 6 ... I went with the IMHO minimal change, which is still using @id, but if you think, sure we could change that.

Let's first talk about the last remaining failure, this is less straightforward than you think. So this is a $form in a block, so we expect data-drupal-selector to be on the form element.
It turns out, with this patch it is not. Let's see why. In \Drupal\block\BlockViewBuilder::buildBlockwe move #attributes basically from $form one level up, so it vanishes.
There is of course a workaround, but on the other hand, doing that feels wrong in the first place. Should we just go with a documented workaround for now?

I am totally fine to continue here, was just thinking of other possible ways:

- starts-with vs. data-drupal-selector -- no hard feelings, can do the minimum, even though I would argue that @id -> @data-drupal-selector is simpler and more correct than adding starts-with.

- @id vs. @data_drupal_selector in ::format() and friends, lets be consistent here.

--

Lets go with a documented work-around, ID also was on the block and not on the form or is ID special cased?

Thank you for your review btw. will come to it later.

- starts-with vs. data-drupal-selector -- no hard feelings, can do the minimum, even though I would argue that @id -> @data-drupal-selector is simpler and more correct than adding starts-with.

Fair, I think its actually what you want, as data-drupal-selector will be the thing which will be used in JS. Let's get rid of the IDs, while we are here.

Lets go with a documented work-around, ID also was on the block and not on the form or is ID special cased?

There is #attributes, #contextual_links and #id and just the former two will be moved up.

#147: I think we want to keep that

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -629,9 +629,6 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
-      // Provide a selector usable by JavaScript. As the ID is unique, its not
-      // possible to rely on it in JavaScript.
-      $form['#attributes']['data-drupal-selector'] = Html::getId($form_id);

If I go to "Configure filter criterion: Content: Type", I see form attributes id="views-ui-config-item-form--y11cpM3mcL0" and drupal-data-selector="views-ui-config-item-form-y11cpm3mcl0".

#158.9: Should we add that form[data-drupal-selector="..."] anyway?

If I go to "Configure filter criterion: Content: Type", I see form attributes id="views-ui-config-item-form--y11cpM3mcL0" and drupal-data-selector="views-ui-config-item-form-y11cpm3mcl0".

Well, then I simply don't get what you wanted to point out as part of your review ...

Note: Ideally we should add a Html::getSelector() or something which does the same as Html::getId() but with a little bit more semantic meaning.

That is cheating ;) - I am okay with that little bug fix being hidden in here though.

Haven't seen anything :P

This should no longer be needed, it was my try to reset the value to the old one, but setup takes care of that now.

Well, that test wants to disable random IDs, the setup method just sets resets it to TRUE.

While that works and is likely more robust, I wonder if simply replacing #... with form[data-drupal-selector="views-ui-handler-form"] would not be simpler to review?

#158.9: Should we add that form[data-drupal-selector="..."] anyway?

Totally agree ... as its turns out, some logic was broken in FormBuilder so that we actually generated random data-drupal-selector attributes as well

Re-testing to see how much we break Tim's test coverage now.

Let's just do a reroll for now.

Let's see how much more elements we fix with that.

A note of caution where we are converting POST to GET, it's more likely to leak secret tokens into log files

Also, realize this code is not new in the patch, but this line should be fixed:

+    $form['#attributes']['data-drupal-selector'] = $form['#id'] = Html::cleanCssIdentifier('views_exposed_form-' . SafeMarkup::checkPlain($view->storage->id()) . '-' . SafeMarkup::checkPlain($display['id']));

Since SafeMarkup::checkPlain() will store the 2 strings that are immediately concatenated and never printed on their own. It also looks like cleanCssIdentifier will make the string safe so those calls aren't needed at all.

To be clear, all the changes since #119 would be ideally needed, as for examples on radios we don't produce the right data-drupal-selector entries,
but well, this can be moved to a follow up, I guess.

This patch now starts with #117 and fixes the tests introduced by tim.

Per #121, RTBC - if tests pass.

Opened #2509970: Apply and use data-drupal-selector consistently. for the follow-up.

Committed/pushed to 8.0.x, thanks!

This was the oldest release blocker by nearly two years...

Awesome

☆。★。☆。★
。☆ 。☆。☆ 。
★。＼｜／。★
★。／｜＼。★
。☆ 。☆。☆ 。
☆。★。 ☆  ★

May be I am slow and this had already reported somewhere, found some bug, I think it is related to this issue: https://www.drupal.org/node/1090592#comment-10280077

Thanks again to everyone who worked on this issue! It's been a great improvement! I love the data-drupal-selector attribute that this introduced. Unfortunately, there's a bug with the logic for how it's set in FormBuilder. I opened #2897377: $form['#attributes']['data-drupal-selector'] is set to a random value so can't be used for its intended purpose to fix it. Please review. Thanks.