Button click shortcut listener does not recognize value change when using control key

[stefanuebe]

A simple use case: a form with some text fields, a rich text editor and a save button. The user changes stuff and wants to submit the form by using the ctrl + enter key.

The text fields are used as they are (value change mode ON_CHANGE), bound with a simple binder. To achieve the "on enter", we set a click shortcut with ENTER and CONTROL.

Now, when filling all fields and using the mouse to click the button, everything works fine.
When filling the fields and using the shortcut, the currently focused field does not get submitted.
Using a self defined shortcut listener and the button method clickInClient does not solve the problem

The problem seems to be the usage of the control key. When setting the short cut listener to ENTER only, it works finely (except for that the rich text editor focus needs to handle additional checkings)

Setting the value change mode to EAGER makes it work as a workaround.

[stefanuebe]

Tested with Vaadin 14, RC1 in bower mode on a Windows server and Windows client with chrome.

package org.vaadin.issue;

import com.vaadin.flow.component.AttachEvent;
import com.vaadin.flow.component.Key;
import com.vaadin.flow.component.button.Button;
import com.vaadin.flow.component.dialog.Dialog;
import com.vaadin.flow.component.html.Span;
import com.vaadin.flow.component.orderedlayout.VerticalLayout;
import com.vaadin.flow.component.textfield.TextArea;
import com.vaadin.flow.component.textfield.TextField;
import com.vaadin.flow.data.binder.Binder;
import com.vaadin.flow.router.Route;

@Route(value = "button")
public class ButtonChangeView extends VerticalLayout {

    private SimpleBean simpleBean = new SimpleBean();

    @Override
    protected void onAttach(AttachEvent attachEvent) {

        VerticalLayout info = new VerticalLayout(new Span("Results"));
        Button open = new Button("Open", event -> {
            Dialog dialog = new Dialog();

            TextField aField = new TextField("A");
            TextField bField = new TextField("B");
            TextArea cField = new TextArea("C");

            Binder<SimpleBean> binder = new Binder<>();
            binder.bind(aField, SimpleBean::getA, SimpleBean::setA);
            binder.bind(bField, SimpleBean::getB, SimpleBean::setB);
            binder.bind(cField, SimpleBean::getC, SimpleBean::setC);

            binder.readBean(simpleBean);

            Button save = new Button("Save", event1 -> {
                if (binder.hasChanges() && binder.writeBeanIfValid(simpleBean)) {
                    info.add(new Span(simpleBean.toString()));
                    dialog.close();
                }
            });

            save.addClickShortcut(Key.ENTER);

            dialog.add(aField, bField, cField, save);
            dialog.open();
        });

        add(open, info);
    }

    private static class SimpleBean {
        private String a;
        private String b;
        private String c;

        public String getA() {
            return a;
        }

        public void setA(String a) {
            this.a = a;
        }

        public String getB() {
            return b;
        }

        public void setB(String b) {
            this.b = b;
        }

        public String getC() {
            return c;
        }

        public void setC(String c) {
            this.c = c;
        }

        @Override
        public String toString() {
            return "SimpleBean{" +
                    "a='" + a + '\'' +
                    ", b='" + b + '\'' +
                    ", c='" + c + '\'' +
                    '}';
        }
    }
}

[denis-anisimov]

I don't see this is a bug at all.

What I've got from your description is:
value change mode ON_CHANGE.

Is it correct ?

Then it may not work as you have described since the currently focused field doesn't notify the
server about changes until the value is changed.
And the value is not changed: it's changed/updated e.g. when you press ENTER.
Also I think changing the focus will change the value.

Ctrl + Enter doesn't change the value. So all other values are changed except the value for currently
focused field.
That's why it's not submitted.

You may not use value change mode ON_CHANGE if you want this kind of functionality.

I'm closing this issue as invalid.
Please reopen if you think this is incorrect/disagree.

[stefanuebe]

I disagree with that.

From a Java developer point of view the shortcut listener calls click on the button, thus I expect the components to behave the same way, as if the user would manually move his mouse to the button and click it (especially when I use the method "clickInClient").

Not doing it this way, but telling the developer "for that you have to use EAGER on all your fields" is a bad DX in my view: Either you cannot use shortcuts or you have a flood of network requests for each character that is typed in - especially when the form is not only 3-4 simple textfields but consisting of text areas or other editor components, which will send their whole content to the server all the time.

I could live with a scenario, where the normal button click + on change takes the current state from the server. But when having the special clickInClient I would expect at least, that this method behaves like user interaction (calling focus on the button and then click).

[denis-anisimov]

I don't get your points.

Do you understand what value change mode ON_CHANGE means ?

Please explain me your understanding.

In your scenario there is no value change. So the behavior is expected.
There is no need to use EAGER.
Anything meaningful except ON_CHANGE .

Will you expect the required behavior If you set TIMEOUT with a long timeout ?
I guess no, so why do you expect it using ON_CHANGE if there is no any change.

Well, I don't want to argue about this. I'm not going to fix this anyway.
I'm just trying to understand whether this is a bug at all or not.

I think this is not a bug.
The behavior is a bit confusing. So let it be an enhancement.

[stefanuebe]

Some simple use cases to show my issue:

We have a form with a server side binder, read some values from the bean. The user changes now all fields (by using the keyboard and typing new characters in each field).

Variants of the use case are:

1. the button has been assigned a shortcut listener listening on Enter
2. the button has been assigned a shortcut listener listening on Ctrl+Enter, since there is some textarea, which is not really usable with Variant 1

Variant 1, usecase 1:
The user changes the values and clicks actively on the mouse. The last changed field loses its focus and sends the value to the server. The server has all updated values and writes the bean correctly. Success.

Variant 1, usecase 2:
The user changes the values and uses the Enter key. The last changed field does not lose its focus but the browser seem to know that using enter should somehow do the same trick and sends the value to the server. The server has all updated values and writes the bean correctly. Success.

Variant 2, usecase 1:
same as Variant 1, uc1,

Variant 2, usecase 2:
The user changes the values and uses Ctrl+Enter key. The last changed field does not lose its focus. The browser does not know what that means, thus it does not send the value to the server. The server has one value not updated writes the bean incorrectly. Fail

Outcome
Now, from a frontend only perspective, I agree with you, that there has been no value change, since the field did not lose its focus nor did a simple enter event happen. But, from the perspective of the user and the Java developer, who do not have an idea of such browser related technical internal stuff, this simply is an issue inside the framework.

I also agree, that the normal "click" method cannot check the client side, if there might be some unsent, but from the user's perspective changed value, that needs to be sent, but, the clickInClient method should (otherwise it makes absolutely no sense to have this method).

So, to sum up everything what the request for fix (or enhance) is:
Using Button.clickInClient should set the focus on the button first before clicking it, to simulate the real click, which includes a auto focus of the button, since the user clicks on it. This should - in my plain understanding - also fire the change event of the text field first. Everyone is happy, since the form works as it should.

[probert94]

I agree with @stefanuebe, adding a click shortcut, I expect it to work the same as if I manually click the Button.
However, it would also be nice to have a solution for other ShortcutListeners as well.
In my case, I have some "commands", which are hidden inside an overflow-menu, thus they are not visible on the UI. A click shortcut would not be triggered in this case, and I cannot call clickInClient manually. However, I want to make sure, that the value of the currently focused element is synchronized with the server, before my listener gets fired.
Using an other ValueChangeMode is, in my opinion, not a suitable solution, ON_CHANGE is exactly what I want, but I also want it to sync the value when some special shortcuts are triggered.
Maybe the ShortcutRegistration could offer an API for this case? Something like syncFocusedFieldValueWithServer?

[SebastianKuehnau]

I can only agree with the messages from @Springrbua and @stefanuebe. The usability of the web page would be much better if an input value from the field could be transferred via shortcut even if the field still has the focus.

As a workaround you can deselect the focus using client side code:

getElement().executeJs("document.activeElement.blur()").then(jsonValue -> {
     Notification.show(textField.getValue());
});

but since the Vaadin classes has Shortcuts and ShortcutRegistration are completely encapsulated, you can't hook any code here either and that reduces the DX experience here.

[hfazai]

@stefanuebe I have TimePicker, DateTimePicker and DatePicker in my form and they dont have change mode method. Any idea how to fix this in my case?

[Avec112]

It is sept 2021 and a Shortcut still cannot trigger value change regardless which KeyModifier (tried ctrl and shift)
If I change the text in my field and click the save button with my mouse the value change is registered.
If I have a Shortcut saveButton.addShortcut(Key.ENTER, KeyModifier.CONTROL) the value change is not registered and therefor it fails.
Same goes for Shortcuts.addShortcutListener(saveButton, saveButton::click, Key.ENTER, KeyModifier.CONTROL) that also do not trigger the value change.

I think this must be addressed.

[Avec112]

An update.

If I do a setValue("some new value") (a hack) I manage to trigger value changed....but

