I have troubles theming the form or better to say, it's not themed like every other core form. Please fix it so it's themed like every other form in the system. There are more issues than only the label.

Examples:

  • theme_form_element() is not used. Custom and standard core classes are missing.
  • theme_form_element_label() is not used. Custom and standard core classes are missing.

Views exposed form:

<form class="mycustom-form-class" action="foo" method="get" id="views-exposed-form-foo" accept-charset="UTF-8">
<div>
  <div class="views-exposed-form">
    <div class="views-exposed-widgets clearfix">
      <div id="edit-postal-code-wrapper" class="views-exposed-widget views-widget-filter-field_address_postal_code">
        <label for="edit-postal-code">Zip code</label>
        <div class="views-widget">
          <div class="form-item form-type-textfield form-item-postal-code mycustom-text-class">
            <input type="text" id="edit-postal-code" name="postal_code" value="" size="30" maxlength="128" class="form-text">
          </div>
        </div>
      </div>
      <div class="views-exposed-widget views-submit-button">
        <input class="mycustom-button-class form-submit" type="submit" id="edit-submit-foo" name="" value="Filter">
      </div>
    </div>
  </div>
</div>
</form>

How it should looks like (normal form code):

<form class="contact-form mycustom-form-class" action="/user/1/contact" method="post" id="contact-personal-form" accept-charset="UTF-8">
<div>
  <div class="form-item form-type-textfield form-item-name mycustom-text-class">
    <label class="mycustom-form-label-class" for="edit-name">Your name</label>
    <input type="text" id="edit-name" name="name" value="Foo" size="60" maxlength="255" class="form-text">
  </div>
...

Labels are normally inside the same DIV like the input. This causes several style issues that can be avoided. Aside of this my custom form theming takes no action as core form API functions are not used, too.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hass’s picture

Title: Exposed form is not using form API functions » Exposed form is not build using form API functions
hass’s picture

Issue tags: +VDC
hass’s picture

views-exposed-form.tpl.php is not like core

dawehner’s picture

Issue tags: -Needs backport to D7

We will not change the output of exposed forms in d7, that's like jumping in cage full of lions.

xjm’s picture

Title: Exposed form is not build using form API functions » Build the exposed form using form API functions
Project: Views (for Drupal 7) » Drupal core
Version: 8.x-3.x-dev » 8.x-dev
Component: Code » views.module
Category: bug » task

Bugs are things that don't work.

mikeker’s picture

Status: Active » Needs review
FileSize
5.97 KB

Here's a first stab at undoing Views' custom theming of exposed filters.

Note: One thing that Views used to do was collect all the single checkbox filters and put the into one widget. The patch removes that as core doesn't do that elsewhere that I'm aware of. And now that exposed forms are using FAPI, the same could be accomplished by hook_form_alter().

Status: Needs review » Needs work

The last submitted patch, exposed-form-fapi-1812048-6.patch, failed testing.

mikeker’s picture

Patch was hit with a Twig... Reroll coming soon.

mikeker’s picture

Rerolled and Twig-ified.

mikeker’s picture

Status: Needs work » Needs review

Knew I forgot to do something...

dawehner’s picture

Issue tags: +Needs manual testing

Just a short review.

+++ b/core/modules/views/views.theme.incundefined
@@ -1049,52 +1049,17 @@ function template_preprocess_views_view_row_rss(&$vars) {
+  // Include basic theming for exposed forms.
+  @$form['#attached']['css'][] = drupal_get_path('module', 'views') . '/css/views.exposed_form.css';

We should use a library for that.

mikeker’s picture

Thanks for the review dawehner.

We should use a library for that.

I was following the pattern in core\modules\views\lib\Drupal\views\ViewExecutable.php @445 where views.base.css is added. I figured adding the CSS only for views with exposed forms would reduce the CSS loaded for other Views-generated pages.

There's really not much in the new CSS file -- it could probably be combined with the existing views.base.css -- but it may grow.

dawehner’s picture

Well, never trust existing code :) There is no reason to not put stuff into a library and use it like that.

mikeker’s picture

Well, never trust existing code :)

Ha! Fair point...

OK, I've added #2005616: Views should use ['#attached']['library'] rather than ['#attached']['css'] to make the change from #attached to drupal_add_library(). Figured since that was outside the scope of this issue, it warranted a new issue. If we can commit this patch with the #attached call, then the patch in #2005616: Views should use ['#attached']['library'] rather than ['#attached']['css'] will apply and can be reviewed and (hopefully) committed.

Pancho’s picture

Rerolled after views.base.css is now views.module.css.
Also rolling in the switch to using views.exposed_form.css as a library which was removed between #2005616-10: Views should use ['#attached']['library'] rather than ['#attached']['css'] and #2005616-13: Views should use ['#attached']['library'] rather than ['#attached']['css'].

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -VDC

The last submitted patch, exposed-form-fapi-1812048-15.patch, failed testing.

