WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#34886 new enhancement

Search Form should not submit empty strings

Reported by: gibrown Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch
Focuses: Cc:

Description

Depending on the styling it is not always obvious to the user that there is a text box to click in before hitting the search button. On WordPress.com we are seeing about 3.5% of all site search queries are an empty string.

For the end user this costs them the extra page load, and the confusion of having the page loaded. By selecting the field instead we avoid the user changing pages, and needing to re-find the search box.

Similar problems are very prevalent in many themes. The most popular single word (that is not a stop word) in site search for WordPress.com is "search" due to the default value text being searched in themes. For most of the Twenty themes this problem is solved by not displaying the search button at all (display: none;). Fixing this in core should help point the way for fixing it in themes.

Twenty Ten does not have the search button hidden and so is a good theme for reproducing the problem.

Attachments (1)

34886.diff (2.3 KB) - added by gibrown 2 years ago.

Download all attachments as: .zip

Change History (9)

@gibrown
2 years ago

#1 @gibrown
2 years ago

The patch was also tested to verify that an existing onclick jQuery event that has been attached to the search button will still fire with:

<?php
wp_enqueue_script( 'jquery' );

function myscript() {
    ?>
    <script type="text/javascript">
        jQuery(document).ready(function () { jQuery("#searchsubmit").on( "click", function(){ alert("yes"); }); });
    </script>
    <?php
}
add_action( 'wp_footer', 'myscript', 1000 );

#2 follow-up: @westonruter
2 years ago

@gibrown Good idea.

I think, however, that jQuery should be used for the new functionality, and that the JS should be loaded from a JS file instead of inlined to PHP.

Additionally, I think that the submit event should be used instead, for example:

jQuery( function( $ ) {
        $( 'form.search-form' ).on( 'submit', function( e ) {
                var input = $( this ).find( 'input[name=s]' );
                if ( ! input.val() ) {
                        e.preventDefault();
                        input.focus();
                }
        } );
} );

Alternatively, this same functionality can be implemented in HTML5 simply by adding a required attribute to the s input element:

  • src/wp-includes/general-template.php

    diff --git src/wp-includes/general-template.php src/wp-includes/general-template.php
    index f554e1f..e65bd84 100644
    function get_search_form( $echo = true ) { 
    226226                        $form = '<form role="search" method="get" class="search-form" action="' . esc_url( home_url( '/' ) ) . '"> 
    227227                                <label> 
    228228                                        <span class="screen-reader-text">' . _x( 'Search for:', 'label' ) . '</span> 
    229                                         <input type="search" class="search-field" placeholder="' . esc_attr_x( 'Search &hellip;', 'placeholder' ) . '" value="' . get_search_query() . '" name="s" title="' . esc_attr_x( 'Search for:', 'label' ) . '" /> 
     229                                        <input type="search" class="search-field" required placeholder="' . esc_attr_x( 'Search &hellip;', 'placeholder' ) . '" value="' . get_search_query() . '" name="s" title="' . esc_attr_x( 'Search for:', 'label' ) . '" /> 
    230230                                </label> 
    231231                                <input type="submit" class="search-submit" value="'. esc_attr_x( 'Search', 'submit button' ) .'" /> 
    232232                        </form>'; 

#3 @westonruter
2 years ago

  • Component changed from Widgets to General

#4 @gibrown
2 years ago

Thanks @westonruter. Will rework html5 version to use "require" (thanks, wasn't aware of that), and the JS to use jQuery on submit.

the JS should be loaded from a JS file instead of inlined to PHP

Seems like a performance impact to potentially load another JS file for such a small number of lines of code. Is there an existing file you would suggest adding this to? I think I heard mentioned that the only place where js is loaded is separately on the front end is for comments.

#5 @westonruter
2 years ago

@gibrown good question on the separate JS file. Given HTTP/2 and “server push”, there will (eventually) be zero performance hit to enqueue additional scripts. Moreover, there are other small scripts that get enqueued with a typical request, e.g. on Twenty Fifteen:

  • twentyfifteen/js/skip-link-focus-fix.js
  • twentyfifteen/js/functions.js
  • wp-includes/js/wp-embed.js
  • wp-includes/js/comment-reply.js

I can also envision that eventually any such search-form.js could eventually include functionality for autocomplete and other search functionality.

Having a separate file allows it to be linted and QUnit-tested.

But I'll defer to others' ultimate recommendation on whether this should be in a separate JS file.

#6 @joehoyle
2 years ago

I think a whole new JS file is too heavy handed for this - I'd be +1 on adding the required attribute for those who support it.

#7 in reply to: ↑ 2 @peterwilsoncc
2 years ago

+1 on required, of recent browser it only misses the Safari family.

Replying to westonruter:

I think, however, that jQuery should be used for the new functionality, and that the JS should be loaded from a JS file instead of inlined to PHP.

If a JavaScript approach is taken, a feature check for querySelector and addEventListener would be preferable to introducing a jQuery requirement for the front end. For similar reasons as in comment:6, loading jQuery and jQuery migrate is too heavy handed.

Uglifying and inlining during the build has worked well to allow unit tests of the emoji and embed JS.

#8 @johnbillion
2 years ago

  • Component changed from General to Themes
  • Keywords has-patch added
  • Version trunk deleted

A separate JS file just for this is definitely overkill. It's handling an edge case.

Switching to the submit event on the search input is a good idea.

Note: See TracTickets for help on using tickets.