Improve markup of search forms

[mercime]

The current search forms are not accessible for users of screen readers and other assistive technologies. For example, the generated markup of the search form on the groups directory screen is currently:

<li class="dir-search groups-search bp-search" role="search" data-bp-search="groups">
    <form action="" method="get" id="dir-groups-search-form">
        <label for="dir-groups-search" aria-describedby="button-text">
            <input id="dir-groups-search" name="groups_search" placeholder="Search Groups..." type="search">
        </label>
        <button type="submit" id="dir-groups-search-submit" class="nouveau-search-submit" name="dir_groups_search_submit" style="display: none;">
            <span class="dashicons dashicons-search"></span>
            <span id="button-text" class="bp-screen-reader-text">Search Groups...</span>
        </button>
    </form>
</li>

Comments:

1. Move role="search" from <li> to <form> element where it belongs.
   Haven't checked what part data-bp-search="groups" in <li> is playing in the search form as yet, but possibly moving it to the <form> element would be acceptable?

2. The <label> is supposed to describe the <input> form control and thus cannot be left empty nor should it be then described by the <button> element because it has no text (empty).

3. It's great that the <label> is explicitly associated with the <input> form control. While nesting is fine in this case, best practice is not nesting the <input> control if possible. And since this search form is for a new template pack and since the <label> text would only be be "seen/read" by screen readers, we should separate the <label> text from the <input>.

4. Add aria-hidden="true" to the <span> surrounding the dashicon so that it would not be read by screen readers.

5. Remove inline style style="display: none;" in the <button> element as users of assistive technologies which only read the source code would not be able to know that there is a <button> to submit the form. In addition, theme devs have to go through hoops to remove such inline styles.

I understand that that inline style is used in JS to show style="display: block;" on click and on focus.
Solution using CSS alone with :active, :hover and :focus is ideal. Or add another class value then use JS to change the class value later, but avoiding display:none in the declaration or else it will become inaccessible.

The new markup after implementing the changes above;

<li class="dir-search groups-search bp-search">
    <form action="" method="get" id="dir-groups-search-form" role="search" data-bp-search="groups">
        <label for="dir-groups-search" class="bp-screen-reader-text">Search Groups</label>
        <input id="dir-groups-search" name="groups_search" placeholder="Search Groups..." type="search">
        <button type="submit" id="dir-groups-search-submit" class="nouveau-search-submit" name="dir_groups_search_submit">
            <span class="dashicons dashicons-search" aria-hidden="true"></span>
            <span id="button-text" class="bp-screen-reader-text">Search</span>
        </button>
    </form>
</li>

To do:

1. Ticket to remove unordered list wrappers from the search form and <select> element to follow.
2. Ticket to remove inline styles, especially those with display: none;

[hnla]

Agree with above with a couple of little comments. These are mainly comments where as long as it doesn't effect accessibility we tread carefully in case causing additional styling work, I attempted to replicate as far as possible what had been created on these search elements in terms of styles and some of which is dependent on elements, however I never considered this 'module' a finished one so we can take opportunity I guess to break it a bit and re-build

Haven't checked what part data-bp-search="groups" in <li> is playing in the search form as yet, but possibly moving it to the <form> element would be acceptable?

I would suggest we try to avoid changing unless good reason, just in case it does need to be there and presently not too sure.

The <label> is supposed to describe the <input> form control and thus cannot be left empty

No you're right, shouldn't really be empty, happy to revise that, although technically the button elements isn't empty it has text wrapped in the spans (an aggregating inline element) assistive devices should see that text or don't they?

It's great that the <label> is explicitly associated with the <input> form control. While nesting is fine in this case, best practice is not nesting the <input> control

In this instance I prefer we wrap and it's fairly accepted , neither are wrong or right, wrapping provides an implicit context, adding the 'for' attr both , required, best practice and ensures the implicit is firmed up by an explicit association. Mainly I think we have it this way, again, due to the required styling/design concerns rather than any technical reason, I don't mind un-nesting but the cost in terms of breaking and re-styling the elements is a concern.

Good point about the button inline styles 'display: none'' not sure how best to play this though.

[mercime]

These are mainly comments where as long as it doesn't effect accessibility we tread carefully in case causing additional styling work, I attempted to replicate as far as possible what had been created on these search elements in terms of styles and some of which is dependent on elements, however I never considered this 'module' a finished one so we can take opportunity I guess to break it a bit and re-build

Your huge work in creating the style sheets is a big deal and very much appreciated. Hopefully, with the addition of Gruntfile.js, some load will be lifted from your shoulders as it would be so much easier to adjust the styles and provide before/after screenshots after changes are made.

Haven't checked what part data-bp-search="groups" in <li> is playing in the search form as yet, but possibly moving it to the

element would be acceptable? I would suggest we try to avoid changing unless good reason, just in case it does need to be there and presently not too sure.

Just to note what I posted above about a new ticket to remove the UL wrap around the search form.

The <label> is supposed to describe the <input> form control and thus cannot be left empty

No you're right, shouldn't really be empty, happy to revise that, although technically the button elements isn't empty it has text wrapped in the spans (an aggregating inline element) assistive devices should see that text or don't they?

Not quite sure what you mean. The text within the button element is separate from the text needed in the label element.

It's great that the <label> is explicitly associated with the <input> form control. While nesting is fine in this case, best practice is not nesting the <input> control

In this instance I prefer we wrap and it's fairly accepted , neither are wrong or right, wrapping provides an implicit context, adding the 'for' attr both , required, best practice and ensures the implicit is firmed up by an explicit association. Mainly I think we have it this way, again, due to the required styling/design concerns rather than any technical reason, I don't mind un-nesting but the cost in terms of breaking and re-styling the elements is a concern.

Thanks for considering un-nesting the input element. From what I see in console, the label element for the search form only has margin: 0; for styling, or did I miss something?

Good point about the button inline styles 'display: none'' not sure how best to play this though.

Hope we get that Gruntfile.js so that it will be easier play with different ways to get it styled to how you want it done.

[hnla]

Testing these changes results in:

But I'll work on styling back to original but think may well just keep things simpler effect wise.

[hnla]

Actually this has re-factored without too many issues so committing and closing this ticket.

We still need to address items like display: none but we'll address those in further tickets.