Pancho’s picture

Status: Needs work » Needs review

"The test did not complete due to a fatal error," but why should LocaleTranslationTest fail on this patch?

#15: exposed-form-fapi-1812048-15.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, exposed-form-fapi-1812048-15.patch, failed testing.

Pancho’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +VDC

"Setup environment: failed to clear checkout directory," so one more time retesting.

#15: exposed-form-fapi-1812048-15.patch queued for re-testing.

mikeker’s picture

@Pancho, Thanks for the rerolls.

FYI: the views.exposed-form library bit was removed from #2005616: Views should use ['#attached']['library'] rather than ['#attached']['css'] so that it would apply and pass the testbot. Now that that issue is committed, we can add views.exposed-form as a library (as you did in the reroll) and everyone (including the testbot) is happy.

Pancho’s picture

I know - that's why I rolled it in... :)
The testbot is quite erratic these days, but as long as it doesn't produce false positives, everything's fine...

dawehner’s picture

What should we do with the chexkbox logic?

mikeker’s picture

What should we do with the checkbox logic?

Are there any other places in core that combine several checkboxes into a single form element? I'm not aware of any, which makes me think removing the old checkbox logic makes this more consistent.

Also, now that we're using FAPI, couldn't that be done with hook_form_alter if needed for a given project?

mikeker’s picture

A few more tweaks:

  • Added LTR tags in the CSS file
  • Removed out-of-date docs from the template file
  • Moved clearfix to the exposed form div so pages like admin/content didn't have the VBO label misaligned
hass’s picture

Status: Needs review » Needs work

"LRT" typo... and what about the rtl file? :-)

mikeker’s picture

Status: Needs work » Needs review

Thanks for the feedback. LRT... Gotta love copy/paste errors!

I've added the rtl file and added some PHPDoc @file comments to be the beginning of both.

mikeker’s picture

Add the patch...

hass’s picture

I'm not 100% sure, but I think you need to set margin-right in RTL to 0 for .views-exposed-form .views-exposed-widgets.

tim.plunkett’s picture

Issue tags: +Needs screenshots

We now have Views in core by default with exposed filters, on admin/content and admin/people.
Can we see screenshots of them with this patch?

For even more real-world testing, screenshots with #2020167: Add a name and email search field to the admin/people view and #2001922: Improve admin/content view would be even better.

mikeker’s picture

@#28: the margin-left: 0 is correct. It's setting the margin between the exposed form widgets, which are floated right, and the Apply/Submit button.

@#29: Will do.

hass’s picture

Left is correct, but right may be wrong as is not reset.

mikeker’s picture

Left is correct, but right may be wrong

That's my quote of the month! :)

Now I understand your point -- I'll investigate what the default is for rtl displays and get back to you. But I have zero experience in rtl so if someone else looking at this issue knows the answer, please chime in!

mikeker’s picture

OK, some screenshots. Finally...

Latest patch in the this issue as well as #2020167-19: Add a name and email search field to the admin/people view

1812048-27-with-2020167-19.png

Compared to #2020167-19: Add a name and email search field to the admin/people view only

2020167-16.png

Compared to D7

1812048-admin-people-d7.png

hass’s picture

Try this in RTL file, please.

.views-exposed-form .views-exposed-widgets {
  float: right;
  margin-left: 1em;
  margin-right: 0;
}
mikeker’s picture

As suggested by @hass in @#34. Thanks, hass!

dawehner’s picture

Isn't that issue about moving exposed forms to FAPI and nothing else? I actually don't think that we want to have a different styling for every exposed form.

hass’s picture

He only kept the current style as I know. We should change style later if required. The main reason for opening the case was that I need the same html structure like core and to make sure css classes injected via preprocess are passed to views forms, too. Otherwise my forms are not styled the same way everywhere on the site.

dawehner’s picture

Yes hass, that is what I thought as well. But doesn't #1812048-33: Build the exposed form using form API functions changes the styles of exposed forms?

mikeker’s picture

Status: Needs review » Needs work

#38: Yes, it does. I changed the styling to make exposed forms act more like FAPI forms (see the D7 vs. D8-with-patch screenshots in #33).

Though you're probably right -- that would make the scope of this issue much larger as it'll be affecting all the newly ported-to-Views admin pages. Yikes...

I'll reroll without the styling changes and get some before/after screenshots together.

mikeker’s picture

OK, reroll keeping the styling as similar as possible. @hass, please check the RTL file to make sure I've done that correctly. Thanks!

Attached are before/after screenshots of the admin/people and admin/content pages. For whatever reason I'm unable to embed those shots in this comment... But I can include img tags! :)

admin/people before:

admin/people after:

admin/content before:

admin/content after:

Status: Needs review » Needs work
Issue tags: -Needs screenshots, -Needs manual testing, -VDC

The last submitted patch, 1812048-40-exposed_form_fapi.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs screenshots, +Needs manual testing, +VDC

#40: 1812048-40-exposed_form_fapi.patch queued for re-testing.

dawehner’s picture

+++ b/core/modules/views/views.moduleundefined
@@ -1349,11 +1356,12 @@ function views_exposed_form($form, &$form_state) {
-    '#value' => t('Apply'),
+    '#value' => t('Filter'),

Let's not change the name of the label ... as this would affect people when upgrading etc.

mikeker’s picture

I was wondering about that... However on the pages with VBO fields it would lead to two "Apply" buttons. Also the D7 version of those pages used "Filter". Finally the text can be overridden in the exposed form settings.

Thoughts?

dawehner’s picture

It is out of scope of this issue sorry, so having this not in will help to get it in.

mikeker’s picture

dawehner’s picture

FileSize
7.27 KB
1.34 KB
+++ b/core/modules/views/views.moduleundefined
@@ -1349,7 +1356,8 @@ function views_exposed_form($form, &$form_state) {
+  $form['actions'] = array('#type' => 'actions');
+  $form['actions']['submit'] = array(

Even that is out of scope, sorry. Actions might make sense in admin forms, but do they make sense on the normal page? Let's discuss that in a follow up.

This also brings back the checkbox handling.

Status: Needs review » Needs work
Issue tags: -Needs screenshots, -Needs manual testing, -VDC

The last submitted patch, vdc-1812048-47.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +Needs screenshots, +Needs manual testing, +VDC

#47: vdc-1812048-47.patch queued for re-testing.

mikeker’s picture

+++ b/core/modules/views/views.moduleundefined
@@ -1349,7 +1356,8 @@ function views_exposed_form($form, &$form_state) {
+  $form['actions'] = array('#type' => 'actions');
+  $form['actions']['submit'] = array(

#47: I'm happy to push that into a followup issue, but I should point out that it's the same form code as the VBO "Apply" button which makes the buttons style similarly (cleared left, flush to the left margin). Otherwise it'll be floated left beside (to the right of) the other exposed form elements.

I suppose the larger question is: are we aiming for consistency with D7's exposed forms or D8's FAPI forms? I guess I was aiming to make D8 exposed forms look like other D8 FAPI forms, but I can see the reasoning for keeping them more like D7 forms.

This also brings back the checkbox handling.

I can't find any single checkbox exposed form elements. Am I missing them, or is that code leftover an earlier era when Views did checkboxes? If this is old code, can we leave it out?

dawehner’s picture

I can't find any single checkbox exposed form elements. Am I missing them, or is that code leftover an earlier era when Views did checkboxes? If this is old code, can we leave it out?

During the time I had several use cases for that, but well we could just drop it now and people should just put some theming if they really want to.

Actions seems to be interconnected with normal edit forms.

mikeker’s picture

re: single checkbox handling: my opinion is that we should limit the custom form editing in Views as much as possible -- that keeps exposed form consistent with other FAPI forms.

If we don't, I promise a bunch of "why doesn't this work like other forms?" bugs being filed in the year after D8 releases! :)

Gotta dash -- will come back to comment on actions later...

dawehner’s picture

You convinced me 100%! Go with actions and remove the custom checkbox handling.

mikeker’s picture

Apologies for the delay -- I had too many relatives in town for the holidays and am just now recovering...

Glad I could convince you! :)

Attached is basically #46 against the most recent 8.x head. Removing "Needs screenshots" as those are in #40 with the only difference being the text on the button changing from Filter to Apply.

dawehner’s picture

Status: Needs review » Needs work

This is not a problem, though that

--- /dev/null
+++ b/core/modules/views/css/views.exposed_form-rtl.cssundefined

+++ b/core/modules/views/css/views.exposed_form-rtl.cssundefined
@@ -0,0 +1,11 @@

@@ -0,0 +1,11 @@
+/**
+ * @file
+ * Right-to-Left styling for Views exposed forms.
+ */
+.views-exposed-form .form-item {
+  float: right;
+  margin-left: .25em;
+}
+.views-exposed-form .form-actions {
+  clear: right;
+}

@@ -0,0 +1,12 @@
+/**
+ * @file
+ * Styling for Views exposed forms.
+ */
+.views-exposed-form .form-item {
+  /* Display exposed form elements horizontally. */
+  float: left; /* LTR */
+  margin-right: .25em; /* LTR */
+}
+.views-exposed-form .form-actions {
+  clear: left; /* LTR */

As far as I know there are new css styles, which does RTL and normal styling in the same file.

hass’s picture

mikeker’s picture

Status: Needs work » Needs review
FileSize
1002 bytes
7 KB

I'm learning something new every day in this issue! :)

Attached patch rolls the RTL into the exposed form CSS as per #56. Thanks for the link, hass.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you very much.

tim.plunkett’s picture

Issue tags: -Needs manual testing +blocker
FileSize
7.04 KB
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Manually tested all looking good...

Committed 535ed41 and pushed to 8.x. Thanks!

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