Bug 1507750 - Compare the flexbox state for any changes before updating on reflows. r=pbro

devtools/client/inspector/flexbox/flexbox.js

@@ -104,51 +104,6 @@ class FlexboxInspector {
     this.walker = null;
   }
 
-  /**
-   * If the current selected node is a flex container, check if it is also a flex item of
-   * a parent flex container and get its parent flex container if any and returns an
-   * object that consists of the parent flex container's items and properties.
-   *
-   * @param  {NodeFront} containerNodeFront
-   *         The current flex container of the selected node.
-   * @return {Object} consiting of the parent flex container's flex items and properties.
-   */
-  async getAsFlexItem(containerNodeFront) {
-    // If the current selected node is not a flex container, we know it is a flex item.
-    // No need to look for the parent flex container.
-    if (containerNodeFront !== this.selection.nodeFront) {
-      return null;
-    }
-
-    const flexboxFront = await this.layoutInspector.getCurrentFlexbox(
-      this.selection.nodeFront, true);
-
-    if (!flexboxFront) {
-      return null;
-    }
-
-    containerNodeFront = flexboxFront.containerNodeFront;
-    if (!containerNodeFront) {
-      containerNodeFront = await this.walker.getNodeFromActor(flexboxFront.actorID,
-        ["containerEl"]);
-    }
-
-    let flexItemContainer = null;
-    if (flexboxFront) {
-      const flexItems = await this.getFlexItems(flexboxFront);
-      flexItemContainer = {
-        actorID: flexboxFront.actorID,
-        flexItems,
-        flexItemShown: this.selection.nodeFront.actorID,
-        isFlexItemContainer: true,
-        nodeFront: containerNodeFront,
-        properties: flexboxFront.properties,
-      };
-    }
-
-    return flexItemContainer;
-  }
-
   getComponentProps() {
     return {
       onSetFlexboxOverlayColor: this.onSetFlexboxOverlayColor,
@@ -172,6 +127,58 @@ class FlexboxInspector {
     return this._customHostColors;
   }
 
+  /**
+   * Returns the flex container properties for a given node. If the given node is a flex
+   * item, it attempts to fetch the flex container of the parent node of the given node.
+   *
+   * @param  {NodeFront} nodeFront
+   *         The NodeFront to fetch the flex container properties.
+   * @param  {Boolean} onlyLookAtParents
+   *         Whether or not to only consider the parent node of the given node.
+   * @return {Object} consisting of the given node's flex container's properties.
+   */
+  async getFlexContainerProps(nodeFront, onlyLookAtParents = false) {
+    const flexboxFront = await this.layoutInspector.getCurrentFlexbox(nodeFront,
+      onlyLookAtParents);
+
+    if (!flexboxFront) {
+      return null;
+    }
+
+    // If the FlexboxFront doesn't yet have access to the NodeFront for its container,
+    // then get it from the walker. This happens when the walker hasn't seen this
+    // particular DOM Node in the tree yet or when we are connected to an older server.
+    let containerNodeFront = flexboxFront.containerNodeFront;
+    if (!containerNodeFront) {
+      containerNodeFront = await this.walker.getNodeFromActor(flexboxFront.actorID,
+        ["containerEl"]);
+    }
+
+    const flexItems = await this.getFlexItems(flexboxFront);
+
+    // If the current selected node is a flex item, display its flex item sizing
+    // properties.
+    let flexItemShown = null;
+    if (onlyLookAtParents) {
+      flexItemShown = this.selection.nodeFront.actorID;
+    } else {
+      const selectedFlexItem = flexItems.find(item =>
+        item.nodeFront === this.selection.nodeFront);
+      if (selectedFlexItem) {
+        flexItemShown = selectedFlexItem.nodeFront.actorID;
+      }
+    }
+
+    return {
+      actorID: flexboxFront.actorID,
+      flexItems,
+      flexItemShown,
+      isFlexItemContainer: onlyLookAtParents,
+      nodeFront: containerNodeFront,
+      properties: flexboxFront.properties,
+    };
+  }
+
   /**
    * Returns an array of flex items object for the given flex container front.
    *
@@ -303,25 +310,36 @@ class FlexboxInspector {
     }
 
     try {
-      const flexboxFront = await this.layoutInspector.getCurrentFlexbox(
-        this.selection.nodeFront);
+      const flexContainer = await this.getFlexContainerProps(this.selection.nodeFront);
 
       // Clear the flexbox panel if there is no flex container for the current node
       // selection.
-      if (!flexboxFront) {
+      if (!flexContainer) {
         this.store.dispatch(clearFlexbox());
         return;
       }
 
       const { flexbox } = this.store.getState();
 
-      // Do nothing because the same flex container is still selected.
-      if (flexbox.actorID == flexboxFront.actorID) {
+      // Compare the new flexbox state of the current selected nodeFront with the old
+      // flexbox state to determine if we need to update.
+      if (hasFlexContainerChanged(flexbox.flexContainer, flexContainer)) {
+        this.update(flexContainer);
         return;
       }
 
-      // Update the flexbox panel with the new flexbox front contents.
-      this.update(flexboxFront);
+      let flexItemContainer = null;
+      // If the current selected node is also the flex container node, check if it is
+      // a flex item of a parent flex container.
+      if (flexContainer.nodeFront === this.selection.nodeFront) {
+        flexItemContainer = await this.getFlexContainerProps(this.selection.nodeFront,
+          true);
+      }
+
+      // Compare the new and old state of the parent flex container properties.
+      if (hasFlexContainerChanged(flexbox.flexItemContainer, flexItemContainer)) {
+        this.update(flexContainer, flexItemContainer);
+      }
     } catch (e) {
       // This call might fail if called asynchrously after the toolbox is finished
       // closing.
@@ -427,10 +445,14 @@ class FlexboxInspector {
    * the layout view becomes visible or a new node is selected and needs to be update
    * with new flexbox data.
    *
-   * @param  {FlexboxFront|null} flexboxFront
-   *         The FlexboxFront of the flex container for the current node selection.
+   * @param  {Object|null} flexContainer
+   *         An object consisting of the current flex container's flex items and
+   *         properties.
+   * @param  {Object|null} flexItemContainer
+   *         An object consisting of the parent flex container's flex items and
+   *         properties.
    */
-  async update(flexboxFront) {
+  async update(flexContainer, flexItemContainer) {
     // Stop refreshing if the inspector or store is already destroyed or no node is
     // selected.
     if (!this.inspector ||
@@ -442,53 +464,35 @@ class FlexboxInspector {
 
     try {
       // Fetch the current flexbox if no flexbox front was passed into this update.
-      if (!flexboxFront) {
-        flexboxFront = await this.layoutInspector.getCurrentFlexbox(
-          this.selection.nodeFront);
+      if (!flexContainer) {
+        flexContainer = await this.getFlexContainerProps(this.selection.nodeFront);
       }
 
       // Clear the flexbox panel if there is no flex container for the current node
       // selection.
-      if (!flexboxFront) {
+      if (!flexContainer) {
         this.store.dispatch(clearFlexbox());
         return;
       }
 
-      // If the FlexboxFront doesn't yet have access to the NodeFront for its container,
-      // then get it from the walker. This happens when the walker hasn't seen this
-      // particular DOM Node in the tree yet or when we are connected to an older server.
-      let containerNodeFront = flexboxFront.containerNodeFront;
-      if (!containerNodeFront) {
-        containerNodeFront = await this.walker.getNodeFromActor(flexboxFront.actorID,
-          ["containerEl"]);
+      if (!flexItemContainer && flexContainer.nodeFront === this.selection.nodeFront) {
+        flexItemContainer = await this.getFlexContainerProps(this.selection.nodeFront,
+          true);
       }
 
-      const flexItemContainer = await this.getAsFlexItem(containerNodeFront);
-      const flexItems = await this.getFlexItems(flexboxFront);
-      // If the current selected node is a flex item, display its flex item sizing
-      // properties.
-      const flexItemShown = flexItems.find(item =>
-        item.nodeFront === this.selection.nodeFront);
       const highlighted = this._highlighters &&
-        containerNodeFront == this.highlighters.flexboxHighlighterShown;
+        flexContainer.nodeFront === this.highlighters.flexboxHighlighterShown;
       const color = await this.getOverlayColor();
 
       this.store.dispatch(updateFlexbox({
         color,
-        flexContainer: {
-          actorID: flexboxFront.actorID,
-          flexItems,
-          flexItemShown: flexItemShown ? flexItemShown.nodeFront.actorID : null,
-          isFlexItemContainer: false,
-          nodeFront: containerNodeFront,
-          properties: flexboxFront.properties,
-        },
+        flexContainer,
         flexItemContainer,
         highlighted,
       }));
 
-      const isContainerInfoShown = !flexItemShown || !!flexItemContainer;
-      const isItemInfoShown = !!flexItemShown || !!flexItemContainer;
+      const isContainerInfoShown = !flexContainer.flexItemShown || !!flexItemContainer;
+      const isItemInfoShown = !!flexContainer.flexItemShown || !!flexItemContainer;
       this.sendTelemetryProbes(isContainerInfoShown, isItemInfoShown);
     } catch (e) {
       // This call might fail if called asynchrously after the toolbox is finished
@@ -497,4 +501,57 @@ class FlexboxInspector {
   }
 }
 
+/**
+ * For a given flex container object, returns the flex container properties that can be
+ * used to check if 2 flex container objects are the same.
+ *
+ * @param  {Object|null} flexContainer
+ *         Object consisting of the flex container's properties.
+ * @return {Object|null} consisting of the comparable flex container's properties.
+ */
+function getComparableFlexContainerProperties(flexContainer) {
+  if (!flexContainer) {
+    return null;
+  }
+
+  return {
+    flexItems: getComparableFlexItemsProperties(flexContainer.flexItems),
+    nodeFront: flexContainer.nodeFront.actorID,
+    properties: flexContainer.properties,
+  };
+}
+
+/**
+ * Given an array of flex item objects, returns the relevant flex item properties that can
+ * be compared to check if any changes has occurred.
+ *
+ * @param  {Array} flexItems
+ *         Array of objects containing the flex item properties.
+ * @return {Array} of objects consisting of the comparable flex item's properties.
+ */
+function getComparableFlexItemsProperties(flexItems) {
+  return flexItems.map(item => {
+    return {
+      computedStyle: item.computedStyle,
+      flexItemSizing: item.flexItemSizing,
+      nodeFront: item.nodeFront.actorID,
+      properties: item.properties,
+    };
+  });
+}
+
+/**
+ * Compares the old and new flex container properties
+ *
+ * @param  {Object} oldFlexContainer
+ *         Object consisting of the old flex container's properties.
+ * @param  {Object} newFlexContainer
+ *         Object consisting of the new flex container's properties.
+ * @return {Boolean} true if the flex container properties are the same, false otherwise.
+ */
+function hasFlexContainerChanged(oldFlexContainer, newFlexContainer) {
+  return JSON.stringify(getComparableFlexContainerProperties(oldFlexContainer)) !==
+    JSON.stringify(getComparableFlexContainerProperties(newFlexContainer));
+}
+
 module.exports = FlexboxInspector;

devtools/client/inspector/flexbox/test/browser.ini

@@ -20,6 +20,7 @@ support-files =
 
 [browser_flexbox_accordion_state.js]
 [browser_flexbox_container_and_item.js]
+[browser_flexbox_container_and_item_updates_on_change.js]
 [browser_flexbox_container_element_rep.js]
 [browser_flexbox_container_properties.js]
 [browser_flexbox_empty_state.js]
@@ -27,6 +28,7 @@ support-files =
 [browser_flexbox_highlighter_color_picker_on_RETURN.js]
 [browser_flexbox_item_list_01.js]
 [browser_flexbox_item_list_02.js]
+[browser_flexbox_item_list_updates_on_change.js]
 [browser_flexbox_item_outline_exists.js]
 [browser_flexbox_item_outline_has_correct_layout.js]
 [browser_flexbox_item_outline_hidden_when_useless.js]
@@ -41,6 +43,7 @@ support-files =
 [browser_flexbox_sizing_info_for_text_nodes.js]
 [browser_flexbox_sizing_info_has_correct_sections.js]
 [browser_flexbox_sizing_info_matches_properties_with_!important.js]
+[browser_flexbox_sizing_info_updates_on_change.js]
 [browser_flexbox_sizing_wanted_to_grow_but_was_clamped.js]
 [browser_flexbox_text_nodes_are_listed.js]
 [browser_flexbox_toggle_flexbox_highlighter_01.js]

devtools/client/inspector/flexbox/test/browser_flexbox_container_and_item_updates_on_change.js

@@ -0,0 +1,46 @@
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test that the flex container accordion is rendered when a flex item is updated to
+// also be a flex container.
+
+const TEST_URI = `
+  <style>
+    .container {
+      display: flex;
+    }
+  </style>
+  <div id="container" class="container">
+    <div id="item">
+      <div></div>
+    </div>
+  </div>
+`;
+
+add_task(async function() {
+  await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  const { inspector, flexboxInspector, testActor } = await openLayoutView();
+  const { document: doc } = flexboxInspector;
+
+  const onFlexItemSizingRendered = waitForDOM(doc, "ul.flex-item-sizing");
+  await selectNode("#item", inspector);
+  const [flexSizingContainer] = await onFlexItemSizingRendered;
+
+  ok(flexSizingContainer, "The flex sizing info is rendered.");
+
+  info("Changing the flexbox in the page.");
+  const onAccordionsChanged = waitForDOM(doc, ".accordion > div", 4);
+  testActor.eval(`
+    document.getElementById("item").className = "container";
+  `);
+  const [flexItemPane, flexContainerPane] = await onAccordionsChanged;
+
+  ok(flexItemPane, "The flex item accordion pane is rendered.");
+  ok(flexContainerPane, "The flex container accordion pane is rendered.");
+  is(flexItemPane.children[0].textContent, "Flex Item of div#container.container",
+    "Got the correct header for the flex item pane.");
+  is(flexContainerPane.children[0].textContent, "Flex Container",
+    "Got the correct header for the flex container pane.");
+});

devtools/client/inspector/flexbox/test/browser_flexbox_item_list_updates_on_change.js

@@ -0,0 +1,44 @@
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test that the flex item list updates on changes to the number of flex items in the
+// flex container.
+
+const TEST_URI = `
+  <style>
+    #container {
+      display: flex;
+    }
+  </style>
+  <div id="container">
+    <div></div>
+  </div>
+`;
+
+add_task(async function() {
+  await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  const { inspector, flexboxInspector, testActor } = await openLayoutView();
+  const { document: doc } = flexboxInspector;
+
+  const onFlexItemListRendered = waitForDOM(doc, ".flex-item-list");
+  await selectNode("#container", inspector);
+  const [flexItemList] = await onFlexItemListRendered;
+
+  info("Checking the initial state of the flex item list.");
+  ok(flexItemList, "The flex item list is rendered.");
+  is(flexItemList.querySelectorAll("button").length, 1,
+    "Got the correct number of flex items in the list.");
+
+  info("Changing the flexbox in the page.");
+  const onFlexItemListChanged = waitForDOM(doc, ".flex-item-list > button", 2);
+  testActor.eval(`
+    const div = document.createElement("div");
+    document.getElementById("container").appendChild(div);
+  `);
+  const elements = await onFlexItemListChanged;
+
+  info("Checking the flex item list is correct.");
+  is(elements.length, 2, "Flex item list was changed.");
+});

devtools/client/inspector/flexbox/test/browser_flexbox_sizing_info_updates_on_change.js

@@ -0,0 +1,41 @@
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test that the flexbox sizing info updates on changes to the flex item properties.
+
+const TEST_URI = `
+  <style>
+    #container {
+      display: flex;
+    }
+  </style>
+  <div id="container">
+    <div id="item"></div>
+  </div>
+`;
+
+add_task(async function() {
+  await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  const { inspector, flexboxInspector, testActor } = await openLayoutView();
+  const { document: doc } = flexboxInspector;
+
+  const onFlexItemSizingRendered = waitForDOM(doc, "ul.flex-item-sizing");
+  await selectNode("#item", inspector);
+  const [flexItemSizingContainer] = await onFlexItemSizingRendered;
+
+  info("Checking the initial state of the flex item list.");
+  is(flexItemSizingContainer.querySelectorAll("li").length, 2,
+    "Got the correct number of flex item sizing properties in the list.");
+
+  info("Changing the flexbox in the page.");
+  const onFlexItemSizingChanged = waitForDOM(doc, "ul.flex-item-sizing > li", 3);
+  testActor.eval(`
+    document.getElementById("item").style.minWidth = "100px";
+  `);
+  const elements = await onFlexItemSizingChanged;
+
+  info("Checking the flex item sizing info is correct.");
+  is(elements.length, 3, "Flex item sizing info was changed.");
+});