Add support for HTML5 placeholder in views exposed filters

Thanks for the patch! I've applied it to a fresh install of Drupal 8.4 for testing.

What worked

• Placeholder attribute added to Content: Title (exposed)
• Placeholder attribute added to Content: Body (exposed)

What seemed weird

The placeholder option is included even if the filter element don't support it. For example, Content: Published (exposed) is a select element. In such cases, I would expect the option to be hidden.

Alternate solution for select menus

This could get very complex very quickly, but I could see the placeholder being used to add the placeholder text as a null-value default option to the select menu. That might cause unexpected behavior if someone tries to submit the form with that option selected, though.

EDIT: The solution described below (only show the option for relevant field types) is cleaner.

@robin.ingelbrecht, good idea!

It's probably best not to make other fields more complex than they have to be.

Pretty nice idea. Moving to 8.5.x because this is a feature.

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -124,6 +124,7 @@ protected function defineOptions() {
+        'placeholder' => ['default' => ''],

As the failing tests show, this needs a place in the filter config schema, which in turn would need an upgrade path and test.

I would lean toward the approach in #5. As we've already established, placeholder text only applies to particular form elements. As far as I can see, there's no way for us to generically apply it at the FilterPluginBase level. It depends on the widget provided by each individual filter plugin, and only that plugin is going to know for sure where and how a placeholder should be applied.

To take an example out of core, if you're using a NumericFilter with the BETWEEN operator, the value form contains both a "Min" and a "Max" field, not just a singular "Value" field. You may well want a placeholder for both of those fields, but FilterPluginBase is going to have no idea how to accommodate that — only NumericFilter does.

And yes, that means that any contributed module's filter derived from FilterPluginBase directly (like SearchApiFullText) will not magically inherit placeholders. Not ideal, but I'm not sure its avoidable.

I'll work on a revised patch that follows the approach of #5.

@kevin.dutra sounds good! I'll be happy to test a new patch that follows that method.

@jkovell, awesome! Here's the first stab at it.

Oops, missed a spot when refactoring.

Thanks, testing now.

While I do, are the descriptions in these fields reversed?

+    $form['expose']['min_placeholder'] = [
+      '#type' => 'textfield',
+      '#default_value' => $this->options['expose']['min_placeholder'],
+      '#title' => $this->t('Min placeholder'),
+      '#size' => 40,
+      '#description' => $this->t('Ghost text that appears inside the Max field.'),
+    ];
+    $form['expose']['max_placeholder'] = [
+      '#type' => 'textfield',
+      '#default_value' => $this->options['expose']['max_placeholder'],
+      '#title' => $this->t('Max placeholder'),
+      '#size' => 40,
+      '#description' => $this->t('Ghost text that appears inside the Min field.'),
+    ];

Oh geez, yep. Good eye!

Fixed descriptions.

So far everything is working great.

I've tried the following fields:

• Text (plain) with single & unlimited values
• Text (formatted) with single & unlimited values
• Number (integer) with single & unlimited values

I've also tested setting default values in exposed views fields - also works!

I had a pedantic UI complaint, but it's part of an existing problem:

• Create a filter using a number (integer) field.
• Check "Expose this filter to visitors"
• Set operator to "Is equal to"
• Check "Expose operator"

There are no fields to enter placeholder text for min/max values unless you save the filter and reopen it - at which point all 3 placeholder fields are available as expected.

You could temporarily set the operator to "between" to set the values there, then switch it back to "Is Equal to"... but that feels like a work around.

As I mentioned above, I realized this is related to an existing issue... the default value fields are wonky in the same way. I'm not sure we need to solve it here? I'll hold off on an answer to that before marking as RTBC, but looks very good!

Looking good.

Couple of things we need to address I feel:

1. +++ b/core/modules/views/config/schema/views.filter.schema.yml
   @@ -73,10 +73,31 @@ views.filter.string:
   +        placeholder:
   +          type: string
   +          label: 'Placeholder'
   ...
   +views.filter.numeric:
   +  type: views_filter
   +  label: 'Numeric'
   +  mapping:
   +    expose:
   +      type: mapping
   +      label: 'Exposed'
   +      mapping:
   +        min_placeholder:
   +          type: string
   +          label: 'Min placeholder'
   +        max_placeholder:
   +          type: string
   +          label: 'Max placeholder'
   +        placeholder:
   +          type: string
   +          label: 'Placeholder'
   

   We need an upgrade path for this (plus a test for that upgrade path)

2. +++ b/core/modules/views/src/Plugin/views/filter/NumericFilter.php
   @@ -26,9 +26,70 @@ class NumericFilter extends FilterPluginBase {
   +      '#description' => $this->t('Ghost text that appears inside the Min field.'),
   ...
   +      '#description' => $this->t('Ghost text that appears inside the Max field.'),
   

   Why 'Ghost text', why not just call it 'placeholder text'?

Also, this needs test for the placeholder functionality and the setting in the UI.

@jkovell: I see what you mean. I'll rework that a bit so that all placeholders are immediately available to edit when you expose the operator. That's relatively easy to fix.

@Lendude:

1. From what I gather, a schema change like this would be picked up after a cache rebuild, so were you thinking something along the lines of what was done in #2623568: Config schema of argument_default plugins is incorrect where an empty hook is used to enforce the cache rebuild? Or is my understanding off and something more substantial is needed? :)
2. Mostly I wasn't sure how well the term "placeholder" was known with average end users. Developers are likely to be familiar with the W3C specs and so they'll know what it is, but for Average Joe, I wasn't as sure. For that reason, I wanted to describe it using some slightly different wording so that I wasn't creating kind of a circular definition. "Ghost text" is terminology I've heard some average end users use in the past, but I'd be perfectly happy to call it something else.

And yes, test coverage is needed and was next on my list -- just didn't want to dive into immediately in case there were some design changes that cropped up immediately. Hopefully I'll have some time tomorrow to add in some coverage.

@kevin.dutra the upgrade path needs something more elaborate, more like #2810097: Allow views to provide the canonical entity URL of all entities, not just nodes.

As a term, maybe 'HTML placeholder text'? If you google that anybody should be able to figure out what that does.

Since the Field's title already uses the technical term "Placeholder", can the description just define it? Something like "Hint text that appears inside the field when empty."

That said, I think "HTML placeholder text" is better than "Ghost text" since it's more Google-able. My only concern was that the "HTML" might make it sound like we can use HTML in that field.

Working on the remaining items.

Alrighty, here are the updates:

1. Added update hook to populate missing configs.
2. Added test for said update hook.
3. Added test for placeholder text functionality.
4. Adjusted the description for the placeholder fields according to @jkovell's suggestion. (I can see how the mention of "HTML" might misdirect. Again, still happy to massage that text further.)
5. Fixed that JS issue that prevented all the placeholders from being immediately editable once you expose the operator.
6. Slight tweaks to how the placeholder is actually added to the input element to make sure we don't render the placeholder attribute at all if it's not being used.

Tested item #4 and #5 - working as expected. Thanks!

This looks great, some minor nits:

1. +++ b/core/modules/views/src/Plugin/views/filter/StringFilter.php
   @@ -27,11 +27,34 @@ class StringFilter extends FilterPluginBase {
   +    $options['expose']['contains']['placeholder'] = ['default' => ''];
   ...
   +    $this->options['expose']['placeholder'] = NULL;
   

   default once to '' and once to NULL, shouldn't they both be ''?

2. +++ b/core/modules/views/tests/src/Functional/Handler/FilterPlaceholderTextTest.php
   @@ -0,0 +1,63 @@
   +  /**
   +   * Views used by this test.
   +   *
   +   * @var array
   +   */
   

   can just be {@inheritdoc}

3. +++ b/core/modules/views/tests/src/Functional/Handler/FilterPlaceholderTextTest.php
   @@ -0,0 +1,63 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   

   can just be {@inheritdoc}

4. +++ b/core/modules/views/tests/src/Functional/Handler/FilterPlaceholderTextTest.php
   @@ -0,0 +1,63 @@
   +  /**
   +   * Runs other test methods.
   +   */
   

   Needs an update.

Thanks for another review pass @Lendude!

1. Good question. I was using the description option from FilterPluginBase as a basis. There NULL is used for defaultExposeOptions() whereas an empty string is used for defineOptions().
   My guess is that there would be no functional difference if it was empty string instead of NULL, but since there was precedent,
   I figured it was safer to just follow along in the off-chance there was actually a reason it was done that way.
2. Can you point out where the doc would be inheriting from? I see $modules farther up the chain, but I didn't spot a definition for $testViews that it could inherit from.
3. Fixed
4. D'oh! I usually forget to verify method doc blocks before the final sendoff. Oh well, better late than never. Fixed.

1. Fair enough, I can't tell why you'd need to set them in both at all, but you are right it happens for some other settings too, lets leave it
2. Hah! you are right, I could have sworn it was on ViewTestBase, but you are right, it's not (though it probably should be).

I think this is ready.

Unrelated fail, back to RTBC

Testbot is smoking something...everything is still green.

Another unrelated fail. I'll open a DrupalCI issue.

@kevin.dutra a critical already exist for this fail #2926309: Random fail due to APCu not being able to allocate memory

Updating review credits, adding @jwkovell and @Lendude

Committed b022734 and pushed to 8.5.x

A bit late, but can we get a change notice here to herald the new feature.

For the posthumous change record.

Added a CR: https://www.drupal.org/node/2935063

Thanks @Lendude, published the CR

Found a problem with the upgrade path, see #2936457: views_post_update_filter_placeholder_text() can result in errors in filter plugins due to incomplete definition