Components: when applying md-button on a button, (click) is not triggered

[maxime1992]

Bug, feature request, or proposal:

Bug

What is the expected behavior?

Even if a button has the md-button attribute we should be able to bind the click event.

What is the current behavior?

Checkout this line of HTML :

<button
*ngFor="let possibleState of getPossibleStateActions(serviceUnit.state)"
(click)="changeState(possibleState.newStateAfterAction)"
[disabled]="serviceUnit.isUpdatingState"
>
  {{ possibleState.actionName }}
</button>

the changeState function is called.

---

But if I add md-button or md-raised-button on the button, the design is the one expected but clicking on the button never triggers changeState function.

---

What are the steps to reproduce?

git clone https://gitlab.com/linagora/petals-cockpit.git
git checkout ab7f5d5816
cd frontend
yarn
ng serve

I did try to reproduce on Plunkr but I couldn't.
I didn't check the version on Plunkr but maybe Angular is running on 2.x and my project is running 4.0.0-rc.2.

Which versions of Angular, Material, OS, browsers are affected?

4.0.0-rc.2 (didn't try under that version)

[maxime1992]

Similar problem today. I wanted to create a tooltip and it was not showing up.

I really didn't understand why and in ended up moving my span with the tooltip before my *ngFor :
Tooltip is working.

Here's the code :

(exact same line except that the second one is within an *ngFor.

[christophechevalier]

Hello everyone.

@crisbeto

It's been 3 weeks since the issue was opened, and there is no news.
Is this bug being resolved ?

Thanks guys. :octopus:

[crisbeto]

I haven't had the chance to look at it yet. I'll check it out tonight.

[crisbeto]

@maxime1992 I wasn't able to reproduce it locally with a similar HTML structure. Can you try to do another Plunkr? Now our template uses Angular 4 as well.

[maxime1992]

@crisbeto couldn't repro in the Plunkr :disappointed:

If you want to try in our repo :

git clone [email protected]:linagora/petals-cockpit.git
cd petals-cockpit/frontend
yarn
ng serve

Open your browser at : http://localhost:4200/workspaces/idWks0/petals/service-units/idSu0

Open the file
src/app/features/cockpit/workspaces/petals-content/petals-service-unit-view/petals-service-unit-overview/petals-service-unit-overview.component.html

Change those lines :

<button
  *ngFor="let possibleState of getPossibleStateActions(serviceUnit.state)"
  (click)="changeState(possibleState.newStateAfterAction)"
  [disabled]="serviceUnit.isUpdatingState"
>

By :

<button
  md-button
  *ngFor="let possibleState of getPossibleStateActions(serviceUnit.state)"
  (click)="changeState(possibleState.newStateAfterAction)"
  [disabled]="serviceUnit.isUpdatingState"
>

(notice the md-button attribute on the button)

With the button like that :

The click works and you'll see the UI updated

With the material button, click doesn't work :

[crisbeto]

Your issue comes from the fact that the button actually gets re-rendered on mouseup. It just happens so fast that you can't notice that the button got swapped out. You can break it in the same way if you add a blank mouseup to a regular button:

<button
  *ngFor="let possibleState of getPossibleStateActions(serviceUnit.state)"
  (click)="changeState(possibleState.newStateAfterAction)"
  [disabled]="serviceUnit.isUpdatingState"
  (mouseup)="noop()"
>
  {{ possibleState.actionName }}
</button>

What happens in this case is that the button ripple effect triggers change detection on the mouseup event (this has actually been fixed in https://github.com/angular/material2/pull/3066) and because getPossibleStateActions returns a completely new array every time, ngFor will destroy the button and re-create it. Since it was destroyed before you released the button, the click event won't end up firing at all. This is something that you should fix in your app by making getPossibleStateActions return the same array or assigning its result to a property.

Here's a similar issue from a month ago.

[maxime1992]

It sounds like I could have post my question on StackOverflow now but thank you for your time @crisbeto, I really appreciate the (detailed) explanation ! :smiley: :beers:

[victornoel]

@crisbeto there is something I didn't understand in your explanation/workaroud.

What I understood is: getPossibleStateActions should return the same array every time it is called (or at least if the set of actions contained in the array does not change).

My question is: is it needed to return the same array with getPossibleStateActions ONLY because the mousup event is broken (and fixed in #3066), or are you saying that getPossibleStateActions should always manage to return the same array (for the same set of possible action of course)?

Thank you for your time :)

[crisbeto]

The mouseup thing is just a symptom that this does work as expected. When getPossibleActions returns a new array every time, ngFor will re-render the entire repeater every time you do something on the page that triggers change detection, which is very inefficient considering that nothing has changed. You can see how often this happens if you add a console.log in getPossibleStateActions.

[victornoel]

@crisbeto that's the thing, if we put a console.log in the method (and put back the md-button and let the method return a new array every time), it is triggered only when the page is loaded, and when we click on a button… whatever we do around that has no impact apparently (even the mouseup actually does not trigger that!).
Note that the component is a "dumb" component with the OnPush change detection, that may be why.

But, could you really confirm something to me: do we agree that once #3066 is applied, the original problem should be solved EVEN IF getPossibleActions returns new arrays every time?

[crisbeto]

Yes, I believe that it should be fixed once #3066 is in. You can try out the latest build with this:

npm install https://github.com/angular/material2-builds.git

[victornoel]

ok, thanks for your help :)

[victornoel]

For the record (if someone comes around here with the same problem), another solution is to exploit trackBy of *ngFor with a function indexed on the input of getPossibleActions (since it is a pure function, we know its result won't change if its input doesn't).
Like this, even the mouseup event won't replace the button or stuffs like this.

[angular-automatic-lock-bot[bot]]

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._}