feat:allow explicitly reveiving certain dom events for inert elements

[mstahv]

Added API to allow bypassing inert check for DOM events.

Fixes #19116

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.

[mstahv]

There is bit ugly API changes, but luckily only for internal classes (marked properly with documentation). Some code smell, but don't have good ideas how to refactor as I don't understand all the details related to the implementation.

[mcollovati]

A comment after a quick and not in-depth look: do we really need to change handleNode() and fireEvent() to add the inert flag?
I mean, the flag seems to be used only by ElementListenerMap.fireEvent() and I wonder if we could get the same information from the event as we do for isEnabled (e.g. event.getSource().getNode().isInert()).
This way, we may probably be able to also remove the hard-coded instanceof EventRpcHandler in AbstractRpcInvocationHandler; then EventRpcHandler should override allowInert(...) to return true (similar to PublishedServerEventHandlerRpcHandler) since the check will be performed later in fireEvent.

I apologize in advance if the above statements are incorrect 😁

[knoobie]

I would follow Marco's suggestion if it works to reduce the possibility of any breaking change 😅

[mstahv]

The "breaking changes" are only in classes marked with "For internal use only. May be renamed or removed in a future release.", so I think those ought to be rather safe.

I spent quite a while looking into a way to get the right details, but I sure might have missed some opportunity due to the complexity of the code. I have an hour or two in car so I can give it an another look now, but my my true wish is that somebody who actaully understands this code would step in :-)

@mcollovati is there any kind of "developer docs" for this internals? Like some kind of brief overview or description of what these classes do or even just some class/sequence diagrams? Currently it is very hard to understand how the thing are supposed to work and why it is build in this way...

[mstahv]

Oh yes, the allowInter is an instance method, overriding that might help 👍

[mstahv]

If we can assume that this piece in AbstractRpcInvocationHandler:

StateNode node = ui.getInternals().getStateTree()
                .getNodeById(getNodeId(invocationJson));

... returns the same instance as event.getSource().getNode() in ElementListenerMap, then passing around the inert flag could be omitted as well. I have no idea, don't really even understand the purpose of StateNode 😬

[mstahv]

Refactored, the current state isn't tested in any ways though now...

[github-actions]

Test Results

1 099 files  ± 0  1 099 suites  ±0   1h 28m 37s ⏱️ + 3m 57s
6 947 tests + 1  6 898 ✅ + 1  49 💤 ±0  0 ❌ ±0 
7 304 runs  +23  7 243 ✅ +23  61 💤 ±0  0 ❌ ±0 

Results for commit 35b7a78. ± Comparison against base commit a07e2ad.

♻️ This comment has been updated with latest results.

[mstahv]

There is now a unit test IT will be there throught the resize event improvement. I think this is ready. Annotation support could be added later if needed, I don't need it.

[mcollovati]

If we can assume that this piece in AbstractRpcInvocationHandler: ...

The node get from UI in AbstractRpcInvocationHandler is the same used to create theElement object for the DomEvent, so event.getSource().getNode() returns the exactly same instance.

[mcollovati]

It looks like this change is breaking navigation on modal components (InertComponentIT.modalComponentAdded_routerLinkClicked_navigation).
Probably ElementListenerMap.fireEvent() should allow navigation events (BrowserNavigateEvent.EVENT_NAME and BrowserLeaveNavigationEvent.EVENT_NAME) regardless of the inert node flag, as AbstractRpcInvocationHandler.isNavigationInvocation() does.

[mcollovati]

@mstahv if you are busy, I can apply the changes and then continue the discussion

[mstahv]

@mcollovati Go ahead if you have these in IDE already! Otherwise I can open this again at some point when I can't get text out of my brain 😁

[mstahv]

Probably the whole isNavigationInvocation logic should be in that event specific implementation 🤔

[mstahv]

I'll spend a minute on this...

[mstahv]

Nah, if you have good ideas @mcollovati, go for it. My idea was to use the new API for those navigation events, but they are some "special cases" and it started to look complicated...

[tepi]

Looks good to me! @mcollovati did you have any remaining comments about this one?

[sonarcloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[mcollovati]

Looks good to me too. There's still a pending discussion about the message log level, but if we want to differentiate between dev and prod, we can do it in another PR

[mstahv]

Excellent! I migth even be so bold that I would throw illegal state exception, at least in dev mode.

[vaadin-bot]

This ticket/PR has been released with Vaadin 24.4.0.