-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix(material/chips): provide ability to edit for all screen readers with a click on already focused chip #30983
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
base: main
Are you sure you want to change the base?
Conversation
Deployed dev-app for c0bd1c1 to: https://ng-dev-previews-comp--pr-angular-components-30983-dev-9kax7hje.web.app Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt. |
src/material/chips/chip-row.ts
Outdated
if (!this._isEditing && !this.disabled) { | ||
// Need a timeout to ensure flag not set while handling the first click. | ||
this._ngZone.runOutsideAngular(() => { | ||
setTimeout(() => (this._alreadyFocused = true), 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do this without the timeout by checking whether the chip is focused on the mousedown
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! That works well and actually handles the long press better as well. Well, I could have argued that a long press to edit is a feature, but I won't now.
Only thing is then the spec needs to have a mousedown as well.
src/material/chips/chip-row.ts
Outdated
* edit mode on a subsequent click. Otherwise, the chip appears focused when handling the | ||
* first click event. | ||
*/ | ||
_alreadyFocused = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make this private? Also maybe something more descriptive like _wasFocusedOnLastClick
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make it private. I think name is okay though, as the main point is that it has already been focused via a previous click or tab. I think with the comments and only used internally, that works. Else it would have to be really long.
src/material/chips/chip-row.ts
Outdated
@@ -135,6 +147,17 @@ export class MatChipRow extends MatChip implements AfterViewInit { | |||
} | |||
} | |||
|
|||
/** Sets _alreadyFocused (ahead of click) when chip already has focus. */ | |||
_handleMouseDown(event: MouseEvent) { | |||
this._alreadyFocused = this._hasFocus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth binding this outside the NgZone
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
…ith a click on already focused chip
Some screen readers have difficulty with allowing users to say double click or enter with voice control, preventing them from being able to edit a chip which currently requires a double click or an enter key event.
Single click conflicts with other behaviors, but allowing a click when the chips has already been focused is a natural way to edit the chip and does not affect other current behaviors.
Fixes b/290806246.