We use a custom theme that adds classes on form elements to the parent div for extra styling control. This generates valid HTML but breaks the jQuery selector used in l10n_client.js '#l10n-client .string-search', because #l10n_client now contains two elements with the sting-search class.

I could not find any good reason why a class selector is used here instead of an ID selector. Changing the selector to '#l10n-client #edit-search' fixes this issue, and is more break-proof, in valid HTML this should never return more than 1 element.

On a side note, changing the name/id of the search input to something a little more namespaced than 'search' might also be a nice idea. It's not outside the realm of posibilities that something else uses 'search' as it's identifier and the Localization client gets loaded on every page, so chances of ending up with 2 elements with id='edit-search' on the same page are fairly real.

I included a patch to change the jQuery selector from '#l10n-client .string-search' to '#l10n-client #edit-search'.

Len

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lendude’s picture

Just discovered that the same goes for '#l10n-client-form .translation-target'.

So this patch also changes '#l10n-client-form .translation-target' to '#l10n-client-form #edit-target'

Len

Gábor Hojtsy’s picture

Status: Active » Needs work

The reason we use class selectors is that id selectors are not as predictable. Drupal autogenerates the ids and based on whether the are other equally named form elements on the page, you either get this id or not. If the class is not specific enough, we should give it a more specific name.

Lendude’s picture

I see your point. I didn't even know the Form API was smart enough not to generate double id's on a page, nice!

The problem with using a class selector remains though, that you can have valid HTML, and still break the selector.

You could make it more break proof by changing the selector to '#l10n-client input.string-search', this would make it less likely to return 2+ elements, but still possible (however unlikely).

Another option would be to hardcode the id of the textfield by changing:
$form['search'] = array(
'#type' => 'textfield',
'#attributes' => array('class' => 'string-search'),
);

to

$form['search'] = array(
'#type' => 'textfield',
'#attributes' => array('class' => 'string-search'),
'#id' => 'l10n_client_search', //or something along those lines
);

This will stop the Form API from generating the id, so it should give consistent results. However it will also prevent the Form API from generating an alternative id if there IS an overlap (however unlikely).

Gábor Hojtsy’s picture

Yeah, so including the ID in the selector again is an issue because it can change, and specifying the ID specifically is an issue because it can clash. What's the problem with making the class names more verbose?

Dean Reilly’s picture

Version: 6.x-2.0 » 7.x-1.x-dev

I just found this issue because I was outputting the l10n client forms twice. Drupal did indeed give them a different id's but then the js couldn't find the second form because of the new ids.

I think creating a more verbose class and only selecting on that would solve both of these problems.

sonictruth’s picture

Priority: Normal » Major
Issue summary: View changes
FileSize
2.33 KB

I'm having this issue too. What about '#l10n-client input.string-search'—you're know that your only ever wanting to target the input with the key up event. Changing the priority to major because this module becomes useless on a lot of themes. I've attached a patch