Follow up to #1174694: Allow FAPI usage of the placeholder attribute

Add widget settings for the new #placeholder FAPI property, in the widgets for which it is relevant (textfield and textarea widgets for text, number and term fields).

The shape of options and corresponding UI will probably need some thinking. Some use cases :
- no placeholder
- instance label as #placeholder rather than regular #title
- custom text as #placeholder rather than or in addition to instance label as regular #title

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue tags: +html5
Jacine’s picture

ericduran’s picture

sub.

idflood’s picture

sub

Bojhan’s picture

Issue tags: -Needs usability review
ericduran’s picture

Assigned: Unassigned » ericduran
litwol’s picture

$form['field'] = array(
 '#attributes' => array(
    'placeholder' => 'your text here',
  ),
);

Although i imagine #placeholder support would come equipped with backwards compatibility JS support.

RobLoach’s picture

Status: Active » Postponed

Should have it in Form API before we stick the support for it into Field API: #1174694: Allow FAPI usage of the placeholder attribute.

RobLoach’s picture

Status: Postponed » Active

Oh, it was committed? Well then.

ericduran’s picture

Placeholder support is already in form api. It was probably the 1st html5 patch to get in, a while back.

This issue is about adding placeholder support in the field api aka when someone creates a new field, and you get the option to add labels, default text, and now also placeholder :)

ericduran’s picture

Oh and no, backwards compatibility is not included. Even thought is probably the simplest fallback ever.

But so far none of the html5 feature have had backwards compatibility added to them.Anyways this should be discuss in a separate issue.

Everett Zufelt’s picture

- no placeholder
- instance label as #placeholder rather than regular #title
- custom text as #placeholder rather than or in addition to instance label as regular #title

I think that this can be simplified to:

- No placeholder (default)
- Instance label (Use label as placeholder)
- Custom text

How do we communicate, through the UI, what a 'Placeholder' is, and what it does?

ericduran’s picture

It should be clear to everyone that placeholder is not in anyway a replacement for title/label. Placeholder is a completely different attribute and should in no way have anything to do with the title/label.

So what should be expected in this patch is another text field, with the Title of "Placeholder". Now the description can be for now be what's explain in the actual spec which does a good job -- "The placeholder attribute represents a short hint (a word or short phrase) intended to aid the user with data entry. A hint could be a sample value or a brief description of the expected format. ".

This way the user chooses what to put in the placeholder we don't need to have any default or anything like that. The default is nothing if nothing is entered :)

Niklas Fiekas’s picture

Status: Active » Needs review
FileSize
3.04 KB

Here is a first try to implement it for the text field type. Would it be done like that for the other field types, as well?

yched’s picture

Thanks for attacking this :-)

As a 1st remark, though, this would rather be a widget setting (for the 'text_textfield' widget) than an instance setting. Supporting placeholders is a property of the widget, not of the field type. For instance, a slider widget on a number field wouldn't support placeholders, while a basic textfield widget would.

Status: Needs review » Needs work

The last submitted patch, 1241938-field-placeholder.patch, failed testing.

Niklas Fiekas’s picture

Thanks for looking at it. Yeah ... I suspected something like that, so I only did it for text, first. Rather like this?

Niklas Fiekas’s picture

Status: Needs work » Needs review
Niklas Fiekas’s picture

So ... here's the same thing for all the relevant widgets.

yched’s picture

Looks good for a start :-), but, as I wrote in the OP, I think we need a couple more options.
@Everett Zufelt's proposals in #12 sound good.

This could be a set of 3 radio buttons :

Placeholder:
o No placeholder (default)
o Instance label (Use label as placeholder) [note: that probably means we also hide the regular #title]
o Custom text (selecting this reveals a textfield input for the custom text, through the #states API

Internally, this means two settings:
'placeholder' : 'none', 'label', 'custom', defaults to 'none'
'placeholder_custom': (only used if the above is 'custom'), defaults to ''

Niklas Fiekas’s picture

@yched: I think #13 makes a legitimate point about that, though.

Niklas Fiekas’s picture

Reroll with the new default settings.

Niklas Fiekas’s picture

ericduran’s picture

Issue tags: +ericduran-todo

Bumping and tagging for myself :)

ericduran’s picture

Issue tags: -html5, -ericduran-todo

#23: 1241938-field-placeholder-23.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +html5, +ericduran-todo

The last submitted patch, 1241938-field-placeholder-23.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
33.97 KB
26.34 KB
8.24 KB

This is so cool, let's get this in asap! Patch (+tests) takes the approach so widget plugins only need to add the 'placeholder' setting to their settings and it will be available automatically on the field instance settings screens and on the entity forms. I think this is a nice DX and also makes sure we don't have to copy the same $placeholder element form to every widget form of every field type. It *could* be a helper function of course, but I really like this approach.

I also agree with #13 as well. Let's make this initially easy and not confusing for users.

And some screenshots.

Screen Shot 2012-11-24 at 18.06.42.png

Screen Shot 2012-11-24 at 18.06.25.png

@ericduran Hope I didn't make you waste valuable time, I initially read the date wrong - I though you assigned it 4 months ago :)

yched’s picture

Thanks for reviving this, swentel !

I'd rather not hardcode behavior on the presence of a specific "setting", though. What is in 'settings' is the playground of each widget type, what is in there and the meaning it has is entirely up to them, and the system applies no hardcoded semantics on the presence or absence of a setting with a given name - that's why those properties were separated in a 'settings' entry in the first place, keeps "who owns what" cleanly separated, I'd rather keep it that way.

Typically in our current way of working it would be a property in the widget_info (annotations), 'supports_placeholders' = TRUE or something.
However, we're trying to *remove* stuff from there rather than add more. So I'd suggest controlling it by a boolean $supportsPlaceholders property in WidgetInterface.

yched’s picture

Or it stays a 'setting', but the supporting code is taken care of by each widget separately :-)

