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.

Sign up for GitHub

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

Jump to bottom

fix(selection-list): incorrect cursor if disabled #9963

Merged
merged 1 commit into from
Feb 18, 2018
Merged

fix(selection-list): incorrect cursor if disabled #9963

merged 1 commit into from
Feb 18, 2018

Conversation

devversion
Copy link
Member

  • Currently if a list option is disabled through the [disabled] binding, the disabled option still has the pointer cursor.
  • Fixes that list options are still focusable if disabled.
  • Fixes that the selection list is still focusable if disabled.

Fixes #9952

@devversion devversion added pr: needs review target: patch This PR is targeted for the next patch release labels Feb 15, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 15, 2018
crisbeto
crisbeto reviewed Feb 15, 2018
View reviewed changes
src/lib/list/selection-list.ts Outdated
'[class.mat-list-item-disabled]': 'disabled',
'[class.mat-list-item-focus]': '_hasFocus',
'[attr.aria-selected]': 'selected.toString()',
'[attr.aria-disabled]': 'disabled.toString()',
'[attr.tabindex]': 'disabled ? null : -1'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether it should be completely un-focusable when it's disabled. If that's the case, users might not know that the list is even there. There are some examples in the a11y guidelines where items are still focusable when disabled, but they don't do anything when clicked. E.g. here's an excerpt from the menu guidelines:

Disabled menu items are focusable but cannot be activated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. How about keeping it focusable? but just ignoring keypress events?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thought a bit more about it. Even if the disabled list is focusable, the user wouldn't be able to walk through the individual options, and therefore it wouldn't really help knowing that there is a list?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but the question is whether the user shouldn't be able to tab through the list items, even if they're disabled.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. I think allowing to tab through the list items makes sense. We would just disable the toggling behavior.

Also if we do this, then we'd need to find a way for the ListKeyManager that still allows the user to navigate to disabled items. e.g. a new option for the list key manager. Thoughts?

devversion added a commit to devversion/material2 that referenced this pull request Feb 16, 2018
@devversion
fix(list-key-manager): infinite loop if all items are disabled
9a41033 
* Currently if the `ListKeyManager` is being used in wrap mode, and all items are disabled, pressing the down arrow key will cause the key manager to go into an infinite loop that searches for the next item.

Related angular#9963
devversion added a commit to devversion/material2 that referenced this pull request Feb 16, 2018
@devversion
fix(list-key-manager): infinite loop if all items are disabled
ad3bdab 
* Currently if the `ListKeyManager` is being used in wrap mode, and all items are disabled, pressing the down arrow key will cause the key manager to go into an infinite loop that searches for the next item.

Related angular#9963
@devversion devversion mentioned this pull request Feb 16, 2018
Merged
@devversion
fix(selection-list): incorrect cursor if disabled
196f4c4 
* Currently if a list option is disabled through the `[disabled]` binding, the disabled option still has the `pointer` cursor.

* No longer delegates the keydown event to the ListKeyManager when disabled. Right now there can be an exception because all items are disabled. See angular#9981.

Fixes angular#9952.
@devversion devversion force-pushed the fix/selection-list-disabled-state branch from 8c9ce52 to 196f4c4 Compare February 17, 2018 11:20
@devversion devversion mentioned this pull request Feb 17, 2018
Closed
@devversion
Copy link
Member Author

devversion commented Feb 17, 2018 •
edited
Loading

@crisbeto I decided to not touch the tabIndex stuff in this PR. The main intention of this PR is the incorrect cursor if options are disabled.

Also to avoid any runtime exceptions in combination with #9981 I added a security check to the _keydown handler. I want to address the accessibility issues as part of another issue: #9995

crisbeto
crisbeto approved these changes Feb 17, 2018
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crisbeto crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Feb 17, 2018
jelbourn pushed a commit that referenced this pull request Feb 18, 2018
@devversion @jelbourn
fix(list-key-manager): infinite loop if all items are disabled (#9981)
230e22b 
Currently if the `ListKeyManager` is being used in wrap mode, and all items are disabled, pressing the down arrow key will cause the key manager to go into an infinite loop that searches for the next item.

Related #9963
@jelbourn jelbourn merged commit e6e2091 into angular:master Feb 18, 2018
tinayuangao pushed a commit that referenced this pull request Feb 20, 2018
@devversion @tinayuangao
fix(list-key-manager): infinite loop if all items are disabled (#9981)
3013acc 
Currently if the `ListKeyManager` is being used in wrap mode, and all items are disabled, pressing the down arrow key will cause the key manager to go into an infinite loop that searches for the next item.

Related #9963
tinayuangao pushed a commit that referenced this pull request Feb 20, 2018
@devversion @tinayuangao
fix(selection-list): incorrect cursor if disabled (#9963)
b2d00c5 
* Currently if a list option is disabled through the `[disabled]` binding, the disabled option still has the `pointer` cursor.
* No longer delegates the keydown event to the ListKeyManager when disabled. Right now there can be an exception because all items are disabled. See #9981.

Fixes #9952.
andrewseguin pushed a commit that referenced this pull request Feb 20, 2018
@devversion @andrewseguin
fix(selection-list): incorrect cursor if disabled (#9963)
13e809a 
* Currently if a list option is disabled through the `[disabled]` binding, the disabled option still has the `pointer` cursor.
* No longer delegates the keydown event to the ListKeyManager when disabled. Right now there can be an exception because all items are disabled. See #9981.

Fixes #9952.
andrewseguin pushed a commit that referenced this pull request Feb 20, 2018
@devversion @andrewseguin
fix(list-key-manager): infinite loop if all items are disabled (#9981)
775f560 
Currently if the `ListKeyManager` is being used in wrap mode, and all items are disabled, pressing the down arrow key will cause the key manager to go into an infinite loop that searches for the next item.

Related #9963
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto approved these changes

@jelbourn jelbourn Awaiting requested review from jelbourn

Assignees

@jelbourn jelbourn

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MatSelectionList disabled input is only checked at creation time
4 participants
@devversion @crisbeto @jelbourn @googlebot