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(selection-list): tabIndex should respect disabled state #7039

Merged

Conversation

devversion
Copy link
Member

  • Currently if the selection-list is disabled, the tabIndex may be still set to a valid value that allows tabbing to the element. The mixinTabIndex respects the disabled state of the selection list.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 13, 2017
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.

Code-wise LGTM, but I'm not sure whether disabling focus on the list is the way to go since it's also a presentational component. Thoughts @jelbourn?

@devversion
Copy link
Member Author

devversion commented Sep 13, 2017

@crisbeto Not too sure about that. The selection list seems more intended to be an interactive control than for presentation. But I see what you mean. Maybe in that case it would make more sense to use a normal list.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM, selection-list is specifically a form contorl (listbox), so this should be the right behavior

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Sep 13, 2017
@devversion devversion force-pushed the fix/selection-list-disabled-state-tabindex branch from 30a6efc to 9589b3c Compare September 16, 2017 13:11
@devversion devversion added the P2 The issue is important to a large percentage of users, with a workaround label Sep 18, 2017
@devversion devversion force-pushed the fix/selection-list-disabled-state-tabindex branch from 9589b3c to e199178 Compare September 21, 2017 15:32
@devversion devversion added P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful and removed P2 The issue is important to a large percentage of users, with a workaround labels Sep 21, 2017
@devversion devversion force-pushed the fix/selection-list-disabled-state-tabindex branch from e199178 to 8a68ed4 Compare September 22, 2017 14:54
@devversion devversion added P2 The issue is important to a large percentage of users, with a workaround and removed P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful labels Sep 27, 2017
@devversion devversion force-pushed the fix/selection-list-disabled-state-tabindex branch 2 times, most recently from 16dd5ac to c3d9672 Compare September 29, 2017 13:28
@andrewseguin
Copy link
Contributor

Needs rebase.

@andrewseguin andrewseguin added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Sep 29, 2017
@devversion devversion force-pushed the fix/selection-list-disabled-state-tabindex branch from c3d9672 to 86b3151 Compare September 30, 2017 08:14
@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Sep 30, 2017
@kara
Copy link
Contributor

kara commented Oct 4, 2017

@devversion Rebase one more time?

@kara kara added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Oct 4, 2017
* Currently if the selection-list is disabled, the tabIndex may be still set to a valid value that allows tabbing to the element. The `mixinTabIndex` respects the disabled state of the selection list.
@devversion devversion force-pushed the fix/selection-list-disabled-state-tabindex branch from 86b3151 to e46a98d Compare October 5, 2017 15:13
@kara kara merged commit c2a9516 into angular:master Oct 5, 2017
@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 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants