Make radiobutton focusable

[bjarnef]

Description

Not sure why the radiobutton is different from checkbox and set tabindex="-1" which mean it isn't focusable when none of the radiobuttons are checked.
So it wasn't possible to set focus on radiobutton and check this via space key.
It seems there a single radiobutton isn't focusable because of tabindex="-1" on host element unlike checkbox, but use in radiobutton group to focus only a single element (first or selected item).

I think much of the styling could be cleaned up (maybe there should be some base styling to import, so we don't need to write the same CSS regarding much of the custom outline styling).

In both radiobutton and checkbox (extending boolean input) the structure is like the following:

<label>
    <input
      id="input"
      type="checkbox|radio"
      name=${this.name}
      value=${this.value}
      .checked=${this.checked}
      .disabled=${this.disabled}
      @change=${this._onChange} />
    <div id="button|ticker"></div>
    Label text here...
</label>

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Motivation and context

How to test?

Screenshots (if appropriate)

Checklist

• If my change requires a change to the documentation, I have updated the documentation in this pull request.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.

[github-actions]

Hi there @bjarnef, thank you for this contribution! 👍

While we wait for the team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[bjarnef]

I think the radiobutton need tabindex="0" by default, when not inside radiobutton group. But adding tabindex here seems to require double tab to focus the input, which is why the focus works on checkbox.

Furthermore I think there should also be a checkbox group as it is possible to add name to a checkbox input and submitting a form would have the field values available in the specific group.

[nielslyngsoe]

Hi @bjarnef Thanks for your attention to these details.

I agree that this is a problem "which means it isn't focusable when none of the radio buttons are checked." I'm not sure why you outcommented that.

Which leads me to having a hard time seeing how this PR actually changes/fixes anything with keyboard navigation, cause it does not change anything regarding keyboard navigation on my end.

I do see that you cleaned up a bit and fixed the styling for the disabled state. But then this should not be linked with the Issue any more? Because we should not accidentally close the issue when closing this PR.

Let me know if I'm not right about that this does not fix or change keyboard navigation.

Thanks :-)

[bjarnef]

@nielslyngsoe I originally worked on making the radiobutton focusable here: https://uui.umbraco.com/?path=/story/uui-radio--aaa-overview

but I then noticed is causes issues with a radiobutton group https://uui.umbraco.com/?path=/story/uui-radio--radio-group where it set tabindex="0" on the selected option (or first option) and the rest tabindex="-1"

but it also meant that for a single radiobutton it has tabindex="-1" and therefore never focus the radiobutton.
https://uui.umbraco.com/?path=/story/uui-radio--aaa-overview

If I change to tabindex="0"it outline the radiobutton, but hitting space doesn't select the radio, it requires another hit on tabs to get to the actual radio input.

This also seemed to be a be inconsistent for checkbox, because it doesn't set the tabindex attribute on the host element, where it works well with focus and selection via keyboard.

The aria-checked attribute should also has value true or false instead or set attribute with empty value and remove the attribute.

[nielslyngsoe]

@bjarnef I have changed your PR description so it does not reference the issue any longer as this does not fix the keyboard navigation.

also there is linting issues with this one, so it would be nice if you could run the npm run lint:fix script on this branch. Thanks

[nielslyngsoe]

@bjarnef this seems close to be ready for merge. Would you like to finish it of and then I will do a final review and get it in? Or do you want me to take it from here?

[bjarnef]

@nielslyngsoe you are welcome to take a look.
Removing tabindex from radiobutton makes it focusable (because it focus the radio input element) and similar to checkbox/checkbox list, but IIRC that caused focus on multiple radiobuttons in the list.

I also find some inconsistency because radiobutton allow to set name attribute, while checkbox doesn't although a native checkbox does allow that.
In a regular form this is used to get submitted values from a named group.

I would like it the uui-radio and uui-checkbox are as close to each other as possible like <umb-checkbox> and <umb-radiobutton> in the current backoffice.

[nielslyngsoe]

@bjarnef I had one of our Developers looking at this, as well as my self. And I have to be honest it is really hard to review this type of PR where you combine a few things. As well you described somethings that I cant find in the code changes.

I completely agree with your goal. So to make it easier to review, I will ask you to split this into individual PRs. with a clear description of "The problem", "The solution", and "Steps to reproduce the problem".

Cause the problem with this PR is that we can't see any difference and we dont know what exactly to look for.
Things like the name attribute and tab-index is not touched as part of this code, so we end up spending a lot of time understanding what the changes did affect.
From code I would assume you did fix something with the styling when the component is Disabled — for that specific case it would be useful to get a PR separate for that, with a Storybook-Story exemplifying the case, so others in the future can be aware about the case — so they dont break that scenario.