fix(foundation): checkbox remove read-only support #6515
Conversation
BREAKING CHANGE: removed readOnly property from checkbox's API
yinonov
requested review from
EisenbergEffect,
chrisdholt,
nicholasrice and
janechu
as code owners
There was a problem hiding this comment.
Accessibility is not my area of expertise, so take this with a grain of salt. I don't see any problem with this general change though. However, I did leave a couple of comments related to what seems a potential bug. Bug may have already existed. I don't quite understand why the click and space key behavior are different WRT disabled.
| @@ -69,10 +55,6 @@ export class FASTCheckbox extends FormAssociatedCheckbox { | |||
| * @internal | |||
| */ | |||
| public keypressHandler = (e: KeyboardEvent): void => { | |||
| if (this.readOnly) { | |||
There was a problem hiding this comment.
Shouldn't this be updated to be if (this.disabled) in order to match the clickHandler?
There was a problem hiding this comment.
Opening this back up as I do think that this is a bug if the event is emitted even when disabled here...likely should be a noop...should we handle here or in a new fix? Considering we've already condensed another existing method as part of this, while I prefer to not batch commits, this may be a reasonable addition. Thoughts @yinonov?
|
@yinonov Just quick FYI: there are quite a few folks out here and there over this week and next. So, this might take a bit longer than usual to get merged. Thank you for your patience. |
of course |
|
ping? |
Most folks are on holiday this week, so this likely won't get resolved until next week. I assigned Chris since his review is what you really need and he can handle the merge once that's taken care of. |
Thank you |
|
Thanks @yinonov! |
* fix(foundation): checkbox remove read-only support BREAKING CHANGE: removed readOnly property from checkbox's API * remove any readOnly evidence * Change files * factor expressions to method * prevent emit when disabled
BREAKING CHANGE: remove readOnly property from checkbox's API
by MDN, read-only attribute should not exist in checkbox.