Improve the markup/CSS for content and user filter forms

@Jacine: This patch does what you suggest. Testing it now.

Here's a sample of what happens to the fieldset once those styles are removed. If I'm reading you OP correctly, you are saying that the responsibility of rendering these form elements properly should fall onto the theme's shoulders. Do I have that right?

For Seven there is also a lot of css in style.css that effects these definition lists. Should that stay?

Kill the #theme functions, to start with.

Second, #type = container around the list of enabled filters and around the form to enhance the filter.

Third, #field_prefix is now allowed on all form elements (or is my patch still in the RTBC queue?), so the select list labels _could_ be output with that. OTOH, looking at that form, I see a pure two-col form layout. Theming 101 :P

Due to the container of "second", the form actions can float around the filters form items.

Using Firebug: I was able to test out your idea. I'll post the code an a snippet of how it looks below:

<fieldset id="edit-filters" class="container-inline">
<legend><span class="fieldset-legend">Show only items where</span></legend>
  <div class="fieldset-wrapper">
    <ul class="clearfix"></ul>
    <dl class="multiselect">
      <dd>
        <div class="form-item form-type-select form-item-status">
          <label for="edit-status-2">status </label>
          <select id="edit-status-2" class="form-select" name="status">
            <option selected="selected" value="[any]">any</option>
            <option value="status-1">published</option>
            <option value="status-0">not published</option>
            <option value="promote-1">promoted</option>
            <option value="promote-0">not promoted</option>
            <option value="sticky-1">sticky</option>
            <option value="sticky-0">not sticky</option>
          </select>
        </div>
        <div class="form-item form-type-select form-item-type">
          <label for="edit-type">type </label>
          <select id="edit-type" class="form-select" name="type">
            <option selected="selected" value="[any]">any</option>
            <option value="article">Article</option>
            <option value="page">Basic page</option>
            <option value="poll">Poll</option>
          </select>
        </div>
      </dd>
    </dl>
    <div id="node-admin-buttons" class="form-actions container-inline form-wrapper">
      <input type="submit" class="form-submit" value="Filter" id="edit-submit" name="op">
    </div>
  </div>
</fieldset>

Starting point.

1) Same needs to be applied to User admin filter form.

2) Search for .multiselect across core and nuke/adapt everything.

3) Filter form actions may need minimal margin tweaking to be horizontally in line with first select list. Ideally, we apply an additional class to the container + apply some special CSS here, so we do not duplicate the same PHP/CSS code...

subscribe

Is anyone able to continue here? I'd be happy to assist. However, I've already mentioned in IRC that I have little interest in this patch myself (aside from the very nice clean-ups), because I'm using the admin_views module that ships with admin_menu, which replaces those pages entirely with dynamic, ajaxified Views + VBO.

+++ modules/system/system-behavior.css	4 Mar 2010 21:29:30 -0000
@@ -182,22 +182,15 @@ tr .ajax-progress .throbber {
+ * Multi-filter form.

1) Rename to "Two column form layout".

2) These styles are totally unrelated to JavaScript behaviors. If there is a system.css, let's move the new styles in there.

Powered by Dreditor.

