Allow FAPI usage of the placeholder attribute

Simple enough,

Apply patch, modified a couple of forms and added #placeholder => 'Testing' to a couple of elements.

I saw my placeholder text in my browser.

Not much more to do after that.

+1

I'm pretty comfortable with us adding the ability to use this, so I am good with this patch. I would be much more hesitant to approve of a patch that was actually adding placeholders in Core without some accessibility testing.

In a followup issue, would be cool to add corresponding features to the relevant Field API widgets (textfield, textarea for text and number fields)

@yched, Yea. I think we need a tickets for Field API widgets. Is came up in a couple of issues, I've been trying to stay away from them for now. :)

@ericduran : sure, those deserve separate, followup issues

This is probably the simplest patch out of all the html5 issues.

Getting this patch would be a huge win, mainly because it'll mean we got something in ;)

adding tag.

/me adding password and textarea now.

Here's an updated patch. Now is testing textfield, password and the textarea.

*needs whitespace cleanup.

+++ b/modules/simpletest/tests/form.testundefined
@@ -387,6 +387,29 @@ class FormElementTestCase extends DrupalWebTestCase {
+    $this->assertTrue(!empty($element), t('Placeholder text placed in textarea.'));
+    ¶
+  }

Newline issues, a few.

Powered by Dreditor.

Applied the patch in #17. In my test module, all three elements (textfield, textarea, and password) look and behave correctly. Marking RTBC.

Tagging.

#17: 865536-drupal_add_js-add-browser-options.patch queued for re-testing.

#11: 1174694-10-fapi-placeholder-property.patch queued for re-testing.

#1: 1174694-fapi-placeholder-property.patch queued for re-testing.

#12: 1174694-11-fapi-placeholder-property.patch queued for re-testing.

#14: 1174694-11-fapi-placeholder-property-2.patch queued for re-testing.

@iflista, is there a reason you keep retesting? also no need to test the previous patches only the last one.

Sorry. I was trying to figure out what I have to do here. It was the first time I've applied retesting. If you have any info about testing and reviewing patches, I will be very appreciated. Thanks anyway, ericduran.