// value changed listener
binder.addValueChangeListener(e -> {
  if(e.isFromClient()) {// can only get passed this when clicking form save button with mouse
    hasChanged = true;
  }
}

// My shortcut
Shortcuts.addShorcutListener(saveButton, {
  var value = randomField.getValue(); // hold whatever value
  randomField.setValue(""); // this triggers value changed event first time
  randomField.setValue(value); // triggers value changed a second time
  saveButton.click();
},
Key.ENTER, KeyModifier.CONTROL);

while the explicit setValue("some new value") triggers the value changed listener it will not let me get passed if(e.isFromClient()) {...} this way.

[hfazai]

@Avec112 Did you try with setting value change mode to TIMEOUT?

randomField.setValueChangeMode(ValueChangeMode.TIMEOUT)

[Avec112]

I had not but I have now. There is no difference.

[probert94]

@hfazai With TIMEOUT it will probably only work, if you stop typing for long enough.
If the user uses the shortcut directly after he stops typing, the value on the server will probably be updated after the shortcut listener gets triggered

[Avec112]

I see but I will not try to circumvent this issue anymore. I see this as an clear bug unless Vaadin can convince me otherwise. I still think this is a bug and not an issue which should be labeled enhancement. It can't be so that a mouse click on a button the binder will fully populate the bean based on any changes done while trigging the same button event with button.click() it will not populate the changes.

Conclusion : Shortcuts cannot be used related to binding changed values.

[hfazai]

@hfazai With TIMEOUT it will probably only work, if you stop typing for long enough. If the user uses the shortcut directly after he stops typing, the value on the server will probably be updated after the shortcut listener gets triggered

EAGER mode may work for you, but it makes more communication between client and server. Le mer. 22 sept. 2021 1:18 PM, Avec112 ***@***.***> a écrit :

…

I see but I will not try to circumvent this issue anymore. I see this as an clear bug unless Vaadin can convince me otherwise. I still think this is a bug and not an issue which should be labeled enhancement. It can't be so that a mouse click on a button the binder will fully populate the bean based on any changes done while trigging the same button event with button.click() it will not populate the changes. - Conclusion* : Shortcuts cannot be used related to binding changed values. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5959 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANS5JQIAE5A3MN6MSTHHMGDUDHCRPANCNFSM4H2OQQFQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[Frettman]

Just encountered this issue. It's really not the behavior one would expect. The (maybe) naive expectation is that any user command (like clicking a button or triggering a shorcut) flushes all pending changes to inputs to the server beforehand. For button clicks it just seems to work as a happy accident because the input will lose its focus (and send its new value) before the button can be clicked.
I tried calling the combination of button.focus(); button.clickInClient() in the shortcut, but that didn't seem to work either.

Judging from what I know, I'm not even sure this can be fixed transparently or generically, as it involves very component-specific behavior. I'd be happy with a workaround that simply imitates clicking an actual button, like I tried above.

[Frettman]

After some more experimentation, this is at least somewhat related to the following issue: vaadin/flow-components#530
Basically, the ValueChangeModes TIMEOUT and LAZY do not send their value earlier, when the input field loses focus. So when the user is too fast or the timeout too long, even when actually clicking on a button the handler might not have the current value of the input.
However, this also means that with ValueChangeMode ON_CHANGE (the default), the following can be used as a workaround at least for a few cases:

      TextField textField = new TextField();
      Button button = new Button("Send");

      Shortcuts.addShortcutListener(textField, event -> {
        textField.blur();
        button.clickInClient();
      }, Key.ENTER).listenOn(textField);

      button.addClickListener(event -> {
        Notification.show(textField.getValue());
      });

button.clickInClient() forces another roundtrip to the client which gives textField.blur() the chance to send its value before the click listener is executed. You'd think that button.focus() should also work as long as the input loses its focus, but it didn't for me.
This might work in OP's use case with multiple text fields, if blur() is called for all of them, but I haven't tested it. (Also listenOn(textField) would have to be replaced with a containing layout.)

[probert94]

My current workaround is similar to the one posted by @Frettman but in my case, I don't have a Button and the focussed TextField is unknown.
Basically I register a ShortcutEventListener which first executes JavaScript to force the synchronisation of the value and then calls the real ShortcutEventListener. The JavaScript searches for the currently focussed element, which might be inside a shadow dom, and then calls blur() and focus() to force a value change, while keeping the focus.
Here is the code:

public Registration registerShortcut(Component lifecycleOwner, ShortcutEventListener listener, Key key, KeyModifier... keyModifiers) {
  return Shortcuts.addShortcutListener(lifecycleOwner, patchListener(lifecycleOwner, listener), key, keyModifiers);
}
private ShortcutEventListener patchListener(Component lifecycleOwner, ShortcutEventListener listener) {
  return event -> {
    lifecycleOwner.getElement().executeJs("" +
      "let fe = document.activeElement;" +
      "while (fe.shadowRoot) fe = fe.shadowRoot.activeElement;" +		// Search activeElement in ShadowRoot
      "if (fe) {" +
	  "fe.blur();" +
	  "fe.focus();" +
      "}"	
    ).then(r -> listener.onShortcut(event));
  }
}

The drawback of this method is, that it needs another roundtrip, which is something I would like to prevent.
My suggestion therefor is to add a possibility to add custom JavaScript to the ShortcutRegistration. Something like that is already possible with the filter in the DomListenerRegistration, the ShortcutRegistration even "abuses" the filter to call event.preventDefault and event.stopPropagation.
So if the ShortcutRegistration would offer a similar API so that you are able to execute client side code when a Shortcut is triggered.

[vaadin-bot]

This ticket/PR has been released with Vaadin 24.3.0.alpha1 and is also targeting the upcoming stable 24.3.0 version.

[Frettman]

Maybe it's a silly question, but couldn't or shouldn't this ResetFocusOnActiveElement flag be active by default for all click shortcuts? Would there be any negative impact for doing so?
The decision when to use the flag seems simple enough and boil down to "If the click action needs any input whatsoever, then activate it." But it still seems like it's easily missed by devs that there is this decision to make in the first place; and it's not easily discovered in case of issues (caused by outdated values).
So, like I said, one idea would be to enable this by default for click shortcuts. Another would be to extend addClickShortcut(...) with a boolean for ResetFocusOnActiveElement. That would make it at least more prominent, as right I feel it's a bit to hidden.

By the way, has this any practical use for focus shortcuts (since it's available in ShortcutRegistration which is used for both shortcut types)?

[mshabarov]

Maybe it's a silly question, but couldn't or shouldn't this ResetFocusOnActiveElement flag be active by default for all click shortcuts? Would there be any negative impact for doing so?

It used to be a default in the beginning, but then a couple of regressions bite us (e.g. this vaadin/flow-components#5733) and we reverted it to be opt-it, so not a default.

[mshabarov]

Another would be to extend addClickShortcut(...) with a boolean for ResetFocusOnActiveElement. That would make it at least more prominent, as right I feel it's a bit to hidden.

Sounds like a good idea to me.
@tltv what do you think about the idea to add one more overload with the "reset focus" flag ?

By the way, has this any practical use for focus shortcuts (since it's available in ShortcutRegistration which is used for both shortcut types)?

The patch was mainly for click shortcuts. I recall no direct usages for focus shortcuts.

[tltv]

You can do it now with addClickShortcut(Key.ENTER).resetFocusOnActiveElement() which is already quite clean solution. New boolean flags in the API tempt to make code harder to read (e.g. addClickShortcut(Key.ENTER, true).
How about short note about resetFocusOnActiveElement() in the JavaDoc of addClickShortcut, would that make it easier to find this new feature?

[Frettman]

It used to be a default in the beginning, but then a couple of regressions bite us (e.g. this vaadin/flow-components#5733) and we reverted it to be opt-it, so not a default.

That issue just reads like a duplicate of this one. What were those regressions? If you don't completely trust this feature (yet), I'd like to know what I should be looking out for in terms of potential side effects :)

[Frettman]

You can do it now with addClickShortcut(Key.ENTER).resetFocusOnActiveElement() which is already quite clean solution. New boolean flags in the API tempt to make code harder to read (e.g. addClickShortcut(Key.ENTER, true). How about short note about resetFocusOnActiveElement() in the JavaDoc of addClickShortcut, would that make it easier to find this new feature?

Yes, usability is fine. My concern was finding this feature (and ideally being aware of it before you encounter the problem) in the first place. Mentioning this in addClickShortcut would definitely help.
After thinking about it, I'm kind of on the fence about the overload myself. Ideally this would just work transparently out of the box (and hopefully it will at some point). And while the overload would help with visibility, I'd be reluctant to add something which is basically a workaround to a major API like ClickNotifier.

[tltv]

That issue just reads like a duplicate of this one. What were those regressions? If you don't completely trust this feature (yet), I'd like to know what I should be looking out for in terms of potential side effects :)

There was regression with use case where click shortcut key like 'x' was also handled by TextField/Input to print out letter in an input field, leading to every second character following a press of the click shortcut key getting cleared. Ticket for more details: #18048

[Frettman]

I see. Thanks for the information. So it seems rather safe to use in typical use cases (like ENTER to submit a form).

[vaadin-bot]

This ticket/PR has been released with Vaadin 23.4.0.alpha1 and is also targeting the upcoming stable 23.4.0 version.