Material: autocomplete(chips): deleting chips breaks ability to add or render new chips

[Splaktar]

Bug, feature request, or proposal:

Bug

What is the expected behavior?

Deleting chips should not break the ability to add or render other chips.

What is the current behavior?

We support multi value with separator comma, enter, space and tab.
It works good as long as we don't delete any chip. As soon as we delete the chip, nothing works.
We made sure model looks good even after deleting but somehow md-item-template doesn't get rendered.

CodePen and steps to reproduce the issue:

CodePen Demo which shows your issue: TBD

<md-chips
      flex="100"
      md-autocomplete-snap
      class="..."
      readonly="..."
      ng-model="..."
      ng-if="..."
      md-transform-chip="..."
      md-on-remove="...">
</md-chips>

Detailed Reproduction Steps: TBD

What is the use-case or motivation for changing an existing behavior?

It worked as expected in 1.1.7.

Which versions of AngularJS, Material, OS, and browsers are affected?

AngularJS Material 1.1.7
Others TBD

Is there anything else we should know? Stack Traces, Screenshots, etc.

Verified that this is fixed by reverting https://github.com/angular/material/pull/11166 in https://github.com/angular/material/pull/11214.

[Splaktar]

@jelbourn the issue mentions md-item-template, but I assume that it means md-chip-template?
Do you have any details on what that template looks like?

It also appears that the custom logic within the md-transform-chip and md-on-remove expressions could be causing the problem here. If the md-transform-chip expression starts returning null, this will block chips from being created.

I tested this on the docs for HEAD and I cannot reproduce any problems related to deleting and adding more chips.

[Splaktar]

This demo is pretty close to the example that we were given and everything seems to work properly against HEAD. We need more details in order to reproduce this issue.

[Splaktar]

@jelbourn Here is a Demo where md-on-remove can cause md-transform-chip to start returning null and blocking chip creation. Is this what is happening? Is this behavior (chip being removed when it should be added) what the team is seeing?

[hetal0713]

Hi Splaktar,

@jelbourn reported issue on behalf of me. See my comments below

@jelbourn the issue mentions md-item-template, but I assume that it means md-chip-template?
Do you have any details on what that template looks like?

yes its chip template. Below is template (some info changed from original template)

    <md-chip-template
        username-chip
        class="xyz"
        ng-class="{'invalidTag': $chip.invalid}">
      <span>
         {{$chip.name}}
      </span>
      <md-tooltip>
        {{$chip.name}}
        <span ng-if="$chip.xyz">({{$chip.xyz}})</span>
        <span ng-if="$chip.abc">({{$chip.abc}})</span>
      </md-tooltip>
    </md-chip-template>

@jelbourn Here is a Demo where md-on-remove can cause md-transform-chip to start returning null and blocking chip creation. Is this what is happening? Is this behavior (chip being removed when it should be added) what the team is seeing?

Yes we are seeing issue on chip removal. As soon as we remove the chip, it starts creating new chips. I wasn't able to add chips in the demo link you gave. if i enter "abc xyz" and press tab.. it doesn't create chips. It doesn't even create for single output.

[Splaktar]

@hetal0713 I posted two demos. I assume that you weren't able to create chips when using the Broken Demo? That would be expected.

If you look at the code for this demo, you can see how the onRemove function is changing some state that causes the transformChip function to return null. Is something like this possible in your code? If so, do you have more details or can you fix it in your custom code?

In the meantime, the code that caused this has been temporarily reverted from master.

[Splaktar]

@hetal0713 If you remove md-transform-chip is it still possible to reproduce the bug in your app?

We don't yet have enough information to reproduce a bug here. All indications so far are that this is due to a bug in application code and not AngularJS Material.

If there is still a feeling that this is an AngularJS Material bug, then we need more information in order to reproduce it. An updated CodePen demo would be idea, but if that can't be provided, then it would be especially helpful to understand what expressions are related to md-on-remove and md-transform-chip.

[hetal0713]

