vaadin / flow-components

Java counterpart of Vaadin Web Components
82 stars 63 forks source link

Dialog listener should check if event is default prevented before sending it to server. #6241

Open abdullahtellioglu opened 2 months ago

abdullahtellioglu commented 2 months ago

Description

I added vaadin-overlay-outside-click event listener to prevent the dialog from being closed in Copilot. However, event.preventDefault() does not work because it is not checked in

 private void registerClientCloseHandler() {
        //@formatter:off
        getElement().executeJs("const listener = (e) => {"
                + "  if (e.type == 'vaadin-overlay-escape-press' && !this.noCloseOnEsc ||"
                + "      e.type == 'vaadin-overlay-outside-click' && !this.noCloseOnOutsideClick) {"
                + "    e.preventDefault();"
                + "    this.$server.handleClientClose();"
                + "  }"
                + "};"
                + "this.$.overlay.addEventListener('vaadin-overlay-outside-click', listener);"
                + "this.$.overlay.addEventListener('vaadin-overlay-escape-press', listener);");
        //@formatter:on
    }

https://github.com/vaadin/flow-components/assets/100126447/3a04fbf2-2c1c-4269-944d-aa9fab7ce75b

Expected outcome

Event should not be sent to the server if it is defaultPrevented. Adding e.defaultPrevented condition to listener would solve the issue

Minimal reproducible example

// view

        Dialog dialog = new Dialog("Dialog ");
        dialog.setCloseOnOutsideClick(true);

        TextField textField = new TextField("Hello");
        dialog.add(textField);

        Button closeBtn = new Button("Hide dialog");
        dialog.add(closeBtn);
        dialog.open();

//in client

document.documentElement.addEventListener('vaadin-overlay-outside-click', (e: Event) => {e.preventDefault();} , {
      capture: true,
    });

Steps to reproduce

Environment

Vaadin version(s): 24.4.0.alpha23 OS:

Browsers

Issue is not browser related

vursen commented 2 months ago

Would stopImmediatePropagation() work for you as a workaround?

abdullahtellioglu commented 2 months ago

It works, thanks

vursen commented 2 months ago

It's a bit unclear what value the proposed check would provide for Flow users, since they are generally expected to control whether the dialog should close from the server-side. If so, I'm concerned that the only purpose of this check would be to accommodate a specific Copilot scenario.

I'm now thinking that using stopImmediatePropagation() in the capture phase might actually be an appropriate solution here, as it ensures that no vaadin-overlay-outside-click listeners are be executed at all. This approach would also work for grid-pro which exits edit mode on vaadin-overlay-outside-click and does not check for preventDefault():

https://github.com/vaadin/web-components/blob/178d0e0f144b788ee0f4431f8d535f92d59c75cb/packages/grid-pro/src/vaadin-grid-pro-inline-editing-mixin.js#L324-L325