Bugfix/click shortcut timing

[mstahv]

Description

This patch fixes timing issues that may cause events to happen in wrong order on the server side.

Fixes # (issue)

Type of change

• [ ] Bugfix
• [ ] Feature

Checklist

• [ ] I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
• [ ] I have added a description following the guideline.
• [ ] The issue is created in the corresponding repository and I have referenced it.
• [ ] I have added tests to ensure my change is effective and works as intended.
• [ ] New and existing tests are passing locally with my change.
• [ ] I have performed self-review and corrected misspellings.

Additional for Feature type of change

• [ ] Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

[github-actions[bot]]

Unit Test Results

   906 files  +1     906 suites  +1   53m 48s :stopwatch: - 4m 51s 5 897 tests +1  5 853 :heavy_check_mark: +1  43 :zzz:  - 1  1 :x: +1  6 115 runs   - 3  6 065 :heavy_check_mark:  - 3  49 :zzz:  - 1  1 :x: +1 

For more details on these failures, see this check.

Results for commit 0789ad80. ± Comparison against base commit 1cacea3e.

:recycle: This comment has been updated with latest results.

[mstahv]

@Legioth I added a test view to the branch but the fix seems non-functional to me, would you spend a moment and see if it works for you?

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
0.0% Duplication

[mcollovati]

@Legioth I added a test view to the branch but the fix seems non-functional to me, would you spend a moment and see if it works for you?

Just tried it and it seems to work for me. I entered some text in the textfield and pressed CTRL+S and I can see the paragraph contents are immediately in sync.

I'll try to add an integration test for the use case

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[mcollovati]

Current implementation executes sendCommand twice, first in Debouncer.flushAll() and then immediately after the previous call. We can somehow prevent to invoke sendCommand.accept(null) if already done in Debouncer.flushAll().

But there is a failure in DomEventFilterIT.debounce. If we clear the debouncers at every flush, the expect sequence of events is no more respected.

[Legioth]

I suspect DomEventFilterIT.debounce is no longer a valid test if we change the definition of debouncing to always trigger immediately in case some other event occurs. It might be necessary to refactor it to test each feature (basic debounce, leading debounce and throttle) separately instead of the current implementation that tries to be clever by testing all three features at once.

[Legioth]

We might also want to make sure ON_CHANGE also works in a sensible way when shortcut keys are triggered. My assumption is that it would just automatically do that based on what's already changing, but it might be good to check it explicitly.

[wfischlein]

Here's the code for testing whether the ON_CHANGE behavior is working:

		TextField tf = new TextField();
		Button button = new Button("Bound to enter", event -> {
			Notification.show(tf.getValue());
		});
		button.addClickShortcut(Key.ENTER);

Found:

• write some text into the textfield
• press enter: see empty notification

Expected: Notifocation shows entered trext

[mstahv]

This fix wont fix the issue if ONCHANGE is in use, only when used with the lazy mode. And it can't be fixed without some totally different kind of solution. IIRC in Vaadin 6,7,8 we always tracked the currently focused input field and somehow synchronized its value always before getting other events through to the server side.

This fix still probably fixes other timing issues though. Even with a trick described above, there could still be lazy events in the queue from other fields still...

[Legioth]

I realized that there might be an alternative approach to this issue that would also help make ON_CHANGE work as expected (with some adjustments to how ON_CHANGE is implemented internally). I haven't fleshed out all the details yet, but the high-level idea would be to add a new small core communication feature similar to @Delayed(lastOnly = true) that we have in Vaadin 7 and 8.

With the debounce feature, each individual event would either add a "delayed" event or update a previously added one that is still pending, whereas the debounce trigger condition would also lead to flushing all pending delayed events. Correspondingly, ON_CHANGE would be changed (or we would introduce a new mode) to add or update a delayed event every time the input event is fired and then the change event would trigger a flush.

[mstahv]

Sounds like you are describing the Vaadin client->server event/message queue that I know 😁 As I'm not familiar with the current codebase, I don't even have a hunch that how hard that would be to implement. Somebody who actually understands this code before and has worked with it earlier would be much more efficient to tackle this issue (or cluster of issues actually).

But two comments:

• The new mode would/could bring back the previous style buffering for normal input fields, so that only a field with a listener or e.g. button click listener would cause a server visit. Big thumb 👍 for that. Although it don't affect performance that much in reality, would help marketing wise when tackling a common concern of "chattiness" caused by our architecture.
• Iterative improvement allowed. I find the current issues quite critical. And there are constantly signs that it is not "just Matti nagging". Understanding the root of these issues is just so hard that we are not getting good bug reports/reactions. These are the core functionalities that Flow should tackle well, otherwise creating good experience (through quality components) is impossible. If this current change fixes at least some bugs, I'd take that in right away and then provide more advanced fixes (with potential new API) later.

[Legioth]

I was actually not proposing the mode that only fields with explicit listeners would trigger round trips, but that's indeed a potential incremental step that we could take if we build the low-level support for specific events to only be enqueued but not trigger a flush.

[github-actions[bot]]

