Duplicate search widgets cause ID conflicts

[solarissmoke]

It's possible to put multiple Search widgets into a template. Problem is each one uses the same IDs for the form and input elements, which messes up validation/Javascript.

Possible options:

• Remove the id's from the search form
• Add functionality to detect if an instance already exists and bump the IDs with a suffix accordingly.

[kawauso]

The widget is just a wrapper for get_search_form(), which loads searchform.php if present or falls back to default code if not. The ID conflict could happen as easily with an inline search form and a search widget instance.

Related: #16541

[solarissmoke]

Looks like removing the IDs is the only surefire option

[kawauso]

Related: #16551

[ryanimel]

I'll get the ball rolling here. I swapped the IDs in the <form> and submit <input>, though the text <input> requires the unique ID for the <label>'s "for='s'" to work and select the correct input when clicked. Swapping the IDs for classes solves the problem for those first two elements, but doesn't for the third.

We'll need an incremental number added to the text <input> and the <label> to make that work right. My patch doesn't include that. I did make the tweaks to the Twenty Eleven theme as well (searchform.php and style).

[kurtpayne]

Building on Ryan's work above, I added the incremental counter so all search forms and child elements have a unique id. I added class names to the search input and submit fields. Tested with Twenty Eleven theme.

[SergeyBiryukov]

Replying to kurtpayne:

Building on Ryan's work above, I added the incremental counter

$search_form_counter could probably be a static variable rather than global.

[azaozz]

Replying to kurtpayne:

The problem here is: what happens to all the themes after changing or removing the ID? We can fix Twenty Ten and Twenty Eleven, but what about all the rest? Should we break a huge number of sites whose themes use that ID for styling?

A possible workaround would be to remove the ID from the second, third, etc. search form.

[ryanimel]

Replying to azaozz:

One other option would be to add incremental numbers to the second, third, etc. forms. So the first form would be searchform, the following searchform-02, searchform-03, etc. It won't break current styling using the searchform ID, but will allow for each additional form to have a unique ID as well.

[nacin]

The first one should probably have #searchform. That one and all subsequent ones should probably have .searchform (with all subsequent ones thus having no ID). Not sure if incremental IDs are necessary. That said, if we were to do it, it should be done the way ryanimel writes.

[kurtpayne]

Thanks for all the feedback on this.

16539_searchform.2.2.diff​ should meet all the criteria mentioned above. Didn't tick the "overwrite existing" so that's why there's a dupe, sorry about that.

This patch has a static function variable as @SergeyBiryukov suggested. It also has the first form with an id of "searchform" as @azaozz suggested (with text input and submit ids of "s" and "searchsubmit" respectively) and subsequent forms, texts, and submits using and id with a counter as @ryanimel suggests.

It also has a class of searchform as @nacin suggested. The form and submit ids are possible to remove, but the label and and text ids are linked via the label, so the id counter is still necessary. For example: <label for="s-0"><input id="s-0" />.

[dd32]

Just a quick suggestion to reduce the code:

$form_id = 'searchform';
...
if ( !$search_form_counter ) {
   $form_id .= '-' . $search_form_counter;
   ...
}

so that's why there's a dupe

Not sure if there's any differences between the 2, however, you shouldn't ever need to use the overwrite function, only use that when there's a need to replace the patch (ie. You accidentally included wp-config in it), we like to see the various stages in the patches (and have a reference for the comments to a original patch)

[azaozz]

I'm with @nacin on this. We need to preserve all IDs in the first widget but don't think IDs are needed in the second, third, etc. widgets. We are trying to move away from HTML IDs and replace them with classes as much as possible.

Having said that, we will need the ID on the search text field so we are able to attach the label (as @kurtpayne notes above), but should skip all the rest.

[kurtpayne]

Replying to dd32:

Just a quick suggestion to reduce the code

Thank you. I've integrated this into the latest patch.

@azaozz, The 16539_searchform.3.diff​ should do what you and @nacin have described. There will be an id on the form, text, and submit elements on the first form. On subsequent forms, there will only be an id on the text input.

[azaozz]

Replying to kurtpayne:

Thanks for the patch, I've cleaned it a bit and removed the duplicate selectors for the (old) IDs and (new) classes. Don't think they are needed.

I'm still a bit worried this may break some sites, so testing with as many existing themes as possible is in order.

[kurtpayne]

Replying to azaozz:

I'm still a bit worried this may break some sites, so testing with as many existing themes as possible is in order.

