[fix](alert, actionSheet, picker and toast): Call pressed button handler, regardless 'role'
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:
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:
@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.
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
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:
- 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! - 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.
- 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 onion-button. We can just use a HTMLbuttonin 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!