WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 14 months ago

Last modified 11 months ago

#15081 closed enhancement (fixed)

Search Form should use type='search'

Reported by: jorbin Owned by: SergeyBiryukov
Milestone: 3.6 Priority: normal
Severity: normal Version:
Component: Accessibility Keywords: has-patch twentythirteen commit needs-codex
Focuses: Cc:

Description

same reasons as #15080

Attachments (5)

a11y.patch (630 bytes) - added by jorbin 4 years ago.
15081.diff (1.3 KB) - added by georgestephanis 14 months ago.
15081.2.diff (1.7 KB) - added by georgestephanis 14 months ago.
15081.3.diff (1.8 KB) - added by lancewillett 14 months ago.
15081.4.diff (2.2 KB) - added by SergeyBiryukov 14 months ago.
Refreshed after [23666]

Download all attachments as: .zip

Change History (37)

jorbin4 years ago

comment:1 c_t_montgomery3 years ago

  • Severity changed from normal to minor

I checked out the codebase, and still saw an

<input type="text"....

instead of an

<input type="search"....

This is my first attempt at contributing via Trac, so I apologize if I did this incorrectly, but I updated the svn and attempted to re-download the codebase. I still had my changed, correct version. Hopefully it works! Regardless, when I just checked it out, it was not fixed yet and I did (I think). Sorry for the n00bness... I'm trying!

comment:2 duck_3 years ago

Replying to c_t_montgomery:

I checked out the codebase, and still saw an

<input type="text"....

instead of an

<input type="search"....

This is my first attempt at contributing via Trac, so I apologize if I did this incorrectly, but I updated the svn and attempted to re-download the codebase. I still had my changed, correct version. Hopefully it works! Regardless, when I just checked it out, it was not fixed yet and I did (I think). Sorry for the n00bness... I'm trying!

When providing fixes on Trac you should attach patch/diff files. On this ticket jorbin has already done so, see attachment:ticket:15081:a11y.patch

The codex has a subversion guide which will provide some help (see westi's windows toolbox if you're using Windows and TortoiseSVN).

comment:3 GamajoTech3 years ago

  • Cc gary@… added

comment:4 nacin3 years ago

  • Milestone changed from 3.1 to Future Release

Have an idea for add_theme_support( 'html5' ) come a future release to tick on various bits like this.

comment:5 nacin3 years ago

  • Type changed from defect (bug) to enhancement

comment:6 rkunboxed17 months ago

Applied patch and it works as expected. This should be flagged for Twenty Thirteen like related ticket #15080.

comment:7 lancewillett14 months ago

  • Milestone changed from Future Release to 3.6
  • Priority changed from lowest to normal

Nacin brought up in IRC today a better plan (see the log):

Basic premise is to avoid the add_theme_support() this time around by adding in new arguments to functions like wp_list_comments().

Based on that argument being true it should switch and output HTML5 markup.

This technique will allow back compat because any theme calling without the argument will see no change at all.

comment:8 lancewillett14 months ago

  • Keywords twentythirteen added

georgestephanis14 months ago

comment:9 georgestephanis14 months ago

This is irrelevant to twentythirteen, as twentythirteen manually specifies type="search" in searchform.php

However, 15081.diff attached adding the toggle into get_search_form()

comment:10 lancewillett14 months ago

This is very relevant to Twenty Thirteen -- which is why we need to work on it now.

After this lands we can remove a bunch of code from Twenty Thirteen such as the custom search form, and custom comment callbacks.

comment:11 follow-up: lancewillett14 months ago

Patch is a great start. A few suggestions:

If 'html5' is the format:

  1. add placeholder element to search input, with something like <?php echo esc_attr_x( 'Search &hellip;', 'placeholder' ); ?> as the text

General improvements:

  1. Add two class values: form: "searchform", submit button input: "submit"
  2. Use better gettext for translators in the label: <?php _ex( 'Search', 'assistive text' ); ?>
  3. Better gettext in the submit button text: <?php echo esc_attr_x( 'Search', 'submit button' ); ?>
  4. Does the submit button element need name="submit" also?

comment:12 follow-up: georgestephanis14 months ago

Sounds reasonable.

Submit button does not need a name. If it did, that'd be an extra arg passed in the query string, and it's much cleaner without. :)

comment:13 in reply to: ↑ 11 ; follow-up: GaryJ14 months ago

Replying to lancewillett:

Patch is a great start. A few suggestions:

If 'html5' is the format:

  1. add placeholder element to search input, with something like <?php echo esc_attr_x( 'Search &hellip;', 'placeholder' ); ?> as the text

Please avoid using named entity references - polyglot documents MUST use character references instead, and SHOULD use hexadecimal references where possible.

<?php echo esc_attr_x( 'Search &#x2026;', 'placeholder' ); ?>

If necessary, add a comment if you don't like magic numbers (not much worse than magic abbreviations) without an explanation:

<?php echo esc_attr_x( 'Search &#x2026;', 'placeholder' ); // horizontal ellipsis ?>

comment:14 in reply to: ↑ 13 lancewillett14 months ago

Replying to GaryJ:

Please avoid using named entity references - polyglot documents MUST use character references instead, and SHOULD use hexadecimal references where possible.

Hmm, this contradicts other decisions in core WordPress recently where named entities were determined to be the best approach.

#9030 #14224 #14225 #16049

comment:15 in reply to: ↑ 12 lancewillett14 months ago

Replying to georgestephanis:

Sounds reasonable.

Submit button does not need a name. If it did, that'd be an extra arg passed in the query string, and it's much cleaner without. :)

George, are you going to update the patch? If not I can take it from here.

georgestephanis14 months ago

comment:16 georgestephanis14 months ago

Patch revised and uploaded. I did use 'label' for it rather than 'assistive text' -- as it's structurally in a label, and many themes may likely use it as a label, not merely assistive text.

comment:17 obenland14 months ago

When nacin gave me feedback on my patch for #20088 he pointed out I should use 'html5' === $format rather than 'html5' == strtolower( $format ).

Could we also add a class to the search field?

comment:18 georgestephanis14 months ago

There's been a lot of requests for extra classes ... but I guess my question is why do we need them? Anything there, we should be able to select as-is with CSS and JS as-needed.

comment:19 follow-up: obenland14 months ago

I just noticed all other elements got one :)

Seriously though, of course you can select it with #searchform input {}, but classes are a lot easier to override than IDs. It doesn't hurt do add them and could go a long way for theme and plugin developers.

comment:20 in reply to: ↑ 19 ; follow-up: lancewillett14 months ago

Replying to obenland:

I just noticed all other elements got one :)

Heh. I'd be OK with dropping the class on input and submit elements, and in Twenty Thirteen use type selectors with the form class instead. Simpler, doesn't rely on IDs.

comment:21 in reply to: ↑ 20 ; follow-up: obenland14 months ago

Replying to lancewillett:

I'd be OK with dropping the class on input and submit elements, and in Twenty Thirteen use type selectors with the form class instead. Simpler, doesn't rely on IDs.

Sounds fair. IF we can use .search-form instead of .searchform please.

comment:22 lancewillett14 months ago

  • Severity changed from minor to normal

Added #23701 for removing the search template from Twenty Thirteen once this core changes gets committed.

comment:23 in reply to: ↑ 21 lancewillett14 months ago

Replying to obenland:

Replying to lancewillett:

I'd be OK with dropping the class on input and submit elements, and in Twenty Thirteen use type selectors with the form class instead. Simpler, doesn't rely on IDs.

Sounds fair. IF we can use .search-form instead of .searchform please.

As discussed in IRC I think even though the WP CSS naming standards recommend using hyphens between words, this is an exception, for the following reason: the ID is already there, and it's more user-friendly for themers to use the same value for both class and ID.

I was wrong about older themes using the class already, though it's in Twenty Thirteen now. Twenty Eleven uses the ID selector only.

lancewillett14 months ago

comment:24 lancewillett14 months ago

.3 patch:

  • Update the comment block
  • Align assigments
  • Use the same logical structure noted by obenland above
  • Add only one class value, for the form element
  • Indent the form HTML a bit better

comment:25 lancewillett14 months ago

  • Keywords commit added

SergeyBiryukov14 months ago

Refreshed after [23666]

comment:26 SergeyBiryukov14 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 23667:

Add $format argument to get_search_form() to allow HTML5 markup. props georgestephanis, lancewillett. fixes #15081.

comment:27 ramiy13 months ago

Guys, you missed the discussion on #14851 regarding to get_search_form() function. In my opinion the xhtml/html5 should be in the "searchform.php" theme file or in "wp-includes/theme-compat/searchform.php".

Using the path in #14851 theme authors can make several forms, like "searchform-xhtml.php" and "searchform-html5.php".

comment:29 SergeyBiryukov13 months ago

In 23798:

Use a filter instead of recently added $format argument in get_search_form(). props WraithKenny. fixes #23850. see #15081.

comment:30 follow-up: nacin13 months ago

I like the switch to a filter here. Otherwise, I would have suggested turning $echo = true into $args = array().

I do wonder if this could turn into a theme support call, at some point.

comment:31 DrewAPicture11 months ago

  • Keywords needs-codex added

New 'search_form_format' filter added to get_search_form() in [23798]

comment:32 in reply to: ↑ 30 SergeyBiryukov11 months ago

Replying to nacin:

I do wonder if this could turn into a theme support call, at some point.

Follow-up: ticket:23850:19

Note: See TracTickets for help on using tickets.