swentel’s picture

What is in 'settings' is the playground of each widget type

Hmm I agree that it might be annoying and even bad DX that a potential key in your settings is reserved. But I don't like the code duplication either. Hmm, hmmm, let me think about this. Food first, I need to eat something today as well :)

swentel’s picture

So I'd suggest controlling it by a boolean $supportsPlaceholders property in WidgetInterface.

(while eating) Not immediately a big fan of that. Some stuff then lives in annotations, other as properties. It's better DX in a way than to know all allowed properties in annotations, but it feels so schizofrenic to me. If we introduce properties, it then feels like we should remove 'default_value' as well, unless that's the plan :) (eating further)

yched’s picture

it then feels like we should remove 'default_value' as well, unless that's the plan :)

Yes, that's totally the plan :-) - that and 'multiple_values' (for which I opened #1846162: Cleanup Widgets API : add a separate base class for 'multiple' widgets already)

I'd personally be fine with just putting 'support_placeholders' in the annotations for now, and cleanup later along with the other two, but I fear that's going to make it more difficult to get the patch in :-(.

swentel’s picture

I'd personally be fine with just putting 'support_placeholders' in the annotations for now

(almost done eating). I'm almost thinking to let this live in settings but let every widget handle itself. This would drop (kind of ugly) following code:

+  // Add default placeholder element if exposed by the widget definition.
+  if (isset($instance['widget']['settings']['placeholder'])) {

This way, widgets are fully responsible like you also suggested. Also, we're in 'trouble' in case of the 'Link' field that prints 2 text fields in its widget.
This patch then isn't an (small) API change anymore and probably much easier to get in and no hassle afterwards to refactor or whatever :)

yched’s picture

I think I agree. Except for basic fields like text and number, other field types that want to have placeholders will most likely be more complex.

swentel’s picture

FileSize
11.13 KB

And there we go :)

Status: Needs review » Needs work

The last submitted patch, 1241938-36.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
660 bytes
11.15 KB

Fixing tests.

sun’s picture

Assigned: ericduran » Unassigned
Issue tags: -ericduran-todo

This looks great. I agree it makes more sense to rather get this functionality in now. We can think about potential ways to automate/harmonize it in a separate follow-up issue.

I'd RTBC this patch, but it looks like Link field is missing?

swentel’s picture

@sun two placeholder fields then for the title and the link ?

swentel’s picture

Also on link, screenshot attached as well :)

Screen Shot 2012-11-24 at 22.56.12.png

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

And I guess that Link field widget pretty much clarifies why an automated/harmonized solution for all widgets will be hard to achieve ;)

Awesome, let's get this in! :)

yched’s picture

Hm, sorry, something doesn't look right: for text, number, email ... the placeholder feature is implemented as widget settings (as it should), but for link it's implemented as (instance) settings of the field type ?

Also, field_ui's ManageFieldsTest doesn't look like the right place to test this, since it's a feature of individual widgets.
This being said, I don't see us adding separate tests for each and every widget... :-(

swentel’s picture

FileSize
4.58 KB
13.93 KB

Crap, you're right, I keep confusing those two so heavily. Attached. Gimme another hour with tests for every widget!

swentel’s picture

FileSize
8.25 KB
19.47 KB

That even didn't take an hour .. :)

sun’s picture

Looks even better! :) Thanks!

yched’s picture

Status: Reviewed & tested by the community » Needs work

Ok, last nitpicks :-)

link_field_widget_info() should provide default values for the settings :

'settings' => array(
  'placeholder_title' => ''
  'placeholder_url' => '',
),

and then the isset checks when using the settings are not needed :

'#default_value' => $settings['placeholder_url'],
+      '#description' => t('The placeholder attribute represents a short hint (a word or short phrase) intended to aid the user with data entry. A hint could be a sample value or a brief description of the expected format.'),

I don't think we need to mention the notion of 'attribute' in the UI. Proposal :"The placeholder is a short hint..." ?

swentel’s picture

Status: Needs work » Needs review
FileSize
7.47 KB
19.27 KB

Okidoki :)

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks :-)
RTBC when it comes back green !

andypost’s picture

+1 to RTBC
PS:
Suppose this setting should be a part of each text input like format for textarea. Let's discus this in follow-ups, probably we need a change on element level

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Wow, what a lovely little patch! :D

Committed and pushed to 8.x. Thanks!

ericduran’s picture

Wow, that was crazy fast. I blink and it was done! :)

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