Test Results

   929 files  +1     929 suites  +1   49m 10s :stopwatch: - 3m 29s 5 833 tests +5  5 798 :heavy_check_mark: +5  35 :zzz: ±0  0 :x: ±0  6 087 runs  +9  6 045 :heavy_check_mark: +9  42 :zzz: ±0  0 :x: ±0 

Results for commit 488c18ad. ± Comparison against base commit 430c56b4.

:recycle: This comment has been updated with latest results.

[mcollovati]

@mstahv @Legioth I'm working on the tests for this fix and I have a couple of question about the behavior we expect after this change is applied. I don't know that much about Debouncer its events ordering, so I think you can help me better understanging how it should work.

I summarize my understandings, that may be entirely wrong

• for ValueChangeMode ON_CHANGE and ON_BLUR nothing should change; the shortcut listener should not get the actual value in the input field because it will be sent to the server when the events occur
• ValueChangeMode EAGER works fine, since every value change is immediately sent to the server and when the shortcut listener is executed it gets the actual input value
• ValueChangeMode LAZY and TIMEOUT is where the patch has effect: currently the shortcut listener does not get the value in the input field, but with the fix value is sent to the server before the shortcut listener is executed.

However, with this patch, the DomEventFilterIT.debounce test is failing. The test set listeners for the different debounce phases, and it expects the following execution order.

• LEADING on the first key press
• INTERMEDIATE after another key press, but before debounce period ends
• TRAILING after the debounce period

What we get now is that after the second key press, the LEADING listener is invoked again.

So, the question is: is it wrong to have LEADING listener executed twice (and we should fix this in this PR), or are the test assumptions that does not hold anymore (and we should fix the test)?

[mstahv]

A lot of time has passed since I tried to worked on this so not sure if I remember the details and what works and what not, but let's try...

With LAZY mode, we should send only reasonable amount of events from client to server. Still, when any other event is triggered by user, the latest value should be synchronised. So basically on each text change in the client we need to queue the most recent value and make sure that is handled in the server before other event handlers gets triggered.

Getting the first events in lazy mode should preferably be configurable, but that should be tackled in an other issue.

With ON_CHANGE/ON_BLUR it can be bit more academic, but in reality it would be very weird if when whatever other event happens, on the same or any other component, if the already typed in text is not available when calling field.getValue(). Thus I'd suggest to push those events(or technically latest text contents) to some queue as well on each keypress. Firing the actual value change events is a separate discussion, but less critical.

Whatever the "value change mode" is, it is more relevant that getValue returns the actual content of the text field than when the value change event itself (from the textfield) happens. If people use value change listener in the text field to trigger some action, there usually isn't a problem today. The problem currently is that when a user triggers an action through some other event (like a click shortcut, which is very typical), the content of the text field might be something totally different than expected. And this is not just UX, this might cause severe unintended damages in applications built with Vaadin. Consider somebody firing Marc through a HR management app, when they actually typed in Marco 🤓

Old fart remembering how things were better in the past: In earlier versions (V7 and 8 🤔) we more purists on these value change events. They happened when they actually happened (in the browser). For getting "intermediate" text content between value change events, textfield had getLastKnownTextContent method and there was a separate TextChangeEvent that could be used for example when doing modern filtering screens with LAZY mode, which was set implicitly when a TextChangeListener was registered. There it was all quite clear how it should work, even though for end users it was bit more complex. Or maybe it actually wasn't as they didn't need to think about any value change modes at all🤷‍♂️

[mcollovati]

@mstahv Thanks for clarification! The current fix works for LAZY and TIMEOUT value change mode, and it should at least fix #14090 (needs some code cleanup) I propose to focus this PR only on the above issue use case, and create a new ticket to handle issues with other value change mode types.

[Legioth]

So, the question is: is it wrong to have LEADING listener executed twice (and we should fix this in this PR), or are the test assumptions that does not hold anymore (and we should fix the test)?

LEADING and TRAILING should always come in pairs, and INTERMEDIATE should never happen "outside" such as pair. If there's some side effect that triggers a new LEADING before without a TRAILING in between, then that's a bug.

At the same time, I would urge some caution with the existing GWT test since the test setup might be short circuiting some of the real-life conditions. My gut feeling is that the test needs to be split up to test the different scenarios in isolation rather than trying to be "efficient" by going through all cases in a single test method.

[mcollovati]

I would urge some caution with the existing GWT test

Do you mean the tests src/test-gwt/java folder? Or also the Testbench integration tests like DomEventFilterIT?

[mcollovati]

LEADING and TRAILING should always come in pairs, and INTERMEDIATE should never happen "outside" such as pair. If there's some side effect that triggers a new LEADING before without a TRAILING in between, then that's a bug.

With the latest changes, it works as described; no additional LEADING is triggered

[Legioth]

Do you mean the tests src/test-gwt/java folder?

I have a faint memory of such assumptions in the test-gwt folder. I don't remember if it was also somewhere else.

[mcollovati]

I have a faint memory of such assumptions in the test-gwt folder

I'll check, thanks

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[vaadin-bot]

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