Skip to content

refactor(mdc-chips): Base isEmptyInput check on chipInput value. #16977

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

Merged
merged 1 commit into from
Sep 9, 2019
Merged

refactor(mdc-chips): Base isEmptyInput check on chipInput value. #16977

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/material-experimental/mdc-chips/chip-grid.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,9 +514,8 @@ export class MatChipGrid extends _MatChipGridMixinBase implements AfterContentIn

/** Returns true if element is an input with no value. */
private _isEmptyInput(element: HTMLElement): boolean {
if (element && element.nodeName.toLowerCase() === 'input') {
let input = element as HTMLInputElement;
return !input.value;
if (element && element.id === this._chipInput!.id) {
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 why we need this check in the first place. Wouldn't the chip grid always be associated with the proper input anyway because of its DOM structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is called in a generic _keyDown handler, so we're attempting to determine first if the element that received the keyDown event is the input element, and second, whether that input is empty.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, but it seems a little fragile because it assumes that the input will have an ID. We could check it by identity instead (element === this._chipInput).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem there comes that _chipInput is not actually a reference to an html element, it's an interface defined in chip-text-control.ts that's provided to the grid (that provides the necessary references/methods to drive interaction with the input), so the two aren't comparable. For context, _chipInput must always have an id, it's a required field. Inputs that use a matChipInputFor directive have the id generated and assigned automatically. It would be the onus of any person using a custom input to properly implement the interface and assign the id.

return this._chipInput.empty;
}

return false;
Expand Down