I've picked the newest themes and the most popular themes currently listed on ​wordpress.org along with twentyeleven and twentyten and taken a screenshot of each theme before and after 16539-4.patch​. I used ​PerceptualDiff to show the differences between each set of images (if there was any difference).

This testing was done in FF6 on the front page with the ​theme unit test content and does not account for any :hover or :active pseudo classes.

There weren't any visual differences related to the search fields. You can download the screen shots and diffs ​here. The differences were related to randomized text / images, animated headers, or footer text (e.g. query count).

I don't currently have any theme site subscriptions for testing paid themes. Does anybody have any specific theme requests for this type of testing?

[TechDagan]

I think it's important to keep screen readers in mind here. Having a form field not properly linked to a label element is a violation of WCAG 2 guidelines. The obvious solution is for compliant sites to have only one search form, but I think iterating additional input ids is the better route.

[kurtpayne]

@TechDagan, the latest patch would link the text input and label on all search forms with a unique id. However, only the first search form and first search submit button would have a unique id. All duplicate search forms would omit the id field for the form and submit button.

[TechDagan]

Replying to kurtpayne:

Thanks for the quick response, kurtpayne. My only concern is that the described behavior would make for a confusing experience for screen reader users when multiple search forms exist. Basically, as they navigate the page they'll land on a text input but there will be no label to announce the field.

[azaozz]

Replying to TechDagan:

... Basically, as they navigate the page they'll land on a text input but there will be no label to announce the field.

No, there will be a label for all text input fields. To keep the labels the patch makes all subsequent HTML IDs unique.

[TechDagan]

Replying to azaozz:

Thanks for the clarification, azaozz. If I'm reading kurtpayne's comment right, he's saying the opposite. But since you submitted the patch, I'll go with your answer. :-)

[jane]

It doesn't seem like any further testing happened here, should we punt to revisit in 3.4? I'd also consider this an enhancement (allow addition duplicate search boxes) since the expected behavior (one search box) works fine.

[azaozz]

Replying to jane:

Agreed.

[solarissmoke]

Replying to jane:

I'd also consider this an enhancement (allow addition duplicate search boxes) since the expected behavior (one search box) works fine.

But WordPress already allows duplicate search boxes? There's nothing in the Widget manager that stops you adding more than one, and when you do, it causes conflicts.

[WraithKenny]

Just a note: In TwentyEleven (and just about any theme) The search results page contains two search fields, One in the header, another in the body, both with the same IDs.

[WraithKenny]

@TechDagan: The subsequent form elements and input[type="submit"] buttons will be lacking unique id attributes but that's not what's important. The important thing (Which I think there was some confusion) is that all the label and input elements will have matching for and id attributes for accessibility. @azaozz and @kurtpayne were actually saying the same thing ;-)

It also sounds like kurtpayne did somewhat extensive testing. As for concerns about styling breakage, the structure and IDs will be exactly the same for the first searchform (and apparently, no one is concerned about subsequent ones, lol). Worst case is a degradation of the "graceful" sort: styling may not be exactly perfect, but would still be usable. The trade off for that is fixing broken validation and accessibility (if duplicate input ids cause accessibility issues that is).

For a theme to use CSS rules like .entry-content #s, .widget_search #s, and #branding #s when #s appears in those contexts simultaneously, is a bug in the theme itself (i.e. id's are unique). This shouldn't stop core from fixing its own bugs.

Leaving it as is does more harm then good. Designers who understand this problem are forced to add wrapper elements (for styling) or implementing the patch in searchform.php themselves (for validation and accessibility). Fixing these bugs now, both in core, and in the default theme will allow designers to not jump through hoops to do the right thing, and show new designers how to do it correctly.

+1 The patch looks good. I think I've demonstrated that this is actually a bug, not an enhancement, as on a default widget setup with the TwentyEleven Theme the search field appears 2 times on most pages, 3 times on the search results page, which can be considered expected behavior.

[WraithKenny]

For those of you interested in implementing this directly into a Theme, try:

<?php
	global $search_form_counter;
	if ( ! isset( $search_form_counter ) ) $search_form_counter = 0;
	$form_id = $search_form_counter ? '' : ' id="searchform"';
	$text_id = $search_form_counter ? 's_' . $search_form_counter : 's';
	$submit_id = $search_form_counter ? '' : ' id="searchsubmit"';
	$search_form_counter++;
?>
	<form role="search" method="get"<?php echo $form_id; ?> class="searchform" action="<?php echo esc_url( home_url( '/' ) ); ?>">
		<label class="screen-reader-text assistive-text" id="<?php echo $text_id; ?>"><?php _e( 'Search', 'simple' ); ?></label>
		<input type="search" class="searchinput field" value="<?php echo get_search_query(); ?>" name="s" id="<?php echo $text_id; ?>" placeholder="<?php esc_attr_e( 'Search', 'simple' ); ?>" />
		<input type="submit" class="searchsubmit submit" name="submit"<?php echo $submit_id; ?> value="<?php esc_attr_e( 'Search', 'simple' ); ?>" />
	</form>

[WraithKenny]

Now that there is a 'html5' switch, perhaps this can be considered again?

[WraithKenny]

related #15081

[SergeyBiryukov]

16539-4.patch​ looks good at a glance.

Needs a refresh. Bundled themes should be patched in a separate ticket, let's just focus on get_search_form() in this one.

[WraithKenny]

There is a small window of opportunity here to remove the id's on the 'html5' version of this since it is opt-in. It'd have to be done before 3.6 ships though.

The form and submit buttons were being kept for back-compat, so we don't have to worry about that, and the other issue is connecting the label to the input, which can be done by wrapping the input in the label.

[azaozz]

There is a small window of opportunity here to remove the id's on the 'html5' version...

This is the best approach imho. There's no need to use IDs for styling. If we need to generate IDs for connecting <label> to <input> (think that was a requirement for some screen readers), we can keep that part, i.e. only keep $text_id in get_search_form() in the patch.

[WraithKenny]

The <label> and <input> are connected by nesting in the 16539-5.patch, avoiding the need for id altogether

[WraithKenny]

related: #23848

[SergeyBiryukov]

In 16539-5.patch​, id="searchform5" in line 172 looks like a typo.

[WraithKenny]

fixed typo.

Any feedback on the move to if then? It seemed to make more sense that way.

[WraithKenny]

related, #23850

[azaozz]

Replying to WraithKenny:

The <label> and <input> are connected by nesting in the 16539-5.patch, avoiding the need for id altogether

Yes they are connected for the browser, but as far as I remember IDs are still needed so screen readers can "see" the connection too. Worth investigating if this is still the case.

[WraithKenny]

I'm trying to gather resources to see if that's true and having much difficulty. At any rate, having multiple conflicting for/IDs surely is a larger accessibility barrier, no?

[WraithKenny]

here's a chart I've found ​http://www.html5accessibility.com/tests/form-labels.html

[WraithKenny]

It appears the relevant links are:
​http://www.w3.org/TR/2012/NOTE-WCAG20-TECHS-20120103/H44
​http://www.w3.org/TR/2012/NOTE-WCAG20-TECHS-20120103/H65

The concern apparently is IE6.

Even if the Tests under the first link fail (nested label) under IE6 and Jaws7, The second link specifies that a title attr on the input would pass, achieving WCAG 2.0 (for this one part at least) compliance.

Anyway, having two or more identical IDs fails compliance too. ​http://www.w3.org/TR/WCAG20-TECHS/F62.html

[azaozz]

Replying to WraithKenny:

Nice job on clearing that out :)

[WraithKenny]

16539-6.patch​ is an iteration on 16539-4.patch​ utilizing the $format param. It uses the counter to increment s in both 'html5' and default. It doesn't add id to the form or the submit for 'html5'. The patch also strips theme modifications (to be handled separately).

[WraithKenny]

16539-5.WCAG.patch​ or 16539-6.patch​. Not sure which to prefer...

[WraithKenny]

I apologize if anyone tried to apply the patches, they were set to git-format. I've fix and re-uploaded them.

[bpetty]

Replying to WraithKenny:

I apologize if anyone tried to apply the patches, they were set to git-format. I've fix and re-uploaded them.

Set this, and all diffs generated with git will work with GNU patch (with the typical -p0 instead of -p1) and most SVN clients:

$ git config diff.noprefix true

Of course, that also means git-apply needs -p0, but you might already be used to that with everyone else's SVN patches.

By the way, I definitely prefer the WCAG version.

[WraithKenny]

Thanks for the tip!

[WraithKenny]

16539-5.WCAG.2.patch​ moves screen-reader-text from label to span.

Tested with TwentyThirteen.

[WraithKenny]

Version 7:

1. uses the counter to increment the xhtml id's and drop form/submit id's after the first.
2. avoids id's altogether in html5
3. makes $search_form_counter, $form_id, $submit_id, and $text_id available to templates (nice side-effect)

Tested with 2013, 2012, 2011, 2010. Successful with all except 2011 which has a custom template.

[WraithKenny]

refreshed v7 against changeset 23800

[WraithKenny]

related to TwentyEleven's id conflict not being solved by this ticket: #23868

[WraithKenny]

The patch on #23868 utilizes the $variables in scope of get_search_form

[WraithKenny]

My testing was by going to each theme's search page (?s=gibberish), and having a search widget active.

[azaozz]

In 23801:

In the HTML outputted from get_search_form(): increment the ID connecting <label> to <input> when the function is called multiple times, remove the form and submit button IDs for HTML 5.0 and for XHTML after the first call. Props WraithKenny, fixes #16539

[obenland]

r23801 will break themes that rely on #searchform or #s for styling and do not provide their own searchform template.

Any chance we could limit this to the HTML5 version (and lose the generic <div> in the process)?

[WraithKenny]

New patch would default to not fixing the bug on this ticket ( :sadface: ) but would make the incrementing id's available via "opting in".

I don't endorse "fixing" this. It leaves the default way broken.

Generally, the themes doing crazy stuff involving the id's would have a searchform template. Even for themes without searchform.php, who styled the id's, the first form would appear the same as always. Only subsequent forms would have some style tweaks missing.

[obenland]

In 16539.9.diff​:

• Removed <div> wrapper in html5 search form.
• Made ids in default version static again for backwards compatibility.
• Removed a bunch trailing white spaces.

[bpetty]

In 16539.10.diff​, removed unused variables from last patch version.

[azaozz]

Looking at 16539.10.diff, removing the id from <input type="search"> in html5 mode would break older screen readers (Jaws7). Since this is for the front-end, are we OK with that?

Few years ago older Jaws versions were used a lot since the cost to upgrade is (was?) very high. Not sure if that has changed recently.

[bpetty]

Replying to azaozz:

Looking at 16539.10.diff, removing the id from <input type="search"> in html5 mode would break older screen readers (Jaws7). Since this is for the front-end, are we OK with that?

The id is already removed in the HTML5 version committed to SVN.

The HTML4 version in the patch chooses to use duplicate IDs, as opposed to incrementing (dynamically changing) IDs for the purpose of not breaking compat with themes/plugins (since it was using the same duplicated ID in 3.5 and below) - see ticket:23868#comment:7. Basically it reverts the changes made earlier in this ticket to the HTML4 version so it behaves like it did in 3.5 and below. If Jaws wasn't broken in 3.5, it shouldn't be with 16539.10.diff.

So, from what I gather here, older Jaws should be fine with the HTML4 version in 16539.10.diff, and if theme designers care to remain compat with older Jaws, they will probably stick with writing HTML4 anyway rather than opting for the HTML5 version.

For HTML4, either we break CSS/JS compat with themes/plugins by using incremented IDs, we break Jaws7 in IE6 by not using an ID, or we break HTML validation (and possibly JS) with duplicate IDs. It's one of those 3 options. I'm inclined to stick with what was already broken before - option number 3.

[SergeyBiryukov]

Replying to bpetty:

I'm inclined to stick with what was already broken before - option number 3.

Makes sense to me.

[azaozz]

Yeah, makes sense to me too. Seems we are never going to remove these IDs :)

[WraithKenny]

What here suggests that Jaws wasn't broken in 3.5?

[azaozz]

In 23916:

In the HTML outputted from get_search_form(): remove incrementing the IDs for XHTML after the first call, remove the <div> wrapper for HTML5. Props obenland, props bpetty. Fixes #16539

[WraithKenny]

The scope of this hypothetical "breaking" of css style is closer to "trival" on the scale of severity. If there is "breakage" it's not that core is creating a bug, but exposing theme errors that presently exist whether or not anyone notices. Not fixing this bug to hide errors in themer's css is not right.

[WraithKenny]

The title attribute enables conformance to accessibility standards in the absence of the for-id combo. It fixes any issue for the screenreaders. Ids are not needed.

Duplicate IDs violates the WCAG because it causes problems for screenreaders, (no work-around). AFAIK, WCAG compliance is not optional for government sites.

I wish we had access to testers using accessibility devices, it'd surely help here.

Maybe the resolution should be "wontfix" as the bug on the ticket isn't fix unless a theme opts in via the filter.

