feat: implement disable on click for menu item (#6815)

* feat: implement disable on click for menu item

* fix: defer setting disabled

* test: add unit tests for controller

* test: fix test class

* fix: check if element is there on disableonclick init and cleanup

* test: fix button tests

* Update vaadin-flow-components-shared-parent/vaadin-flow-components-base/src/main/java/com/vaadin/flow/component/shared/internal/DisableOnClickController.java

Co-authored-by: Sergey Vinogradov <[email protected]>

* Update vaadin-flow-components-shared-parent/vaadin-flow-components-base/src/main/java/com/vaadin/flow/component/shared/internal/DisableOnClickController.java

Co-authored-by: Sergey Vinogradov <[email protected]>

* Update vaadin-flow-components-shared-parent/vaadin-flow-components-base/src/main/java/com/vaadin/flow/component/shared/internal/DisableOnClickController.java

Co-authored-by: Sergey Vinogradov <[email protected]>

* refactor: update disable on click init logic and add tests

* refactor: rename and return early when initializing

* test: reuse elements and simplify disabled asseertion

---------

Co-authored-by: Sergey Vinogradov <[email protected]>

Author: ugur-vaadin

vaadin-button-flow-parent/vaadin-button-flow-integration-tests/src/main/java/com/vaadin/flow/component/button/tests/ButtonView.java

@@ -49,7 +49,7 @@ public ButtonView() {
         createButtonsWithTabIndex();
         createDisabledButton();
         createButtonWithDisableOnClick();
-        createButtonWithDisableOnClickThatEnablesInSameRoundtrip();
+        createButtonWithDisableOnClickThatEnablesInSameRoundTrip();
         createButtonWithDisableOnClickThatIsHidden();
         createButtonWithDisableOnClickAndPointerEventsAuto();
         addVariantsFeature();
@@ -207,20 +207,6 @@ private void createButtonWithDisableOnClick() {
         });
         disableOnClickButton.setDisableOnClick(true);
 
-        Button temporarilyDisabledButton = new Button(
-                "Temporarily disabled button", event -> {
-                    try {
-                        // Blocking the user from clicking the button
-                        // multiple times, due to a long running request that
-                        // is not running asynchronously.
-                        Thread.sleep(1500);
-                        event.getSource().setEnabled(true);
-                    } catch (InterruptedException e) {
-                        Thread.currentThread().interrupt();
-                    }
-                });
-        temporarilyDisabledButton.setDisableOnClick(true);
-
         final Div disabledMessage = new Div();
         disabledMessage.setId("disabled-message");
 
@@ -240,7 +226,7 @@ private void createButtonWithDisableOnClick() {
         toggle.setId("toggle-button");
 
         addCard("Button disabled on click", disableOnClickButton, enable,
-                toggle, disabledMessage, new Div(temporarilyDisabledButton));
+                toggle, disabledMessage);
 
         disableOnClickButton.addClickListener(evt -> disabledMessage
                 .setText("Button " + evt.getSource().getText()
@@ -249,14 +235,23 @@ private void createButtonWithDisableOnClick() {
                         + runCount.incrementAndGet() + " clicks"));
 
         disableOnClickButton.setId("disable-on-click-button");
-        temporarilyDisabledButton.setId("temporarily-disabled-button");
         enable.setId("enable-button");
     }
 
-    private void createButtonWithDisableOnClickThatEnablesInSameRoundtrip() {
+    private void createButtonWithDisableOnClickThatEnablesInSameRoundTrip() {
         Button button = new Button(
-                "Disabled on click and re-enabled in same roundtrip", event -> {
-                    event.getSource().setEnabled(true);
+                "Disabled on click and re-enabled in same round-trip",
+                event -> {
+                    try {
+                        // Blocking the user from clicking the button
+                        // multiple times, due to a long-running request that
+                        // is not running asynchronously.
+                        Thread.sleep(500);
+                    } catch (InterruptedException e) {
+                        throw new RuntimeException(e);
+                    } finally {
+                        event.getSource().setEnabled(true);
+                    }
                 });
         button.setDisableOnClick(true);
         button.setId("disable-on-click-re-enable-button");

vaadin-button-flow-parent/vaadin-button-flow-integration-tests/src/test/java/com/vaadin/flow/component/button/tests/ButtonIT.java

@@ -68,7 +68,7 @@ public void textContains() {
 
         buttonElements = $(ButtonElement.class).withTextContaining("button")
                 .all();
-        Assert.assertEquals(4, buttonElements.size());
+        Assert.assertEquals(3, buttonElements.size());
 
         buttonElements = $(ButtonElement.class)
                 .withTextContaining("nonexistent").all();
@@ -83,11 +83,11 @@ public void textBiPredicate() {
 
         buttonElements = $(ButtonElement.class)
                 .withText("button", String::endsWith).all();
-        Assert.assertEquals(3, buttonElements.size());
+        Assert.assertEquals(2, buttonElements.size());
 
         buttonElements = $(ButtonElement.class)
                 .withText("button", ButtonIT::containsIgnoreCase).all();
-        Assert.assertEquals(5, buttonElements.size());
+        Assert.assertEquals(4, buttonElements.size());
     }
 
     @Test
@@ -109,7 +109,7 @@ public void captionContains() {
 
         buttonElements = $(ButtonElement.class).withCaptionContaining("button")
                 .all();
-        Assert.assertEquals(4, buttonElements.size());
+        Assert.assertEquals(3, buttonElements.size());
 
         buttonElements = $(ButtonElement.class)
                 .withCaptionContaining("nonexistent").all();
@@ -290,29 +290,8 @@ public void clickDisableOnClickButton_newClickNotRegistered() {
         }
     }
 
-    @Test // https://github.com/vaadin/vaadin-button-flow/issues/115
-    public void disableButtonOnClick_canBeEnabled() {
-        getCommandExecutor().disableWaitForVaadin();
-        ButtonElement button = $(ButtonElement.class)
-                .id("temporarily-disabled-button");
-
-        for (int i = 0; i < 3; i++) {
-            button.click();
-
-            Assert.assertFalse("button should be disabled", button.isEnabled());
-            waitUntil(ExpectedConditions.elementToBeClickable(
-                    $(ButtonElement.class).id("temporarily-disabled-button")),
-                    2000);
-
-            Assert.assertTrue("button should be enabled again",
-                    button.isEnabled());
-        }
-
-        getCommandExecutor().enableWaitForVaadin();
-    }
-
     @Test
-    public void removeDisabled_buttonWorksNormally() {
+    public void removeDisableOnClick_buttonWorksNormally() {
         WebElement button = layout
                 .findElement(By.id("disable-on-click-button"));
         Assert.assertTrue(
@@ -347,17 +326,18 @@ public void removeDisabled_buttonWorksNormally() {
     }
 
     @Test
-    public void disableOnClick_enableInSameRoundtrip_clientSideButtonIsEnabled() {
-        WebElement button = layout
-                .findElement(By.id("disable-on-click-re-enable-button"));
+    public void disableOnClick_enableInSameRoundTrip_clientSideButtonIsEnabled() {
+        var itemId = "disable-on-click-re-enable-button";
+        getCommandExecutor().disableWaitForVaadin();
+        var button = findElement(By.id(itemId));
         for (int i = 0; i < 3; i++) {
-            Boolean disabled = (Boolean) executeScript(
-                    "arguments[0].click(); return arguments[0].disabled",
-                    button);
-            Assert.assertTrue(disabled);
-
-            waitUntil(ExpectedConditions.elementToBeClickable(button));
+            button.click();
+            getCommandExecutor().getDriver()
+                    .executeAsyncScript("requestAnimationFrame(arguments[0])");
+            Assert.assertFalse(button.isEnabled());
+            waitUntil(driver -> button.isEnabled());
         }
+        getCommandExecutor().enableWaitForVaadin();
     }
 
     @Test

vaadin-button-flow-parent/vaadin-button-flow-integration-tests/src/test/java/com/vaadin/flow/component/button/tests/DetachReattachDisableOnClickButtonIT.java

@@ -50,10 +50,11 @@ private void clickDisableOnClickButton(ButtonElement disableOnClickButton) {
         disableOnClickButton.click();
 
         // Check 'Disable on click' button is disabled
-        assertDisableOnClickButtonDisabled(disableOnClickButton);
+        waitUntil(driver -> !$(ButtonElement.class).id("disable-on-click")
+                .isEnabled(), 2);
 
         waitUntil(ExpectedConditions.elementToBeClickable(
-                $(ButtonElement.class).id("disable-on-click")), 2000);
+                $(ButtonElement.class).id("disable-on-click")), 2);
 
         // Check 'Disable on click' button is enabled again
         assertDisableOnClickButtonEnabled(disableOnClickButton);
@@ -76,15 +77,15 @@ public void testDetachingAndReattachingShouldKeepDisabledOnClick() {
         removeFromViewButton.click();
 
         waitUntil(ExpectedConditions
-                .numberOfElementsToBe(By.id("disable-on-click"), 0), 2000);
+                .numberOfElementsToBe(By.id("disable-on-click"), 0), 2);
 
         // Re-attach 'Disable on click" button
         ButtonElement addToViewButton = $(ButtonElement.class)
                 .id("add-to-view");
         addToViewButton.click();
 
         waitUntil(ExpectedConditions
-                .numberOfElementsToBe(By.id("disable-on-click"), 1), 2000);
+                .numberOfElementsToBe(By.id("disable-on-click"), 1), 2);
 
         disableOnClickButton = getDisableOnClickButton();
 

vaadin-button-flow-parent/vaadin-button-flow/src/main/java/com/vaadin/flow/component/button/Button.java

@@ -20,7 +20,6 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
-import com.vaadin.flow.component.AttachEvent;
 import com.vaadin.flow.component.ClickEvent;
 import com.vaadin.flow.component.ClickNotifier;
 import com.vaadin.flow.component.Component;
@@ -35,13 +34,12 @@
 import com.vaadin.flow.component.dependency.JsModule;
 import com.vaadin.flow.component.dependency.NpmPackage;
 import com.vaadin.flow.component.html.Image;
-import com.vaadin.flow.component.page.PendingJavaScriptResult;
 import com.vaadin.flow.component.shared.HasPrefix;
 import com.vaadin.flow.component.shared.HasSuffix;
 import com.vaadin.flow.component.shared.HasThemeVariant;
 import com.vaadin.flow.component.shared.HasTooltip;
+import com.vaadin.flow.component.shared.internal.DisableOnClickController;
 import com.vaadin.flow.dom.Element;
-import com.vaadin.flow.shared.Registration;
 
 /**
  * The Button component allows users to perform actions. It comes in several
@@ -54,24 +52,15 @@
 @JsModule("@vaadin/polymer-legacy-adapter/style-modules.js")
 @NpmPackage(value = "@vaadin/button", version = "24.6.0-alpha8")
 @JsModule("@vaadin/button/src/vaadin-button.js")
-@JsModule("./buttonFunctions.js")
 public class Button extends Component
         implements ClickNotifier<Button>, Focusable<Button>, HasAriaLabel,
         HasEnabled, HasPrefix, HasSize, HasStyle, HasSuffix, HasText,
         HasThemeVariant<ButtonVariant>, HasTooltip {
 
     private Component iconComponent;
     private boolean iconAfterText;
-    private boolean disableOnClick = false;
-    private PendingJavaScriptResult initDisableOnClick;
-
-    // Register immediately as first listener
-    private final Registration disableListener = addClickListener(
-            buttonClickEvent -> {
-                if (disableOnClick) {
-                    setEnabled(false);
-                }
-            });
+    private final DisableOnClickController<Button> disableOnClickController = new DisableOnClickController<>(
+            this);
 
     /**
      * Default constructor. Creates an empty button.
@@ -330,55 +319,32 @@ public boolean isAutofocus() {
     }
 
     /**
-     * Set the button so that it is disabled on click.
+     * Sets whether the button should be disabled when clicked.
      * <p>
-     * Enabling the button needs to happen from the server.
+     * When set to {@code true}, the button will be immediately disabled on the
+     * client-side when clicked, preventing further clicks until re-enabled from
+     * the server-side.
      *
      * @param disableOnClick
-     *            true to disable button immediately when clicked
+     *            whether the button should be disabled when clicked
      */
     public void setDisableOnClick(boolean disableOnClick) {
-        this.disableOnClick = disableOnClick;
-        if (disableOnClick) {
-            getElement().setAttribute("disableOnClick", "true");
-            initDisableOnClick();
-        } else {
-            getElement().removeAttribute("disableOnClick");
-        }
+        disableOnClickController.setDisableOnClick(disableOnClick);
     }
 
     /**
-     * Get if button is set to be disabled on click.
+     * Gets whether the button is set to be disabled when clicked.
      *
-     * @return {@code true} if button gets disabled on click, else {@code false}
+     * @return whether button is set to be disabled on click
      */
     public boolean isDisableOnClick() {
-        return disableOnClick;
-    }
-
-    /**
-     * Initialize client side disabling so disabled if immediate on click even
-     * if server-side handling takes some time.
-     */
-    private void initDisableOnClick() {
-        if (initDisableOnClick == null) {
-            initDisableOnClick = getElement().executeJs(
-                    "window.Vaadin.Flow.button.initDisableOnClick($0)");
-            getElement().getNode()
-                    .runWhenAttached(ui -> ui.beforeClientResponse(this,
-                            executionContext -> this.initDisableOnClick = null));
-        }
+        return disableOnClickController.isDisableOnClick();
     }
 
     @Override
     public void setEnabled(boolean enabled) {
         Focusable.super.setEnabled(enabled);
-        // Force updating the disabled state on the client
-        // When using disable on click, the client side will immediately
-        // run JS to disable the button. If the button is then disabled and
-        // re-enabled during the same round trip, Flow will not detect any
-        // changes and the client side button would not be enabled again.
-        getElement().executeJs("this.disabled = $0", !enabled);
+        disableOnClickController.onSetEnabled(enabled);
     }
 
     private void updateIconSlot() {
@@ -444,11 +410,4 @@ private void updateThemeAttribute() {
             getThemeNames().remove("icon");
         }
     }
-
-    @Override
-    protected void onAttach(AttachEvent attachEvent) {
-        if (isDisableOnClick()) {
-            initDisableOnClick();
-        }
-    }
 }

vaadin-button-flow-parent/vaadin-button-flow/src/main/resources/META-INF/resources/frontend/buttonFunctions.js

@@ -15,24 +15,19 @@
  */
 package com.vaadin.flow.component.button.tests;
 
-import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
 
 import org.junit.Assert;
 import org.junit.Test;
-import org.mockito.Mockito;
 
 import com.vaadin.flow.component.HasAriaLabel;
 import com.vaadin.flow.component.Text;
 import com.vaadin.flow.component.button.Button;
 import com.vaadin.flow.component.button.ButtonVariant;
 import com.vaadin.flow.component.icon.Icon;
 import com.vaadin.flow.component.icon.VaadinIcon;
-import com.vaadin.flow.component.internal.PendingJavaScriptInvocation;
 import com.vaadin.flow.component.shared.HasTooltip;
-import com.vaadin.flow.dom.Element;
-import com.vaadin.flow.internal.StateNode;
 
 public class ButtonTest {
 
@@ -342,28 +337,6 @@ public void setAriaLabel() {
         Assert.assertEquals("Aria label", button.getAriaLabel().get());
     }
 
-    @Test
-    public void initDisableOnClick_onlyCalledOnceForSeverRoundtrip() {
-        final Element element = Mockito.mock(Element.class);
-        StateNode node = new StateNode();
-        button = Mockito.spy(Button.class);
-
-        Mockito.when(button.getElement()).thenReturn(element);
-
-        Mockito.when(element.executeJs(Mockito.anyString()))
-                .thenReturn(Mockito.mock(PendingJavaScriptInvocation.class));
-        Mockito.when(element.getComponent()).thenReturn(Optional.of(button));
-        Mockito.when(element.getParent()).thenReturn(null);
-        Mockito.when(element.getNode()).thenReturn(node);
-
-        button.setDisableOnClick(true);
-        button.setDisableOnClick(false);
-        button.setDisableOnClick(true);
-
-        Mockito.verify(element, Mockito.times(1))
-                .executeJs("window.Vaadin.Flow.button.initDisableOnClick($0)");
-    }
-
     private void assertButtonHasThemeAttribute(String theme) {
         Assert.assertTrue("Expected " + theme + " to be in the theme attribute",
                 button.getThemeNames().contains(theme));
@@ -388,9 +361,4 @@ private void assertIconAfterText() {
         Assert.assertTrue(button.isIconAfterText());
         Assert.assertEquals("suffix", icon.getElement().getAttribute("slot"));
     }
-
-    private Element getButtonChild(int index) {
-        return button.getElement().getChild(index);
-    }
-
 }