While working on #1269734: Add form name class to forms for styling purposes I discovered that search-block-form is a strange case. Although search-block-form.tpl.php is a block template, the 'render element' in search_theme() is set to 'form'. This causes the markup to be generated by:

1. search_block_view() does a drupal_get_form() to get the form.
2. template_preprocess_search_block_form() creates a $variables['search_form'] which contains all of the rendered form children.
3. search-block-form.tpl.php prints $search_form, wrapped in the block wrappers.
4. Somewhere the <form ...> ... </form> is being generated and wrapped around the output of search-block-form.tpl.php

The problem is that the $form root element from the drupal_get_form() call in search_block_view() is never being rendered, and therefore not being rendered in a way consistent with all other forms in Core.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Everett Zufelt’s picture

Status: Active » Needs review
FileSize
3.05 KB

This should remove the search-block-form.tpl.php and render the block / form properly. The invisible title is missing now, so we need to address that.

Everett Zufelt’s picture

We should be able to do the hidden title with:

block['subject'] = t('Search');

And then by adding

$title_attributes['class'][] = 'element-invisible';

But, I'm not quite sure where to add the variable to the template, I know that $title_attributes is already defined in block.tpl.php

Everett Zufelt’s picture

This patch:

- Removes verything related to search-block-form.tpl.php
- Renders the search_block_form completely within the block
- Adds the 'subject t('Search form') to the block
- Hides the block title (subject) with element-invisible

Everett Zufelt’s picture

Title: Correct form rendering inconsistency with search_block_form » Remove search-block-form template for form rendering consistency
Issue tags: +html5
Jacine’s picture

I don't think we should hide the title by default. The UI would be too inconsistent. People would get confused when they override the subject and never see anything. Also the whole reason to do this is for consistency. Let's make the title "Search" and if themes want to hide it, let them worry about it.

We can make adjustments to the core themes where needed in this patch, like for when the block is placed in the header, if it's necessary. I know Bartik has code in place for this already:

function bartik_preprocess_block(&$variables) {
  // In the header region visually hide block titles.
  if ($variables['block']->region == 'header') {
    $variables['title_attributes_array']['class'][] = 'element-invisible';
  }
}
Everett Zufelt’s picture

@jacine

Thanks for the feedback, I've re-rolled. I was just copying the behavior of the current template. Actually, the current template hard codes t('Search form'). I definitely like your suggestion better.

andypost’s picture

What's about themes that directly prints search form into page.tpl?

Everett Zufelt’s picture

@andypost

I'm not quite sure I understand your question. If you are asking what happens to themes that print $drupal_render(drupal_get_form('search_block_form')) then.

1. They are not following proper conventions and it doesn't matter if they fail in D8.

2. There, as far as I know, should be no change. There is no change to the search_block_form in this patch, only changes to the search block that contains the form.

andypost’s picture

Status: Needs review » Needs work

@Everett Zufelt make sense, container-inline wrapper could be added in page|block template

+++ /dev/nullundefined
@@ -1,37 +0,0 @@
-?>
-<div class="container-inline">
-  <?php if (empty($variables['form']['#block']->subject)): ?>
-    <h2 class="element-invisible"><?php print t('Search form'); ?></h2>
-  <?php endif; ?>
-  <?php print $search_form; ?>
-</div>

I see the only purpose of this template to add the "inline" class

7 days to next Drupal core point release.

Everett Zufelt’s picture

@andypost

Are you suggesting that I add to the patch:

/**
 * Implements hook_preprocess_block().
 */
function search_preprocess_block(&$variables) {
  if ($variables['block']->module == 'search' && $variables['block']->delta == 'form') {
    $variables['classes_array'][] = 'container-inline';
  }
}
andypost’s picture

Sure because textfield and submit button should be placed on one line

Everett Zufelt’s picture

Status: Needs work » Needs review
FileSize
3.63 KB

Adds container-inline suggested in #9 to place search field and button inline.

Everett Zufelt’s picture

Moves .container-inline to content_attributes_array.

andypost’s picture

FileSize
3.98 KB

probably we need tests for this because content_attributes_array for "class" does not work for blocks (FF, Chrome tested) because of class alredy defined in template
<div class="content"<?php print $content_attributes; ?>>

So I changed search_box() probably we need check this with

if ($form_id' == 'search_block_form') {
  $form['#attributes']['class'] = 'container-inline';
}
 

Also added back "element-invisible" for block title

EDIT suppose we should file another issue about block.tpl.php about useless $content_attributes for default block template, I can't find similar, but this should be fixed and backported to D7

Jacine’s picture

Status: Needs review » Needs work

Ugh! Sorry, I forgot about the fact that class is not allowed in $content_attributes. That really bugs me to no end and I really, really hope we can fix this in D8. The form itself is a good candidate though.

I really don't think we need to make anything invisible here. IMO, we tend to abuse .element-invisible and are introducing little Easter eggs all over the place. In this case, if we make the title invisible we introduce a regression. Right now, you can change the title in the block edit UI and it will appear. If we do this, it wont. We don't do this for other blocks either, so it's inconsistent.

