fix(selection-list): do not allow toggling disabled options #12617
Conversation
* Currently due to a misplaced `disabled` check in the selection list, users can toggle the state of a disabled `<mat-option>` by using the keyboard. Fixes angular#12608
devversion
added
P2
googlebot
added
the
cla: yes
src/lib/list/selection-list.ts
Outdated
| } | ||
| this._toggleFocusedOptionIfEnabled(); | ||
| // Always prevent space from scrolling the page since the list has focus | ||
| event.preventDefault(); | ||
| break; |
There was a problem hiding this comment.
Wouldn't it be simpler to fix the issue by having a return here instead of the break?
There was a problem hiding this comment.
How does the issue relate to the break here? The issue is that we just check the state of SelectionList#disabled instead of looking at the disabled state of the individual option that should be toggled.
There was a problem hiding this comment.
No problem, I've updated the PR description to explain what's causing the issue.
src/lib/list/selection-list.ts
Outdated
| /** Toggles the selected state of the currently focused option. */ | ||
| private _toggleSelectOnFocusedOption(): void { | ||
| /** Toggles the state of the currently focused option if enabled. */ | ||
| private _toggleFocusedOptionIfEnabled(): void { |
There was a problem hiding this comment.
Even though this method is private, the naming sounds weird. If we end up doing this, instead of what I mentioned above, I think it should be turned into _getFocusedOption and then the place it gets consumed would check whether it's disabled.
There was a problem hiding this comment.
As discussed on Slack. In my opinion, this would result in code duplication that can be avoided by just having such a method.
I'm not sure about the method name, though. I agree that IfEnabled sounds weird. For now, changed it to just _toggleFocusedOption.
crisbeto
added
pr: lgtm
action: merge
* fix(selection-list): do not allow toggling disabled options * Currently due to a misplaced `disabled` check in the selection list, users can toggle the state of a disabled `<mat-option>` by using the keyboard. Fixes #12608 * Rename function
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently due to a misplaced
disabledcheck in the selection list, users can toggle the state of a disabled<mat-option>by using the keyboard.The issue is that we just check for the
disabledstate of the selection list, instead of looking at the disabled state of the individual option that should be toggled.--
Note: We can also have tests for
ENTERandSHIFTnavigation, but not sure if that's necessary. Having theSPACEtest, seems to already cover the actual "fault".Fixes #12608