This used to not be the case.

This constrains the formatting of some webform types.
I'm using it to produce quizzes, the question itself is in a fieldset, and I have to put an artificial label on the select field that depends on the fieldset...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

Title: widget labels are mandatory » Make field labels optional
Category: bug » feature

To clarify the request: Webform labels used to be optional, so that a field could be displayed without a label to the user filling out the form.

Junni’s picture

Yes, this would be nice. When using select elements, a label needs to be filled in, even when it has the same caption as the select option(s).

espie’s picture

Version: 5.x-2.1.3 » 6.x-2.7
FileSize
908 bytes

Here is a more complete patch, current for 6.x-2.7.

This one:
- makes field label optional
- displays something sane in the component list.

espie’s picture

Ping ?

This is a simple patch, it yields a useful feature that used to be there. What blocks it ?

quicksketch’s picture

Version: 6.x-2.7 »

For one, the patch contains an XSS security problem:

+        $indents . filter_xss(empty($component['name']) ? $component['form_key'] : $component['name']),

$component['name'] needs to be sanitized on the printed version.

This patch also doesn't take into account multiple other places where the label is used: CSV downloads, analysis, or table display. No new features are being added to the 2.x version so this will have to wait for 3.x.

quicksketch’s picture

espie’s picture

Err... this is NOT a new feature request. Basically, it used to be possible to have empty labels, and it no longer is.
I had to insert THIS to get old forms to perform with the new code.

I'm going to keep using the flawed patch. Frankly, the total lack of feedback on issues is tiresome. I've given up on checking that queue regularly. Is there a way to get live feedback in <3 months time ????

quicksketch’s picture

Espie, I have other obligations besides maintain this module for free. That's great that you can use the existing patch (even though it contains a XSS security vulnerability), but I won't be including these changes in 2.x because I can't maintain multiple branches with the time I have available.

Alan D.’s picture

Version: » 6.x-3.x-dev

It would be great to have this feature in the 6.x-3.x branch. The "<none>" title was going to be my first choice when looking at this, but this complicates other parts such as the components form, CSV export, et al. Maybe an additional option (e.g. a checkbox) to hide the title OR a form title textfield that accepts the option.

e.g.

Label: *
[_________________________]
This is used as a descriptive label when displaying this form element.
[ ] Hide the label on forms

or

Label: *
[_________________________]
This is used as a descriptive label when displaying this component.
Form label: *
[_________________________]
This is an alternative label to use when displaying this component as a form element. Use "<none>" to hide the label in the form. Leave empty to use the component label.

I'm having a look at this branch for the first time and it is a fantastic improvement so far. Great work! How do you find the time? :)

Alan D.’s picture

Here is a patch for the first of the above two options for a select [select, radios, checkboxes] element. It was complicated by two things.

1) Validation

The validation function required a title, so this needed to be passed through with another FAPI parameter. I used #label to pass it through.

2) select_or_other integration

This module clones a fixed number of FAPI properties from the element to the child select, so the #label and #validated parameters do not get copied. As such, the select_or_other patch is also required.

quicksketch’s picture

I found that Drupal 7 has a useful #title_display option, meaning we could simply set #title_display = 'none' in Drupal 7. I think this would definitely be the better way to go, unfortunately it's extremely difficult to emulate that sort of behavior in Drupal 6, since this property is part of theme_form_element(). Perhaps we could do something in #pre_render?

Alan D.’s picture

Sounds good. I've spent too much time of work today looking at the webform today, but I may get a chance tonight if you would like to partition this off onto someone else.

quicksketch’s picture

I'm not particularly interested in this feature, so help implementing it would be great. We might take an approach where we add a checkbox for "Hide title", which would map directly to #title_display = 'none'. Not sure if this would be a UI improvement over two fields or not. Generally I'm not real enthused about adding this option at all, since for most users it will be unnecessary cruft in our UI.

Alan D.’s picture

Status: Active » Needs review
FileSize
4.82 KB

I forgot about the #pre_render hook. That was so much easier than the #preprocess one that I tried earlier!

This only covers the select component with & without select_or_other support, as I agree that this does clutter the UI, but I've lost count on how many times that I've done a form alter to hide the title on a singular checkbox so I think that it is a valid option here. Most of the others should be trivial if there is the need, but the fieldset could cause some headaches as the title attribute is required for the JQuery collapse unless someone knows of a workaround.

I found the need to pass through the select_or_other type through to the prerender hook using #original_type attribute. This seemed safer than testing for a select and other child element.

quicksketch’s picture

Something funny happened with your patch encoding. Could you recreate and make sure it's UTF8?

Alan D.’s picture

duh. I do not know why it saved as utf16. This time

quicksketch’s picture

This looks pretty good, but let's do one further with feature-parity with Drupal 7. Instead of this segment:

+  if ($component['extra']['title_display'] === 'none') {
+    $element['#pre_render'] = array('_webform_client_form_remove_title');
+    $element['#original_type'] = $element['#type'];
+  }

Let's do something like this:

+  if ($component['extra']['title_display'] === 'none') {
+    $element['#title_display'] = 'none';
+  }

An then up above, unconditionally add the '_webform_client_form_remove_title' to #pre_render. This makes it so that our Drupal 7 version of the patch (which will need to be made, the D7 version is practically done already in HEAD), we'll just remove the #pre_render and the _webform_client_form_remove_title() function.

Alan D.’s picture

This would remove the check on the select_or_other element. This moves the title into the select child element.

Happy to lodge a request on the select_or_other queue to see if they want to use this #title_display option and let them deal with the title display.

So how about:

<?php
  if ($element['#type'] != 'select_or_other') {
    $element['#pre_render'] = array('_webform_client_form_remove_title');
  }
# OR [ The _webform_client_form_remove_title() would have no negative effects on select_or_other element. ]
  $element['#pre_render'] = array('_webform_client_form_remove_title');

# AND [ Are there other title display options other than 'none'? This would make this section of code compatiable with future uses of #title_display. ] 
  $element['#title_display'] = empty($component['extra']['title_display']) ? NULL : $component['extra']['title_display'];
?>
quicksketch’s picture

Sorry I wasn't quite clear, the "_webform_client_form_remove_title" function would actually check the #title_display property and set #title to NULL if #title_display == 'none'. So calling this pre_render function on any element won't have any negative side-effect. The second suggestion of this:

$element['#title_display'] = empty($component['extra']['title_display']) ? NULL : $component['extra']['title_display'];

Sounds good to me.

Alan D.’s picture

I think that we were on the same page, but I was confusing you with the select_or_other element integration.

The select_or_other element has a #preprocess call that creates two child elements 'select' & 'other', which I'll lodge a request for them to handle the #title_display after this is committed.

e.g. in select or other module preprocess callback, <?php $element = array( '#type' => 'select_or_other') ?> is converted to <?php $element = array( '#type' => 'markup', 'select' => array('#type' => '[select|radios|checkboxes]'), 'other' => array('#type' => 'textfield')) ?>.

I'll create the patch tonight or in the weekend. (We are in the middle of shifting flats and my computer is in multiple boxes)

quicksketch’s picture

Oh! Okay I gotcha now, since #process runs before #pre_render, we'll have two elements instead of one. Filing an issue with select_or_other sounds like a good approach, since this is future-proofing in a way.

Alan D.’s picture

K. This one. All custom code about select_or_other has been removed. ;)

Alan D.’s picture

And the patch

Alan D.’s picture

espie’s picture

We *all* have other responsabilities. If you google my name, you'll notice I do quite a lot of things outside of drupal.
Not maintaining the "stable" branch of a module is, in my opinion, irresponsible. I'm happy Alan D. stepped in so that,
eventually, drupal7 may fix the issue...

