chore: disable sending updates to client for effectively non-visible …

…nodes (CP: 1.0) (#16875)

* chore:disable sending updates to client for effectively non-visible nodes (#15885) (CP: 1.0)
Manually cherry-picked for 1.0
New changes through cherry picking:
- small changes around the stream peek (sentToBrowser is not existing in this code base)
- and the filtering invocations part,
- and owner Optional handling,

Old changes:
- 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.

* chore: #16836 and #16855 PR-s merged to this one, and fine tuned for this code-base (remove detach listener when javascript execution completes, and nullify registration)
- Added handling Optional<StateNode> for computeIfAbsent and other places,
- Covering test class (and so tests) is/are fully missing from this version (need to add somewhere else),
- Deleted/Commented  invocation.then(callback, callback) as this concept is not existing here, but added other snippet for handling
registration removes,
- Tailored the code to the codebase.

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

@@ -179,6 +179,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);
@@ -203,20 +205,35 @@ 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) {
-                getterValue = ((Optional<?>) getterValue).get();
+            Optional<?> getterValueOpt = ((Optional<?>) getterValue);
+            if (isOptional && getterValueOpt.isPresent()) {
+                getterValue = getterValueOpt.get();
             }
             Assert.assertEquals(getter + " should return the set value",
                     testValue, getterValue);

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

@@ -22,10 +22,13 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.Optional;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 
+import com.vaadin.flow.server.Command;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -174,6 +177,8 @@ public List<Object> getParameters() {
 
     private List<JavaScriptInvocation> pendingJsInvocations = new ArrayList<>();
 
+    private final HashMap<StateNode, JavaScriptInvocationDetachListener> pendingJsInvocationDetachListeners = new HashMap<>();
+
     /**
      * The related UI.
      */
@@ -542,6 +547,7 @@ public ExecutionCanceler addJavaScriptInvocation(
             JavaScriptInvocation invocation) {
         session.checkHasLock();
         pendingJsInvocations.add(invocation);
+
         return () -> pendingJsInvocations.remove(invocation);
     }
 
@@ -557,11 +563,83 @@ public List<JavaScriptInvocation> dumpPendingJavaScriptInvocations() {
             return Collections.emptyList();
         }
 
-        List<JavaScriptInvocation> currentList = pendingJsInvocations;
+        List<JavaScriptInvocation> readyToSend = getPendingJavaScriptInvocations()
+                .stream()
+                .filter(invocation -> invocation.getOwner().map(StateNode::isVisible).orElse(true))
+                .collect(Collectors.toList());
+
+        StateNode rootNode = ui.getInternals().getStateTree().getRootNode();
+        readyToSend.forEach(i -> {
+            JavaScriptInvocationDetachListener listener = pendingJsInvocationDetachListeners.get(i.getOwner().orElse(rootNode));
+            if (listener != null) {
+                listener.removePendingStatusAndRegistrationAndDetachListener(i);
+            }
+        });
+
+        pendingJsInvocations = getPendingJavaScriptInvocations()
+                .stream()
+                .filter(invocation -> invocation.getOwner().map(node -> !node.isVisible()).orElse(false))
+                .collect(Collectors.toCollection(ArrayList::new));
+
+        pendingJsInvocations
+                .forEach(this::registerDetachListenerForPendingInvocation);
+
+        return readyToSend;
+    }
+
+    /**
+     *  Registers a detach listener for the given invocation.
+     *
+     * @param invocation - invocation should always be having an existing owner if we call with this.
+     */
+    @SuppressWarnings({ "rawtypes", "unchecked" })
+    private void registerDetachListenerForPendingInvocation(
+            JavaScriptInvocation invocation) {
+
+        StateNode ownerNode = invocation.getOwner().orElse(ui.getInternals().getStateTree().getRootNode());
+
+        JavaScriptInvocationDetachListener listener = pendingJsInvocationDetachListeners
+                .computeIfAbsent(ownerNode, node -> {
+                    JavaScriptInvocationDetachListener detachListener = new JavaScriptInvocationDetachListener();
+                    Registration registration = node.addDetachListener(detachListener);
+                    detachListener.registration = () -> {
+                        pendingJsInvocationDetachListeners.remove(node);
+                        registration.remove();
+                    };
+                    return detachListener;
+                });
+        listener.invocationList.add(invocation);
+    }
+
+    private class JavaScriptInvocationDetachListener implements Command {
+        private final Set<JavaScriptInvocation> invocationList = new HashSet<>();
+
+        private Registration registration;
 
-        pendingJsInvocations = new ArrayList<>();
+        @Override
+        public void execute() {
+            if (!invocationList.isEmpty()) {
+                List<JavaScriptInvocation> copy = new ArrayList<>(
+                        invocationList);
+                invocationList.clear();
+                copy.forEach(this::removePendingInvocation);
+            }
+        }
+
+        private void removePendingInvocation(
+                JavaScriptInvocation invocation) {
+            UIInternals.this.pendingJsInvocations.removeIf(
+                    pendingInvocation -> pendingInvocation.equals(invocation));
+            if (invocationList.isEmpty() && registration != null) {
+                registration.remove();
+                registration = null;
+            }
+        }
 
-        return currentList;
+        void removePendingStatusAndRegistrationAndDetachListener(JavaScriptInvocation invocation) {
+            invocationList.remove(invocation);
+            removePendingInvocation(invocation);
+        }
     }
 
     /**
@@ -598,7 +676,7 @@ public void setTitle(String title) {
     /**
      * Gets the page title recorded with {@link Page#setTitle(String)}.
      * <p>
-     * <b>NOTE</b> this might not be up to date with the actual title set since
+     * <b>NOTE</b> this might not be up-to-date with the actual title set since
      * it is not updated from the browser and the update might have been
      * canceled before it has been sent to the browser with
      * {@link #cancelPendingTitleUpdate()}.

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

@@ -1439,8 +1439,8 @@ public <T extends Component> T as(Class<T> componentType) {
      * {@link Page#executeJavaScript(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
      *
@@ -1521,6 +1521,10 @@ public Optional<ShadowRoot> getShadowRoot() {
     /**
      * Sets the element visibility value.
      *
+     * Also execute 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

@@ -40,6 +40,7 @@
 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;
@@ -398,6 +399,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
@@ -684,7 +694,7 @@ public boolean isReportedFeature(Class<? extends NodeFeature> featureType) {
      * Implementation Note: this is done as a separate method instead of
      * calculating the state on the fly (checking all features) because each
      * node needs to check this status on its own <em>AND</em> on its parents
-     * (may be all parents up to the root).
+     * (maybe all parents up to the root).
      *
      * @see NodeFeature#allowsChanges()
      */
@@ -706,6 +716,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 getFeatures().values().stream()
                 .filter(feature -> !feature.allowsChanges());