drupal_render_children() is your friend. If you are stuck, the best recommendation I can give is simply to lookup similar stuff in Drupal core. (That's how I learned about drupal_render_children() myself ;) Probably not the best example, but: http://api.drupal.org/api/function/theme_filter_admin_format_filter_order/7

Code review. Didn't apply the patch and look at the actual output yet.

+++ includes/form.inc	6 Mar 2010 22:24:23 -0000
@@ -3133,6 +3133,38 @@ function theme_form_element_label($varia
+ * Theme an exposed filter form.
+*

Wrong indentation.

+++ includes/form.inc	6 Mar 2010 22:24:23 -0000
RCS file: modules/node/node-rtl.css
diff -N modules/node/node-rtl.css

+++ modules/system/system.css	6 Mar 2010 22:24:25 -0000
--- modules/user/user-rtl.css	3 Jan 2010 21:01:04 -0000	1.7
+++ modules/user/user-rtl.css	6 Mar 2010 22:24:26 -0000

We have to remember to put back corresponding RTL styles into system-rtl.css.

+++ modules/node/node.admin.inc	6 Mar 2010 22:24:24 -0000
@@ -150,8 +150,13 @@ function node_filter_form() {
+    '#attributes' => array('class' => array('form-layout-twocol clearfix')),

@@ -177,9 +190,17 @@ function node_filter_form() {
+    '#attributes' => array('class' => array('form-layout-twocol', 'clearfix')),

+++ modules/user/user.admin.inc	6 Mar 2010 22:24:26 -0000
@@ -40,7 +40,11 @@ function user_filter_form() {
+    '#attributes' => array('class' => array('form-layout-twocol clearfix')),

@@ -55,16 +59,30 @@ function user_filter_form() {
+    '#attributes' => array('class' => array('form-layout-twocol', 'clearfix')),

We can remove those .form-layout-twocol classes with this approach.

+++ modules/node/node.admin.inc	6 Mar 2010 22:24:24 -0000
@@ -177,9 +190,17 @@ function node_filter_form() {
+    '#prefix' => ($i ? '<div class="additional-filters"><em>' . t('and') . '</em> ' . t('where') . '</div>' : ''),

+++ modules/user/user.admin.inc	6 Mar 2010 22:24:26 -0000
@@ -55,16 +59,30 @@ function user_filter_form() {
+    '#prefix' => ($i ? '<div class="additional-filters"><em>' . t('and') . '</em> ' . t('where') . '</div>' : ''),

1) Not sure whether this doesn't rather belong into the theme function? Entirely not sure though. Perhaps not.

2) t() should contain the entire phrase - inline markup is allowed. Otherwise, translators have no chance to translate these words for this context. (The DIV doesn't count as inline markup of course)

+++ modules/system/system-behavior.css	6 Mar 2010 22:24:25 -0000
--- modules/system/system.css	18 Jan 2010 17:29:28 -0000	1.73
+++ modules/system/system.css	6 Mar 2010 22:24:25 -0000

btw, do we also have an admin.css? This stuff only appears on administrative pages, so actually admin.css would make the most sense... (if I'm not mistaken, then system.css is loaded unconditionally for all pages)

Powered by Dreditor.

+++ modules/node/node.admin.inc	6 Mar 2010 23:28:49 -0000
@@ -150,8 +150,13 @@ function node_filter_form() {
+    '#theme' => 'exposed_filters',

+++ modules/user/user.admin.inc	6 Mar 2010 23:28:51 -0000
@@ -40,7 +40,11 @@ function user_filter_form() {
+    '#theme' => 'exposed_filters',

Thank god we talked about theme overrides just before... this should be

'#theme' => 'exposed_filters__node',

See http://api.drupal.org/api/function/theme/7 for details.

+++ includes/form.inc	6 Mar 2010 23:28:47 -0000
@@ -3133,6 +3133,37 @@ function theme_form_element_label($varia
+ * @param $variables
+ *   An associative array containing an exposed filter form.
+ *   @todo - Get help to do this right.

Actually, it's a fragment of a form, i.e. a partial form structure. Not sure how that is described elsewhere, but yeah, let's look elsewhere ;)

+++ includes/form.inc	6 Mar 2010 23:28:47 -0000
@@ -3133,6 +3133,37 @@ function theme_form_element_label($varia
+ * @return
+ *   A string containing an HTML-formatted form.

I don't think that @return is required here.

+++ includes/form.inc	6 Mar 2010 23:28:47 -0000
@@ -3133,6 +3133,37 @@ function theme_form_element_label($varia
+  $items = array();
+  if (!empty($form['current'])) {
+    foreach (element_children($form['current']) as $key) {

This !empty() will always be TRUE due to form_builder(). We can simply remove it, element_children() will take care for the rest.

+++ modules/system/admin.css	6 Mar 2010 23:28:50 -0000
@@ -135,3 +135,45 @@ table.screenshot {
+
+

Duplicate newline.

+++ modules/system/admin.css	6 Mar 2010 23:28:50 -0000
@@ -135,3 +135,45 @@ table.screenshot {
\ No newline at end of file

This should not happen :)

144 critical left. Go review some!

Helping you out a bit.

Tested on my browsers...
RTBC

There isn't enough space between the dropdown boxes after patch (see screens in #26)

EDIT:

role, permission, ... are bold after patch

@jacine: I had my second training session in telepathy last week. Glad that your thoughts crossed my mind.

@jacine: Just enable Locale module, add the "English" language one more time, edit that new language, and set it to RTL -- and you'll see the "switched" output.

Everthing looks good now, and in #32 works this is rtbc (leave this to jacine).
In the IE screen you see that some buttons are way to close to a drop down box.
But that was also before the patch, so that's an other issue!

Forgot screenshots...

Can someone test with a RTL language setting (see #32 for how to) and mark this RTBC?

This one is rtbc I tested it. Same screenshots as #34 but in mirror view.

I found an rtl error but I don't think it's related.
Do I have to open new issue? (screenshot)

1) TBH, all of those text formattings always confused me. No idea why "and" is always emphasized, the property too + bold on top, and the value only bold... ?

Can we remove those formattings and just use %-placeholders in the t()-strings? %-placeholders are triggering http://api.drupal.org/api/function/drupal_placeholder/7, so we could style them differently, if needed. Most probably, it would make sense to remove the italics and just make 'em bold.

2) The list bullets in the "after" shots do not look like Garland's list bullets (as in the "before" shots). Intentional? Not sure whether that's a good idea.

+++ modules/node/node.admin.inc	22 Mar 2010 06:52:55 -0000
@@ -177,9 +188,17 @@ function node_filter_form() {
+    '#prefix' => ($i ? '<div class="additional-filters">' . t('<em>and</em> where') . '</div>' : ''),

Still an EM here.

+++ modules/node/node.test	22 Mar 2010 06:52:55 -0000
@@ -1140,7 +1140,10 @@ class NodeAdminTestCase extends DrupalWe
+    $this->assertRaw(t('<strong>%property</strong> is', array('%property' => t('status'))), t('Content list is filtered by status.'));

@@ -1150,8 +1153,13 @@ class NodeAdminTestCase extends DrupalWe
+    $this->assertRaw(t('<strong>%property</strong> is', array('%property' => t('status'))), t('Content list is filtered by status.'));
...
+    $this->assertRaw(t('<em>and</em> where <strong>%property</strong> is', array('%property' => t('type'))), t('Content list is filtered by content type.'));

The test assertions need to be updated. ;)

+++ modules/system/admin.css	22 Mar 2010 06:52:56 -0000
@@ -116,6 +116,7 @@ table.screenshot {
+ * @group

Can we remove that? (no consensus reached yet)

+++ modules/system/admin.css	22 Mar 2010 06:52:56 -0000
@@ -135,3 +136,53 @@ table.screenshot {
+/**
+* Exposed filters
+*/

Wrong indentation.

+++ modules/system/admin.css	22 Mar 2010 06:52:56 -0000
@@ -135,3 +136,53 @@ table.screenshot {
+.exposed-filters .form-item,
+.exposed-filters .form-actions,
+.exposed-filters .form-item label {
+  margin: 0;
+  padding: 0;
+  overflow: hidden;
+}
+.exposed-filters .form-item {
+  margin-bottom: 0.1em;
+}

I'm not a fan of wildcard overrides like this + subsequent re-overrides of the previous overrides. In themes, we can do that, but I'd prefer to avoid it in default/module CSS.

+++ modules/system/admin.css	22 Mar 2010 06:52:56 -0000
@@ -135,3 +136,53 @@ table.screenshot {
+.exposed-filters .filters {
+  float: left; /* LTR */
+  width: 22em;
+  overflow: hidden;

Why does this need a width? Same question for overflow?

+++ modules/system/admin.css	22 Mar 2010 06:52:56 -0000
@@ -135,3 +136,53 @@ table.screenshot {
+/* Current Filters */
+.exposed-filters .item-list {
+  margin-bottom: 1em;
+}
+.exposed-filters li {
+  margin-left: 0; /* LTR */
+}
+.exposed-filters li .form-item,
+.exposed-filters li .form-item label {

TBH, this looks a bit awkward. Can't we target the current filters a bit more precisely?

(is this also the reason for the overrides mentioned earlier?)

+++ modules/system/system.css	22 Mar 2010 06:52:56 -0000
@@ -113,7 +113,6 @@ div.ok, tr.ok {
 .item-list ul li {
   margin: 0 0 0.25em 1.5em; /* LTR */
   padding: 0;
-  list-style: disc;
 }

Actually, I'd prefer to remove this styling altogether. Stark can rely on browser default styling, and for every theme out in the wild, styles like this get horribly in the way. We can do this change here, but should definitely create a follow-up to fix system.css.

+++ modules/system/system.module	22 Mar 2010 06:52:56 -0000
@@ -3629,6 +3629,35 @@ function theme_system_settings_form($var
+function theme_exposed_filters($variables) {
...
+  foreach (element_children($form['current']) as $key) {
+    $items[] = drupal_render($form['current'][$key]);
+  }
+  if ($items) {
+    $output .= theme('item_list', array('items' => $items, 'attributes' => array('class' => 'clearfix')));

+++ modules/user/user.admin.inc	22 Mar 2010 06:52:56 -0000
@@ -40,7 +40,11 @@ function user_filter_form() {
+  $form['filters']['current'] = array(
+    '#type' => 'container',
+    '#attributes' => array('class' => array('clearfix')),
   );

aha! We don't apply a class for the current filters yet.

140 critical left. Go review some!

This one should come back green.

I've additionally streamlined the markup and CSS a bit. Feel free to yell at me. ;)

So far this looks like an improvement. Testing in Safari we see that /admin/content's dropdowns have a clear seperation. When the dropdowns are selected a very visible highlight of the field is present. However, /admin/reports/dblog needs a better seperation of the two multi-select boxes (Type and Severity). There is no visible gap between them (see: selection_017.png)

That reports filter form is not touched by this patch, sorry. :)

EDIT: And shouldn't/couldn't, as it's using a different structure/workflow.

RTBC, anyone?

Yay, that almost sounds like yelling :)

.clearfix is used to clear the float that is applied to align the buttons next to the additional filters.

If we want to separate the additional filters text prefix, then it should simply be added as new, first subkey:

  $form['filters']['status']['additional'] = array(
    '#type' => 'container',
    '#attributes' => array('class' => array('additional-filters')),
  );
  $form['filters']['status']['additional']['prefix'] = array(
    '#markup' => t('and where'),
  );

I'm not sure if this is something this patch should be concerned with but I'm just going to post it here anyways. In IE7, on admin/content, the buttons seem inconsistent in their design. one button is flush with the right side of a select box the other is not. See screenshots.

Testing the rest...

This issue seems to work well for Safari (on windows). Good improvement.

I've tested firefox 3.6, Opera 10.51, IE 6, 7, and 8, and Safari 4. The only issue I found for the area of the page that Capture2.png shows is with IE 6. See the screenshot I attached.

There doesn't seem to be a major issue with IE6, perhaps the area that these fields need can be widened for IE6.

patch doesn't apply anymore. Here's the content of the .rej file:

***************
*** 1005,1042 ****
    drupal_set_message(t('The role has been deleted.'));
    $form_state['redirect'] = 'admin/people/permissions/roles';
  }
-
- /**
-  * Theme user administration filter selector.
-  *
-  * @ingroup themeable
-  */
- function theme_user_filters($variables) {
-   $form = $variables['form'];
-
-   $output = '<ul class="clearfix">';
-   if (!empty($form['current'])) {
-     foreach (element_children($form['current']) as $key) {
-       $output .= '<li>' . drupal_render($form['current'][$key]) . '</li>';
-     }
-   }
-   $output .= '</ul>';
-
-   $output .= '<div class="clearfix">';
-
-   $output .= '<dl class="multiselect">' . (!empty($form['current']) ? '<dt><em>' . t('and') . '</em> ' . t('where') . '</dt>' : '');
-
-   $output .= '<dd>';
-
-   foreach (element_children($form['status']) as $key) {
-     $output .= drupal_render($form['status'][$key]);
-   }
-   $output .= '</dd>';
-
-   $output .= '</dl>';
-   $output .= drupal_render($form['actions']);
-
-   $output .= '</div>';
-
-   return $output;
- }
--- 1012,1014 ----
    drupal_set_message(t('The role has been deleted.'));
    $form_state['redirect'] = 'admin/people/permissions/roles';
  }

I tried to reroll this patch myself but it's those last two lines that have me confused. The theme_user_filters function is at the end of the page. It seems like in this patch it isn't and i'm not sure if I simply need to remove the the theme_user_filters function or if there is more that needs to be done.

#55: 732914-55.patch queued for re-testing.

Oh dear, I thought this was in already.

Re-rolled against HEAD. (only, no other changes)

This patch makes my brain explode from too much awesome.

This works beautifully. Especially nice that the width of the labels is now increased to 10em (was only 6em before) which solves problems for translated installs where 6em was sometimes to narrow.

Tested in Safari (Mac), FF (Mac, Windows), IE7

The width of 22em for .exposed-filters .filters in admin.css is too small since .exposed-filters .form-item label has 10em and .exposed-filters .form-select has 14em. We need 25em for .exposed-filters .filters since .exposed-filters .form-select also has 2px padding.

Otherwise select boxes are positioned below the label.

Patch attached.

Thanks!

This is an awesome clean-up. Removes more lines than it adds, and improves the situation while doing so. I think this is ready to fly. If there are any remaining issues, we can deal with them in follow-ups.

Just saw that this patch is still sitting in the queue. Still ready to go in, doesn't break anything, but allows modules (if they want) to remove lots of duplicate code.

Just for reviewers two screenshots showing the filter select controls on admin/content as an example. Note the added margins between the selext controls in after.png.

This even works in IE6.

The patch is a simple reroll aginst current HEAD since the patch from #69 didn't apply cleanly anymore.

No changes in code against #69 at all, just the reroll since the patch in #69 didn't apply cleanly anymore against today's HEAD.

This is a great patch and one that really should go through - I just found an odd situation when Locale module is enabled and the front end theme and admin theme are the same theme - locale loads some CSS that restricts the width of the form-item to just 15em, so it wraps. If we add a width: auto; to .exposed-filters .form-item we can negate this ever happening.

.exposed-filters .form-item {
  margin: 0 0 0.1em;
  padding: 0;
  width: auto;
}

I can't reproduce the problem from #78.

I tried with Bartik and Garland both enabled as front end and admin theme at the same time. Locale module is enabled. Seems that in my install (fresh checkout from today) locale.css doesn't get loaded for these pages like /admin/content at all.

Did you test this with the latest HEAD? If so, can you describe more exactly the steps you took to arrive at this situation?

From what i can tell locale.css is only meant to load on admin/config/regional/translate/translate but I don't really have the time to dig into Locale and figure out why or when this loads - from what i can tell I have English as my default language and Arabic and Swedish installed as well. Apart from that I can't tell you much more. I always run patches against latest head - don't you ;)

EDIT - yes Jacine, I would say its a bug also, I can't see anywhere or why this CSS would load ever for the Content Pages.

Just out of interest are you guys using Overlay? I wasn't, but when I turned on the Overlay the issue went away.

OK, I posted as issue against Locale #914194: locale.css loaded and applied on other pages and elements - but I still think we should add width:auto; as this seems pretty fragile without it.

Committed to CVS HEAD. Thanks.

Patch in #90 didn't apply any more as core moved to core/ folder.

I have recreated the patch. Just needs testing.

93: 732914-93.patch queued for re-testing.

93: 732914-93.patch queued for re-testing.

Reroll..

The patch is just CSS so the errors don't make sense to me.

The theme function for Exposed filters has been removed in Drupal 8 in favor of Views exposed filters in #2087239: Remove theme_exposed_filters(). The CSS is no longer in use since it has been abstracted into new class .form--inline in #2333719: Abstract Views Exposed Form styling out into a reusable class, so this issue is only relevant for D7