As far as I'm concerned, it's still a regression on the 6.x-2.x branch (and I'll call it that, since it used to work).
If something like this works in 3.x-dev, eventually, do make a formal release. I'd rather work on a patched 6.x-2* version where I know the problems rather than a 6.x-3.x-dev version.

your comment leads me to conclude that, basically, webform 6.x-2.* is mostly dead, since you won't fix regressions...

if you did explain what the xss vulnerability is, in this context, I would probably fix it (I'm a bit appalled at the loops php + drupal
still makes you go thru wrt such vulnerabilities, how much shit is exposed, and how hard they are to fix).

Alan D.’s picture

$component['name'] needs to be sanitized on the printed version.

This is user input. aka has a risk of JScript insertion. Use a check_plain if your not sure. I put some notes about this here (cut n paste from another projects notes):

Typical text field (no HTML)

<?php
$no_markup = isset($data['no_markup']['body']) ? check_plain($data['no_markup']['body']) : NULL;
?>

Text field with tags (HTML)

<?php
# You can use filter_xss_admin for TRUSTED users.
$free_text = isset($data['free_text']['body']) ? filter_xss($data['free_text']['body']) : NULL;
?>

Rich text (WYSIWYG)

<?php
$rich_text = '';
if (isset($data['rich_text']) && !empty($data['rich_text']['body'])) {
  $rich_text = check_markup($data['rich_text']['body'], $data['rich_text']['format'], FALSE);
}
?>

WF6.2 works great, 6.3 is even better! Help test the patches in the queue and these will be pushed through much faster.

Alan D.’s picture

And one other note. If user supplied format key, use this to ensure that the format field itself is valid by making the function check the format permissions for the user against the filter.

<?php
 $rich_text = check_markup($data['rich_text']['body'], $data['rich_text']['format']);
?>
quicksketch’s picture

Simply making the title field not required isn't going to work (as I stated in #5), because the title field is also used on CSV downloads and when displaying the results in table and analysis tabs. The fact that 2.x didn't make the field required was actually a bug that users conveniently exploited to not create labels on the form. The new approach we're taking here solves this problem and makes the field label hidden on display of the form.

We're speeding along towards a 3.0 final release, the last beta was created at the start of the month and we should have another one out shortly. I'm hoping that we can make the final version in less than a month.

As for Alan D's patch so far: it looks great! I think that since you started the patch we added a consistent location for "Display" options that are always grouped together. I think it would make sense to add this checkbox down in that set of options. However I think the current patch is plenty far along and there's no need to reroll, I can pick it up from here I think.

zdean’s picture

Any progress on this? thanks to everyone's contributions!

Alan D.’s picture

Pinging.

After travelling for 4 months, I've have just got some remote work. I'm creating 6 web forms for a client and 5 have single checkbox fields, so I'd love it if this got in. This is so much simpler than #264181: Creating a single checkbox.

quicksketch’s picture

Interesting, I hadn't thought of this as a solution to a single checkbox, but clearly this would solve the same problem (though Drupal's default markup/CSS would probably add an excessive amount of margin around the checkbox).

Alan D: I know I said I'd pick this up from here, but it would be a big help if you could reroll and port for D7. Then again... D7 is totally borked right now due to numerous API changes. Maybe just a D6 reroll (if needed)?

Alan D.’s picture

FileSize
2.9 KB

An updated patch from CVS.

If people review / test this patch and give feedback, it will help the maintainers out.

Alan D.’s picture

FileSize
2.46 KB

This time without the eclipse .project file

Alan D.’s picture

FileSize
11.86 KB

And if this idea could continue onto the other elements. I'm using the current project that I'm working on for the use cases.

They have an address field of multiple textfields, and just the label on the first "Address". Rather than "Address 1", "Address 2", etc

A simple message form, a textarea, no label. The introduction / page title would explain single field forms, making the label clutter the form.

So this is a useful feature in a more general context.

Here is a rough combined patch for the following fields:

Date
Email
File
Select
Textarea
Textfield
Time

It does have some general tidying up to do, but it should be functional for testing.

quicksketch’s picture

Thanks again Alan D. I've become more interested in this after you pointed out that it also solves #264181: Creating a single checkbox. I rerolled and updated for D7 in this patch. The biggest change is that I've made the "title_display" configuration options always there by default (similar to how we have the "name" and "title" configuration options for every component), so we don't have to duplicate that form for every component. Components that don't need it (pagebreak, markup, hidden) can remove it in their own form callbacks.

I don't like doing these massive change-all-component updates, so while I was spending the time updating all the components I bundled in these changes:

- All components now respect the $filter variable that is passed into _webform_render_[component].
- All components are now consistent and use #webform_component (instead of excluding it or using #component sometimes).
- Remove indentation from _webform_render_[component], which was inconsistently used and something I prefer to not do.

So apologies for the bundled changes, but reviewing all the components to make sure they act consistently is quite a bit of time that I'd prefer to save while I'm updating all the components. Also included is a screenshot of all the components with their titles hidden for demonstration.

quicksketch’s picture

Status: Needs review » Fixed

I've committed this round of changes. Please let me know if you find any problems.

Alan D.’s picture

I have a play latter and see if I run into any problems.

This is a bit late, but I was thinking earlier, that having a display title field that supports is better than having a visibility flag. I was going to try rolling something out for this, but I am flat out with work. :(

quicksketch’s picture

While a "Display Title" field would probably be more flexible, I'm not sure I'd like to subject users to entering the title 3 different ways (Form Key, Label, Display Label). So I'm happy with this solution for the time being.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

knalstaaf’s picture

I don't want to sound like a dumbass, but how do you have to apply the new method?

From the above thread I'm getting the impression that the trick is to

  • give a random label to the on/off checkbox,
  • add a full title to the "readable option" of the key value (eg "yes|Call me back on the given number"),
  • eventually set the label display to "none".

Is this correct, or am I missing something?

Alan D.’s picture

"random label" is still used in the exports, etc from memory, but yes that is the short howto :)