[bpetty]

Replying to WraithKenny:

What here suggests that Jaws wasn't broken in 3.5?

If you're saying that it is broken (due to duplicate IDs), it really doesn't matter anyway because then the choice is either between that, or breaking CSS/JS compat with themes/plugins with incrementing IDs. And between those two, it still makes sense to stick with duplicate IDs because from what I gathered in your links and info is that it's not only just older Jaws, but also only IE6. However, incrementing IDs to fix just that would break CSS/JS in all browsers on those same pages that output two search forms for everyone.

It sucks, but in that situation, it's pretty clear that compatibility has to come before accessibility. That's partially our own fault for adding the IDs in the first place that put us in this situation, but it's also Jaws fault for leaving a bug in their software too (which they supposedly have fixed already anyway).

By the way, if you looked at #23868, you'd see that breaking the CSS is not really "trivial" since even the default themes that shipped with WP made this mistake. So we're not only talking about themes, but also tons of child themes too.

Leaving the duplicate IDs in there is just taking the less crappy road not only for the reasons above, but also because regressions really are worse than something that never worked in the first place. We've handled "compliance" by providing the alternative HTML5 version that doesn't use duplicate IDs. In doing so, we've also provided a "fix" for this ticket - use HTML5.

[WraithKenny]

What are the chances that a child-themer noticed and deliberately targeted those non-unique ids against all common sense and best practice rules? When I trying to child-theme TwentyEleven, I noticed the error, which led me to this ticket in the first place. Core's error doesn't cause themer's to make errors but it certainly could be inspiring them to. It sets a terrible example.

You missed that the Jaws7/IE6 issue is for removing the id/for linkage. (It's easily solved by adding a title attr.) That's what my links point to. It has nothing to do with the issue of Duplicated IDs.

#23868 is my ticket heavily commented by me so of course I've read it. There's no evidence that a single child-theme would "break."

You'd have to find one

1. that didn't copy the searchform.php and
2. that didn't inherit the parent's style.css and
3. then chose to deliberately target non-unique ids. (even tho it isn't based on the parent's css)

Does one theme like this even exist? Can we get a screenshot of just how "Severe" that "breakage" truly is? Out of your "tons" of themes, most will not be affected, and none severely.

The css is truly absolutely insignificantly trivial. "ZOMG! My second little search box's blue border reverted to grey! End of World!" said no one ever.

This isn't any kind of "regression" at all. At most it reveals a css error that is there with or without this fix. This doesn't *cause* a bug, it merely stops hiding it.

On that last part, your right. At least sites wishing to fix this bug can open up their functions.php file, add the filter and set about fixing the css themselves. If they even notice.

This revert also means that TwentyTen and TwentyTwelve are back to being invalid (see ​http://www.w3.org/TR/WCAG20-TECHS/F77.html ).

[bpetty]

Replying to WraithKenny:

You missed that the Jaws7/IE6 issue is for removing the id/for linkage. (It's easily solved by adding a title attr.) That's what my links point to. It has nothing to do with the issue of Duplicated IDs.

The HTML5 version does use title as you describe (and as you wrote in your patch), but it certainly couldn't hurt the HTML4 version too. Want to open a new ticket on that with a patch? You did mention that originally, but you didn't include it in the HTML4 version of any of your patches.

[bpetty]

Or are you saying it still won't work with the ID on the input element?

[WraithKenny]

There's been some limited concern brought up through out this ticket about screen-readers. According to the WCAG, you have to:

1. 1. Match for and id tags
   2. OR wrap input in the label (but doesn't work in Jaws7/IE6)
   3. OR Use a title matching the text of the label.
2. AND ids must be unique if used.

The reason for this research was only to answer the question of whether the ids could be "safely" removed (with regards to screen readers), to which the answer was "yes." The 'html5' version now satisfies these conditions. In the default version though, worrying about 1. is moot if it's violating 2.

I just found it weird that themes are burdened with fixing core's bug. (HTML5 themes will get the fix as a bonus when they use the filter for the new search form.) The decision seems made, so I'll move on to something else :) Cheers!

[afercia]

In 36222:

Accessibility: remove the title attribute from the get_search_form() HTML5 search field.

It was added in [23801] as a workaround for issues with very old browser/screen reader combos (Jaws7/IE6).
No more necessary today. Reduces redundancy and noise for screen reader users.

See #16539.
Fixes #33952.