Everett Zufelt’s picture

Status: Needs work » Needs review

@andypost

1. I think that we should leave the block title visible by default, if themes want to hide it they can.
2. I like 'Search' better than 'Search form', but this is a personal preference.
3. Can you please explain why using $variables['content_attributes_array']['class'][] == 'container-inline'; isn't working? It is definitely generating the correct markup.

Everett Zufelt’s picture

Okay, I see now... class="content" is hard coded in the tpl. I'll open a followup, because that is all kinds of wrong!

Everett Zufelt’s picture

This patch:

- Removes the search-block-form template
- Adds class .container-inline to the form
- Adds subject (title) 'Search' to the block

Followup is that themes will need to hide / alter the title to make it go away / be invisible.

andypost’s picture

I've filed patch & test for block content class #1286532: Fix $content_attributes_array does not work for block default template
Feel free to rework if I wrong

andypost’s picture

+++ b/modules/search/search.moduleundefined
@@ -1007,6 +1004,7 @@ function search_form($form, &$form_state, $action = '', $keys = '', $module = NU
+  $form['#attributes']['class'] = 'container-inline';

Still not sure about setting this directly for all forms. Search uses this with hook_forms()

7 days to next Drupal core point release.

andypost’s picture

FileSize
1.86 KB
2.7 KB

Screens:
1) By default in bartik we have block title now visible
2) Assinging < none > form block title helps

Everett Zufelt’s picture

The patch in #13 is dependent on #1286532: Fix $content_attributes_array does not work for block default template and is what should be tested.

Everett Zufelt’s picture

Everett Zufelt’s picture

#1286532: Fix $content_attributes_array does not work for block default template has been committed, so patch #13 should be good to go now.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

#13 works fine - screens from #21 are valid

Everett Zufelt’s picture

Follow up should be for Bartik maintainer to decide if they want to hide the title, I agree with Jacine that it should not be hidden.

function bartik_preprocess_block(&$variables) {
  if ($variables['block']->module == 'search' && $variables['block']->delta == 'form') {
    $variables['title_attributes_array']['class'][] = 'element-invisible';
  }
}
Jeff Burnz’s picture

Looks all very good to me and brings back some consistency - if someone wants to remove/change the title in Bartik they can do so, hiding it by default feels quite wrong and inconsistent with core.

So yes, leave the title to show in Bartik, it will still be hidden in the header region for style purposes, but everywhere else we let it show. +1.

Jacine’s picture

Sweet! Thanks everyone. ;)

Also, just want to note for future reference that this patch got emmajane and the TNT crew's blessing as well in IRC.

catch’s picture

Status: Reviewed & tested by the community » Needs work

It looks like #13 is the right patch to commit here, but please upload that so it's the last patch on this issue. Needs work in the sense that someone needs to attach the patch ;)

Everett Zufelt’s picture

Status: Needs work » Needs review
FileSize
3.64 KB

Attaching patch from #13.

Everett Zufelt’s picture

Status: Needs review » Reviewed & tested by the community

Setting back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Really lovely to see a hook_theme() and template get replaced by a three line preprocess. Committed to 8.x.

Everett Zufelt’s picture

Status: Fixed » Active

Curious if we should document that we just removed a template.

jhodgdon’s picture

Status: Active » Needs work
Issue tags: +Needs change record

Someone needs to create a change notice that describes this change.

Also, there needs to be a minor patch addition: the function search_box() 's docblock still links to this removed theme template file, and that link will now be broken:
http://api.drupal.org/api/drupal/modules--search--search.module/function...

(I guess the API site hasn't updated to the new code yet).

Everett Zufelt’s picture

@jhodgdon

Can you please let me know where I got to learn about / create change notifications?

jhodgdon’s picture

Sure! The only place they're really described that I know of is in the "documentation gate" section of the Drupal 8 "Gates" page. The description is in the last row of the table in the Documentation section.

Basically, they are a new content type on d.o. To add a new one:
http://drupal.org/node/add/changenotice

To see a list of the existing change notices for Drupal core:
http://drupal.org/list-changes/drupal

Everett Zufelt’s picture

Status: Needs work » Fixed
Issue tags: -Needs change record

Added change notification http://drupal.org/node/1294082

jhodgdon’s picture

Status: Fixed » Needs work
Issue tags: +Novice

I think we also still need a small doc patch, since there is still one mention of search-block-form.tpl.php in search.module, and that has been removed.

This follow-up patch would be a good Novice project.

Everett Zufelt’s picture

oops, forgot that part, and agree re: #34:

Also, there needs to be a minor patch addition: the function search_box() 's docblock still links to this removed theme template file, and that link will
now be broken:
http://api.drupal.org/api/drupal/modules--search--search.module/function...

(I guess the API site hasn't updated to the new code yet).

Cliff’s picture

Status: Needs work » Needs review
FileSize
455 bytes

Adding patch to achieve #38.

Everett Zufelt’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Simple patch to remove the reference to search-block-form.tpl.php from function search_box().

catch’s picture

Status: Reviewed & tested by the community » Fixed

Well spotted. Committed and pushed.

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