Skip to content

Commit ad591d1

Browse files
sissbrueckerclaude
andauthored
fix: respect disabled parent when syncing client disabled state (#10023)
## Description `DisableOnClickController` forces the client-side `disabled` property from `setEnabled`, because Flow does not send a change when a component is disabled and re-enabled in the same round trip. It used the value passed to `setEnabled`, so enabling a button under a disabled parent cleared `disabled` on the client while the server kept rejecting clicks. - Changed `DisableOnClickController.onSetEnabled()` to schedule the client update with `beforeClientResponse` and read the effective enabled state at that point, so a disabled parent keeps the component disabled on the client - Coalesced repeated `setEnabled` calls in one round trip into a single client update - Deferred the update until the component is attached instead of queueing a JavaScript call per `setEnabled` call while detached - Removed the unused `enabled` parameter from `onSetEnabled()` and updated `Button` and `MenuItemBase` - Added unit tests for the disabled parent, same round trip, and detached cases ## Type of change - Bugfix --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
1 parent ff28dde commit ad591d1

4 files changed

Lines changed: 135 additions & 13 deletions

File tree

  • vaadin-button-flow-parent/vaadin-button-flow/src/main/java/com/vaadin/flow/component/button
  • vaadin-context-menu-flow-parent/vaadin-context-menu-flow/src/main/java/com/vaadin/flow/component/contextmenu
  • vaadin-flow-components-shared-parent/vaadin-flow-components-base/src

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -475,7 +475,7 @@ public boolean isDisableOnClick() {
475475
@Override
476476
public void setEnabled(boolean enabled) {
477477
Focusable.super.setEnabled(enabled);
478-
disableOnClickController.onSetEnabled(enabled);
478+
disableOnClickController.onSetEnabled();
479479
}
480480

481481
/**

vaadin-context-menu-flow-parent/vaadin-context-menu-flow/src/main/java/com/vaadin/flow/component/contextmenu/MenuItemBase.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ public boolean isDisableOnClick() {
280280
@Override
281281
public void setEnabled(boolean enabled) {
282282
HasComponents.super.setEnabled(enabled);
283-
disableOnClickController.onSetEnabled(enabled);
283+
disableOnClickController.onSetEnabled();
284284
}
285285

286286
/**

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

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ public class DisableOnClickController<C extends Component & HasEnabled>
4646

4747
private final C component;
4848
private boolean disableOnClick = false;
49+
private boolean clientUpdateScheduled = false;
4950

5051
/**
5152
* Creates a new controller for the given component.
@@ -57,6 +58,8 @@ public class DisableOnClickController<C extends Component & HasEnabled>
5758
public DisableOnClickController(C component) {
5859
this.component = Objects.requireNonNull(component);
5960

61+
component.addDetachListener((event) -> clientUpdateScheduled = false);
62+
6063
ComponentUtil.addListener(component, ClickEvent.class,
6164
(ComponentEventListener) (event -> {
6265
if (isDisableOnClick()) {
@@ -95,18 +98,29 @@ public boolean isDisableOnClick() {
9598

9699
/**
97100
* Forces the client-side component's {@code disabled} property to be
98-
* updated immediately.
101+
* updated before the response is sent to the client, so that it matches the
102+
* component's effective enabled state, including whether any parent is
103+
* disabled.
99104
* <p>
100105
* This method should be called from the component's
101-
* {@link HasEnabled#setEnabled} method.
102-
*
103-
* @param enabled
104-
* value to set
106+
* {@link HasEnabled#setEnabled} method, after the enabled state has been
107+
* updated.
105108
*/
106-
public void onSetEnabled(boolean enabled) {
107-
// If the component is then disabled and re-enabled during the same
108-
// round trip, Flow will not detect any changes and the client side
109-
// component would not be enabled again.
110-
component.getElement().executeJs("this.disabled = $0", !enabled);
109+
public void onSetEnabled() {
110+
// If the component is disabled and re-enabled during the same round
111+
// trip, Flow will not detect any changes and the client side component
112+
// would not be enabled again. The property is updated before the
113+
// response so that the effective state at that point is used, for
114+
// example when a parent is disabled or enabled in the same round trip.
115+
if (clientUpdateScheduled) {
116+
return;
117+
}
118+
clientUpdateScheduled = true;
119+
component.getElement().getNode().runWhenAttached(
120+
ui -> ui.beforeClientResponse(component, context -> {
121+
clientUpdateScheduled = false;
122+
component.getElement().executeJs("this.disabled = $0",
123+
!component.isEnabled());
124+
}));
111125
}
112126
}

vaadin-flow-components-shared-parent/vaadin-flow-components-base/src/test/java/com/vaadin/flow/component/shared/DisableOnClickControllerTest.java

Lines changed: 109 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,26 +15,39 @@
1515
*/
1616
package com.vaadin.flow.component.shared;
1717

18+
import java.util.List;
1819
import java.util.concurrent.atomic.AtomicBoolean;
1920

2021
import org.junit.jupiter.api.Assertions;
2122
import org.junit.jupiter.api.BeforeEach;
2223
import org.junit.jupiter.api.Test;
24+
import org.junit.jupiter.api.extension.RegisterExtension;
2325

2426
import com.vaadin.flow.component.ClickEvent;
2527
import com.vaadin.flow.component.ClickNotifier;
2628
import com.vaadin.flow.component.Component;
29+
import com.vaadin.flow.component.HasComponents;
2730
import com.vaadin.flow.component.HasEnabled;
2831
import com.vaadin.flow.component.Tag;
32+
import com.vaadin.flow.component.internal.PendingJavaScriptInvocation;
2933
import com.vaadin.flow.component.shared.internal.DisableOnClickController;
34+
import com.vaadin.tests.MockUIExtension;
3035

3136
class DisableOnClickControllerTest {
3237

38+
@RegisterExtension
39+
final MockUIExtension ui = new MockUIExtension();
40+
41+
private TestParent parent;
3342
private TestComponent component;
3443

3544
@BeforeEach
3645
void setup() {
46+
parent = new TestParent();
3747
component = new TestComponent();
48+
parent.add(component);
49+
ui.add(parent);
50+
ui.fakeClientCommunication();
3851
}
3952

4053
@Test
@@ -88,6 +101,101 @@ void setDisableOnClick_clickRevertsDisabled_componentIsEnabled() {
88101
Assertions.assertTrue(component.isEnabled());
89102
}
90103

104+
@Test
105+
void setEnabled_clientSideDisabledPropertyUpdated() {
106+
component.setEnabled(false);
107+
Assertions.assertEquals(List.of(true), dumpClientSideDisabledValues());
108+
109+
component.setEnabled(true);
110+
Assertions.assertEquals(List.of(false), dumpClientSideDisabledValues());
111+
}
112+
113+
@Test
114+
void setEnabled_noChange_clientSideDisabledPropertyUpdated() {
115+
component.setEnabled(true);
116+
Assertions.assertEquals(List.of(false), dumpClientSideDisabledValues());
117+
}
118+
119+
@Test
120+
void disabledAndEnabledInSameRoundTrip_clientSideEnabledOnce() {
121+
component.setEnabled(false);
122+
component.setEnabled(true);
123+
Assertions.assertEquals(List.of(false), dumpClientSideDisabledValues());
124+
}
125+
126+
@Test
127+
void click_clickListenerEnablesComponent_clientSideEnabled() {
128+
component.addClickListener(event -> event.getSource().setEnabled(true));
129+
component.setDisableOnClick(true);
130+
component.click();
131+
Assertions.assertEquals(List.of(false), dumpClientSideDisabledValues());
132+
}
133+
134+
@Test
135+
void parentDisabled_setEnabled_clientSideStaysDisabled() {
136+
parent.setEnabled(false);
137+
ui.fakeClientCommunication();
138+
139+
component.setEnabled(true);
140+
Assertions.assertEquals(List.of(true), dumpClientSideDisabledValues());
141+
}
142+
143+
@Test
144+
void parentDisabled_setEnabledAndParentEnabledInSameRoundTrip_clientSideEnabled() {
145+
parent.setEnabled(false);
146+
ui.fakeClientCommunication();
147+
148+
component.setEnabled(true);
149+
parent.setEnabled(true);
150+
Assertions.assertEquals(List.of(false), dumpClientSideDisabledValues());
151+
}
152+
153+
@Test
154+
void parentDisabledInSameRoundTrip_setEnabled_clientSideDisabled() {
155+
component.setEnabled(true);
156+
parent.setEnabled(false);
157+
Assertions.assertEquals(List.of(true), dumpClientSideDisabledValues());
158+
}
159+
160+
@Test
161+
void detached_setEnabled_clientSideUpdatedAfterAttach() {
162+
parent.remove(component);
163+
ui.fakeClientCommunication();
164+
165+
component.setEnabled(false);
166+
Assertions.assertEquals(List.of(), dumpClientSideDisabledValues());
167+
168+
parent.add(component);
169+
Assertions.assertEquals(List.of(true), dumpClientSideDisabledValues());
170+
}
171+
172+
@Test
173+
void detachedBeforeResponse_attachedToAnotherUi_setEnabled_clientSideDisabledPropertyUpdated() {
174+
component.setEnabled(false);
175+
component.getElement().removeFromTree();
176+
ui.fakeClientCommunication();
177+
178+
ui.replaceUI();
179+
ui.add(component);
180+
ui.fakeClientCommunication();
181+
182+
component.setEnabled(true);
183+
Assertions.assertEquals(List.of(false), dumpClientSideDisabledValues());
184+
}
185+
186+
private List<Object> dumpClientSideDisabledValues() {
187+
return ui.dumpPendingJavaScriptInvocations().stream()
188+
.map(PendingJavaScriptInvocation::getInvocation)
189+
.filter(invocation -> invocation.getExpression()
190+
.contains("this.disabled = $0"))
191+
.map(invocation -> invocation.getParameters().get(0)).toList();
192+
}
193+
194+
@Tag("test-parent")
195+
private static class TestParent extends Component
196+
implements HasComponents, HasEnabled {
197+
}
198+
91199
@Tag("test")
92200
private static class TestComponent extends Component
93201
implements HasEnabled, ClickNotifier<TestComponent> {
@@ -112,7 +220,7 @@ public void click() {
112220
@Override
113221
public void setEnabled(boolean enabled) {
114222
HasEnabled.super.setEnabled(enabled);
115-
disableOnClickController.onSetEnabled(enabled);
223+
disableOnClickController.onSetEnabled();
116224
}
117225
}
118226
}

0 commit comments

Comments
 (0)