Bug 1507750 - Compare the flexbox state for any changes before updating on reflows. r=pbro
authorGabriel Luong <gabriel.luong@gmail.com>
Wed, 28 Nov 2018 11:25:19 -0500
changeset 507733 8d0013f9a06dc1a374e5725876ff24aa880f8978
parent 507732 4201f7161e7a5323d7a9250f08c1d046aa85456d
child 507734 f2915d3ee5f8705676e4bb643eab54b62246f25f
child 507818 25e1436ae7e8d405bde04fd23fba5cc1757d4e67
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs1507750
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1507750 - Compare the flexbox state for any changes before updating on reflows. r=pbro
devtools/client/inspector/flexbox/flexbox.js
devtools/client/inspector/flexbox/test/browser.ini
devtools/client/inspector/flexbox/test/browser_flexbox_container_and_item_updates_on_change.js
devtools/client/inspector/flexbox/test/browser_flexbox_item_list_updates_on_change.js
devtools/client/inspector/flexbox/test/browser_flexbox_sizing_info_updates_on_change.js
--- a/devtools/client/inspector/flexbox/flexbox.js
+++ b/devtools/client/inspector/flexbox/flexbox.js
@@ -99,61 +99,16 @@ class FlexboxInspector {
     this.hasGetCurrentFlexbox = null;
     this.inspector = null;
     this.layoutInspector = null;
     this.selection = null;
     this.store = null;
     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,
       onToggleFlexboxHighlighter: this.onToggleFlexboxHighlighter,
     };
   }
 
   /**
@@ -168,16 +123,68 @@ class FlexboxInspector {
 
     // Cache the custom host colors to avoid refetching from async storage.
     this._customHostColors = await asyncStorage.getItem("flexboxInspectorHostColors")
       || {};
     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.
    *
    * @param  {FlexboxFront} flexboxFront
    *         A flex container FlexboxFront.
    * @return {Array} of objects containing the flex item front properties.
    */
   async getFlexItems(flexboxFront) {
     const flexItemFronts = await flexboxFront.getFlexItems();
@@ -298,35 +305,46 @@ class FlexboxInspector {
     if (!this.isPanelVisible() ||
         !this.store ||
         !this.selection.nodeFront ||
         !this.hasGetCurrentFlexbox) {
       return;
     }
 
     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.
     }
   }
 
   /**
    * Handler for a change in the flexbox overlay color picker for a flex container.
@@ -422,79 +440,118 @@ class FlexboxInspector {
     telemetry.getHistogramById(TELEMETRY_ELEMENT_TYPE_DISPLAYED).add(elementType);
   }
 
   /**
    * Updates the flexbox panel by dispatching the new flexbox data. This is called when
    * 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 ||
         !this.store ||
         !this.selection.nodeFront ||
         !this.hasGetCurrentFlexbox) {
       return;
     }
 
     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
       // closing.
     }
   }
 }
 
+/**
+ * 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;
--- a/devtools/client/inspector/flexbox/test/browser.ini
+++ b/devtools/client/inspector/flexbox/test/browser.ini
@@ -15,33 +15,36 @@ support-files =
   !/devtools/client/shared/test/shared-head.js
   !/devtools/client/shared/test/shared-redux-head.js
   !/devtools/client/shared/test/telemetry-test-helpers.js
   !/devtools/client/shared/test/test-actor.js
   !/devtools/client/shared/test/test-actor-registry.js
 
 [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]
 [browser_flexbox_highlighter_color_picker_on_ESC.js]
 [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]
 [browser_flexbox_item_outline_renders_basisfinal_points_correctly.js]
 [browser_flexbox_item_outline_rotates_for_column.js]
 [browser_flexbox_pseudo_elements_are_listed.js]
 [browser_flexbox_sizing_flexibility_not_displayed_when_useless.js]
 [browser_flexbox_sizing_info_do_not_show_unspecified_min_dimension.js]
 [browser_flexbox_sizing_info_exists.js]
 [browser_flexbox_sizing_info_for_different_writing_modes.js]
 [browser_flexbox_sizing_info_for_pseudos.js]
 [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]
 [browser_flexbox_toggle_flexbox_highlighter_02.js]
new file mode 100644
--- /dev/null
+++ b/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.");
+});
new file mode 100644
--- /dev/null
+++ b/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.");
+});
new file mode 100644
--- /dev/null
+++ b/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.");
+});