Correct aria-described by on ValidatingFormInput
Closed this issue · 17 comments
On a page like give/whoami the input control has aria-describedby="undefined-feedback" and the label associated with it has a "for" of "email-"
@edwh - Just taken a look at this issue. Thinking of options to fix...
- Set up a default in the component for the ID like "email"
- Make the ID mandatory.
- Generate a unique ID - Looks like there is a store method to generate something.
The problem with the first two is you could end up with a page having duplicate IDs so I think the only valid option would be to generate an id. Do you have any thoughts on this?
NB: There are actually two problems wrapped up in this issue. For now I am just talking about the "label for" issue.
On further investigation it looks like this does encompass the aria-describedby as the mixin creates this from the ID
Sorry no. I meant that fixing one issue will end up fixing both of them.
Basically the ID is used to link up the label and the input field. (And subsequently the error field too). If you pass an ID in then it works. If you don't then it doesn't. If you leave it up to the developer to pass in then they could forget or they could enter a duplicate.
@edwh - Not sure if this was you or Nick...
The EmailValidator component generates a label and associated input field. On a page like give/whoami this works great. On CommunityEventModal the page creates a label itself and then adds the EmailValidator with a null label so EmailValidator doesn't add the label itself.
Do you know why the difference in use here?
It looks exactly the same - on that page I mentioned at least.
All the other fields on that page are
<form-group>
<validating-form-input/>
<invalid-feedback/>
</form-group>
The email field is now:
<form-group>
<form-group>
<validating-form-input/>
<invalid-feedback/>
</form-group>
</form-group>
I can only think that it's for no good reason then. I'll update a few of them and see if I come across any issues.
Looks like it's only 2 out of about 10 that are like this.
Just found another (existing) issue in this area in that the email component is adding elements with duplicate IDs. I'll look into that as well as part of this change.
The duplicate ID issue is caused by vue adding the ID to the main component container (as is standard) but the input field is using "v-bind="$attrs" which adds ID to that as well. If you pass ID as a prop then it doesn't get included in $attrs. But you then need to explicitly pass it.
I think $attrs is used so all of the potential props don't need to be explicitly passed in. Because validatingforminput is generic it could take a whole bunch of different properties. There are other troubles with the use of $attrs. I can see why it's used but it causes other issues like it adds "type=email" to a div and stuff like that. All of the attributes get added to the container div and also the input itself. I'm ignoring this general problem for now so as not to expand the scope of this specific issue.
Adding ID as a prop stops it being automatically added when not required. General users will just pass in some text like "contactemail" as the ID and it will work fine. Because emailvalidator is a component then it is safer for it to generate the ID itself as the users of the component could just pass in "email" for two on the same page. If you has two email on the same page then this would definitely cause an issue. You can get around it on the other users of validatingforminput as they can be explicit.
The ID only needs to be generated once per component in emailvalidator so I don't think it would really work as a computed prop unless you put a "if id=null then generate id" type thing. Unless I've missed your point?
I'll create a draft PR with what I've done so far. I haven't really tested it yet though but it might make more sense if you see the code.
Raised. This is definitely still work in progress though but if you see anything wrong then do shout!