WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 14 months ago

#19579 new enhancement

Make get_search_form() more filterable/extensible

Reported by: chipbennett Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.3
Component: Themes Keywords: has-patch needs-testing
Focuses: template Cc:

Description

I notice that a lot of the time when Themes include custom search form markup via searchform.php, they are making very minor changes to the default markup. (A common on is adding onfocus/onblur attributes to the input.) Unfortunately, I've also noticed that custom search form markup tends to be... forgotten, and ends up becoming more insecure over time.

So, in the interest of encouraging/facilitating use of the core markup, I propose making get_search_form() more extensible, similar to the extensibility of comment_form().

This patch replaces $echo = true with $args = array() as the parameter passed to get_search_form(), while retaining backward compatibility with $echo = true. Also, a (self-explanatory) search_form_defaults filter is added.

Also addresses #16538

Does not address #16541, which is potentially related.

Probably conflicts with #14851

Also related: #19321

Attachments (3)

wp-includes.general-template.get_search_form.diff (5.1 KB) - added by chipbennett 3 years ago.
Replacement get_search_form(), with updated phpDocs
wp-includes.general-template.get_search_form.rev2.diff (5.7 KB) - added by chipbennett 3 years ago.
Adds $templates array for locate_template(), to allow for searchform-$name.php. See #14851
wp-includes.general-template.get_search_form.rev3.diff (5.7 KB) - added by chipbennett 3 years ago.
Remove placeholder="search" from default atts array. Reverts changes related to #16538

Download all attachments as: .zip

Change History (15)

@chipbennett3 years ago

Replacement get_search_form(), with updated phpDocs

comment:1 follow-up: @toscho3 years ago

  • Cc info@… added

label and placeholder are different things. Imagine a screenreader user listening to: Search for: Search.

Enter one or more search terms would be slightly better; usually you just don’t need a placeholder.

There should be a space between the input elements.

Besides that, I think this is a very good enhancement.

@chipbennett3 years ago

Adds $templates array for locate_template(), to allow for searchform-$name.php. See #14851

comment:2 in reply to: ↑ 1 ; follow-up: @chipbennett3 years ago

Replying to toscho:

label and placeholder are different things. Imagine a screenreader user listening to: Search for: Search.

Enter one or more search terms would be slightly better; usually you just don’t need a placeholder.

I'm open to direction on this. Basically, I just incorporated #16538. What's the best approach?

There should be a space between the input elements.

Isn't there? I'm imploding the atts arrays with spaces before outputting them in the HTML markup.

comment:3 @GaryJ3 years ago

Could you use submit_button()?

How about adding an aria landmark role as a default? (And if that's added, then adding an ID to the label (input field ID + '-label') and a aria-labelledby attribute to the input field would also increase the accessibility potential.)

Is there not a existing WP function that can take an array of HTML attributes name => value and build the string for you?

comment:4 in reply to: ↑ 2 @GaryJ3 years ago

Replying to chipbennett:

Replying to toscho:

There should be a space between the input elements.

Isn't there? I'm imploding the atts arrays with spaces before outputting them in the HTML markup.

He's talking about the elements, not the attributes. I think I disagree though - visual whitespace can be added with CSS, and I'm not aware of any accessibility issues with them following right on from each other within the markup. Easier for developers to add some margin between the elements, than having to filter the output just to trim extra spaces that is being suggested to be added.

comment:5 @toscho3 years ago

Replying to chipbennett:

I'm open to direction on this. Basically, I just incorporated #16538. What's the best approach?

For a simple search field, the best approach is to leave the placeholder out. The UI has to work without placeholderanyway, because not all browsers support it. You cannot style the `placeholder` in all browsers – and risk bad contrasts.
placeholder may be useful for difficult forms (taxes etc.), but a clearly labeled search form doesn’t need it. The filter for additional attributes is good enough.

There should be a space between the input elements.

Isn't there? I'm imploding the atts arrays with spaces before outputting them in the HTML markup.

        $form .= '<label for="' . esc_attr( $args['id'] ) . '" ' . $label_atts . '>' . esc_attr( $args['label_text'] ) . '</label>'; 
        $form .= '<input name="' . esc_attr( $args['id'] ) . '" id="' . esc_attr( $args['id'] ) . '" ' . $input_atts . ' />'; 
        $form .= '<input id="' . esc_attr( $args['form_id'] ) . '" ' . $submit_atts . ' />'; 

Search for:SearchSearch :)

Last edited 3 years ago by toscho (previous) (diff)

comment:6 @toscho3 years ago

In my themes I use accept-charset='<?php bloginfo( 'charset' ); ?>' for the form just to raise the possibility to get UTF-8.

And aria-required='true' required for the search field may prevent accidental clicks on the submit button.

comment:7 @chipbennett3 years ago

How about adding an aria landmark role as a default? (And if that's added, then adding an ID to the label (input field ID + '-label') and a aria-labelledby attribute to the input field would also increase the accessibility potential.)

In my themes I use accept-charset='<?php bloginfo( 'charset' ); ?>' for the form just to raise the possibility to get UTF-8.

And aria-required='true' required for the search field may prevent accidental clicks on the submit button.

Since the $atts arrays are extensible/filterable, these can easily be added by the Theme developer. I've basically taken the form markup in the current version of get_search_form(), and replicated it here. The only difference is adding placeholder="search", which I now think should be removed, and left to Themes/Plugins to filter in.

He's talking about the elements, not the attributes. I think I disagree though - visual whitespace can be added with CSS, and I'm not aware of any accessibility issues with them following right on from each other within the markup. Easier for developers to add some margin between the elements, than having to filter the output just to trim extra spaces that is being suggested to be added.

Search for:SearchSearch :)

Unless I'm missing something, I agree with GaryJ that this is a presentational/CSS issue, that should be left up to Themes to determine how to implement.

Is there not a existing WP function that can take an array of HTML attributes name => value and build the string for you?

Is there? If you find one let me know. That would be ideal to use!

Could you use submit_button()?

I suppose I could - though I'd like some more core-dev feedback on that before adding it to the patch. I've only ever seen submit_button() used in the admin, not on the front end.

@chipbennett3 years ago

Remove placeholder="search" from default atts array. Reverts changes related to #16538

comment:8 @WraithKenny2 years ago

  • Cc Ken@… added

comment:9 @ramiy2 years ago

  • Cc r_a_m_i@… added

comment:10 @ramiy2 years ago

Related: #15081

comment:11 @WraithKenny2 years ago

Patch/plan should probably be updated with regards to the above related.

comment:12 @nacin14 months ago

  • Component changed from Template to Themes
  • Focuses template added
Note: See TracTickets for help on using tickets.