Button's setDisabledOnClick(true) has an enabling side effect when receiving click events due to pointer-events: auto (instead of none)

[enver-haase]

Description of the bug

Any button that has setDisabledOnCLick(true) could possibly wrongly set a button to be enabled due to this code snippet:

function disableOnClickListener({
  currentTarget: E
}) {
  E.disabled = E.hasAttribute("disableOnClick")
}

As one can see, the code assumes it is only executed when the button is enabled. This does not have to be the case, e.g. when setting pointer-events: auto then the click listener is executed too -- and it has the (wrong) side effect of enabling a button that was not enabled before (and should stay disabled).

better:

if ( E.hasAttribute("disableOnClick" ) {
  E.disabled = true;
}

After all, the better code knows there was a click and it knows it should disable upon a click. There is no side effect when the click happens and the button was not enabled. I consider this to be more robust.

Expected behavior

Button should not surprisingly enable itself.

Minimal reproducible example

Set the pointer-events: auto for the disabled button.

[disabled] {
  cursor: default;
  pointer-events: auto;
}

Versions

• Vaadin / Flow version: 24.3
• Java version: 17
• OS version: n/a
• Browser version (if applicable): Chrome latest / Firefox latest
• Application Server (if applicable): n/a
• IDE (if applicable): n/a

[enver-haase]

Button disableButton = new Button("Don't disable on click");
disableButton.setEnabled(false);
disableButton.setDisableOnClick(true);
disableButton.setDisableOnClick(false);
add(disableButton);

[web-padawan]

when setting pointer-events: auto then the click listener is executed too

Why would anyone set pointer-events: auto on the disabled button? It's intnetionally set to none by the theme styles.

[enver-haase]

Oh, that I don't know. I was guessing my customer would have a legit use case. Querying it, hold on...

[enver-haase]

They say they'd need the "pointer-events: auto" for several things such as focus or click handling even for disabled items. While they did not provide me with exact, concrete example, they do insist they know what they're doing and that there is a good reason for that.

[web-padawan]

they'd need the "pointer-events: auto" for several things such as focus or click handling even for disabled items.

OK, this is already tracked at vaadin/web-components#4585. Moved the issue to the Flow components repo.

[enver-haase]

@mshabarov @web-padawan Please note the BFP label.

[web-padawan]

This issue has been added to the backlog priority queue for further investigation.

[web-padawan]

While the disableOnClickListener needs to be updated indeed, please note that the fact that click event can be fired by the disabled button is considered a bug - see vaadin/web-components#7334.

[vursen]

They say they'd need the "pointer-events: auto" for several things such as focus or click handling even for disabled items. While they did not provide me with exact, concrete example, they do insist they know what they're doing and that there is a good reason for that.

I wonder if a different attribute could be used instead of disabled to achieve the desired effect? It still seems like the disabled state is being used inappropriately, and there actually might be a better alternative.