Seven doesn't give any visual hint when a form element is #disabled.
Just lets you click desperately and blame your browser for not giving focus :-).
See screenshot.
Comment | File | Size | Author |
---|---|---|---|
#131 | 690980_test.patch | 973 bytes | grendzy |
#131 | 690980_fix.patch | 1.53 KB | grendzy |
#127 | 690980-password-value-regression.patch | 777 bytes | Dave Reid |
#111 | bartik_test.png | 78.91 KB | aspilicious |
#111 | garland_test.png | 69.7 KB | aspilicious |
Comments
Comment #1
seutje CreditAttribution: seutje commentedBecause 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
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
Comment #2
seutje CreditAttribution: seutje commentedhmz, 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)
Comment #3
seutje CreditAttribution: seutje commentedattached 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
Comment #4
seutje CreditAttribution: seutje commentedwinging some colors, doesn't include buttons
wouldn't mind some eyes on this
Comment #5
yched CreditAttribution: yched commentedseutje: thanks for tackling this.
The FAPI part looks good. A screenshot would definitely help the CSS part be approved :-).
Comment #6
webchickWe should have tests for this.
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.
Comment #7
yched CreditAttribution: yched commented@webchick: you need to look at the edit form for a field that already has values.
I.e :
- create an article node, fill in a body
- go to admin/structure/types/manage/article/fields/body/edit
Comment #8
seutje CreditAttribution: seutje commented@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
Comment #9
Bojhan CreditAttribution: Bojhan commentedSo all we need is tests?
Comment #10
seutje CreditAttribution: seutje commentedScreenshots: Textfield - Textarea - Select list
Comment #11
yoroy CreditAttribution: yoroy commentedSorry 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".
Comment #12
seutje CreditAttribution: seutje commentedchanged colors to be less obtrusive -> screenshot
Comment #13
yoroy CreditAttribution: yoroy commentedMuch better
Comment #14
aspilicious CreditAttribution: aspilicious commentedA lot better and green: http://qa.drupal.org/pifr/test/26698
Comment #15
seutje CreditAttribution: seutje commentedheh? this does not yet contain a test for it
Comment #16
yoroy CreditAttribution: yoroy commentedneeds work for tests then
Comment #17
alexanderpas CreditAttribution: alexanderpas commentedmaybe 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
Comment #18
sun.core CreditAttribution: sun.core commentedNot critical. Please read and understand Priority levels of Issues, thanks.
Comment #19
alexanderpas CreditAttribution: alexanderpas commented/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
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?)
Comment #20
Dave ReidAck 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.
Comment #21
seutje CreditAttribution: seutje commentediono, 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
Comment #22
webchickIt's critical. Now can we get a RTBC patch? :)
Comment #23
Bojhan CreditAttribution: Bojhan commentedtagging,
Comment #24
casey CreditAttribution: casey commentedElements 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
Comment #25
Bojhan CreditAttribution: Bojhan commented@alexanderpas Can you provide a patch perhaps, seems this issue can easily be fixed if we have a initial patch
Comment #26
casey CreditAttribution: casey commentedHad a little chat with seutje. I agree with him we should use his approach:
Using the class .form-element-disabled like in the patch of @seutje in #12 you can.
Seutje will post a new patch.
Comment #27
seutje CreditAttribution: seutje commentedadded comment
Comment #28
yoroy CreditAttribution: yoroy commenteddoh :)
Comment #29
sgabe CreditAttribution: sgabe commentedI've tested the patch in #27 and works fine, but the file upload field is missing from the CSS.
Comment #30
bleen CreditAttribution: bleen commentedextra white space at the end of each line
whitespace
Powered by Dreditor.
Comment #31
bleen CreditAttribution: bleen commentedComment #32
seutje CreditAttribution: seutje commented@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
Comment #33
sgabe CreditAttribution: sgabe commented@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.
Comment #34
Bojhan CreditAttribution: Bojhan commentedCan anyone whip up a new patch?
Comment #35
seutje CreditAttribution: seutje commented@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
Comment #36
seutje CreditAttribution: seutje commentedhmm, 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?
Comment #37
stephenhay CreditAttribution: stephenhay commentedI would advise turning off the inset on disabled fields, similar to your first screenshot. It's quite clear that it's disabled.
Comment #38
sunCan we shorten this class to .form-disabled, please?
113 critical left. Go review some!
Comment #39
bsherwood CreditAttribution: bsherwood commented@sun: Wouldn't .element-disabled be a better choice? Since that class wouldn't disable an entire form, just individual elements within it.
Comment #40
sunI 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.
Comment #41
seutje CreditAttribution: seutje commentedI really don't care about the naming, but yea, form-element-disabled might be a bit verbose
changed it to form-disabled
Comment #42
aspilicious CreditAttribution: aspilicious commentedneeds review
Comment #43
sun1) 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!
Comment #44
seutje CreditAttribution: seutje commentedremoved 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
Comment #45
seutje CreditAttribution: seutje commentedforgot to add the actual patch :x
Comment #46
seanyo CreditAttribution: seanyo commentedHow's this for a revision of the comment text:
Comment #47
sunThere you go.
Comment #48
seutje CreditAttribution: seutje commentedawesome, thanks <3
will try to focus on writing a test for this tomorrow
Comment #49
seanyo CreditAttribution: seanyo commentedOkay - 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
Comment #50
sunI can only guess that #49 didn't reload this issue page prior to posting, so further patches should be based on #47.
Comment #51
aspilicious CreditAttribution: aspilicious commentedI don't think we put two spaces behind a .
102 critical left. Go review some!
Comment #52
cweagans#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.
Comment #53
seutje CreditAttribution: seutje commentedoh, no test for it then?
Comment #54
yoroy CreditAttribution: yoroy commentedseanyo: congrats on your first submitted patch though! :)
Comment #55
gowriabhaya CreditAttribution: gowriabhaya commentedCode sprint tag
Comment #56
seanyo CreditAttribution: seanyo commentedThanks! 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
Comment #57
TR CreditAttribution: TR commentedAfter 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.
Comment #58
seutje CreditAttribution: seutje commentedugh, 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
Comment #59
seutje CreditAttribution: seutje commentedugh, 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
Comment #60
yoroy CreditAttribution: yoroy commentedSeanyo: yes, correct. Need to apply the latest patch first so that you build on the work already done.
Comment #61
sun1) 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)
Mapping to the above: This also needs to account for .form-button and .form-image-button, not just .form-submit.
Either use background-color or also specify a background image.
96 critical left. Go review some!
Comment #62
seutje CreditAttribution: seutje commented1) 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
Comment #63
acouch CreditAttribution: acouch commentedAttached 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.
Comment #64
seutje CreditAttribution: seutje commented@sun actually, I was wrong, the button types are like the type of the input ("submit", "reset" and "button")
Comment #65
sun@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.
Comment #66
seutje CreditAttribution: seutje commented@sun so does this make more sense?
thank you so much for that test, I included it in this patch
Comment #67
sunIf 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.
Trailing white-space, and missing newline before test.
Can a disabled form element become :active at all?
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.
Comment #68
seutje CreditAttribution: seutje commentedyea, 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
Comment #69
sunThanks!
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.
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.
Comment #70
johndtaylor CreditAttribution: johndtaylor commentedHi - 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.
Comment #71
acouch CreditAttribution: acouch commentedhi @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.
Comment #72
seutje CreditAttribution: seutje commentedsun'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
Comment #73
seutje CreditAttribution: seutje commentedyay! 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)
Comment #74
seutje CreditAttribution: seutje commentedok, 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)
Comment #75
sunAlright. 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.
Comment #76
sunThis looks RTBC to me.
Comment #77
seutje CreditAttribution: seutje commentedoh 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?
Comment #79
effulgentsia CreditAttribution: effulgentsia commentedsubscribing
Comment #80
adam.hastings CreditAttribution: adam.hastings commented#75: drupal.form-disabled.75.patch queued for re-testing.
Comment #81
adam.hastings CreditAttribution: adam.hastings commented#76: drupal.form-disabled.76.patch queued for re-testing.
Comment #83
adam.hastings CreditAttribution: adam.hastings commentedI 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:
That hunk seems to be:
Look at this same line in the patch and in the current CVS HEAD:
(patch)
(CVS HEAD)
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?
Comment #84
adam.hastings CreditAttribution: adam.hastings commentedOn 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?
Comment #85
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #86
adam.hastings CreditAttribution: adam.hastings commentedOh, 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?
Comment #87
effulgentsia CreditAttribution: effulgentsia commentedYep. 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!
Comment #88
sunWe 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.
Comment #89
effulgentsia CreditAttribution: effulgentsia commentedI'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.
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.
Comment #90
sunShine strikes back.
Comment #92
sunPass, please, pass off.
Comment #94
sunPass. Please. Pass away.
RTBC.
Comment #95
sunCommand & Conquer vs. #776956: [beta blocker blocker] Complex widgets are not respecting "#disabled" state
Comment #97
lotyrin CreditAttribution: lotyrin commented#95: drupal.form-disabled.95.patch queued for re-testing.
Comment #99
sunThose new, existing tests are quite fragile... quite a pain to maintain. Anyway, this one should pass again.
Comment #100
effulgentsia CreditAttribution: effulgentsia commentedNow that #776956: [beta blocker blocker] Complex widgets are not respecting "#disabled" state landed, do we need this?
Powered by Dreditor.
Comment #101
sunThanks, 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
Comment #102
casey CreditAttribution: casey commented<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
Comment #103
sunSame applies to form_process_password_confirm().
Comment #104
sun@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.
Comment #105
yoroy CreditAttribution: yoroy commentedHow 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?
Comment #106
sunTo 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).
Comment #107
yoroy CreditAttribution: yoroy commentedCan I download an archive of the module somewhere?
Comment #108
sunmh... just because this issue is critical.
Comment #109
yoroy CreditAttribution: yoroy commentedThanks, but it's not clear to me what the module does or how it's used.
Edit. found it
Comment #110
philbar CreditAttribution: philbar commentedtag
Comment #111
aspilicious CreditAttribution: aspilicious commentedI 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.
Comment #112
aspilicious CreditAttribution: aspilicious commentedEDIT: maybe garland and bartik buttons needs some work...
Comment #113
seutje CreditAttribution: seutje commentedyea seven and garland don't yet have rules that act upon those classes
Comment #114
yoroy CreditAttribution: yoroy commentedThank 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
Comment #115
sun#103: drupal.form-disabled.102.patch queued for re-testing.
Comment #116
marcingy CreditAttribution: marcingy commented#103: drupal.form-disabled.102.patch queued for re-testing.
Comment #117
Dries CreditAttribution: Dries commentedCan't we summarize $element['#attributes]['foo'] to $element['foo'] in all the functions?
Comment #118
casey CreditAttribution: casey commented@Dries please no, that way you can't get a list of all attributes set for an element.
Comment #119
sunI 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. :)
Comment #120
Dries CreditAttribution: Dries commentedOK, let's commit this for now. Committed to CVS HEAD.
Comment #122
yched CreditAttribution: yched commentedLooks like this broke theme_image_button(). See the screenshot in #796658-184: UI for field formatter settings.
The committed patch has the image_button #src property go through drupal_attributes(), which runs check_plain() on attribute values and thus messes with the src URL.
Comment #123
andypostThis is wrong and fixed in #883336: theme_image_button() broken (needs tests only)
Comment #124
sunReverting status. Let's add some tests over there.
Comment #126
Dave ReidUm, 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.
Comment #127
Dave ReidComment #128
grendzy CreditAttribution: grendzy commentedI agree this is the correct patch. Credit goes to cwgordon7 for discovering the issue.
Comment #129
grendzy CreditAttribution: grendzy commentedupdating priority
Comment #130
webchickSince we don't want to ever introduce this again, can we please add a test for it?
Comment #131
grendzy CreditAttribution: grendzy commentedComment #132
bleen CreditAttribution: bleen commentedRTBC for tests + #128 = RTBC
Comment #133
yched CreditAttribution: yched commentedLooks like webchick committed this : http://drupal.org/cvs?commit=491054