[fix](alert, actionSheet, picker and toast): Call pressed button handler, regardless 'role'

[joselrio]

Issue number: resolves #23037

---

What is the current behavior?

If two buttons in an alert both have the cancel role, only the first handler will ever be called, regardless of which button is pressed.

Based on @sean-perkins comments, fix has been added to ion-action-sheet, ion-picker, and ion-toast as well since they had the same issue.

What is the new behavior?

Now the handler for the pressed button is called, regardless of its 'role'.

Does this introduce a breaking change?

• Yes
• No

Other information

Alert:

Before the fix:

After the fix:

Action Sheet:

Before the fix:

After the fix:

Picker

Before the fix:

After the fix:

Toast:

Before the fix:

After the fix:

[sean-perkins]

Can we write a test that verifies the callback (handler) is called with cancel role buttons?

If you need any help or direction on this, let me know!

[sean-perkins]

I'll need to ask the team, but this change does change the current behavior of a cancel role button with a custom handler that returns false.

Previously, it would have dismissed the overlay. With the proposed changes it would not dismiss.

I think the change in behavior is valid/expected, but never hurts to get a second opinion 🙂

[sean-perkins]

Hello! After speaking with the team, I'd advise we adjust the implementation pattern here. We should de-risk and validate a use case for needing to prevent a dismiss with a cancel role, before making that change.

The proposed fix for the original issue should execute the handler callback, but not prevent dismissing. Something to the effect of:

private async buttonClick(button: AlertButton) {
  const role = button.role;
  const values = this.getValues();
  const returnData = await this.callButtonHandler(button, values);
  if (isCancel(role)) {
    /**
     * Cancel roles should always dismiss the alert,
     * even if the handler function returns `false`.
     */
    return this.dismiss({ values, ...returnData }, role);
  }
  if (returnData !== false) {
    return this.dismiss({ values, ...returnData }, role);
  }
  return false;
}

While investigating this issue further, I found that ion-action-sheet, ion-picker, and ion-toast all have very similar implementations and likely the same problematic behavior.

We can discuss more in Slack, but I think addressing the fix across all these implementations would be great to knock out together, once we validate the path for one. I can help out here if desired.

[sean-perkins]

@joselrio changes look great!

Are we able to write an automated test using Playwright to validate the button handler is called?

Feel free to ping me on Slack if you have questions on how to write the test.

[joselrio]

Hi @sean-perkins,

Can you please take a look at the latest stuff I made to this?
Didn't implement ActionSheet cancel button tests since at this context having more than one cancel button is not allowed.
Let me know if any change is need.

Cheers,
JR

[sean-perkins]

Hello @joselrio can you incorporate the changes that I applied here: 91b5b8b#diff-5ce56bbcb3bd1b1e868dd811c6c96de0bb2d8f72d3dde47fb2d83c48989b6159 for the picker component tests, to the remaining overlays?

A few key points on why we would prefer this change:

1. Our changes in this PR do not differ based on the text direction or the mode (visual appearance at this time). As a result, we don't need to test against those conditions in our E2E test. This means our tests will run faster!
2. Building on above, our changes in this PR are logical - does a callback function get called when we expect it to? We don't need screenshots to verify this. Screenshots are significantly slower to execute than parsing a condition of the DOM.
3. This one we do inconsistently today in Ionic Framework, but we are actively trying to reduce coupling between components and tests, where they are not needed. For example, testing this behavior for ion-picker, does not depend on ion-button. We can just use a HTML button in this case. This is most important with screenshots, but by reducing coupling, we have significantly less risk of something else breaking/changing in the future. We also request less JS per test as a result, which improves performance of the tests.

Let me know if you have any questions or concerns!