Process #autocomplete_path for all form elements; remove custom/duplicated code from theme_textfield()

Sub.... I'm kidding :-p

I'm working on this patch now. I'm doing it as a process fyi.

Bumping to major task, it makes zero sense doing this in the theme layer. I'm sure there's an older issue for this, but can't find it now.

This is not particularly easy to do, as the autocomplete "sub-widget" needs a special location - right after the actual input element, but before the form element description.

We don't have a clean way to squeeze something in there, other than #field_suffix, but everything in #field_suffix is not only wrapped in a SPAN, but also has a dedicated/unique purpose (e.g., suffixing the input with " minutes").

Attached patch moves the code more or less as-is from theme_textfield() into theme_form_element(), which seems to be the most simple resolution for now.

---
The only alternative I could see to this is would be to add a dedicated form_process_autocomplete() #process callback to the element info, and make that callback inject a new #type 'autocomplete' child element below the processed element, and lastly make theme_textfield() & co append the rendered $element['#children'] right after the markup they are producing.

Here's the alternative approach mentioned in #4.

I kinda like this a lot more, because it formalizes the "autocomplete widget" (and thus makes it swappable/pluggable on its own), but it has and clarifies one larger issue we have in the current render/theme system: #theme functions do not render any child elements. To have the injected 'autocomplete' child element rendered, the #theme needs to be turned into a #theme_wrappers callback.

That particular issue should actually be one major discussion topic for #1382350: [discussion] Theme/render system problems...

Yea, I attempted to fix this but I failed miserably were sun succeeded

So the question is whether this particular change is acceptable:

+++ b/core/modules/system/system.module
@@ -384,10 +390,13 @@ function system_element_info() {
-    '#theme' => 'textfield',
-    '#theme_wrappers' => array('form_element'),
+    '#theme_wrappers' => array(
+      // Signify the primary #theme function.
+      'theme' => 'textfield',
+      'form_element',
+    ),

...at least, for the time being...

I think that's fine as a workaround for now, especially considering the duplicate code we're adding on a much bigger scale in #1174620: Add new HTML5 FAPI element: email and other issues. Moving back to CNR.

I discussed this with sun, and suggested using the drupal_render_children() function instead of changing #theme to #theme_wrappers. Here's the patch for that. Otherwise, this looks good to me. I'd RTBC it, except my changes need review.

+++ b/core/includes/form.inc
@@ -3631,6 +3631,52 @@ function theme_vertical_tabs($variables) {
+    $element['#tree'] = TRUE;

Do we need to do this for any reason other than to set the child autocomplete element's id? If not, I'd rather we not mess with #tree here, as that will have other side-effects if the element has any other children, and just set $element['autocomplete']['#id'] = $element['#id'] . '-autocomplete';

I'd be OK with leaving out #tree there. That said, this reminds me of #759222: #tree does not default to TRUE, fails to meet developer expectations, which we could look into changing for D8.

Furthermore:

+++ b/core/includes/form.inc
@@ -3631,6 +3631,52 @@ function theme_vertical_tabs($variables) {
+function form_process_autocomplete($element, &$form_state) {
...
+    $element['autocomplete'] = array(
+      '#type' => 'autocomplete',
...
+function theme_autocomplete($variables) {
...
+  $element['#attributes']['type'] = 'hidden';

+++ b/core/modules/system/system.module
@@ -506,6 +506,10 @@ function system_element_info() {
+  $types['autocomplete'] = array(
+    '#process' => array('ajax_process_form'),
+    '#theme' => 'autocomplete',
+  );

Do we actually need a theme_autocomplete() function then? It themes a hidden input element, for which we have a theme function already. Hence, just set #type autocomplete's #theme to 'hidden'...?

If you'd have an alternative implementation, then that could alter the #theme property of the #type instead of overriding the theme function.

#1174620: Add new HTML5 FAPI element: email went in, so we additionally need to convert that.

With #10-#12 incorporated.

Thanks!

+++ b/core/includes/form.inc
@@ -3632,6 +3632,34 @@ function theme_vertical_tabs($variables) {
+      '#value' => url($element['#autocomplete_path'], array('absolute' => TRUE)),

The original #autocomplete_path value is no longer known to the element. Would it make sense to copy the raw #autocomplete_path into a custom #path property on the 'autocomplete' element in this #process callback?

+++ b/core/includes/form.inc
@@ -3632,6 +3632,34 @@ function theme_vertical_tabs($variables) {
+      '#attached' => array(
+        'library' => array(array('system', 'drupal.autocomplete')),
+      ),

I wonder whether it would make sense to specify this in hook_element_info() instead?

+++ b/core/includes/form.inc
@@ -3703,14 +3731,14 @@ function theme_image_button($variables) {
 function theme_hidden($variables) {
...
-  element_set_attributes($element, array('name', 'value'));
+  element_set_attributes($element, array('id', 'name', 'value'));

ahh. That's the detail of the idea I didn't think of :-/

We intentionally removed the HTML IDs for hidden input elements in #168722: Hidden input elements contain invalid characters in ID attributes

So we'd have two ways out:

1) set the hard-coded ID in #attributes instead and revert this change to theme_hidden().

2) go back to a separate theme_autocomplete() as in earlier patches.

To be honest, I don't understand why autocomplete.js requires a hidden element in the markup to begin with. If it needs a hidden element at all (and I'm not sure it does), why can't it make it instead of requiring the document to be polluted. Anyway, not fixing that here, but given that, what do you think of this?

#174630: Use watchdog to log user login went in :), we need to get this in as soon as possible

I'm fine with this patch converting what it already does, and then a follow-up for anything remaining once it's committed - that's likely to be a quicker route than re-rolling for each new HTML5 element.

Moving back to CNR.

Here's an up to date one anyway, mostly as an excuse to trigger another email alert to sun :)

Ok I just this manually. Everything seems to be looking good.

I used the taxonomy term reference to test this. Everything works well, the hidden field isn't visible and the results list show up correctly.

I would mark this RTBC, but I'll leave it to sun.

#1174628: Add new HTML5 FAPI element: search got in, adding more duplicate code to be removed.

We don't need a re-roll for that, just a follow-up once this is in.

Thanks! Still very similar to my original patch, but leveraging drupal_render_children() instead.

I agree we can convert/fix the additional theme functions in a follow-up patch (in this issue).

Regarding the @todo, note that there's already at least one issue for investigating different autocomplete implementations (e.g., jQuery UI's), and this dedicated form element #process function will definitely help us to adjust or switch the implementation.

Great job. Committed to 8.x. Marking this needs work because more refactoring is possible due to #1174628: Add new HTML5 FAPI element: search (see comment #22 above).

Thank you. Looks excellent, except that the other patches have:

- *     #placeholder, #required, #attributes, #autocomplete_path.
+ *     #placeholder, #required, #attributes.

Ok. We're done after that one - search is the last element.
Do we need a change notification?

Nice! Committed/pushed to 8.x, I think we should add a change notification for this since it's an API addition and something contrib form elements can make use of/be refactored for.

Created a change notification: http://drupal.org/node/1536426.