Disable sending updates to client for effectively non-visible nodes (#…

…15885) (#16819)

* Disable sending updates to client for effectively non-visible nodes

* Fix test: Always set tested component visible first; only assume node has changed if testvalue differs from original value

* Cleanup, and refactoring, in Element, StateNode, UIInternals classes + mvn formatter.

---------

Co-authored-by: czp13 <61667986+czp13@users.noreply.github.com>
Co-authored-by: Peter Czuczor <czuczor@gmail.com>

(cherry picked from commit eab4f09)

Co-authored-by: Teppo Kurki <teppo.kurki@vaadin.com>

flow-html-components/src/test/java/com/vaadin/flow/component/html/HtmlComponentSmokeTest.java

@@ -223,6 +223,8 @@ private static boolean isSpecialSetter(Method method) {
     }
 
     private static void testSetter(HtmlComponent instance, Method setter) {
+        instance.setVisible(true);
+
         Class<?> propertyType = setter.getParameterTypes()[0];
 
         Method getter = findGetter(setter);
@@ -253,16 +255,30 @@ private static void testSetter(HtmlComponent instance, Method setter) {
         StateNode elementNode = instance.getElement().getNode();
 
         try {
+            Object originalGetterValue = null;
+
+            try {
+                originalGetterValue = getter.invoke(instance);
+                if (isOptional) {
+                    originalGetterValue = ((Optional<?>) originalGetterValue)
+                            .orElse(null);
+                }
+            } catch (InvocationTargetException e) {
+                // Unable to retrieve original value, assuming null
+            }
+
             // Purge all pending changes
             elementNode.collectChanges(c -> {
             });
 
             setter.invoke(instance, testValue);
 
             // Might have to add a blacklist for this logic at some point
-            Assert.assertTrue(
-                    setter + " should update the underlying state node",
-                    hasPendingChanges(elementNode));
+            if (!testValue.equals(originalGetterValue)) {
+                Assert.assertTrue(
+                        setter + " should update the underlying state node",
+                        hasPendingChanges(elementNode));
+            }
 
             Object getterValue = getter.invoke(instance);
             if (isOptional) {

flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java

@@ -550,10 +550,17 @@ public void addJavaScriptInvocation(
             PendingJavaScriptInvocation invocation) {
         session.checkHasLock();
         pendingJsInvocations.add(invocation);
+
+        invocation.getOwner()
+                .addDetachListener(() -> pendingJsInvocations
+                        .removeIf(pendingInvocation -> pendingInvocation
+                                .equals(invocation)));
     }
 
     /**
-     * Gets all the pending JavaScript invocations and clears the queue.
+     * Gets all the pending JavaScript invocations that are ready to be sent to
+     * a client. Retains pending JavaScript invocations owned by invisible
+     * components in the queue.
      *
      * @return a list of pending JavaScript invocations
      */
@@ -564,13 +571,16 @@ public List<PendingJavaScriptInvocation> dumpPendingJavaScriptInvocations() {
             return Collections.emptyList();
         }
 
-        List<PendingJavaScriptInvocation> currentList = getPendingJavaScriptInvocations()
+        List<PendingJavaScriptInvocation> readyToSend = getPendingJavaScriptInvocations()
+                .filter(invocation -> invocation.getOwner().isVisible())
                 .peek(PendingJavaScriptInvocation::setSentToBrowser)
                 .collect(Collectors.toList());
 
-        pendingJsInvocations = new ArrayList<>();
+        pendingJsInvocations = getPendingJavaScriptInvocations()
+                .filter(invocation -> !invocation.getOwner().isVisible())
+                .collect(Collectors.toCollection(ArrayList::new));
 
-        return currentList;
+        return readyToSend;
     }
 
     /**

flow-server/src/main/java/com/vaadin/flow/dom/Element.java

@@ -16,6 +16,7 @@
 package com.vaadin.flow.dom;
 
 import java.io.Serializable;
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
@@ -1608,8 +1609,8 @@ public void callFunction(String functionName, Serializable... arguments) {
      * at the same time that {@link Page#executeJs(String, Serializable...)}
      * calls are invoked.
      * <p>
-     * If the element is not attached, the function call will be deferred until
-     * the element is attached.
+     * If the element is not attached or not visible, the function call will be
+     * deferred until the element is attached and visible.
      *
      * @see JsonCodec JsonCodec for supported argument types
      *
@@ -1704,6 +1705,9 @@ public void executeJavaScript(String expression,
      * <code>'prefix' + $0</code> instead of <code>'prefix$0'</code> and
      * <code>value[$0]</code> instead of <code>value.$0</code> since JavaScript
      * variables aren't evaluated inside strings or property names.
+     * <p>
+     * If the element is not attached or not visible, the function call will be
+     * deferred until the element is attached and visible.
      *
      * @param expression
      *            the JavaScript expression to invoke
@@ -1778,6 +1782,10 @@ public Optional<ShadowRoot> getShadowRoot() {
     /**
      * Sets the element visibility value.
      *
+     * Also executes pending javascript invocations, if their execution was
+     * requested while the element was not visible, and the element is now set
+     * as visible.
+     *
      * @param visible
      *            the element visibility value
      * @return this element

flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java

@@ -38,12 +38,14 @@
 
 import com.vaadin.flow.component.ComponentUtil;
 import com.vaadin.flow.component.UI;
+import com.vaadin.flow.component.internal.PendingJavaScriptInvocation;
 import com.vaadin.flow.function.SerializableConsumer;
 import com.vaadin.flow.internal.StateTree.BeforeClientResponseEntry;
 import com.vaadin.flow.internal.StateTree.ExecutionRegistration;
 import com.vaadin.flow.internal.change.NodeAttachChange;
 import com.vaadin.flow.internal.change.NodeChange;
 import com.vaadin.flow.internal.change.NodeDetachChange;
+import com.vaadin.flow.internal.nodefeature.ElementData;
 import com.vaadin.flow.internal.nodefeature.NodeFeature;
 import com.vaadin.flow.internal.nodefeature.NodeFeatureRegistry;
 import com.vaadin.flow.server.Command;
@@ -624,6 +626,15 @@ public void collectChanges(Consumer<NodeChange> collector) {
         if (!isAttached()) {
             return;
         }
+
+        if (isInitialChanges && !isVisible()) {
+            if (hasFeature(ElementData.class)) {
+                doCollectChanges(collector,
+                        Stream.of(getFeature(ElementData.class)));
+            }
+            return;
+        }
+
         if (isInactive()) {
             if (isInitialChanges) {
                 // send only required (reported) features updates
@@ -948,6 +959,25 @@ public boolean isInactive() {
         return getParent().isInactive();
     }
 
+    /**
+     * Checks (recursively towards the parent node) whether the node is
+     * effectively visible.
+     * <p>
+     * Non-visible node should not participate in any RPC communication.
+     *
+     * @return {@code true} if the node is effectively visible
+     */
+    public boolean isVisible() {
+        if (hasFeature(ElementData.class)) {
+            boolean isVisibleSelf = getFeature(ElementData.class).isVisible();
+            if (!isVisibleSelf || getParent() == null) {
+                return isVisibleSelf;
+            }
+            return parent.isVisible();
+        }
+        return getParent() == null || parent.isVisible();
+    }
+
     private Stream<NodeFeature> getDisalowFeatures() {
         return getInitializedFeatures()
                 .filter(feature -> !feature.allowsChanges());

flow-server/src/main/java/com/vaadin/flow/internal/StateTree.java

@@ -459,4 +459,5 @@ private Set<StateNode> doCollectDirtyNodes(boolean reset) {
     public void prepareForResync() {
         rootNode.prepareForResync();
     }
+
 }