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.
Comment | File | Size | Author |
---|---|---|---|
#40 | search.patch | 455 bytes | Cliff |
#30 | remove-search-block-form-tpl_4.patch | 3.64 KB | Everett Zufelt |
#21 | search-title.jpg | 2.7 KB | andypost |
#21 | search-no-title.jpg | 1.86 KB | andypost |
#18 | remove-search-block-form-tpl_5.patch | 3.64 KB | Everett Zufelt |
Comments
Comment #1
Everett Zufelt CreditAttribution: Everett Zufelt commentedThis 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.
Comment #2
Everett Zufelt CreditAttribution: Everett Zufelt commentedWe 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
Comment #3
Everett Zufelt CreditAttribution: Everett Zufelt commentedThis 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
Comment #4
Everett Zufelt CreditAttribution: Everett Zufelt commentedComment #5
JacineI 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:
Comment #6
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #7
andypostWhat's about themes that directly prints search form into page.tpl?
Comment #8
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #9
andypost@Everett Zufelt make sense,
container-inline
wrapper could be added in page|block templateI see the only purpose of this template to add the "inline" class
7 days to next Drupal core point release.
Comment #10
Everett Zufelt CreditAttribution: Everett Zufelt commented@andypost
Are you suggesting that I add to the patch:
Comment #11
andypostSure because textfield and submit button should be placed on one line
Comment #12
Everett Zufelt CreditAttribution: Everett Zufelt commentedAdds container-inline suggested in #9 to place search field and button inline.
Comment #13
Everett Zufelt CreditAttribution: Everett Zufelt commentedMoves .container-inline to content_attributes_array.
Comment #14
andypostprobably 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
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
Comment #15
JacineUgh! 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.
Comment #16
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #17
Everett Zufelt CreditAttribution: Everett Zufelt commentedOkay, I see now... class="content" is hard coded in the tpl. I'll open a followup, because that is all kinds of wrong!
Comment #18
Everett Zufelt CreditAttribution: Everett Zufelt commentedThis 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.
Comment #19
andypostI'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
Comment #20
andypostStill not sure about setting this directly for all forms. Search uses this with hook_forms()
7 days to next Drupal core point release.
Comment #21
andypostScreens:
1) By default in bartik we have block title now visible
2) Assinging < none > form block title helps
Comment #22
Everett Zufelt CreditAttribution: Everett Zufelt commentedThe patch in #13 is dependent on #1286532: Fix $content_attributes_array does not work for block default template and is what should be tested.
Comment #23
Everett Zufelt CreditAttribution: Everett Zufelt commented#13: remove-search-block-form-tpl_4.patch queued for re-testing.
Comment #24
Everett Zufelt CreditAttribution: Everett Zufelt commented#1286532: Fix $content_attributes_array does not work for block default template has been committed, so patch #13 should be good to go now.
Comment #25
andypost#13 works fine - screens from #21 are valid
Comment #26
Everett Zufelt CreditAttribution: Everett Zufelt commentedFollow 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.
Comment #27
Jeff Burnz CreditAttribution: Jeff Burnz commentedLooks 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.
Comment #28
JacineSweet! 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.
Comment #29
catchIt 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 ;)
Comment #30
Everett Zufelt CreditAttribution: Everett Zufelt commentedAttaching patch from #13.
Comment #31
Everett Zufelt CreditAttribution: Everett Zufelt commentedSetting back to RTBC.
Comment #32
catchReally lovely to see a hook_theme() and template get replaced by a three line preprocess. Committed to 8.x.
Comment #33
Everett Zufelt CreditAttribution: Everett Zufelt commentedCurious if we should document that we just removed a template.
Comment #34
jhodgdonSomeone 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).
Comment #35
Everett Zufelt CreditAttribution: Everett Zufelt commented@jhodgdon
Can you please let me know where I got to learn about / create change notifications?
Comment #36
jhodgdonSure! 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
Comment #37
Everett Zufelt CreditAttribution: Everett Zufelt commentedAdded change notification http://drupal.org/node/1294082
Comment #38
jhodgdonI 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.
Comment #39
Everett Zufelt CreditAttribution: Everett Zufelt commentedoops, forgot that part, and agree re: #34:
Comment #40
Cliff CreditAttribution: Cliff commentedAdding patch to achieve #38.
Comment #41
Everett Zufelt CreditAttribution: Everett Zufelt commentedLooks good. Simple patch to remove the reference to search-block-form.tpl.php from function search_box().
Comment #42
catchWell spotted. Committed and pushed.