+++ b/modules/simpletest/tests/form.testundefined
@@ -387,6 +387,28 @@ class FormElementTestCase extends DrupalWebTestCase {
+    foreach (array('textfield','password') as $type) {
+      $element = $this->xpath('//input[@id=:id and @placeholder=:expected]', array(
+        ':id' => 'edit-' . $type,
+        ':expected' => $expected,
+      ));
+      $this->assertTrue(!empty($element), t('Placeholder text placed in @type.', array('@type' => $type)));
+    }
+
+    // Now lets test the textarea.
+    $element = $this->xpath('//textarea[@id=:id and @placeholder=:expected]', array(
+      ':id' => 'edit-textarea',
+      ':expected' => $expected,

The "Now lets test the textarea" comment for this test could be improved. It would make it easier to read if there was a comment saying "Test the input element." and "Test the textarea element." at the top of each chunk of code.

+++ b/modules/simpletest/tests/form_test.moduleundefined
@@ -981,6 +987,22 @@ function form_test_select_submit($form, &$form_state) {
+ * Builds a form to test the place-holder attribute.

There shouldn't be a dash in between place and holder here.

+++ b/modules/simpletest/tests/form_test.moduleundefined
@@ -981,6 +987,22 @@ function form_test_select_submit($form, &$form_state) {
+function form_test_placeholder_test($form, &$form_state) {
+

This is a total nitpick on code style, but could we please remove the blank line at the beginning of the function?

Other than those nitpicks, this looks good to me and will probably be the first patch that goes in, which is exciting! :D

Also, it's been brought to my attention that the issue name implies full support with polyfills and all where needed, so I'm changing the title of the issue to better reflect what it's for.

Better title. Don't have to add anything to #27.

That title isn't better. It's not grammatically correct. :P

Easy enough.

removing tags.

lol, thats what happens when you don't create proper patch and try editing your-self. lol New patch coming right up.

Attempt number 4 maybe 5

Ugh, @ericduran, I'm sorry I missed this last time around, but there are some minor coding style issues with this. I swear this is the last time. Once we fix these, this one is ready to go. Thanks for all the awesome work you've been doing! :D

+++ b/modules/simpletest/tests/form.testundefined
@@ -387,6 +387,29 @@ class FormElementTestCase extends DrupalWebTestCase {
+    // Test to make sure textfields and passwords have the proper placeholder text.

The comment is longer than 80 chars, and should be wrapped.

+++ b/modules/simpletest/tests/form.testundefined
@@ -387,6 +387,29 @@ class FormElementTestCase extends DrupalWebTestCase {
+    foreach (array('textfield','password') as $type) {

There should be a space after the comma ('textfield', 'password').

Sweet! Thank you Lin! :D

+++ b/modules/simpletest/tests/form_test.module
@@ -105,6 +105,12 @@ function form_test_menu() {
+  $items['form-test/placeholder-text'] = array(
+    'title' => t('Placeholder'),

No t() in hook_menu().

RTBC afterwards.

16 days to next Drupal core point release.

We should probably open up a separate issue to remove all the t() from all those menu callbacks in the form_test.module.

+++ b/includes/form.inc
@@ -3650,14 +3650,14 @@ function theme_hidden($variables) {
+  element_set_attributes($element, array('id', 'name', 'value', 'size', 'maxlength', 'placeholder'));

+++ b/modules/simpletest/tests/form.test
@@ -387,6 +387,30 @@ class FormElementTestCase extends DrupalWebTestCase {
+  function testPlaceHolderText() {
...
+    $expected = 'placeholder-text';
...
+        ':expected' => $expected,

+++ b/modules/simpletest/tests/form_test.module
@@ -981,6 +987,21 @@ function form_test_select_submit($form, &$form_state) {
+      '#placeholder' => 'placeholder-text',

Sorry, one final issue:

What's the expected HTML escaping?

In general, Form API array values are expected to be escaped already:

'#placeholder' => check_plain('>> placeholder <<'),

Let's confirm proper escaping in that test. (also no double-escaping)

16 days to next Drupal core point release.

Sorry, I was distracted and totally confused. It's the other way around -- form API array values are escaped automatically.

err, but anyway, let's confirm this in a test, please. :) oh my, sorry for the noise ;)

Sleeping over this once more, I have to play devil's advocate:

Why do we need a Form API #placeholder property for this in the first place?

$form['search'] = array(
  '#type' => 'search',
  '#placeholder' => t('Search…'),
);

vs.

$form['search'] = array(
  '#type' => 'search',
  '#attributes' => array('placeholder' => t('Search…')),
);

Yeah, I agree there. We did the same with #disabled and that was really big for DX; it's no longer the 2nd most asked question in IRC back when it was when it was an attribute.

As mention above is all for DX. Besides that this is all technically possible right now, but this just makes it SO much easier.

There are solid chunks of code throughout Form API / form.inc to ensure that #disabled really means #disabled. Form API does not accept user input on a #disabled input field, even if you manage to hack some fake input into your POST data.

You may set #attributes['disabled'] if you want to skip this logic and make a form input field only "disabled" in the UI, possibly "enabling" it via JavaScript.

Likewise, properties like #maxlength are additionally validated on the server-side.

While I get the DX argument to some extent, there's also a prominent counter-example:

#id and #attributes['id'] -- if you know the difference, then you likely know Form API pretty well. (and because of that, I'd say that's worse DX)

However, that example actually isn't good, since as with #disabled, there is a difference between both. So let's look at this one instead:

$form['search'] = array(
  '#type' => 'search',
  '#class' => array('search'),
);

This does not exist, even though it's one of the topmost use-cases throughout core and contrib.

But why doesn't it exist? Because there's no special logic, parsing, or security involved. Duplicating #attributes['class'] into #class would add nothing but confusion.

Simply consider that you set #attributes['class'] in your code and someone else sets #class in their code. Who wins?

Going back to the examples, the properties with actual meaning behave differently: A custom #attributes['id'] overrides #id (which is auto-generated), whereas #disabled overrides whatever value in #attributes['disabled'].

For those special properties it makes sense to have internal logic tied to them. The system needs it to work.

Now, I can of course see the argument that "we're going to add special logic for #placeholder, too! But later.", which is sound.

However, applying any kind of special logic boils down to replacing

function form_process_placeholder($element, &$form_state) {
  if (isset($element['#placeholder']) && $element['#placeholder'] !== '') {
    // Fill the gap for older browsers.
    drupal_add_js('misc/placeholder.js');
    // ...
  }
}

with:

function form_process_placeholder($element, &$form_state) {
  if (isset($element['#attributes']['placeholder']) && $element['#attributes']['placeholder'] !== '') {
    // Fill the gap for older browsers.
    drupal_add_js('misc/placeholder.js');
    // ...
  }
}

Duplicating all possible #attributes into element #properties is definitely not going to be an improvement. Especially not in terms of DX.

I agree this is RTBC.

Having it in #attributes doesn't make sense to me. If this were a global attribute, it'd be a different story, but it's not.

I'd like to see some other Form API maintainer(s) chime in here.

The argument that the attribute only applies to certain element #types doesn't really hold from my perspective. At least, that should not be the primary reason for introducing a new type-specific #property. As explained above, the primary reason for introducing new #properties is special handling on the server-side.

Please don't get me wrong, I'm absolutely in favor of html5 improvements. However, at times, I also have to put on my Form API maintainer hat in order to ensure and retain sanity in the API. And it's with that hat being on, where this change doesn't really make sense from a technical perspective.

As explained above, the primary reason for introducing new #properties is special handling on the server-side.

Is this documented somewhere? I'd be curious to hear from other FAPI maintainers too.

I think what you are getting at here has nothing to do with HTML5 and is a totally separate issue. I'm not trying to say it has no merit; I just disagree in this case. There are other properties that do not have server side handling such as #rows, #cols and #src.

Here's a couple of other things to consider from a Form api (not maintainer but at least an avid user ;-) ) perspective.

The #disabled property is a good example here because it actually does a couple of things. At is core it just sets $element['#attributes']['disabled'] = 'disabled'; or $element['#attributes']['readonly'] = 'readonly'; It does other things, but this is what people use the $element['#disabled'] property for. Right now nothing is stoping a user from doing $element['#attributes']['readonly'] = 'readonly'; or $element['#attributes']['disabled'] = 'disabled'; except no-body does it because is a lot simpler to do $element['#disabled'] and you know you'll get the same effect.

Essentially doing #placeholder is a DX improvement and it'll also helps clear up what elements have the property available. If I look at the http://api.drupal.org/api/drupal/developer--topics--forms_api_reference.... I can easily tell that I shouldn't use '#cols' in a textfield even thought if I put it in $element['#attributes']['cols'] = 10, is going to show up on the html.

All in all, I guess we're going to required more feedback on this one.

"As explained above, the primary reason for introducing new #properties is special handling on the server-side."

That's news to me, and I have been involved in most FAPI core discussions since the original push to put FAPI into Drupal core back in 4.7.

From a FAPI user perspective, this makes total sense as a top-level property. The fact that 4 other people are saying the same as fellow FAPI users ought to help sway the "formal" definition in favour of meeting user expectations, IMO, unless there's some compelling technical reason not to.

However, it sounds like this is on hold pending chx's review.

Sorry! I can't chime in meaningfully as I have no idea what a #placeholder is, I surmise it's something like #default_value but how it's different?

Edit: i wrote an issue summary based on an IRC chat.

chx now knows what the placeholder attribute is.

We are going to need to have better summaries on these issues going forward.

Hiding something in the dumping ground of the of #attributes when we already have a dumping ground on the top is no good practice. Please file a followup for #class it is a good idea!

This is still RTBC, just tagging to keep track of it, while we wait.

Dries gets back from vacation this week, and I have added this to his to-do list. :)

I reviewed this patch and it looks good. Committed it to 8.x.

Please file a followup for #class it is a good idea!

Any takers? Please link to it from here.

@sun sure I'll see if I can do that later. Question: how do we update the d8 form API reference to include the new attribute?

Unfortunately, the Form API reference guide is still buried in... nirvana, actually no idea. But if I'm not mistaken, "Needs Documentation" should make the right folks aware of this change.

Opened #1241938: Add support for #placeholder to relevant Field API widgets as a followup.

Opinions / proposals welcome over there :-)

@sun #1244338: Add generic #class property for form elements "Please file a followup for #class it is a good idea!"

The FAPI doc is now buried in the Documentation project. Could you possibly file an issue in the Documentation project with component "api documentation files" and explain what needs to be documented, and for which versions of Drupal?

Also, is this an API change -- if so, please create an API change node to document the change.

Removed sprint tags and unassigned Dave Reid. Still needs docs, but hopefully someone can jump in to help with that. This is a summary of what the patch does: http://jsfiddle.net/jacine/7HKXu/embedded/result/

@jhodgdon This my need a change notification too, but I'm not sure. Can you please let me know? There will be more like this in the future, so I'm hoping to get a better handle on this. It's not actually implemented on any form elements (and may or may not be in the future) but it's an API change that needs to be documented.

RE change notification: the guidelines are that if a module developer, themer, Coder module maintainer, Examples project maintainer, or user-facing doc writer would need to know about it, it needs to have a change notification.

And if it needs docs, can you please file separate docs issues explaining what docs are needed?

Someone I think still needs to create a change notification node for this issue:
http://drupal.org/node/add/changenotice

Tagging.

Hmm, Can someone explain to me what a changenotice is? lol I've never had to do one before.

Thanks.

ericduran: see comment #76 above