Incompatible with HTML5 required attribute, affecting Webform 7.x-4.8

Just to follow up, webform 7.x-4.8 introduced html5 required attributes to its select components. The required attribute isn't supported for checkboxes, but for radio buttons, single-select list boxes (select elements) and multi-select list boxes, webform does use required.

Within Webform, jQuery removes the required attribute when it conditionally hides an element, and restores it when shown. I would suggest that the select_or_other jQuery do the same.

Same issue here. However, as stated on Select (or other) module's summery page:

As a Webform element. This functionality is implemented by the Webform project.

I'm not sure who (which module) should be responsible for fixing this. I'm running Drupal 7.36 with Webform 7.x-4.8 with NO select_or_other installed.

At first glance I'd say the responsibility for this issue lies with the webform module, because the problem also exists without select_or_other installed (according to jmouse888).

I will however first try to reproduce the issue myself when i have the time to do so.

Same problem!

How to reproduce:

1. Install Webform 7.x-4.8 and Select (or other) 7.x-2.21
2. Create a webform with a "select" component with requiered and Select (or other)
3. Visit the form and try to submit

Provisional solution: unchek the requiered option in the component.

I have fixed temporally using the webform hook render.

You only need to add this code in a new module or an existing module. Replace "my_module" by the name of your module (you can also add this alter in select_or_other module to solve the problem):

// Temporal fixes: https://www.drupal.org/node/2481127
function my_module_webform_component_render_alter(&$element, &$component) {
  if ($component['type'] == 'select' 
      && $component['required'] == 1 
      && isset($component['extra']['other_option']) 
      && $component['extra']['other_option'] == 1) {
    unset($element['#attributes']['required']);
  }
}

#5 works for me in a separate module. Thanks oriol_e9g.

The issue isn't specific to webform. Select or Other is not compatible with the HTML5 required attribute. It should generate the Other textfield without it, and apply it and remove it as it shows/hides the text field.

Here's a patch that:

1. Detects the html5 required attribute and removes it from the other textfield and adds a 'select-or-other-required' class. This ensures operation if javascript is not enabled.
2. Enhances the javascript to set or remove the required attributed on the other textfield, based upon the new class and whether the textfield is currently in use (based upon the select / radio buttons).
3. Also, the javascript run with pre- and post jQuery 1.6, which added the prop() method.

I made no attempt to clean up the $ vs. jQuery inconsistencies in the js file.

Since this affects webform, a review and, if correct, RTBC and commit would be greatly appreciated.

+++ b/select_or_other.js
@@ -13,15 +13,19 @@
+    var $other_element = jQuery(ele).find(".select-or-other-other").parents("div.form-item").first();

This relies on the class supplied by theme_form_element and will therefore not work in every theme. mothership for example gives the option to hide the form-item class.

Then that theme doesn't work now. That code is exactly the same as the existing code, just factored into a local variable.

I believe it is standard practice for module to rely on the classes they generate to make their jQuery work, no?

My bad @DanChadwick,

I wasn't paying my full attention when reviewing the patch and missed the removed line of code.

You are correct in the sense that modules rely on the classes they generate, but the form-item class is not generated by select_or_other, but by Drupal core itself, which i know gets overridden quite often. You are also correct that the current implementation would not work in those cases, but in my opinion the module's JS desperately needs an overhaul. Most of the behaviour defined by the current JS implementation could be achieved by the correct use of Drupal's form states, but that's a whole other ballgame.

I think the current approach could be sufficiënt to fix the issue, but the overall JS requires a followup in the 3.x branch. However I don't currently have the time to thoroughly test the patch. If someone else reviews/tests your work and marks the issue RTBC before I get around to it, the patch will be committed.

I agree that form #states can easily do the show/hide. I was surprised to see that required can be toggled too. https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_...

achieved by the correct use of Drupal's form states

This seemed like a good idea, but I ran into a dead end because of #1149078: States API doesn't work with multiple select fields

Here's a patch for posterity, should the above (4 year old) issue ever get fixed and backported to D7. Not holding my breath.

What's left to do:

1. Actually remove the .js file.
2. Remove the commented-out #attach and it's comment.
3. Test required (didn't bother, given the dead end).
4. Test multiple select list boxes.

TL;DR: Do not review this patch. It is not currently viable due to a core bug. Review #8 instead.

I'm bumping this to at least "Major" since it affects a lot of forms quite seriously.

I have deployed the patch in #8 to production and it's working great both with and without JavaScript.
I suggest leaving code cleanups for a later issue since this needs to get in fast!
Great work!

Thanks for reviewing @TwoD, I will probably commit this later tonight.

@DanChadwick, thank you for your effort so far. I've read trough #1149078: States API doesn't work with multiple select fields and there seems to be a workaround we could use to prevent hacking core. I think we should investigate using this workaround until the patch in #1149078: States API doesn't work with multiple select fields lands.

I've committed the patch to 2.x and rerolled + committed the patch for 3.x. Let's continue the work on implementing the states API in #2488694: Refactor to make use of form states API

Whoops. Already related. Sorry for the noise.

Oh. Also, if you make a new release of select_or_other I can add that to the release notes. I don't want to put a version dependency into webform because it breaks for anyone who deploys without version (e.g. git without the git_deploy module).

Oh. Also, if you make a new release of select_or_other I can add that to the release notes. I don't want to put a version dependency into webform because it breaks for anyone who deploys without version (e.g. git without the git_deploy module).

I don't understand what you are trying to say here...

Right now, this commit has not been released in a stable version. If released in 7.x-2.22, I can recommend it in the release notes for the next webform, 7.x-4.9.

I *could* add a version dependency on >=7.x-2.22 in webform.info, but I won't. If someone deploys select_or_other in a manner where core doesn't know the version number, then webform will become disabled by virtue of the dependency, breaking the site.

A common way for this to happen is someone using git push to deploy select_or_other. Unless they run the git_deploy module, select_or_other won't be packaged by the package manager, and core won't know its version.

TL;DR: Please make a new release as soon as you are comfortable doing so. I will then make a corresponding release of webform recommending that version.

@DanChadwick,

7.x-2.22 will be out shortly.