Hi Splaktar,

I no longer can reproduce the issue after syncing with latest version of angular material. Just for the reference, here is the md-transform-chip function

  createChip(input) {
    const tmpUrls = input.split(' ');
    for (const url of tmpUrls) {
      this.addUrl_(url);
    }
    return null;
  }

[Splaktar]

Right, it would not be reproducible after the latest sync because the changes that triggered this were backed out. However, we are planning to put those changes back in. We just need to identify and reproduce the issue so that we don't break your app when we put the changes back in.

It looks like your md-transform-chip function is always returning null? Based on the md-chips docs returning null indicates "to prevent the chip from being appended".

md-transform-chip | expression | An expression of form myFunction($chip) that when called expects one of the following return values:

• an object representing the $chip input string
• undefined to simply add the $chip input string, or
• null to prevent the chip from being appended

Hopefully this helps you diagnose the problem. Please let me know if you have any questions.

I'll post a PR soon to bring these changes from PR https://github.com/angular/material/pull/11166 back into master.

[hetal0713]

Since changes are backed out from the head version, i dont know how to reproduce it. Can you engage us when this changes back in git and google3 so we can retest it?

[Splaktar]

OK, I've opened PR https://github.com/angular/material/pull/11237 to put this functionality back in.

[Splaktar]

@hetal0713 OK, these changes are back in master via PR https://github.com/angular/material/pull/11237 which was merged today. It looks like the g3_v0_x branch might be a few commits behind this, but I'm not 100% sure if Google is syncing with that or with master.

[hetal0713]

Thanks Michael. Jeremy, when will these changes be available in Google3?

Hetal

On Tue, Apr 17, 2018 at 8:47 PM Michael Prentice [e-mail]
wrote:

@hetal0713 https://github.com/hetal0713 OK, these changes are back in
master via PR #11237 https://github.com/angular/material/pull/11237
which was merged today. It looks like the g3_v0_x branch might be a few
commits behind this, but I'm not 100% sure if Google is syncing with that
or with master.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/angular/material/issues/11218#issuecomment-382205999,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AkdXkpUURApJbDeyZW55Zov5KpQGFuvGks5tpo0ugaJpZM4TNFhV
.

[mmalerba]

@hetal0713 I'll sync the changes today

[hetal0713]

• my eng team.

On Thu, Apr 19, 2018 at 1:28 PM mmalerba [e-mail] wrote:

@hetal0713 https://github.com/hetal0713 I'll sync the changes today

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/angular/material/issues/11218#issuecomment-382816994,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AkdXkjlBftr5CAHEdgQeEr13wjR5RsVdks5tqMlAgaJpZM4TNFhV
.

[mmalerba]

@hetal0713 changes have been synced to g3

[hetal0713]

Thank you. We will test it out.

On Thu, Apr 19, 2018 at 4:43 PM mmalerba [e-mail] wrote:

@hetal0713 https://github.com/hetal0713 changes have been synced to g3

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/angular/material/issues/11218#issuecomment-382874666,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AkdXktSTFoB7oq7rbZZGQQ5aBsmPTKcpks5tqPcLgaJpZM4TNFhV
.

[hetal0713]

This has again broken our code.
Here is the demo. Basically we want a simple md-chips (for now lets just say without even autocomplete) which accepts input with seperator keys and creates chips based on keys. In the demo i tried with md-seperator-keys and also with md-transform-chip function to break the chips myself. None of the approach works.

Seperator keys doesnt work at all and md-transform-chip works as long as i don't delete chip. Once i delete the chip, it stops working.

[hetal0713]

Also for transform function, if i try to return chip object, it creates extra chip. i.e. veg1 veg2 should create only two chips but it creates three veg1 veg2 and "veg1 veg2".

If i return null, it works fine until i delete any chips. Once i delete the chip, it stops working.

[Splaktar]

OK, thank you very much for the demo, that will help a lot. I'll dig into this later today.

[hetal0713]

Thanks Splaktar, Let us know if you need anything else from us.

[Splaktar]

@hetal0713 OK, I've looked into this a bit more. It appears that you and trying to use md-transform-chip for something other than what it's designed to do. It's designed to take an input string and then either convert it to an Object, keep it as a string, or reject adding the chip (returning null).

md-transform-chip is part of the pipeline for changing the md-chips' ng-model. It's not supposed to directly modify the ng-model during the chip transformation. After md-transform-chip's function is called, the result is passed to MdChip's appendChip() function. appendChip() handles things like deduplication (of both Objects and strings) and empty lists.

It seems like your main use case here, that is running into an issue, is when you are trying to support adding multiple chips at the same time, instead of one chip at a time. md-chips doesn't actually support this, but your workaround seems to get around that (at the expense of making the code a little more fragile). I've got a fix along with some comments about what is happening:

    ...
    self.keys = [
      $mdConstant.KEY_CODE.SPACE,
      $mdConstant.KEY_CODE.COMMA,
      $mdConstant.KEY_CODE.SEMICOLON
    ];
    ...
    function transformChip(chip) {
      // This should only be a single chip, but we're hacking this to support
      // adding multiple chips at one time.
      // NOTE: By doing this, we're missing out on the features of appendChip().
      // We're also only supporting a single hard-coded seperator char, SPACE in
      // this case. Even though you can see above that we also should be supporting
      // COMMA and SEMICOLON as well.
      const veges = chip.split(' ');
      // This means we have to handle the case where the model may be undefined (empty)
      // ourselves, rather than letting appendChip() manage its items Array.
      if (!self.selectedVegetables) {
        self.selectedVegetables = [];
      }
      for (const veg of veges) {
        // Additionally, we're missing out on the duplication checks in appendChip()
        // - Checking for duplicates when the chips are objects
        // - Checking for duplicates when the chips are strings
        // And need to do those here ourselves.
        if (veg && !self.selectedVegetables.includes(veg)) {
          self.selectedVegetables.push(veg);
        }
      }
      // Then we have to return null here so that appendChip() won't add the chip,
      // since we already added it (and possibly others).
      return null;
    }

