Skip to content
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

fix: disabled state in radio-group #6567

Closed
olaf-k opened this issue Dec 14, 2022 · 5 comments · Fixed by #6601
Closed

fix: disabled state in radio-group #6567

olaf-k opened this issue Dec 14, 2022 · 5 comments · Fixed by #6601
Labels
area:fast-foundation Pertains to fast-foundation community:request Issues specifically reported by a member of the community. improvement A non-feature-adding improvement status:needs-investigation Needs additional investigation

Comments

@olaf-k
Copy link
Contributor

olaf-k commented Dec 14, 2022

🐛 Bug Report

The way the radio-group handles an update of its disabled state today is by iterating through its children radios and setting them to the same state. The issue with this mechanism is that it doesn't preserve the children's individual state. I.e., if one of the radios is disabled, and the group is disabled, then re-enabled, all radios will also be enabled.

This is not consistent with the behavior of the select, which simply relies on CSS and shortcuts the clickhandler when disabled, thus preserving its children state, as would the native element. There is no native radio-group, but should there be one, we could probably expect its behavior to follow the same pattern.

💻 Repro or Code Sample

https://codepen.io/olaf-k/pen/rNKXBGZ

🤔 Expected Behavior

The radio-group should preserve its children state.

💁 Possible Solution

A quick (?) solution here would be to mimic the select (and the listbox) by introducing the appropriate CSS for radio widgets and preventing clicks in the radio-group (using pointer-events: none; ?) This wouldn't prevent form submission though? (Would temporarily remove the name be an option?)

@olaf-k olaf-k added the status:triage New Issue - needs triage label Dec 14, 2022
@chrisdholt
Copy link
Member

Thanks for the issue @olaf-k, I think beyond the CSS solution we'd need to ensure that keyboard behaviors are likewise accurately disabled in these scenarios, including when inside a toolbar (where the interaction is a bit different). It's possible that disabling the radiogroup is enough to prevent interaction via AT, but we should test that to confirm. In that case, I think it'd be reasonable to keep the radio states and just prevent any interaction via keyboard with them. Maybe that's a good next step here. Are you open to investigating the Assistive Tech implications here and testing the theory that disabling the radiogroup would prevent AT from interacting with the radio children?

@chrisdholt chrisdholt added improvement A non-feature-adding improvement status:needs-investigation Needs additional investigation area:fast-foundation Pertains to fast-foundation community:request Issues specifically reported by a member of the community. and removed status:triage New Issue - needs triage labels Dec 15, 2022
@olaf-k
Copy link
Contributor Author

olaf-k commented Dec 16, 2022

Ah! I thought about keyboard navigation but didn't know about toolbar, good point.

Are you open to investigating the Assistive Tech implications here and testing the theory that disabling the radiogroup would prevent AT from interacting with the radio children?

Sure! I'll investigate that next week.

@olaf-k
Copy link
Contributor Author

olaf-k commented Dec 22, 2022

All right. Did some playing around, but couldn't come up with a proper solution to "how to disable radios from their group without setting them to disabled individually".

Disabling mouse input ✔

Testing for disabled in clickHandler isn't enough because of event bubbling. This can be worked around by setting a temporary click handler in capture mode while the group is disabled.

// in disabledChanged
this.addEventListener('click', this.captureHandler, true);
// (...)
public captureHandler = (e: MouseEvent): void => {
    e.stopPropagation();
};

On the CSS side, we can dim the children's appearance and prevent them from reacting to hover with

:host([disabled]) ::slotted(*) {
    opacity: var(--disabled-opacity);
    pointer-events: none;
}

Disabling keyboard input ✔

Here again, testing for disabled in keydownHandler isn't enough because of the toolbar use case. The workaround consists in updating the toolbar code to not consider children of disabled elements¹, i.e., in reduceFocusableItems:

if (
    !element.hasAttribute("disabled") &&
    !element.hasAttribute("hidden") &&
    element.childElementCount) {
    return elements.concat(
        Array.from(element.children).reduce(FASTToolbar.reduceFocusableItems, [])
    );
}

¹ Shouldn't this always be the case BTW?

Preventing focus can just be done as usual by setting tabindex to -1.

Hiding from <form> 😐

Not setting the radio children to disabled means they will be taken into account by forms, whatever state the radio-group is in.
The only two ways I tried to prevent that were:

  • temporarily remove the currently checked radio's name attribute
  • temporarily set a valueless form attribute on it

Both are hacks though. The former also may break selectors, which is even worse.

Accommodating screen readers 😐

Aside from the native tab-based keyboard navigation, screen readers provide their own navigation system that allows going over disabled controls. This is normal behavior and in that case, disabled radios are indicated as "unavailable", which unfortunately won't work for us since only the radio-group is disabled.

Using aria-hidden to hide them from ATs would be a mistake as it would be hiding legitimate information from the user (and a departure from normal behavior).

@chrisdholt
Copy link
Member

Thanks for the follow-up @olaf-k, I'll try to dig in myself on the details above and the implications. Most folks are out in the coming week or so for the holidays, but I'll keep this top of mind and likely try to follow up after the break.

@chrisdholt
Copy link
Member

@olaf-k I revisited this last night and I think it may be simpler than the options we initially were looking at. I think it solves the scenario you've outlined here though it does necessitate migrating the click behaviors to the group itself. Feedback welcome on the PR linked above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:fast-foundation Pertains to fast-foundation community:request Issues specifically reported by a member of the community. improvement A non-feature-adding improvement status:needs-investigation Needs additional investigation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants