Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ui5-dialog, ui5-popover, ui5-responsive-popover: before-close Event Fails with preventDefault and programatic closing of the dilog #10501

Open
1 task done
adrian-bobev opened this issue Jan 9, 2025 · 3 comments
Labels
bug This issue is a bug in the code Medium Prio TOPIC RD

Comments

@adrian-bobev
Copy link
Member

Bug Description

When the before-close event is prevented using event.preventDefault(), the dialog does not close when attempting to set open = false.

Affected Component

ui5-dialog, ui5-popover, ui5-responsive-popover

Expected Behaviour

The dialog should still close as expected when the before-close event is prevented using event.preventDefault() and open = false is set programmatically.

Isolated Example

https://sap.github.io/ui5-webcomponents/play/#eyJpbmRleC5odG1sIjp7Im5hbWUiOiJpbmRleC5odG1sIiwiY29udGVudCI6IjwhLS0gcGxheWdyb3VuZC1mb2xkIC0tPlxuPCFET0NUWVBFIGh0bWw-XG48aHRtbCBsYW5nPVwiZW5cIj5cblxuXG5cblxuXG5cblxuXG5cbjxoZWFkPlxuICAgIFxuICAgIDxzdHlsZT5cbiAgICAgICo6bm90KDpkZWZpbmVkKSB7XG4gICAgICAgIGRpc3BsYXk6IG5vbmU7XG4gICAgICB9XG5cbiAgICBodG1sIHtcbiAgICAgIGZvcmNlZC1jb2xvci1hZGp1c3Q6IG5vbmU7XG4gICAgfVxuICAgIDwvc3R5bGU-XG5cbiAgICBcbiAgICA8c3R5bGU-XG4gICAgICAqOm5vdCg6ZGVmaW5lZCkge1xuICAgICAgICBkaXNwbGF5OiBub25lO1xuICAgICAgfVxuXG4gICAgaHRtbCB7XG4gICAgICBmb3JjZWQtY29sb3ItYWRqdXN0OiBub25lO1xuICAgIH1cbiAgICA8L3N0eWxlPlxuXG4gICAgXG4gICAgPHN0eWxlPlxuICAgICAgKjpub3QoOmRlZmluZWQpIHtcbiAgICAgICAgZGlzcGxheTogbm9uZTtcbiAgICAgIH1cblxuICAgIGh0bWwge1xuICAgICAgZm9yY2VkLWNvbG9yLWFkanVzdDogbm9uZTtcbiAgICB9XG4gICAgPC9zdHlsZT5cblxuICAgIFxuICAgIDxzdHlsZT5cbiAgICAgICo6bm90KDpkZWZpbmVkKSB7XG4gICAgICAgIGRpc3BsYXk6IG5vbmU7XG4gICAgICB9XG5cbiAgICBodG1sIHtcbiAgICAgIGZvcmNlZC1jb2xvci1hZGp1c3Q6IG5vbmU7XG4gICAgfVxuICAgIDwvc3R5bGU-XG5cbiAgICBcbiAgICA8c3R5bGU-XG4gICAgICAqOm5vdCg6ZGVmaW5lZCkge1xuICAgICAgICBkaXNwbGF5OiBub25lO1xuICAgICAgfVxuXG4gICAgaHRtbCB7XG4gICAgICBmb3JjZWQtY29sb3ItYWRqdXN0OiBub25lO1xuICAgIH1cbiAgICA8L3N0eWxlPlxuXG4gICAgXG4gICAgPHN0eWxlPlxuICAgICAgKjpub3QoOmRlZmluZWQpIHtcbiAgICAgICAgZGlzcGxheTogbm9uZTtcbiAgICAgIH1cblxuICAgIGh0bWwge1xuICAgICAgZm9yY2VkLWNvbG9yLWFkanVzdDogbm9uZTtcbiAgICB9XG4gICAgPC9zdHlsZT5cblxuICAgIFxuICAgIDxzdHlsZT5cbiAgICAgICo6bm90KDpkZWZpbmVkKSB7XG4gICAgICAgIGRpc3BsYXk6IG5vbmU7XG4gICAgICB9XG5cbiAgICBodG1sIHtcbiAgICAgIGZvcmNlZC1jb2xvci1hZGp1c3Q6IG5vbmU7XG4gICAgfVxuICAgIDwvc3R5bGU-XG5cbiAgICBcbiAgICA8c3R5bGU-XG4gICAgICAqOm5vdCg6ZGVmaW5lZCkge1xuICAgICAgICBkaXNwbGF5OiBub25lO1xuICAgICAgfVxuXG4gICAgaHRtbCB7XG4gICAgICBmb3JjZWQtY29sb3ItYWRqdXN0OiBub25lO1xuICAgIH1cbiAgICA8L3N0eWxlPlxuXG4gICAgPG1ldGEgY2hhcnNldD1cIlVURi04XCI-XG4gICAgPG1ldGEgbmFtZT1cInZpZXdwb3J0XCIgY29udGVudD1cIndpZHRoPWRldmljZS13aWR0aCwgaW5pdGlhbC1zY2FsZT0xLjBcIj5cbiAgICA8dGl0bGU-U2FtcGxlPC90aXRsZT5cbiAgICA8bGluayByZWw9XCJzdHlsZXNoZWV0XCIgaHJlZj1cIi4vbWFpbi5jc3NcIj5cbjwvaGVhZD5cblxuPGJvZHkgc3R5bGU9XCJiYWNrZ3JvdW5kLWNvbG9yOiB2YXIoLS1zYXBCYWNrZ3JvdW5kQ29sb3IpOyBoZWlnaHQ6IDQ1MHB4O1wiPlxuICAgIDwhLS0gcGxheWdyb3VuZC1mb2xkLWVuZCAtLT5cblxuICAgIDx1aTUtYnV0dG9uIGlkPVwiZGlhbG9nT3BlbmVyXCI-T3BlbiBEaWFsb2c8L3VpNS1idXR0b24-XG5cbiAgICA8dWk1LWRpYWxvZyBpZD1cImRpYWxvZ1wiPlxuICAgICAgPGRpdiBpZD1cImNvbnRlbnRcIj5cbiAgICAgICAgRGlhbG9nIENvbnRlbnRcbiAgICAgIDwvZGl2PlxuICAgICAgPHVpNS1iYXIgc2xvdD1cImZvb3RlclwiPlxuICAgICAgICA8dWk1LWJ1dHRvbiBzbG90PVwiZW5kQ29udGVudFwiIGlkPVwiY2xvc2UtYnV0dG9uXCI-Q2xvc2U8L3VpNS1idXR0b24-XG4gICAgICA8L3VpNS1iYXI-XG4gICAgPC91aTUtZGlhbG9nPlxuXG4gICAgPCEtLSBwbGF5Z3JvdW5kLWZvbGQgLS0-XG4gICAgPHNjcmlwdCB0eXBlPVwibW9kdWxlXCIgc3JjPVwibWFpbi5qc1wiPjwvc2NyaXB0PlxuPC9ib2R5PlxuXG48L2h0bWw-XG48IS0tIHBsYXlncm91bmQtZm9sZC1lbmQgLS0-XG4ifSwibWFpbi50c3giOnsibmFtZSI6Im1haW4udHN4IiwiY29udGVudCI6Ii8qIHBsYXlncm91bmQtaGlkZSAqL1xuaW1wb3J0IFwiLi9wbGF5Z3JvdW5kLXN1cHBvcnQuanNcIjtcbi8qIHBsYXlncm91bmQtaGlkZS1lbmQgKi9cbmltcG9ydCBVSTVFbGVtZW50IGZyb20gXCJAdWk1L3dlYmNvbXBvbmVudHMtYmFzZS9kaXN0L1VJNUVsZW1lbnQuanNcIjtcbmltcG9ydCBqc3hSZW5kZXJlciBmcm9tIFwiQHVpNS93ZWJjb21wb25lbnRzLWJhc2UvZGlzdC9yZW5kZXJlci9Kc3hSZW5kZXJlci5qc1wiO1xuaW1wb3J0IHsgY3VzdG9tRWxlbWVudCwgcHJvcGVydHkgfSBmcm9tIFwiQHVpNS93ZWJjb21wb25lbnRzLWJhc2UvZGlzdC9kZWNvcmF0b3JzLmpzXCI7XG5cbkBjdXN0b21FbGVtZW50KHtcbiAgdGFnOiBcIm15LWVsZW1lbnRcIixcbiAgcmVuZGVyZXI6IGpzeFJlbmRlcmVyLFxufSlcbmV4cG9ydCBjbGFzcyBNeUVsZW1lbnQgZXh0ZW5kcyBVSTVFbGVtZW50IHtcbiAgQHByb3BlcnR5KClcbiAgbmFtZT86IHN0cmluZztcblxuICByZW5kZXIoKSB7XG4gICAgcmV0dXJuIChcbiAgICAgIDxkaXY-XG4gICAgICAgICAgSGVsbG8sIHt0aGlzLm5hbWUgfHwgXCJXb3JsZFwifSFcbiAgICAgIDwvZGl2PlxuICAgIClcbiAgfVxuXG4gIHN0YXRpYyBzdHlsZXMgPSBgZGl2IHtcbiAgICAgIHBhZGRpbmc6IDFyZW07XG4gICAgICBjb2xvcjogIzMzNGVmZjtcbiAgfWA7XG59XG5cbk15RWxlbWVudC5kZWZpbmUoKTtcbiJ9LCJtYWluLmpzIjp7Im5hbWUiOiJtYWluLmpzIiwiY29udGVudCI6Ii8qIHBsYXlncm91bmQtaGlkZSAqL1xuaW1wb3J0IFwiLi9wbGF5Z3JvdW5kLXN1cHBvcnQuanNcIjtcbi8qIHBsYXlncm91bmQtaGlkZS1lbmQgKi9cbmltcG9ydCBcIkB1aTUvd2ViY29tcG9uZW50cy9kaXN0L0RpYWxvZy5qc1wiO1xuaW1wb3J0IFwiQHVpNS93ZWJjb21wb25lbnRzL2Rpc3QvQnV0dG9uLmpzXCI7XG5pbXBvcnQgXCJAdWk1L3dlYmNvbXBvbmVudHMvZGlzdC9CYXIuanNcIjtcbmltcG9ydCBcIkB1aTUvd2ViY29tcG9uZW50cy9kaXN0L1Rvb2xiYXJCdXR0b24uanNcIjtcbmltcG9ydCBcIkB1aTUvd2ViY29tcG9uZW50cy9kaXN0L1BvcG92ZXIuanNcIjsgXG5cblxudmFyIGRpYWxvZ09wZW5lciA9IGRvY3VtZW50LmdldEVsZW1lbnRCeUlkKFwiZGlhbG9nT3BlbmVyXCIpO1xudmFyIGRpYWxvZyA9IGRvY3VtZW50LmdldEVsZW1lbnRCeUlkKFwiZGlhbG9nXCIpO1xudmFyIGNsb3NlQnV0dG9uID0gZG9jdW1lbnQuZ2V0RWxlbWVudEJ5SWQoXCJjbG9zZS1idXR0b25cIik7XG5cblxuZGlhbG9nT3BlbmVyLmFkZEV2ZW50TGlzdGVuZXIoXCJjbGlja1wiLCAoKSA9PiB7XG4gICAgZGlhbG9nLm9wZW4gPSB0cnVlO1xufSk7XG5cblxuY2xvc2VCdXR0b24uYWRkRXZlbnRMaXN0ZW5lcihcImNsaWNrXCIsICgpID0-IHtcbiAgICBkaWFsb2cub3BlbiA9IGZhbHNlOyBcbn0pO1xuXG5cbmRpYWxvZy5hZGRFdmVudExpc3RlbmVyKFwiYmVmb3JlLWNsb3NlXCIsIChldmVudCkgPT4ge1xuICBpZiAoIWNvbmZpcm0oXCJBcmUgeW91IHN1cmU_XCIpKSB7XG4gICAgZXZlbnQucHJldmVudERlZmF1bHQoKTtcbiAgfVxufSk7In0sIm1haW4uY3NzIjp7Im5hbWUiOiJtYWluLmNzcyIsImNvbnRlbnQiOiJcblt1aTUtZGlhbG9nXTo6cGFydChjb250ZW50KSB7XG4gIHBhZGRpbmc6IDA7XG59In19

Steps to Reproduce

  1. Open Dialog
  2. Click 'Close' button
  3. Click to cancel the closing

Log Output, Stack Trace or Screenshots

No response

Priority

Medium

UI5 Web Components Version

2.0+

Browser

Chrome, Edge, Firefox, Safari

Operating System

No response

Additional Context

No response

Organization

No response

Declaration

  • I’m not disclosing any internal or sensitive information.
@adrian-bobev adrian-bobev added the bug This issue is a bug in the code label Jan 9, 2025
@unazko
Copy link
Contributor

unazko commented Jan 9, 2025

Hello @adrian-bobev,

I've added the following code into main.js from the sample:

dialog.addEventListener("before-close", (event) => {
    event.preventDefault();
});

and then executed the following in the console and the dialog was closed:

dialog.open = false

I wasn't able to reproduce the issue and for some reason the sample with the provided URL doesn't load for me.
Could you provide another sample on which this issue could be tested?

Best regards,
Boyan

@adrian-bobev
Copy link
Member Author

adrian-bobev commented Jan 9, 2025

Hello @unazko,

Simply go to the "Application" tab in Chrome DevTools and clear the site data. After that do a hard reload of the page. After doing this, the sample should open.

The real issue arises when you have a button inside the dialog that's meant to close it. If the click handler for this button sets open = false, it won't actually close the dialog.

@unazko
Copy link
Contributor

unazko commented Jan 9, 2025

Hello @SAP/ui5-webcomponents-topic-rd,

The behavior is as described by @adrian-bobev with the provided steps.
Could you please evaluate if this is an issue with the component.

Best regards,
Boyan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug in the code Medium Prio TOPIC RD
Projects
Status: New Issues
Development

No branches or pull requests

2 participants