Fix UI of filter on Views list page

Add this fixe's:
a) Remove the description on the filter field.
b) Remove the field label (visually hide it)
c) Add placeholder text saying "Filter by view name"

As per https://www.drupal.org/node/2056089#comment-9372491, made changes, please review once.

Let's add some screenshot to see what is going on before and after.

Having some accessibility review would be also great here.

Changing as per #10.

I am making screenshots

I've done an accessibility testing using Mac's VoiceOver. I don't hear any announcement when I type in the filter but it's also like that before the patch is attached.. so I think it's not the patch's fault.

Below is the screenshot

Before

After

added beta evaluation and minor change, please correct me if I'm wrong.

Css changes for the #12

I don't think it's a good idea to solve the width problem on a specific css base here - the original elements width was defined through the size="30" attribute on the input, so maybe it would be better to tweak the size there.
Apart from that the introduced max-width is already applied from form.css styles.
In general - shouldn't this width issues discussed/solved one level higher and style all instances of the filters in the same way?

Thank you for working on this issue.

We should all try and use the same sprint tag. According to https://groups.drupal.org/node/447258 it should be SprintWeekend2015 with no #.

Looks great!

+++ b/core/modules/views_ui/src/ViewListBuilder.php
@@ -186,9 +186,10 @@ public function render() {
diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml

diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
old mode 100644
new mode 100755
diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php

diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
old mode 100644
new mode 100755

I don't think these changes should be in this patch?

Patch looks good now.

#23 has not yet been addressed.

deleted the change

#SprintWeekend2015
I realise that the patch doesn't apply. I'm going to reroll it

My first contribution/patch!
#SprintWeekend2015
Reroll done

Thanks everyone working on this!

Removed the Needs reroll tag.
Also added Needs screenshots tag. We should update the issue summary with screenshots *before* the patch and *after* the patch.
See tips for adding screenshots.

Reviewing this now for #SprintWeekend2015

Screenshot showing placeholder text, still needs accessibility review

Updated IS with screenshots.

Good idea with the screenshots, Patch: 2380117-32-reroll.patch works fine here..

@monobasic Thanks for testing it!

I guess this needs automatic testing now, then?

Never did one, but I am happy to try! Maybe you could give me some startup advice: What would be senseful to test in this case? Test for the proper markup and the text of the placeholder? And would be the test for views list case only?

I also tested the patch and it worked for me and does all three things listed in the original write-up.

@monobasic Thanks for volunteering!

What I would do is creating a ViewsListFormWebTest test class in the namespace Drupal\views_ui\Tests.

If you look at \Drupal\system\Tests\Form\ModulesListFormWebTest as an example you have a test that checks for some HTML in the modules administration page.

You should do something similar, but asserting that the filter textbox has the right size and texts. Just ensure that the views_ui module is enabled (see the $modules array) and that you are accessing with a user with the right permissions (see the drupalCreateUser call).

If you have any doubts don't hesitate to ask for help here or on IRC.

cheers, looking into that...

Commented in the beta evaluation :) Thanks for writing it.

I've added the test, please review!
Not sure if I get the xpath part right - it seems to me that the input filter field should be targeted specifically, the way it is right now, the assertions just checking for a field containing the attr and value, is that correct? Thanks guys!

@monobasic: Congratulations for your first test, thanks for contributing!

IMHO given xpath it is enough. If you can improve it with a more specific xpath query would be perfect, but not a requirement.
Some minor nitpicks:

1. +++ b/core/modules/views_ui/src/Tests/ViewsListFormWebTest.php
   @@ -0,0 +1,49 @@
   +  public static $modules = array('system_test', 'views_ui');
   ...
   +    \Drupal::state()->set('system_test.module_hidden', FALSE);
   

   system.test is not used, let's remove it from here. Then we can remove setUp() completely.

2. +++ b/core/modules/views_ui/src/Tests/ViewsListFormWebTest.php
   @@ -0,0 +1,49 @@
   +    $this->assertFieldByXPath("//input[contains(@title, 'Enter a part of the view name or description to filter by.')]");
   +    $this->assertFieldByXPath("//input[contains(@placeholder, 'Filter by view name or description')]");
   

   Those should have t() or they will fail if running tests in a translated version. t() is only used in testing when the strings inside it are part of the UI, but we still need it there.

Just a note: when rolling a patch for a bug with a test, it is really useful to upload two patches, one with only the test and one with the test and the fix. So the first one will fail, ensuring that we are fixing the right thing. See https://www.drupal.org/contributor-tasks/write-tests for concrete instructions.

Thanks again!

@penyaskito: Many thanks for your help, much appreciated :) I will include your suggestions and upload two separate patches..

done and added new patches - please review, thanks!

Perfect, thanks!!

I would mark this as RTBC, but we still need an accessibility review. I would try to get someone from the team here.

"I would" means that I shouldn't have changed status.

I can take a look at it from an accessibility perspective. I'm travelling home at the moment, but I'll take a look and update with another comment ASAP.

Nice!

The change looks good but...

+++ b/core/modules/views_ui/src/Tests/ViewsListFormWebTest.php
@@ -0,0 +1,41 @@
+    // Ensure expected markup and values for the search/filter inut field
+    $this->assertFieldByXPath('//input[contains(@class, "views-filter-text")]');
+    $this->assertFieldByXPath('//input[contains(@size, "40")]');
+    $this->assertFieldByXPath('//input[contains(@type, "search")]');
+    $this->assertFieldByXPath('//input[contains(@title, "' . t('Enter a part of the view name or description to filter by.') . '")]');
+    $this->assertFieldByXPath('//input[contains(@placeholder, "' . t('Filter by view name or description') . '")]');

Here we are just testing features of the form API. I think this test is not really testing anything. And therefore shouldn't exist.

Thanks for the review. The test was mentored and approved by @penyaskito, so I don't really know what to do here -> status back to unassigned

I have to agree with @alexpott, ... we don't have any logic which has to be tested here. Adding the test coverage would just make it more complex to change the labels etc. later, if wanted.

OK, so what's next? Using #32? I guessed that having that test would be better than nothing.

This is the complete patch in #49 with suggested removal of some assertions in #55.

RTBC per #54 and #57.

UI text is not yet frozen. Committed 7b64a0a and pushed to 8.0.x. Thanks!