It looks like this approach has gained some popularity due to this StackOverflow answer and the fact that we closed the previous two feature requests (#4657 and #3399) related to adding multiple chips at one time. We have a third feature request for this that is still open: https://github.com/angular/material/issues/10271.

[hetal0713]

Thanks Michael!! I tried it and it still doesn't work. I updated the demo
https://codepen.io/hetalpatel/pen/MGKGPB with what you suggested. It
works for first time but doesn't work as soon as i delete one chip.
I tried "abc xyz" as input

On Tue, Apr 24, 2018 at 11:19 PM Michael Prentice [e-mail]
wrote:

@hetal0713 https://github.com/hetal0713 OK, I've looked into this a bit
more. It appears that you and trying to use md-transform-chip for
something other than what it's designed to do. It's designed to take an
input string and then either convert it to an Object, keep it as a string,
or reject adding the chip (returning null).

md-transform-chip is part of the pipeline for changing the md-chips'
ng-model. It's not supposed to directly modify the ng-model during the
chip transformation. After md-transform-chip's function is called, the
result is passed to MdChip's appendChip() function. appendChip() handles
things like deduplication (of both Objects and strings) and empty lists.

It seems like your main use case here, that is running into an issue, is
when you are trying to support adding multiple chips at the same time,
instead of one chip at a time. md-chips doesn't actually support this,
but your workaround seems to get around that (at the expense of making the
code a little more fragile). I've got a fix along with some comments about
what is happening:

...
self.keys = [
  $mdConstant.KEY_CODE.SPACE,
  $mdConstant.KEY_CODE.COMMA,
  $mdConstant.KEY_CODE.SEMICOLON
];
...
function transformChip(chip) {
  // This should only be a single chip, but we're hacking this to support
  // adding multiple chips at one time.
  // NOTE: By doing this, we're missing out on the features of appendChip().
  // We're also only supporting a single hard-coded seperator char, SPACE in
  // this case. Even though you can see above that we also should be supporting
  // COMMA and SEMICOLON as well.
  const veges = chip.split(' ');
  // This means we have to handle the case where the model may be undefined (empty)
  // ourselves, rather than letting appendChip() manage its items Array.
  if (!self.selectedVegetables) {
    self.selectedVegetables = [];
  }
  for (const veg of veges) {
    // Additionally, we're missing out on the duplication checks in appendChip()
    // - Checking for duplicates when the chips are objects
    // - Checking for duplicates when the chips are strings
    // And need to do those here ourselves.
    if (veg && !self.selectedVegetables.includes(veg)) {
      self.selectedVegetables.push(veg);
    }
  }
  // Then we have to return null here so that appendChip() won't add the chip,
  // since we already added it (and possibly others).
  return null;
}

It looks like this approach has gained some popularity due to this
StackOverflow answer
https://stackoverflow.com/questions/42835551/angular-material-how-to-add-multiple-chips-when-comma-separated-string-is-adde
and the fact that we closed the previous two feature requests (#4657
https://github.com/angular/material/issues/4657 and #3399
https://github.com/angular/material/issues/3399) related to adding
multiple chips at one time. We have a third feature request for this that
is still open: #10271 https://github.com/angular/material/issues/10271.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/angular/material/issues/11218#issuecomment-384148577,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AkdXkloRSFbYjC2iDKhdTU6n1h81EnBrks5tr-srgaJpZM4TNFhV
.

[Splaktar]

@hetal0713 Oh wow, sorry, I forgot to add the link to the fixed CodePen that I created: https://codepen.io/Splaktar/pen/mLPgBb.

In both of our CodePens, I am able to add "abc xyz" and press space/comma/semicolon to see both chips added. Then delete both chips. Paste "abc xyz" again and press space/comma/semicolon and again I see both chips added properly.

Do these steps not match up with what you are doing and seeing? Did I miss something?

[hetal0713]

Then delete both chips.

Just delete one chip and try pasting the same string again.

Exact steps.

• Paste "abc xyz"
• it should create two chips abc xyz
• remove xyz
• paste "abc xyz" again
• it should create "xyz" chip again

On Wed, Apr 25, 2018 at 10:41 PM Michael Prentice [e-mail]
wrote:

@hetal0713 https://github.com/hetal0713 Oh wow, sorry, I forgot to add
the link to the fixed CodePen that I created:
https://codepen.io/Splaktar/pen/mLPgBb.

In both of our CodePens, I am able to add "abc xyz" and press
space/comma/semicolon to see both chips added. Then delete both chips.
Paste "abc xyz" again and press space/comma/semicolon and again I see both
chips added properly.

Do these steps not match up with what you are doing? Did I miss something?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/angular/material/issues/11218#issuecomment-384494978,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AkdXksEG-hpnTLH4KIqDk0XaP0sfG0vFks5tsTPrgaJpZM4TNFhV
.

[Splaktar]

@hetal0713 unfortunately that's another side effect of using md-transform-chip in a way that it wasn't designed for. In this case, we're using the MdChipCtrl's items array to update the view model, but since this workaround avoids updating the items array and modifies the chipBuffer directly (via the ng-model), things get out of sync.

I've added the following code to the CodePen Demo to force the view model to update and get things back in sync:

      // We're missing out on MdChipsCtrl.updateNgModel()'s setting of the model's view value
      // (and chipBuffer) based on the MdChipsCtrl's items array. So we have to manually force
      // the ng-model's view value to update MdChipCtrl's chipBuffer here to keep in sync.
      self.selectedVegetables = self.selectedVegetables.slice(0);

By cloning the selectedVegetables array here, we're forcing its reference to change so that change detection is triggered. There are likely alternative ways of accomplishing this as well.

[hetal0713]

Hi Michael, looks like you have backed out the code on 4/24 and now i can't
apply this fix in our code to see if it works. Is it possible to put the
changes back in?

On Thu, Apr 26, 2018 at 4:13 PM Michael Prentice [e-mail]
wrote:

@hetal0713 https://github.com/hetal0713 unfortunately that's another
side effect of using md-transform-chip in a way that it wasn't designed
for. In this case, we're using the MdChipCtrl's items array to update the
view model, but since this workaround avoids updating the items array and
modifies the chipBuffer directly (via the ng-model), things get out of sync.

I've added the following code to the CodePen Demo
https://codepen.io/Splaktar/pen/mLPgBb?editors=1010 to force the view
model to update and get things back in sync:

  // We're missing out on MdChipsCtrl.updateNgModel()'s setting of the model's view value
  // (and chipBuffer) based on the MdChipsCtrl's items array. So we have to manually force
  // the ng-model's view value to update MdChipCtrl's chipBuffer here to keep in sync.
  self.selectedVegetables = self.selectedVegetables.slice(0);

By cloning the selectedVegetables array here, we're forcing its reference
to change so that change detection is triggered. There are likely
alternative ways of accomplishing this as well.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/angular/material/issues/11218#issuecomment-384774405,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AkdXko57YxWuuovT7N9AlZC0BI90sKgeks5tsiqDgaJpZM4TNFhV
.

[mmalerba]

@hetal0713 are you working in g3? why not just sync to the CL that brought this into g3 and add your changes on top of that?

[Splaktar]

@hetal0713 AngularJS Material actually hasn't backed out the change that caused this. We just released 1.1.9 with that change included.

It looks like the g3_v0_x sync branch is based on a commit from 4/10. @josephperrott is there a need to sync g3 now?

[hetal0713]

I was able to sync to CL which introduced this bug in our code. I applied
your fix which fixed issue in one page but in other page it is still an
issue. It is due to the fact that we are using inter-component
communication using bindings. See here
https://screenshot.googleplex.com/sjJYyK9f8Kw.
We have a component1 where component2 is used as directive and we bind the
model from component1 to component2 using ngbinding. After applying the
fix, md-chips are working fine but component 1 doesn't receive the model
values as reference of that is changed using cloning.

Here is the code for component1
.
Here is the code for component2
.

Since latest changes are backed out from angular branch, we will go ahead
with our deployment. Please let us know before this changes are put back in
so our Legal users are not impacted.
Hetal

On Tue, May 1, 2018 at 1:55 PM Michael Prentice [e-mail]
wrote:

@hetal0713 https://github.com/hetal0713 AngularJS Material actually
hasn't backed out the change that caused this. We just released 1.1.9 with
that change included.

It looks like the g3_v0_x sync branch is based on a commit from 4/10.
@josephperrott https://github.com/josephperrott is there a need to sync
g3 now?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/angular/material/issues/11218#issuecomment-385739956,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AkdXki930Pev2HlZ-qSE2ZLvKpOLK_wuks5tuKGNgaJpZM4TNFhV
.

[Splaktar]

Please note that as part of the fix that introduced this problem for you, we have enabled the use of AngularJS' ng-change directive. This can be used to trigger synchronization between components as you describe above. However, as I can't see your internal code, I can't judge whether that is the right solution or not.

[hetal0713]

Not sure mean. Is there a way to fix this?

[hetal0713]

Sorry but i am not really sure what is breaking the inter component communication and don't know how to fix it. We really need to resolve this issue as this will block our releases after changes goes into g3 head.

[Splaktar]

@hetal0713 Please work with @josephperrott to debug this internal issue. If you can identify an issue with AngularJS Material, please create a CodePen reproduction that we can debug and provide a fix for.

[Splaktar]

I just posted PR https://github.com/angular/material/pull/11310 which may possibly help with this. We'll probably want to re-test this against the internal app once that PR gets merged into master.

[Splaktar]

It looks like this is solved with https://github.com/angular/material/pull/11310#issuecomment-395595523.