Disabled form elements not properly rendered

Because seven sets a bunch of colors for input items, and because selectors like input[disabled] aren't supported by all browsers, I'd like to propose having FAPI add a class of "form-element-disabled" to all disabled form elements

this would then allow us to override stuff like

input.form-autocomplete,
input.form-text,
textarea.form-textarea,
select.form-select {
  padding: 2px;
  border: 1px solid #ccc;
  border-top-color: #999;
  background: #fff;
  color: #333;
}

so I'd like to ask some1 to outline some colors that would distinguish disabled elements (border color, text color and background color) and perhaps a disabled version of the submit button (http://drupalcode.org/viewvc/drupal/drupal/themes/seven/images/buttons.p...)

attached patch only adds the "form-element-disabled" class

NOTE: this would probably require a fat documentation entry for buttons that get enabled/disabled using JS

hmz, chx rightfully pointed out that this should go in theming and while pondering about it, I think it might be more useful to put the class on the wrapper, that would allow us to use a more specific selector, but this might make it more annoying when using JS (it would require an extra .parent() call)

attached patch puts the class on the wrapper and does this from theme_form_element

I'll try to wrap up the styles to go along with this later tonight or tomorrow

winging some colors, doesn't include buttons

wouldn't mind some eyes on this

+++ includes/form.inc
@@ -3028,6 +3028,11 @@ function theme_form_element($variables) {
+  // If disabled, add a class to facilitate cross-browser styling.
+  if (!empty($element['#attributes']['disabled'])) {
+    $class[] = 'form-element-disabled';
+  }

We should have tests for this.

+++ themes/seven/style.css
@@ -827,6 +827,15 @@ div.filter-options select {
+/* Disabled form elements */
+div.form-element-disabled input.form-autocomplete,
+div.form-element-disabled input.form-text,
+div.form-element-disabled textarea.form-textarea,
+div.form-element-disabled select.form-select {
+  color: #666;
+  background-color: #ddd;
+}

Is there a way to make this more generic to include all input types? Seems like radios for example would be inconsistent.

Additionally, could you please post steps to reproduce the original bug? I was not able to find such a problem on the field settings page.

Powered by Dreditor.

@webchick: seven doesn't override the default styling of radio-buttons, so we don't have to override the disabled styles for it
installer of a clean head without this patch when only MySQL is available: http://gyazo.com/35301c11480a37dc8907d9145cdf1cf6.png

all I did for the styling was copy the selectors for input fields that had a color & background color defined, made the copy relative to div.form-element-disabled and set a color & background-color for them (excluding buttons as those are sort of a special case)

to re-create the original bug, form_alter a field to '#disabled' => TRUE, I only tested a textfield, textarea, select, single radio and single checkbox

So all we need is tests?

Screenshots: Textfield - Textarea - Select list

Sorry to be a pain, but the better screenshot would be a full page with both enabled and disapled form elements in the viewport.

This styling might be too heavy and actually attract attention instead of communicating "skip me, you can't do anything with me anyway".

changed colors to be less obtrusive -> screenshot

Much better

A lot better and green: http://qa.drupal.org/pifr/test/26698

heh? this does not yet contain a test for it

needs work for tests then

maybe we should call the offender by it's name, it is only IE6 that doesn't support input[disabled]
http://quirksmode.org/css/contents.html#t13

Not critical. Please read and understand Priority levels of Issues, thanks.

/me does not agree.

- D7 can not be released with this bug.
- There is not a critical meta-issue that contains this bug.

- We're actually currently actively undoing the way the browser renders a disabled element

How disabled elements are rendered depends on the user agent.

http://www.w3.org/TR/html401/interact/forms.html#h-17.12
By fixing this bug, we bring back the different rendition of disabled elements.

(BTW: What about read-only?)

Ack this bit me today working on Pathauto's D7 port. Having a textfield as disabled did not change any look at all and will mess up end users.

iono, if I look at Priority levels of Issues, I'd say this is cosmetics thus minor

considering no1 capable of writing tests seems to pick this up, I'm perfectly fine with dropping the wrapper and going with input[disabled] styling, even if that would mean dropping IE6

It's critical. Now can we get a RTBC patch? :)

tagging,

Elements can be disabled using javascript (states.js for example) so just adding a class serverside isn't going to work.

I think the note of @alexanderpas in #17 is a good one. I suggest to use input[disabled] (and make IE6 users run to their boss crying for another browser).

http://reference.sitepoint.com/css/attributeselector#compatibilitysection

@alexanderpas Can you provide a patch perhaps, seems this issue can easily be fixed if we have a initial patch

Had a little chat with seutje. I agree with him we should use his approach:

• Using just the css selector input[disabled] has the disadvantage you can't style the form element wrapper.
  Using the class .form-element-disabled like in the patch of @seutje in #12 you can.
• We want to be able to style form elements, we need a possiblility to do so. input[disabled] isn't sufficient.
• When you disable form elements using JS you can easily add the class. This should be documented in theme_form_element().
• When you use ahah to disable (replacing the non-disabled element) an element you won't even need to add the class (Drupal does it for you)
• IE6 should still be supported.

Seutje will post a new patch.

added comment

doh :)

I've tested the patch in #27 and works fine, but the file upload field is missing from the CSS.

/* Disabled form elements */
div.form-element-disabled input.form-autocomplete,
div.form-element-disabled input.form-text,
div.form-element-disabled input.form-file,
div.form-element-disabled textarea.form-textarea,
div.form-element-disabled select.form-select {
  color: #777;
  background-color: #ff0000;
}

+++ includes/form.inc
@@ -2893,10 +2893,12 @@ function theme_file($variables) {
+ * Each form element is wrapped in a DIV with #type, #name classes. If ¶
+ * the element is #disabled, a 'form-element-disabled' class will also be ¶
+ * added to the wrapper. In addition to the element itself, the div ¶
+ * contains a label before or after the element based on the optional ¶
+ * #title_display property. After the label and fields this function ¶

extra white space at the end of each line

+++ includes/form.inc
@@ -2943,6 +2945,11 @@ function theme_form_element($variables) {
+  ¶

whitespace

Powered by Dreditor.

@sgabe: File field styling isn't overridden so the disabled style does not need overriding, the reason you aren't seeing it as disabled is because it isn't getting disabled -> #695590: file_managed_file_process does not propagate disabled state

@seutje: I wrote a mini module to test every form field, and the disabled file filed is getting disabled and I see it disabled. The only thing I'm not seeing is the background-color, because it's missing from the style sheet. Maybe with form_alter it's not working, but with a custom module it is. This should be there in the style sheet IMO.

Can anyone whip up a new patch?

@33: It looks like you used '#type' => 'file', which (I think) is not directly used anywhere, the little image field you see on story is '#type' => 'managed_field', whose process function splits it up in a '#type' => 'file' and '#type' => 'button', but doesn't propagate the disabled attribute to those 2 elements

but you are right that it needs a grey background, I'll create a new patch when I find a second

hmm, looks like reset.css is being a lil brat again

as you can see from the following screenshot, the default styling for a filefield on firefox does not have an inset border or darkened background -> http://gyazo.com/919768aa0e8dcf81a2292a287f63975a.png

how it looks in seven: http://gyazo.com/c41ac10baec2a33a391a3b38b26f612e.png

so should I add a grey background and should I return the border to a normal solid one?

I would advise turning off the inset on disabled fields, similar to your first screenshot. It's quite clear that it's disabled.

+++ includes/form.inc
@@ -2943,6 +2945,11 @@ function theme_form_element($variables) {
+    $class[] = 'form-element-disabled';

Can we shorten this class to .form-disabled, please?

113 critical left. Go review some!

@sun: Wouldn't .element-disabled be a better choice? Since that class wouldn't disable an entire form, just individual elements within it.

I don't really mind. But as of now, our .element-* classes have a direct meaning and effect on the HTML element they are applied on. This class only has an effect on the input elements within the element, so I think that .form-disabled is more appropriate; also, because it only applies to form items.

I really don't care about the naming, but yea, form-element-disabled might be a bit verbose

changed it to form-disabled

needs review

+++ includes/form.inc
@@ -2951,10 +2951,12 @@ function theme_file($variables) {
+ * Each form element is wrapped in a DIV with #type, #name classes. If ¶
+ * the element is #disabled, a 'form-disabled' class will also be ¶
+ * added to the wrapper. In addition to the element itself, the div ¶
+ * contains a label before or after the element based on the optional ¶
+ * #title_display property. After the label and fields this function ¶
+ * outputs the optional element #description.

@@ -3001,6 +3003,11 @@ function theme_form_element($variables) {
+  ¶

1) Trailing white-space.

2) Could we split this into two or three paragraphs? i) wrapper ii) label iii) description. The overall wording could you some love.

112 critical left. Go review some!

removed trailing white-space

I don't really know how to put it in better wording, any suggestions?

this is still lacking tests btw, I've tried to figure out how to write those, but not really successful :(

made a little tester-module that just spits out enabled and disabled versions of all element types (I know it's not perfect, but works to test) -> http://drupalbin.com/14359?nocache=1

forgot to add the actual patch :x

How's this for a revision of the comment text:

Each form element is wrapped by a DIV.  This DIV has the #type and
#name classes.  This DIV also contains a label, either before or after
the element as determined by the optional #title_display property.

If the optional element #description has a value, it is included
after the label and all fields.

If an element has the class #disabled, the DIV wrapper will be 
given a corresponding #form-disabled class.

There you go.

awesome, thanks <3

will try to focus on writing a test for this tomorrow

Okay - thanks to deviantintegral who got me set up and walked me through patching 101 - here is the documentation changes I made in a patch.

Hope this worked - it's my first patch...please be gentle *grin*.

//s

I can only guess that #49 didn't reload this issue page prior to posting, so further patches should be based on #47.

+++ includes/form.inc	21 Apr 2010 21:08:57 -0000
@@ -2951,10 +2951,15 @@ function theme_file($variables) {
+ * Each form element is wrapped by a DIV.  This DIV has the #type and #name
+ * classes.  This DIV also contains a label, either before or after the

I don't think we put two spaces behind a .

102 critical left. Go review some!

#47 looks good to me. I think this will be adequate to differentiate between disabled and non-disabled form elements. Just waiting on the testbot, I guess?

RTBC if green.

oh, no test for it then?

seanyo: congrats on your first submitted patch though! :)

Code sprint tag

Thanks! Great to be helping out - DrupalCon has been a great experience and it's awesome to get active.

So, I checked out code and made the documentation change and then posted the patch. I think the step I missed was applying the most recent patch (47) to my checked out code before making my changes and creating then next patch (49).

Is that right?

//s

After applying #47, disabled buttons still look like enable buttons. (True of all types of buttons: #type button, image_button, and submit). All other form elements look correct.

ugh, turns out buttons don't run through theme_form_element and they don't get a wrapper of any type
so I added the same logic to theme_button and theme_image_button to add a class to the element itself, and then re-overriding the styling with an equally specific selector but later on in the file
not sure if this approach is optimal, but it seems to work

ugh, turns out buttons don't run through theme_form_element and they don't get a wrapper of any type
so I added the same logic to theme_button and theme_image_button to add a class to the element itself, and then re-overriding the styling with an equally specific selector but later on in the file
not sure if this approach is optimal, but it seems to work

Seanyo: yes, correct. Need to apply the latest patch first so that you build on the work already done.

+++ includes/form.inc
@@ -2787,6 +2790,9 @@ function theme_button($variables) {
 function theme_image_button($variables) {
   $element = $variables['element'];
   $element['#attributes']['class'][] = 'form-' . $element['#button_type'];
+  if (!empty($element['#attributes']['disabled'])) {
+    $element['#attributes']['class'][] = 'form-' . $element['#button_type'] . '-disabled';
+  }

1) I can only guess that $element['#button_type'] contains the string "image_button", so we need to run drupal_html_class() on this. (ideally only once, of course)

2) Why are we using a new class at all here? Why not .form-disabled? (when changing this, we likely still need to clean the class for the existing line)

+++ themes/seven/style.css
@@ -667,6 +675,13 @@ input.form-submit:active {
+input.form-submit-disabled,
+input.form-submit-disabled:active {

Mapping to the above: This also needs to account for .form-button and .form-image-button, not just .form-submit.

+++ themes/seven/style.css
@@ -667,6 +675,13 @@ input.form-submit:active {
+  background: #eee;

Either use background-color or also specify a background image.

96 critical left. Go review some!

1) using the crappy testing module I posted above, which grabs all types and makes an enabled and disabled element for it, the image_button's button type was "submit", the other types are like "preview" and "delete" I think, which aren't being taken in account in that patch

2) the reason I used a different class is because this is not being applied to a wrapper, whereas the form-disabled class was being applied to the wrapper and using the same class for both is just going to be horribly confusing. But I guess it would make sense to use a single class for all disabled elements and perhaps drop the initial wrapper logic I used, I was just hoping I could handle it all in the same place but obviously I can't because nothing is consistent :/

I didn't add any sanitizing on it, because the line that was already there, that adds the regular form-submit class didn't do that either

Attached is a patch that tests if the form-disabled is placed on disabled form elements. I started working before #59 so this works against #47.

Will try and update it for #59 later.

@sun actually, I was wrong, the button types are like the type of the input ("submit", "reset" and "button")

@seutje: Could you add that disabled form element testing code to the markup_test module in my sandbox? http://drupalcode.org/viewvc/drupal/contributions/sandbox/sun/

Sorry for the confusion. I now see that http://api.drupal.org/api/function/system_element_info/7 defines #button_type = 'submit' for #type 'image_button', so we don't need that additional class name sanitation.

But we still want to make it work for both .form-submit and .form-button, sharing a common class. Not sure whether we need the class on the wrapping container for other form items though.

@sun so does this make more sense?

thank you so much for that test, I included it in this patch

+++ includes/form.inc
@@ -2770,6 +2770,9 @@ function theme_submit($variables) {
+    $element['#attributes']['class'][] = 'element-disabled';

@@ -2787,6 +2790,9 @@ function theme_button($variables) {
+    $element['#attributes']['class'][] = 'element-disabled';

If we can't agree on using .form-disabled also for buttons, then let's use .form-button-disabled for both.

Again, our .element-* classes are currently meant to work on anything, not just forms.

+++ modules/simpletest/tests/form.test
@@ -342,6 +342,21 @@ class FormsElementsLabelsTestCase extends DrupalWebTestCase {
   }
+  /**  ¶
...
+    }    ¶

Trailing white-space, and missing newline before test.

+++ themes/seven/style.css
@@ -667,6 +675,14 @@ input.form-submit:active {
+input.element-disabled:active {

Can a disabled form element become :active at all?

+++ themes/seven/style.css
@@ -667,6 +675,14 @@ input.form-submit:active {
+  background-color: #eee;
+  background-image: none;

If you specify both, then you can use just background. But is the unsetting of the background image required, necessary, or actually desired here?

Powered by Dreditor.

yea, when u click a disabled button, it will still trigger it's :active styles

I used the background shorthand at first, but u complained about that :P
but yea, it is desired to create more contrast between enabled/disabled

also removed an excess line of whitespace above the .form-disabled declarations

Thanks!

+++ modules/simpletest/tests/form.test
@@ -342,6 +342,22 @@ class FormsElementsLabelsTestCase extends DrupalWebTestCase {
+  function testDisabledClass() {
+    $this->drupalGet('form-test/disabled-elements');
+    $form = _form_test_disabled_elements(array(), $form_state);
+    $i=0;
+    foreach(element_children($form) as $key) {
+      $element = $this->xpath('//div[contains(concat(" ",@class," ")," form-disabled ")]');
+      $this->assertTrue(isset($element[$i]), t('The disabled @field has \'form-disabled\' class.', array('@field'=>$key)));
+      $i++;
+    }
+  }

I'm pretty sure that this test does not work at all.

1) $form_state throws an exception, undefined.

2) No idea why $i is increased for each element in the form. The xpath should select the current form element in the page output. But only try to assert if $form[$key]['#disabled'] is set.

3) We are not testing a disabled submit/button here.

4) Coding-style does not adhere to our coding standards.

+++ themes/seven/style.css
@@ -667,6 +674,13 @@ input.form-submit:active {
+  background: #eee;

1) Either use "background-color" and "background-image" and any other additional non-shorthand properties selectively, so as to not change or reset other specific properties. Here: background-color: #eee;

2) Or use the shorthand property "background", which resets all background properties, but at least requires a color and an image. I.e. to reset to nothing, background: transparent none;. Here: background: #eee none;

Powered by Dreditor.

Hi - I am a new user. This patch seemed to work for me.

However, sun seemed to bring up some issues so I'm putting this down to needs work.

hi @sun:

For the testing, it loops through each disabled element in the 'disable-elements' form test page to check to see if the 'form-disabled' class is included. The xpath returns an array because there are a number of elements with the 'form-disabled' class in that test. The xpath only needs to grab that once so it would make more sense before the foreach loop.

That is the best approach I could think of to test this functionality. Is there another that makes more sense? If you lay it out I can attempt to write the test and also include one for the submit image.

sun's right, the thing this test really seems to do is grab all elements, and for each element it tries to match a div in the whole DOM tree that has the class. Which means that if there is 1 such div in the tree, it reports success for all elements, for instance, it reports that the submit button has a form-disabled class, which it doesn't

but it's giving me some inspiration on how to go about it, thank you for that, I'm just still struggling a bit with xpath syntax, but I should be getting there sometime soon

yay! thanks for the inspiration aaroncouch!

I added the 3 types of buttons to the form and made a test that seems to work properly for all elements on that form.
note: I did not yet add the other types of form inputs (such as file, managed_file, ...) and this test only checks for the presence of the right class, it does not test if the elements have the disabled attribute

I'll try to add those other types later tonight when I get back from work, but I wanted to run this through testbot and show u that I didn't forget about this issue or gave up on it (and sorta wanna make sure I'm doing this properly)

ok, I added the other form elements, if this doesn't properly test it, then I don't know what will

I also noticed that #785594: text_format doesn't propagate disabled state to the format select element, which will cause 1 of the tests to fail

lemme know if u want me to remove that one test so we can get this in quicker (not sure if the text_format thing is by design)

Alright. http://drupalcode.org/viewvc/drupal/contributions/sandbox/sun/ now contains a full markup test for disabled form elements.

Sorry, to properly fix this, I had to.... well. :)

This patch now depends on #715142: [beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain() to get in first.

And the added test for disabled form elements can probably be streamlined. Didn't come to that yet.

This looks RTBC to me.

oh snap, I didn't know you could theme them so easily, thx! <3

this also seems to have fixed #695590: file_managed_file_process does not propagate disabled state so I'm marking that as duplicate

I manually tested it (after also applying #715142: [beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain() coz otherwise u can't even install) and I got 2 successful passes for submit(aside from button and image_button), is it wrongfully checking twice due to the presence of the normal submit button?

subscribing

#75: drupal.form-disabled.75.patch queued for re-testing.

#76: drupal.form-disabled.76.patch queued for re-testing.

I saw that this critical issue was not worked on for almost a month, so I queued the last patch to be re-tested. Previously it had failed because Drupal could not even install. Now, however, it fails when applying the patch, with the error:

Hunk #13 FAILED at 3106.
1 out of 13 hunks FAILED -- saving rejects to file includes/form.inc.rej

That hunk seems to be:

     $class[] = 'form-item-' . strtr($element['#name'], array(' ' => '-', '_' => '-', '[' => '-', ']' => ''));
   }
 
+  // Add a class for disabled elements to facilitate cross-browser styling.
+  if (!empty($element['#attributes']['disabled'])) {
+    $class[] = 'form-disabled';
+  }
+
   // If #title is not set, we don't display any label or required marker.
   if (!isset($element['#title'])) {
     $element['#title_display'] = 'none';
   }
 
-  $output = '<div class="' . implode(' ', $class) . '">' . "\n";
+  $output = '<div class="' . check_plain(implode(' ', $class)) . '">' . "\n";
 
   $prefix = isset($element['#field_prefix']) ? '<span class="field-prefix">' . $element['#field_prefix'] . '</span> ' : '';
   $suffix = isset($element['#field_suffix']) ? ' <span class="field-suffix">' . $element['#field_suffix'] . '</span>' : '';

Look at this same line in the patch and in the current CVS HEAD:

(patch)

$class[] = 'form-item-' . strtr($element['#name'], array(' ' => '-', '_' => '-', '[' => '-', ']' => ''));

(CVS HEAD)

$attributes['class'][] = 'form-item-' . strtr($element['#name'], array(' ' => '-', '_' => '-', '[' => '-', ']' => ''));

It seems the array $class has been moved into $attributes. The line above is not being changed by the patch, but the patch does use the array $class. Maybe changing this to $attributes['class'] in the patch would at least allow it to be applied?

On second thought, using the incorrect array would probably cause the patch to not work or fail other testing, but it should still apply correctly. Maybe because the line numbers aren't matching up?

I think this issue hasn't been worked on, because according to #77, it requires #715142: [beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain() to be fixed first. That issue is waiting for a security review from someone on the security team, or someone who feels they have enough experience with XSS attacks in Drupal to confidently RTBC it.

Oh, I see, thanks! Then at that point the patch would have to be updated? It seems it isn't applying anymore just because it is outdated and the files have been modified since. Is that how it works?

Yep. As Dries and webchick commit patches from other issues, HEAD keeps changing, sometimes in a way that causes other patches to break, requiring them to be "re-rolled" (i.e., applied, then manually fixing the rejections, and uploading an updated patch file). Thanks for helping out on the issue queue! I hope you can find an issue to help out on that isn't being blocked by something else!

We badly need #715142: [beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain() in to move forward here.

+++ includes/form.inc	29 Apr 2010 22:21:59 -0000
 function theme_textfield($variables) {
-    $extra =  '<input class="autocomplete" type="hidden" id="' . $element['#id'] . '-autocomplete" value="' . check_url(url($element['#autocomplete_path'], array('absolute' => TRUE))) . '" disabled="disabled" />';
+    $attributes['value'] = url($element['#autocomplete_path'], array('absolute' => TRUE));
 }
 function theme_form($variables) {
-  $action = $element['#action'] ? 'action="' . check_url($element['#action']) . '" ' : '';
+  if (!empty($element['#action'])) {
+    $element['#attributes']['action'] = check_url($element['#action'], FALSE);
+  }
 }

I'm about to split #715142: [beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain() into 2 separate issues: leave that one dealing with the double check_plain() problem, and open a new issue for the protocol stripping question. The above two hunks are the ones waiting on resolution to those questions.

+++ modules/file/file.module	29 Apr 2010 22:15:36 -0000
@@ -442,6 +442,12 @@ function file_managed_file_process($elem
+  if (!empty($element['#disabled'])) {
+    $element['upload']['#disabled'] = $element['#disabled'];
+    $element['upload_button']['#disabled'] = $element['#disabled'];
+    $element['remove_button']['#disabled'] = $element['#disabled'];
+  }
+

@@ -843,6 +843,9 @@ function filter_process_format($element)
     '#attributes' => array('class' => array('filter-list')),
     '#parents' => array_merge($element['#parents'], array('format')),
   );
+  if (!empty($element['#disabled'])) {
+    $element['format']['format']['#disabled'] = $element['#disabled'];
+  }

This is being addressed in #776956: [beta blocker blocker] Complex widgets are not respecting "#disabled" state, so it probably makes sense for that to land first and be pulled out of this patch. So this issue now has 2 pre-req issues.

Powered by Dreditor.

Shine strikes back.

Pass, please, pass off.

Pass. Please. Pass away.

RTBC.

Command & Conquer vs. #776956: [beta blocker blocker] Complex widgets are not respecting "#disabled" state

#95: drupal.form-disabled.95.patch queued for re-testing.

Those new, existing tests are quite fragile... quite a pain to maintain. Anyway, this one should pass again.

+++ modules/file/file.module	11 Jul 2010 16:12:31 -0000
@@ -422,6 +422,12 @@ function file_managed_file_process($elem
+  if (!empty($element['#disabled'])) {
+    $element['upload']['#disabled'] = $element['#disabled'];
+    $element['upload_button']['#disabled'] = $element['#disabled'];
+    $element['remove_button']['#disabled'] = $element['#disabled'];
+  }
+
...
+++ modules/filter/filter.module	11 Jul 2010 16:12:31 -0000
@@ -829,6 +829,9 @@ function filter_process_format($element)
+  if (!empty($element['#disabled'])) {
+    $element['format']['format']['#disabled'] = $element['#disabled'];
+  }

Now that #776956: [beta blocker blocker] Complex widgets are not respecting "#disabled" state landed, do we need this?

Powered by Dreditor.

Thanks, removed both hunks.

On a related note: The new core theme Bartik does not implement styles for disabled form buttons. Not the end of the world, as you can't input anything or click a disabled button in the first place. Separate and also minor issue, compared to the other styling issues I've encountered so far and collecting in #849770: Bartik design problems

<form>'s action attribute is an URL; do URLs survive check_plain()? #40870: setting class, type, name, id, or value on forms api #attributes on checkbox results in duplicate attributes

Same applies to form_process_password_confirm().

@casey: Yes, all HTML attributes need to be output via check_plain(). Not entirely sure what that other issue is about, let's discuss over there.

How to test this? Does it need visual review or is the last part of this issue mainly about code tests?
Without applying the patch, I already see a select list styled as disabled: http://skitch.com/yoroy/dcnhq/d7
Where do I find other cases of disabled form elements?

To test this manually, you may use the http://drupalcode.org/viewvc/drupal/contributions/sandbox/sun/markup_test module from my sandbox (can be CVS-checked out like any other module, just note the different path).

Can I download an archive of the module somewhere?

mh... just because this issue is critical.

Thanks, but it's not clear to me what the module does or how it's used.

Edit. found it

tag

I think this works...
I attached some screenshots, they are high resolution ones so srry for those with smaller screens.
Someone can rtbc this if they feel this fixes the issue.

EDIT: maybe garland and bartik buttons needs some work...

yea seven and garland don't yet have rules that act upon those classes

Thank you aspilicious. Stark is looking fine, so core defaults are covered. I opened followups for Garland and Bartik

#854436: Add styling for disabled form elements
#854432: Add styling for disabled form elements

#103: drupal.form-disabled.102.patch queued for re-testing.

#103: drupal.form-disabled.102.patch queued for re-testing.

+++ includes/form.inc	11 Jul 2010 22:29:51 -0000
@@ -3047,7 +3064,11 @@ function theme_image_button($variables) 
 function theme_hidden($variables) {
   $element = $variables['element'];
-  return '<input type="hidden" name="' . $element['#name'] . '" id="' . $element['#id'] . '" value="' . check_plain($element['#value']) . "\" " . drupal_attributes($element['#attributes']) . " />\n";
+  $element['#attributes']['type'] = 'hidden';
+  $element['#attributes']['name'] = $element['#name'];
+  $element['#attributes']['id'] = $element['#id'];
+  $element['#attributes']['value'] = $element['#value'];
+  return '<input' . drupal_attributes($element['#attributes']) . " />\n";
 }

Can't we summarize $element['#attributes]['foo'] to $element['foo'] in all the functions?

@Dries please no, that way you can't get a list of all attributes set for an element.

I considered that, but went with the more verbose #attributes variable usage. I think it is more clear this way.

Anyway, we can also change this minor coding style later on - let's get this critical out of the way. :)

OK, let's commit this for now. Committed to CVS HEAD.

@@ -3024,15 +3032,24 @@ function theme_button($variables) {
  */
 function theme_image_button($variables) {
   $element = $variables['element'];
+  $element['#attributes']['type'] = 'submit';

This is wrong and fixed in #883336: theme_image_button() broken (needs tests only)

Reverting status. Let's add some tests over there.

+++ includes/form.inc	11 Jul 2010 22:29:51 -0000
@@ -3149,12 +3188,19 @@ function theme_textarea($variables) {
-  $size = $element['#size'] ? ' size="' . $element['#size'] . '" ' : '';
-  $maxlength = $element['#maxlength'] ? ' maxlength="' . $element['#maxlength'] . '" ' : '';
-
+  $element['#attributes']['type'] = 'password';
+  $element['#attributes']['name'] = $element['#name'];
+  $element['#attributes']['id'] = $element['#id'];
+  $element['#attributes']['value'] = $element['#value'];
+  if (!empty($element['#size'])) {
+    $element['#attributes']['size'] = $element['#size'];
+  }
+  if (!empty($element['#maxlength'])) {
+    $element['#attributes']['maxlength'] = $element['#maxlength'];
+  }
   _form_set_class($element, array('form-text'));
-  $output = '<input type="password" name="' . $element['#name'] . '" id="' . $element['#id'] . '" ' . $maxlength . $size . drupal_attributes($element['#attributes']) . ' />';
-  return $output;
+

Um, this is actually a very serious regression. Notice that before the patch, a password field never has a value attribute. Now it always does.

What does this mean? Go to user/login, enter your username and an incorrect password and submit. Notice that the password field is pre-filled out, and in the source code of the input element is the plain-text password you entered.

Now let's say you are at a library and forget to close your browser window. Someone can easily view the source code and have a very good guess at something close to your password, and your correct user account.

Powered by Dreditor.

I agree this is the correct patch. Credit goes to cwgordon7 for discovering the issue.

updating priority

Since we don't want to ever introduce this again, can we please add a test for it?

RTBC for tests + #128 = RTBC