ionic-framework icon indicating copy to clipboard operation
ionic-framework copied to clipboard

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

Open joselrio opened this issue 2 years ago • 3 comments

Issue number: resolves https://github.com/ionic-team/ionic-framework/issues/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
  • [X] No

Other information

Alert:

Before the fix: WithoutTheFix

After the fix: WithTheFix

Action Sheet:

Before the fix: action-sheet-issue

After the fix: action-sheet-issue-fix

Picker

Before the fix: picker-issue

After the fix: picker-issue-fix

Toast:

Before the fix: toast-issue

After the fix: toast-issue-fix

joselrio avatar Feb 01 '24 11:02 joselrio

@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.

sean-perkins avatar Feb 14 '24 02:02 sean-perkins

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

joselrio avatar Mar 12 '24 20:03 joselrio

Hello @joselrio can you incorporate the changes that I applied here: https://github.com/ionic-team/ionic-framework/pull/28952/commits/91b5b8b3bb82da20fd0cfed373c478809f01e12d#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!

sean-perkins avatar Mar 14 '24 17:03 sean-perkins