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(chips): focus not restored properly if chip has been removed by click #12788

Merged
merged 1 commit into from
Aug 24, 2018

Conversation

devversion
Copy link
Member

@devversion devversion commented Aug 22, 2018

  • Currently if someone tries to remove a chip by clicking the [matChipRemove], the click event will bubble up to the potential MatFormField and cause the onContainerClick to be invoked. This causes the focus to be always moved to the first chip (which is not good for accessibility).

  • Replaces multiple subscriptions in the chip list with a takeUntil. Also fixes that the chip remove subscription is re-created multiple times but not cleaned up.

  • Simplifies and cleans up the logic to restore focus after a chip has been destroyed.

Small demo that shows the issue in plain Angular: https://stackblitz.com/edit/chip-list-event-bubble-demo

NOTE: PR is based on: #12416

@devversion devversion added blocked This issue is blocked by some external factor, such as a prerequisite PR target: patch This PR is targeted for the next patch release labels Aug 22, 2018
@devversion devversion requested a review from jelbourn as a code owner August 22, 2018 14:16
@googlebot

This comment has been minimized.

@googlebot googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Aug 22, 2018
@devversion
Copy link
Member Author

@crisbeto @jelbourn This PR is based on #12416, and should be merged afterwards. Though it would be nice if we can prioritize Kristiyan and my PR because I'd consider this a pretty common chips issue.

URL to only see my changes: 5c1a972

@devversion devversion changed the title fix(chips): focus not restored properly if chip has been removed through click fix(chips): focus not restored properly if chip has been removed by click Aug 22, 2018
@devversion devversion force-pushed the fix/chips-focus-next-chip-click branch from 5c1a972 to eef6a1e Compare August 22, 2018 15:20
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

@ngbot

This comment has been minimized.

@devversion devversion force-pushed the fix/chips-focus-next-chip-click branch from eef6a1e to a9fcf08 Compare August 23, 2018 15:35
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Aug 23, 2018
@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed blocked This issue is blocked by some external factor, such as a prerequisite PR labels Aug 23, 2018
@devversion
Copy link
Member Author

@jelbourn Since #12416 just has been merged, this should be good to go now. Marking as merge ready because it has been approved.

@ngbot
Copy link

ngbot bot commented Aug 23, 2018

Hi @devversion! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

…lick

* Currently if someone tries to remove a chip by clicking the `[matChipRemove]`, the click event will bubble up to the potential `MatFormField` and cause the `onContainerClick` to be invoked. This causes the focus to be always moved to the first chip (which is not good for accessibility).

* Replaces multiple subscriptions in the chip list with a `takeUntil`. Also fixes that the chip remove subscription is re-created multiple times but not cleaned up.

* Simplifies and cleans up the logic to restore focus after a chip has been destroyed.

PR is based on: angular#12416
@devversion devversion force-pushed the fix/chips-focus-next-chip-click branch from a9fcf08 to 93c4b1f Compare August 24, 2018 13:48
@devversion
Copy link
Member Author

@jelbourn Rebased.

@jelbourn jelbourn merged commit 3da390e into angular:master Aug 24, 2018
@devversion devversion deleted the fix/chips-focus-next-chip-click branch August 24, 2018 14:53
jelbourn pushed a commit that referenced this pull request Aug 29, 2018
…lick (#12788)

* Currently if someone tries to remove a chip by clicking the `[matChipRemove]`, the click event will bubble up to the potential `MatFormField` and cause the `onContainerClick` to be invoked. This causes the focus to be always moved to the first chip (which is not good for accessibility).

* Replaces multiple subscriptions in the chip list with a `takeUntil`. Also fixes that the chip remove subscription is re-created multiple times but not cleaned up.

* Simplifies and cleans up the logic to restore focus after a chip has been destroyed.

PR is based on: #12416
@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 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

Successfully merging this pull request may close these issues.

4 participants