This patch provides a bit of a backend change that will allow #514990: Add a UI for browsing tokens to function. There is no front-end change to functionality.

Basically, the autocomplete has two mechanisms: an acJs object, which acts as a direct interface with the UI element, responding directly to it. This defers the actual AJAX / database interactions to another element, called ACDB (AutoComplete DataBase). This object can be interchangeable (this code makes it so), so that the token database could differ slightly.

This patch also does two other things:

  1. Moves the ugly -autocomplete element into a JS setting (pretty critical for this to function)
  2. Moves the bit of logic which decides when to hide the popup into the database (just two lines of code), because with tokens most of the time we'll want to keep the popup open.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dmitrig01’s picture

I forgot to mention - it looks like the code was originally meant to have this, Steven just never seemed to get around to it - the distinction is pretty clear and they could have been one object if it wasn't meant to be this way.

Dave Reid’s picture

What happens when there are two autocomplete elements on the same page with this patch? It seems like the JS settings might conflict?

dmitrig01’s picture

array('Drupal.ACDB' => array('#' . $element['#id']))

It works as intended, and sticks with the original approach - there is one ACDB object per URL, but multiple jsAC objects - one for each element. I tried it on the node form, which has two (tags and username) and it worked great.

[Edit: since the element ID is in an array, and drupal_add_js uses array_merge_recursive, you can add multiple IDs per uri and ACDB]

Status: Needs review » Needs work

The last submitted patch, autocomplete-exchangeableACDB.patch, failed testing.

dmitrig01’s picture

Status: Needs work » Needs review
FileSize
5.17 KB

Whoops, fixed

sun’s picture

+++ includes/form.inc	7 Jan 2010 02:54:47 -0000
@@ -2838,7 +2838,8 @@ function theme_textfield($variables) {
+    drupal_add_js(array('autocomplete' => array($autocomplete_uri => array('Drupal.ACDB' => array('#' . $element['#id'])))), 'setting');

Why not #attached here?

+++ misc/autocomplete.js	7 Jan 2010 02:54:47 -0000
@@ -6,17 +6,19 @@
+      for (uri in settings.autocomplete) {
+        for (type in settings.autocomplete[uri]) {
...
+          for (i = 0; i < settings.autocomplete[uri][type].length; i++) {

Missing 'var' in for statements

Powered by Dreditor.

dmitrig01’s picture

wrt. #1, I'm just following existing code, but should I still split it out?

sun’s picture

We should use #attached wherever we can, because only #attached page requisites can be cached (and more easily altered).

dmitrig01’s picture

Status: Needs review » Needs work

The last submitted patch, autocomplete-exchangeableACDB.patch, failed testing.

sun’s picture

+++ includes/form.inc	8 Jan 2010 16:01:22 -0000
@@ -2826,6 +2826,19 @@ function theme_hidden($variables) {
+ * Process autocomplete.

That's a bit sparse ;)

+++ includes/form.inc	8 Jan 2010 16:01:22 -0000
@@ -2826,6 +2826,19 @@ function theme_hidden($variables) {
+  if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'])) {

Use !empty()

+++ includes/form.inc	8 Jan 2010 16:01:22 -0000
@@ -2826,6 +2826,19 @@ function theme_hidden($variables) {
+    $element['#attached'][] = array('misc/autocomplete.js');
...
+    $element['#attached'][] = array(array('autocomplete' => array($autocomplete_uri => array('Drupal.ACDB' => array('#' . $element['#id'])))), 'setting');

Missing 'js' key after #attached

+++ includes/form.inc	8 Jan 2010 16:01:22 -0000
@@ -2826,6 +2826,19 @@ function theme_hidden($variables) {
+    $element['attributes']['class'][] = 'form-autocomplete';

Missing hash '#' prefix for attributes.

+++ misc/autocomplete.js	8 Jan 2010 16:01:22 -0000
@@ -6,17 +6,19 @@
+      for (uri in settings.autocomplete) {
+        for (type in settings.autocomplete[uri]) {

Still missing 'var' statements here.

+++ misc/autocomplete.js	8 Jan 2010 16:01:22 -0000
@@ -6,17 +6,19 @@
+          var acdb = eval('new ' + type + '(uri)');

Did you test whether eval() is actually needed here?

Powered by Dreditor.

dmitrig01’s picture

Status: Needs work » Needs review
FileSize
5.19 KB

Yes, the eval is needed.

Status: Needs review » Needs work

The last submitted patch, autocomplete-exchangeableACDB.patch, failed testing.

dmitrig01’s picture

Status: Needs work » Needs review
FileSize
7.25 KB

whoops

dmitrig01’s picture

Status: Needs review » Needs work
Issue tags: -Token UI

The last submitted patch, autocomplete-exchangeableACDB.patch, failed testing.

Status: Needs work » Needs review
Issue tags: +Token UI

Re-test of autocomplete-exchangeableACDB.patch from comment #15 was requested by dmitrig01.

dmitrig01’s picture

Some thing strange happened with the CSS – this fixes it.

sun’s picture

+++ includes/form.inc	10 Jan 2010 00:15:49 -0000
@@ -2826,6 +2826,35 @@ function theme_hidden($variables) {
+            'Drupal.ACDB' => array('#' . $element['#id'])

Missing trailing comma.

+++ misc/autocomplete.js	10 Jan 2010 00:15:49 -0000
@@ -6,17 +6,19 @@
+          var acdb = eval('new ' + type + '(uri)');

I'd be ok with this, since the overall changes of this patch are a big step forward... but ideally, I'd like to have kkaefer, Rob Loach, or quicksketch sign this off.

+++ modules/system/system.module	10 Jan 2010 00:15:49 -0000
@@ -375,7 +375,7 @@ function system_element_info() {
-    '#process' => array('form_process_text_format', 'ajax_process_form'),
+    '#process' => array('form_process_text_format', 'ajax_process_form', 'form_process_autocomplete'),

We support autocomplete on a textarea?

4 days to code freeze. Better review yourself.

Dave Reid’s picture

Status: Needs review » Needs work

@dmitrig01: Up for fixing this one more time?

Dave Reid’s picture

Adding the generic 'token' tag for core. Anyone up for re-rolling this?

Dave Reid’s picture

Issue tags: +token
Dave Reid’s picture

Issue tags: +autocomplete
bcn’s picture

Status: Needs work » Needs review
FileSize
8.45 KB

re-roll

retester2010’s picture

Status: Needs review » Needs work
+++ includes/form.inc	28 Mar 2010 04:34:49 -0000
@@ -2807,6 +2807,35 @@
+ * Add autocomplete to an element.
+ * ¶
+ * @param $element

trailing white space

40 critical left. Go review some!

nod_’s picture

Version: 7.x-dev » 8.x-dev
Assigned: dmitrig01 » Unassigned
Category: task » feature

Who's up for a reroll of a 2 years old patch?

tkoleary’s picture

Issue summary: View changes

Please follow progress on: #2346973: Improve usability, accessibility, and scalability of long select lists

Changing to Select2 may render this issue obsolete. If we move to Select2 AFAIK it has a much more robust method for handling tokens.

nod_’s picture

Status: Needs work » Closed (works as designed)

I think we're covered with the new jQuery UI autocomplete. If not feel free to reopen.