Bug 1507736 - Make sure the Flexbox inspector correctly shows if the current flex container is also a flex item. r=pbro
authorMicah Tigley <mtigley@mozilla.com>
Thu, 29 Nov 2018 19:54:41 +0000
changeset 505297 ed8f77cd2f9e0b81363085f6cc1dce6f41cffc14
parent 505296 da4984e0016a1beac3845f64ce329b67dfa81baa
child 505298 531d30645f389f8ab0502bf9d5f27fb6f820ce26
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs1507736
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 1507736 - Make sure the Flexbox inspector correctly shows if the current flex container is also a flex item. r=pbro Differential Revision: https://phabricator.services.mozilla.com/D12584
devtools/client/inspector/flexbox/components/FlexItemList.js
devtools/client/inspector/flexbox/components/Flexbox.js
devtools/client/inspector/flexbox/test/browser.ini
devtools/client/inspector/flexbox/test/browser_flexbox_item_list_01.js
devtools/client/inspector/flexbox/test/browser_flexbox_non_flex_item_is_not_shown.js
devtools/client/locales/en-US/layout.properties
devtools/server/actors/layout.js
--- a/devtools/client/inspector/flexbox/components/FlexItemList.js
+++ b/devtools/client/inspector/flexbox/components/FlexItemList.js
@@ -31,17 +31,20 @@ class FlexItemList extends PureComponent
       onShowBoxModelHighlighterForNode,
       scrollToTop,
       setSelectedNode,
     } = this.props;
 
     return (
       dom.div(
         { className: "flex-item-list" },
-        dom.div({ className: "flex-item-list-header" }, getStr("flexbox.flexItems")),
+        dom.div({ className: "flex-item-list-header" },
+          !flexItems.length ?
+            getStr("flexbox.noFlexItems") :
+            getStr("flexbox.flexItems")),
         flexItems.map((flexItem, index) => FlexItem({
           key: flexItem.actorID,
           flexItem,
           index: index + 1,
           onHideBoxModelHighlighter,
           onShowBoxModelHighlighterForNode,
           scrollToTop,
           setSelectedNode,
--- a/devtools/client/inspector/flexbox/components/Flexbox.js
+++ b/devtools/client/inspector/flexbox/components/Flexbox.js
@@ -101,31 +101,30 @@ class Flexbox extends PureComponent {
       return (
         dom.div({ className: "devtools-sidepanel-no-result" },
           getStr("flexbox.noFlexboxeOnThisPage")
         )
       );
     }
 
     const {
-      flexItems,
       flexItemShown,
     } = flexContainer;
 
     return (
       dom.div({ id: "layout-flexbox-container" },
         Header({
           flexContainer,
           getSwatchColorPickerTooltip,
           onHideBoxModelHighlighter,
           onSetFlexboxOverlayColor,
           onShowBoxModelHighlighterForNode,
           onToggleFlexboxHighlighter,
           setSelectedNode,
         }),
-        !flexItemShown && flexItems.length > 0 ? this.renderFlexItemList() : null,
+        !flexItemShown ? this.renderFlexItemList() : null,
         flexItemShown ? this.renderFlexItemSizing() : null,
       )
     );
   }
 }
 
 module.exports = Flexbox;
--- a/devtools/client/inspector/flexbox/test/browser.ini
+++ b/devtools/client/inspector/flexbox/test/browser.ini
@@ -29,16 +29,17 @@ support-files =
 [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_non_flex_item_is_not_shown.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]
--- a/devtools/client/inspector/flexbox/test/browser_flexbox_item_list_01.js
+++ b/devtools/client/inspector/flexbox/test/browser_flexbox_item_list_01.js
@@ -1,13 +1,15 @@
 /* Any copyright is dedicated to the Public Domain.
  http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
+const { getStr } = require("devtools/client/inspector/layout/utils/l10n");
+
 // Test the flex item list is empty when there are no flex items for the selected flex
 // container.
 
 const TEST_URI = `
   <div id="container" style="display:flex">
   </div>
 `;
 
@@ -16,17 +18,18 @@ add_task(async function() {
   const { inspector, flexboxInspector } = await openLayoutView();
   const { document: doc } = flexboxInspector;
   const { highlighters } = inspector;
 
   const onFlexHeaderRendered = waitForDOM(doc, ".flex-header");
   await selectNode("#container", inspector);
   const [flexHeader] = await onFlexHeaderRendered;
   const flexHighlighterToggle = flexHeader.querySelector("#flexbox-checkbox-toggle");
-  const flexItemList = doc.querySelector(".flex-item-list");
+  const flexItemListHeader = doc.querySelector(".flex-item-list-header");
 
   info("Checking the state of the Flexbox Inspector.");
   ok(flexHeader, "The flex container header is rendered.");
   ok(flexHighlighterToggle, "The flexbox highlighter toggle is rendered.");
-  ok(!flexItemList, "The flex item list is not rendered.");
+  is(flexItemListHeader.textContent, getStr("flexbox.noFlexItems"),
+    "The flex item list header shows 'No flex items' when there are no items.");
   ok(!flexHighlighterToggle.checked, "The flexbox highlighter toggle is unchecked.");
   ok(!highlighters.flexboxHighlighterShown, "No flexbox highlighter is shown.");
 });
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/flexbox/test/browser_flexbox_non_flex_item_is_not_shown.js
@@ -0,0 +1,28 @@
+/* vim: set ts=2 et sw=2 tw=80: */
+/* Any copyright is dedicated to the Public Domain.
+http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test that an element that is the child of a flex container but is not actually a flex
+// item is not shown in the sidebar when selected.
+
+const TEST_URI = `
+<div style="display:flex">
+  <div id="item" style="position:absolute;">test</div>
+</div>
+`;
+
+add_task(async function() {
+  await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  const { inspector, flexboxInspector } = await openLayoutView();
+  const { document: doc } = flexboxInspector;
+
+  info("Select the container's supposed flex item.");
+  await selectNode("#item", inspector);
+  const noFlexContainerOrItemSelected =
+    doc.querySelector(".flexbox-pane .devtools-sidepanel-no-result");
+
+  ok(noFlexContainerOrItemSelected,
+    "The flexbox pane shows a message to select a flex container or item to continue.");
+});
--- a/devtools/client/locales/en-US/layout.properties
+++ b/devtools/client/locales/en-US/layout.properties
@@ -19,16 +19,20 @@ flexbox.flexItemOf=Flex Item of %S
 
 # LOCALIZATION NOTE (flexbox.noFlexboxeOnThisPage): In the case where there are no CSS
 # flex containers to display.
 flexbox.noFlexboxeOnThisPage=Select a Flex container or item to continue.
 
 # LOCALIZATION NOTE (flexbox.flexItems): Header label displayed for the flex item list.
 flexbox.flexItems=Flex Items
 
+# LOCALIZATION NOTE (flexbox.noFlexItems): Label shown in the flex items list section if
+# there are no flex items for the flex container to display.
+flexbox.noFlexItems=No flex items
+
 # LOCALIZATION NOTE (flexbox.itemSizing.baseSizeSectionHeader): Header label displayed
 # at the start of the flex item sizing Base Size section.
 flexbox.itemSizing.baseSizeSectionHeader=Base Size
 
 # LOCALIZATION NOTE (flexbox.itemSizing.flexibilitySectionHeader): Header label displayed
 # at the start of the flex item sizing Flexibility section.
 flexbox.itemSizing.flexibilitySectionHeader=Flexibility
 
--- a/devtools/server/actors/layout.js
+++ b/devtools/server/actors/layout.js
@@ -330,23 +330,22 @@ const LayoutActor = ActorClassWithSpec(l
    * selected node. The current node can be a grid/flex container or grid/flex item.
    * If it is a grid/flex item, returns the parent grid/flex container. Otherwise, returns
    * null if the current or parent node is not a grid/flex container.
    *
    * @param  {Node|NodeActor} node
    *         The node to start iterating at.
    * @param  {String} type
    *         Can be "grid" or "flex", the display type we are searching for.
-   * @param  {Boolean|null} onlyLookAtCurrentNode
-   *         Whether or not to consider only the current node's display (ie, don't walk
-   *         up the tree).
+   * @param  {Node|null} container
+   *         The container of the current node.
    * @return {GridActor|FlexboxActor|null} The GridActor or FlexboxActor of the
    * grid/flex container of the give node. Otherwise, returns null.
    */
-  getCurrentDisplay(node, type, onlyLookAtCurrentNode) {
+  getCurrentDisplay(node, type, container) {
     if (isNodeDead(node)) {
       return null;
     }
 
     // Given node can either be a Node or a NodeActor.
     if (node.rawNode) {
       node = node.rawNode;
     }
@@ -357,20 +356,28 @@ const LayoutActor = ActorClassWithSpec(l
 
     // If the node is an element, check first if it is itself a flex or a grid.
     if (currentNode.nodeType === currentNode.ELEMENT_NODE) {
       if (!displayType) {
         return null;
       }
 
       if (type == "flex") {
-        if (displayType == "inline-flex" || displayType == "flex") {
+        // If only the current node is being considered when finding its display, then
+        // return it as only a flex container.
+        if ((displayType == "inline-flex" || displayType == "flex") &&
+              !container) {
           return new FlexboxActor(this, currentNode);
-        } else if (onlyLookAtCurrentNode) {
-          return null;
+
+        // If considering the current node's container, then we are trying to determine
+        // the current node's flex item status.
+        } else if (container) {
+          if (this.isNodeFlexItemInContainer(currentNode, container)) {
+            return new FlexboxActor(this, container);
+          }
         }
       } else if (type == "grid" &&
                  (displayType == "inline-grid" || displayType == "grid")) {
         return new GridActor(this, currentNode);
       }
     }
 
     // Otherwise, check if this is a flex/grid item or the parent node is a flex/grid
@@ -382,17 +389,19 @@ const LayoutActor = ActorClassWithSpec(l
       if (!currentNode) {
         break;
       }
 
       displayType = this.walker.getNode(currentNode).displayType;
 
       if (type == "flex" &&
           (displayType == "inline-flex" || displayType == "flex")) {
-        return new FlexboxActor(this, currentNode);
+        if (this.isNodeFlexItemInContainer(node, currentNode)) {
+          return new FlexboxActor(this, currentNode);
+        }
       } else if (type == "grid" &&
                  (displayType == "inline-grid" || displayType == "grid")) {
         return new GridActor(this, currentNode);
       } else if (displayType == "contents") {
         // Continue walking up the tree since the parent node is a content element.
         continue;
       }
 
@@ -426,21 +435,23 @@ const LayoutActor = ActorClassWithSpec(l
    * @param  {Node|NodeActor} node
    *         The node to start iterating at.
    * @param  {Boolean|null} onlyLookAtParents
    *         Whether or not to only consider the parent node of the given node.
    * @return {FlexboxActor|null} The FlexboxActor of the flex container of the given node.
    * Otherwise, returns null.
    */
   getCurrentFlexbox(node, onlyLookAtParents) {
+    let container = null;
+
     if (onlyLookAtParents) {
-      node = node.rawNode.parentNode;
+      container = node.rawNode.parentNode;
     }
 
-    return this.getCurrentDisplay(node, "flex", onlyLookAtParents);
+    return this.getCurrentDisplay(node, "flex", container);
   },
 
   /**
    * Returns an array of GridActor objects for all the grid elements contained in the
    * given root node.
    *
    * @param  {Node|NodeActor} node
    *         The root node for grid elements
@@ -466,16 +477,45 @@ const LayoutActor = ActorClassWithSpec(l
 
     const frames = node.querySelectorAll("iframe, frame");
     for (const frame of frames) {
       gridActors = gridActors.concat(this.getGrids(frame.contentDocument));
     }
 
     return gridActors;
   },
+
+  /**
+   * Returns whether or not the given node is actually considered a flex item by its
+   * container.
+   *
+   * @param  {Node|NodeActor} supposedItem
+   *         The node that might be a flex item of its container.
+   * @param  {Node} container
+   *         The node's container.
+   * @return {Boolean} Whether or not the node we are looking at is a flex item of its
+   * container.
+   */
+  isNodeFlexItemInContainer(supposedItem, container) {
+    const containerDisplayType = this.walker.getNode(container).displayType;
+
+    if (containerDisplayType == "inline-flex" || containerDisplayType == "flex") {
+      const containerFlex = container.getAsFlexContainer();
+
+      for (const line of containerFlex.getLines()) {
+        for (const item of line.getItems()) {
+          if (item.node === supposedItem) {
+            return true;
+          }
+        }
+      }
+    }
+
+    return false;
+  },
 });
 
 function isNodeDead(node) {
   return !node || (node.rawNode && Cu.isDeadWrapper(node.rawNode));
 }
 
 exports.FlexboxActor = FlexboxActor;
 exports.FlexItemActor = FlexItemActor;