New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(form-group): Switch to fieldset+legend for better semantic/ARIA markup #1129

Merged
merged 4 commits into from Oct 4, 2017

Conversation

Projects
None yet
3 participants
@tmorehouse
Member

tmorehouse commented Sep 28, 2017

Fieldset + label automatically associates with any containing input element.

Also provides better ARIA support when there are multiple inputs (i.e. radio's and checkboxes) in a form-group

feat(form-group): Switch to fieldset+legend for better semantic markup
Fieldset + label automatically associates with any containing input element

@tmorehouse tmorehouse changed the title from feat(form-group): Switch to fieldset+legend for better semantic markup to feat(form-group): Switch to fieldset+legend for better semantic/ARIA markup Sep 28, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Sep 28, 2017

Codecov Report

Merging #1129 into dev will decrease coverage by 0.24%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1129      +/-   ##
==========================================
- Coverage   33.18%   32.93%   -0.25%     
==========================================
  Files         109      109              
  Lines        2866     2854      -12     
  Branches      890      886       -4     
==========================================
- Hits          951      940      -11     
+ Misses       1542     1539       -3     
- Partials      373      375       +2
Impacted Files Coverage Δ
lib/components/form-group.vue 70% <0%> (-1.43%) ⬇️
lib/utils/dom.js 47.86% <0%> (-1.71%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aabc54d...859c6b4. Read the comment docs.

tmorehouse added some commits Sep 29, 2017

@tmorehouse tmorehouse removed the Status: WIP label Oct 4, 2017

@tmorehouse tmorehouse merged commit 7a62b75 into dev Oct 4, 2017

1 of 2 checks passed

License Compliance FOSSA analyzing commit
Details
ci/circleci Your tests passed on CircleCI!
Details

@tmorehouse tmorehouse deleted the aria/form-group branch Oct 4, 2017

@tornography

This comment has been minimized.

tornography commented Nov 30, 2017

Colud you please revert this change, because in my opinion this is kind of breaking the internet.

If you remove the label, that corresponds with an input, your "win" of ARIA is obsolete. I want to click on a label to set focus on the input.
Additionally I dont want every label+input beeing wrapped in a fieldset. I can understand this is wanted with groups of radios or checkboxes. But not with single text inputs.

Please stick to the original bootstrap version. https://getbootstrap.com/docs/4.0/components/forms/

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 30, 2017

The problem is that the label is not easily associated with input (especially if there are multiple inputs). which is what triggers the focusing of the input. Also note that the "official" bootstrap V4 supports both <label> and <legend>.

How about if a click handler was added to the legend, which then finds the first input within the content, and if one found, it focuses it.

@tornography

This comment has been minimized.

tornography commented Nov 30, 2017

Ok, I see the issue.
For things like that we use a unique/random like id/hash that's generated for each label + input pair.
(not for radios/checkboxes, as they are wrapped inside the label)

The second thing is, that the input is kind of described by the label, which mast be manually added tuo the input, pointing to the legend.

Third, still every input now has its own fieldset, what is not what one might want.
It's more likely to wrap a set of inputs (like you said with the checkboxes/radios) to group them semantically.

So my suggestion is to separate two components:

  1. b-form-group => with label + input grouped in a div.form-group
  2. b-formfield => to wrap sets of inputs semantically

Maybe off topic in this issue: Is it necessary to wrap each form group in rows and cols?
I see the use when having a horizontal label/input layout.
But it's bloating code when having a useless row > col-12.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 30, 2017

b-form-group doesn't limit one to placing a single input inside it (i.e. you could have multiple text fields representing an address, which is labeled by the fieldset legend, and alabel would not work in this situation, so using fieldset and legend is a good compromise that retains full ARIA accessibility).

For semantics, the legend describes the content of the fieldset, much in the same way a label can describe an input, but without the need to use the for attribute (and for can only point to a single input element)

We might be able to alter the render function to not wrap the content in rows/cols when not in horizontal mode, although the trade off for less HTML would be more javascript.

And remember, you can always markup a form-fieldset/group using the sub-components to have more control over the layout

<div class="form-group">
 <label for="foobar">Label</label>
 <b-input id="foobar" aria-describedby="desc invalid valid" />
 <b-form-invalid-feedback id="invalid">Bad Input</b-form-invalid-feedback>
 <b-form-valid-feedback id="valid">Good Input</b-form-valid-feedback>
 <b-form-text id="desc">This is an input</b-form-text>
</div>

The invalid/valid feedback will only be shown if the input has a state of false or true respectively (if null, neither will be shown) or if the input fails/validates via native browser validation

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 30, 2017

How about this compromise:

If we add a prop label-for, and one passes an input ID to the label-for prop, we generate a <label> element instead of a <legend> element.

A <label> must be associated with an input (either by wrapping the input, or via the label's for attribute) in order for the "click on label" to focus the input.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Nov 30, 2017

@tornography

PR #1423 adds in the label-for prop. If specified, <b-form-group> will render a <label for={id}"> element. It assumes the ID provided exists on the input that is inside the form-group.

If an ID is not passed to label-for, then the default <legend> will be rendered with a click handler that focuses the first non-disabled visible input found in the form-group (emulating the label click, when a label has a for attribute.

I think this is a good compromise. If we were to auto 'for' the label, then we would need to add in additional DOM change observers, etc, for picking the first input ID, And adding in additional code for dealing with multiple inputs, etc, which would end up bloating the component code.

The 'div' wrapper around the input(s) is needed for grouping/alignment purpose (for the input+feedback+description), and to provide a container $refs, so it is not something we can remove easily without increasing the code size.

@tornography

This comment has been minimized.

tornography commented Dec 1, 2017

This compromise is something I can live with, or at least one can work with. Thank you.

But I'm a little afraid, that users could get confused when using b-form-group expecting a form-group and getting a fieldset + legend. This one component servers two different purposes. And "hiding" this switch behind a label-for does not help to tell what is is rendered.

So another idea: <b-form-group variant="fieldset"> so the user can explicitly choose to use fieldset.

Thanks for getting back to me with your quick responses.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Dec 1, 2017

The documentation quite clearly explains what is used (i.e. fieldset and legend) in b-form-group, see: https://bootstrap-vue.js.org/docs/components/form-group#label

So, tell me this... without the 'label-for` switch, how are you going to associate the label with the input?

The label, if it has no for attribute, will never be associated with the input (as it doesn't wrap the input). Clicking on the label (if we just had label instead of legend) will not activate the input. So your point is moot about "breaking the internet".

So because of this, we chose to use fieldset and legend to provide ARIA accessibility (i.e. for sight-impaired users, as a alegend inside a field set is always associated with the contents (and is spoken out as so by screen readers).

@tornography

This comment has been minimized.

tornography commented Dec 1, 2017

I don't want to omit the label-for. That's absolutely necessary. Just wanted to point out, that the code <b-form-group label-for="id"> might not indicate that the group is rendered differently. That's why I thought an explicit property could make this clear in the code. Like
<b-form-group label-for="id"> for label, vs. <b-form-group variation="fieldset">.
I find it strange to switch expected behavior and just pointing to the documentation.

But I guess we can use it is now. I just need to get everyone in the company aware of this.

Thanks.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Dec 1, 2017

If you look at PR #1423, you will notice the documentation update included, which explains this.

variation, as a prop name, does not explain what variation is, and would be confused with variant most likely.

If someone doesn't specify a label-for prop, then the label will never be associated with an input, and becomes useless. Legend on the other hand, does by nature of markup provide better ARIA accessibility than a dangling label with no association.

@tornography

This comment has been minimized.

tornography commented Dec 1, 2017

Oh, ok. I guess I did't make myself clear. Sorry. I'll try by giving extended examples.

<b-form-group label-for="id" label="text">
  <b-form-input id="id" />
</b-form-group>

would render

<div class="form-group">
  <label for="id">text</label>
  <input id="id" />
</div>

So label-for is need. As you said.
Now if we don't want a <label> we would need to use a boolean to switch to fieldset element, instead of checking if label-for is set.

<b-form-group fieldset="true" label="one, two">
  <b-form-input />
  <b-form-input />
</b-form-group>

would render

<fieldset class="form-group" label="text">
  <legend>text</legend>
  <input />
  <input />
</fieldset>

The logic is the same, but the user is more aware of his choice. But ether way, the checking the existence of label-for to switch will work fine. So all is good.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Dec 1, 2017

Your inputs are missing IDs, and hence the label will not be associated to the input

setting label-for will not automatically add IDs the inputs.

Using a single switch label-for is easier than having to use multiple props, and if the user forgets to set label-for they will get markup that is still somewhat accessible to screen reader users (i.e. no labels that are not associated with input(s))

@tornography

This comment has been minimized.

tornography commented Dec 1, 2017

Oh, sure. Updated the example and added the id. Of course the id is not generated automatically.
And you're right, the fallback to fielset will still be accessible. I'm ok with the solution.

Thanks again for your feedback and changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment