Fix embedding: allow removing virtual children

[pleku]

Fixes #6822

---

This change is 

[denis-anisimov]

Reviewed 4 of 4 files at r1.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

---

flow-server/src/main/java/com/vaadin/flow/dom/impl/AbstractNodeStateProvider.java, line 120 at r1 (raw file):

 if (child.isVirtualChild() && node.hasFeature(VirtualChildrenList.class)) {

For me personally it's confusing semantically.
There is no any single method which allows consider a virtual child element as a child (no any way to get it from parent except directly via VirtualChildrenList) so why there is only one method which allows to remove it ?
E.g. there is no getter for virtual child.

And we have a number of places where an element is checked whether it's a real child or not.
In every such place removeChild is not called for virtual child (since it's not a child).

I would may be avoid modifying removeChild API impl at all and use VirtualChildrenList feature directly when removal is needed.

Not blocking since I cannot invent obvious confusion with this method right now.
But I expect such confusion in the future.

---

flow-server/src/test/java/com/vaadin/flow/router/EventUtilTest.java, line 275 at r1 (raw file):

removingVirtualChildrenIsPossible(

EventUtilTest doesn't seem correct for such test method.

[pleku]

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

---

flow-server/src/main/java/com/vaadin/flow/dom/impl/AbstractNodeStateProvider.java, line 120 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

 if (child.isVirtualChild() && node.hasFeature(VirtualChildrenList.class)) {

For me personally it's confusing semantically.
There is no any single method which allows consider a virtual child element as a child (no any way to get it from parent except directly via VirtualChildrenList) so why there is only one method which allows to remove it ?
E.g. there is no getter for virtual child.

And we have a number of places where an element is checked whether it's a real child or not.
In every such place removeChild is not called for virtual child (since it's not a child).

I would may be avoid modifying removeChild API impl at all and use VirtualChildrenList feature directly when removal is needed.

Not blocking since I cannot invent obvious confusion with this method right now.
But I expect such confusion in the future.

I agree that semantically it should be kept separate, not supporting it here.

But since the design decision was already made that adding is visible in Node/Element level, I think it should be 1-1 for the API for removing those.
Hiding it in VirtualChildrenList could make the code just even more cumbersome to understand and maintain.

---

flow-server/src/test/java/com/vaadin/flow/router/EventUtilTest.java, line 275 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

removingVirtualChildrenIsPossible(

EventUtilTest doesn't seem correct for such test method.

Done (somehow I missed ElementTest)

[pleku]

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

---

flow-server/src/main/java/com/vaadin/flow/dom/Node.java, line 201 at r2 (raw file):

     * timeout.
     */
    public N removeVirtualChild(Element... children) {

Not sure how this method gets +1.0.0instead of +0.1.0 but for backporting this fix, this cannot be done as is

[denis-anisimov]

Reviewed 4 of 4 files at r2.
Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained

[denis-anisimov]

Reviewed 3 of 3 files at r3.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained