bugfix: fixes and optimizes everything regarding event debouncing :-)

[mstahv]

Description

The event order when using debouncing sends events in wrong order. This was tried to be fixed in #14035 but apparently only issues with the core TextField was fixed (or then this was again regressed in the regression fix #15615) 🤷‍♂️

Using this trivial Element API usage like below, type in for example asdfg to the input field -> g-event fires before k-event and there are multiple server visits (there should be just one).

    Element el = new Element("input");
    el.executeJs("""
            const el = this;
            var id = 0;
            this.addEventListener('keydown', function(event) {
                id++;
                const ke = new Event("k-event");
                ke.id = id;
                el.dispatchEvent(ke);
            
                if(event.key == 'g') {
                    const ge = new Event("g-event");
                    ge.id = id;
                    el.dispatchEvent(ge);
                }
            });
    """);
    DomListenerRegistration keyreg = el.addEventListener("k-event", e -> {
        System.out.println(LocalTime.now() + " k-event" + e.getEventData().getNumber("event.id") + " phase: " + e.getPhase());
    });
    keyreg.addEventData("event.id");
    keyreg.debounce(3000);

    // Note, with multiple debounce phases, things get really weird...

    // I'm interested of g-event eagerly, so I don't debounce it
    // BUT, AFAIK, key-event should ALWAYS come in before!!
    // But it is not... :(
    DomListenerRegistration greg = el.addEventListener("g-event", e -> {
        System.out.println(LocalTime.now() + " g-event " + e.getEventData().getNumber("event.id"));
    });
    greg.addEventData("event.id");

    getElement().appendChild(el);

While investigating the this issue there was also a dozen of other related isseus, incomplete docs and missing integration test.

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.

[sonatype-lift]

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

[github-actions]

Test Results

1 001 files  ±0  1 001 suites  ±0   58m 50s ⏱️ - 6m 13s
6 389 tests ±0  6 348 ✔️ ±0  41 💤 ±0  0 ❌ ±0 
6 642 runs   - 4  6 594 ✔️  - 4  48 💤 ±0  0 ❌ ±0 

Results for commit c0a8745. ± Comparison against base commit a49a588.

♻️ This comment has been updated with latest results.

[mcollovati]

Just some typos

[mcollovati]

The overall change looks good, and the tests are more reliable.
However, since the change involves a behavior change (), we need to:

• update documentation (https://vaadin.com/docs/latest/create-ui/creating-components/events#limiting-event-frequency)
• update DebouncePhase javadoc

[mstahv]

Converted back to draft. After the latest changes, obsolete requests are back or my manual test fails somehow 👎

[mstahv]

Nope, it is fine, "just" other slightly related issues (#17430, #17429) causing obsolete server visit.

[mstahv]

Checked the reference docs. Nothing urgent needs to change there. Of course more hands on examples of everything would be nice. And maybe mentioning that combining all modes is weird and may cause double calls for the same event 🤷‍♂️

[mshabarov]

@mstahv With this patch I see the same behaviour as previously without it: "g" event is triggered on the server first, and "k" goes after it:

14:34:24.148306 g-event 5.0
14:34:27.798952 k-event6.0 phase: TRAILING

How did you test it?

[mstahv]

Regression in the branch 🤔 Make sure you are not causing another keydown (like shortcut to change focus to IDE) event if you are using the original case in the issue description. I tested the original case with a separate project, but then the changed the implementation still while making the IT test suit more complete 🤷‍♂️ But it still don't seem to contain test with two different event types.

Would have been easier to check two weeks ago 😤

[mstahv]

Added case to DomEventFilterIT/View locally testing with Safari all seems to work fine, but can't these days be sure as the front-end bundle things are rather complex 🤓

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
12 Code Smells

No Coverage information
0.0% Duplication

[mshabarov]

The same behaviour was observed because of pre-compiled dev bundle was used, which did not contain these changes. With forcing the frontend build, these changes are now shown and seems to work as expected: one request to server is sent and events go in order.
Let's apply this patch and check Flow Components tests later.
I propose to include this only to 24.2 until enough time passes by, then we can pick it to other branches.

Marco is on vacation and seems his comments have been addressed.

[vaadin-bot]

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