Bug 1578097 - Add prefs to persist the open states of flexbox accordion items separately; r=gl

Differential Revision: https://phabricator.services.mozilla.com/D44331

--HG--
extra : moz-landing-system : lando

browser/app/profile/firefox.js

@@ -2006,6 +2006,10 @@ pref("devtools.gridinspector.maxHighlighters", 3);
 pref("devtools.layout.boxmodel.opened", true);
 // Whether or not the flexbox panel is opened in the layout view
 pref("devtools.layout.flexbox.opened", true);
+// Whether or not the flexbox container panel is opened in the layout view
+pref("devtools.layout.flex-container.opened", true);
+// Whether or not the flexbox item panel is opened in the layout view
+pref("devtools.layout.flex-item.opened", true);
 // Whether or not the grid inspector panel is opened in the layout view
 pref("devtools.layout.grid.opened", true);
 

devtools/client/inspector/boxmodel/test/browser_boxmodel_layout-accordion-state.js

@@ -108,7 +108,11 @@ async function testAccordionStateAfterReopeningLayoutView(toolbox) {
   );
 
   info("Checking the state of the box model panel.");
-  ok(!bContent, "The box model panel content is not rendered.");
+  is(
+    bContent.children.length,
+    0,
+    "The box model panel content is not rendered."
+  );
   ok(
     !Services.prefs.getBoolPref(BOXMODEL_OPENED_PREF),
     `${BOXMODEL_OPENED_PREF} is pref off.`

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

@@ -3,39 +3,55 @@
 
 "use strict";
 
-// Test that the flexbox's accordion state is persistent through hide/show in the layout
+// Test that the flexbox accordions state is persistent through hide/show in the layout
 // view.
 
 const TEST_URI = URL_ROOT + "doc_flexbox_specific_cases.html";
 
-const FLEXBOX_OPENED_PREF = "devtools.layout.flexbox.opened";
-
 add_task(async function() {
   await addTab(TEST_URI);
-  const { inspector, flexboxInspector, toolbox } = await openLayoutView();
-  const { document: doc } = flexboxInspector;
 
-  await testAccordionStateAfterClickingHeader(doc);
-  await testAccordionStateAfterSwitchingSidebars(inspector, doc);
-  await testAccordionStateAfterReopeningLayoutView(toolbox);
-
-  Services.prefs.clearUserPref(FLEXBOX_OPENED_PREF);
+  await testAccordionState(
+    ":root",
+    FLEXBOX_OPENED_PREF,
+    ".flex-accordion-none"
+  );
+  await testAccordionState(
+    "#container-only",
+    FLEX_CONTAINER_OPENED_PREF,
+    ".flex-accordion-container"
+  );
+  await testAccordionState(
+    "#item-only",
+    FLEX_ITEM_OPENED_PREF,
+    ".flex-accordion-item"
+  );
 });
 
-function testAccordionStateAfterClickingHeader(doc) {
-  const header = doc.querySelector(".flex-accordion ._header");
-  const content = doc.querySelector(".flex-accordion ._content");
+async function testAccordionState(target, pref, selector) {
+  const context = await openLayoutViewAndSelectNode(target);
 
+  await testAccordionStateAfterClickingHeader(pref, selector, context);
+  await testAccordionStateAfterSwitchingSidebars(pref, selector, context);
+  await testAccordionStateAfterReopeningLayoutView(pref, selector, context);
+
+  Services.prefs.clearUserPref(pref);
+}
+
+async function testAccordionStateAfterClickingHeader(pref, selector, { doc }) {
   info("Checking initial state of the flexbox panel.");
+
+  const header = await waitFor(() => doc.querySelector(selector + " ._header"));
+  const content = await waitFor(() =>
+    doc.querySelector(selector + " ._content")
+  );
+
   is(
     content.style.display,
     "block",
     "The flexbox panel content is 'display: block'."
   );
-  ok(
-    Services.prefs.getBoolPref(FLEXBOX_OPENED_PREF),
-    `${FLEXBOX_OPENED_PREF} is pref on by default.`
-  );
+  ok(Services.prefs.getBoolPref(pref), `${pref} is pref on by default.`);
 
   info("Clicking the flexbox header to hide the flexbox panel.");
   header.click();
@@ -46,18 +62,21 @@ function testAccordionStateAfterClickingHeader(doc) {
     "none",
     "The flexbox panel content is 'display: none'."
   );
-  ok(
-    !Services.prefs.getBoolPref(FLEXBOX_OPENED_PREF),
-    `${FLEXBOX_OPENED_PREF} is pref off.`
-  );
+  ok(!Services.prefs.getBoolPref(pref), `${pref} is pref off.`);
 }
 
-function testAccordionStateAfterSwitchingSidebars(inspector, doc) {
+async function testAccordionStateAfterSwitchingSidebars(
+  pref,
+  selector,
+  { doc, inspector }
+) {
   info(
     "Checking the flexbox accordion state is persistent after switching sidebars."
   );
 
-  const content = doc.querySelector(".flex-accordion ._content");
+  const content = await waitFor(() =>
+    doc.querySelector(selector + " ._content")
+  );
 
   info("Selecting the computed view.");
   inspector.sidebar.select("computedview");
@@ -71,30 +90,41 @@ function testAccordionStateAfterSwitchingSidebars(inspector, doc) {
     "none",
     "The flexbox panel content is 'display: none'."
   );
-  ok(
-    !Services.prefs.getBoolPref(FLEXBOX_OPENED_PREF),
-    `${FLEXBOX_OPENED_PREF} is pref off.`
-  );
+  ok(!Services.prefs.getBoolPref(pref), `${pref} is pref off.`);
 }
 
-async function testAccordionStateAfterReopeningLayoutView(toolbox) {
+async function testAccordionStateAfterReopeningLayoutView(
+  pref,
+  selector,
+  { target, toolbox }
+) {
   info(
-    "Checking the flexbox accordion state is persistent after closing and re-opening " +
-      "the layout view."
+    "Checking the flexbox accordion state is persistent after closing and re-opening the layout view."
   );
 
   info("Closing the toolbox.");
   await toolbox.destroy();
 
   info("Re-opening the layout view.");
-  const { flexboxInspector } = await openLayoutView();
-  const { document: doc } = flexboxInspector;
-  const content = doc.querySelector(".flex-accordion ._content");
+  const { doc } = await openLayoutViewAndSelectNode(target);
+  const content = await waitFor(() =>
+    doc.querySelector(selector + " ._content")
+  );
 
   info("Checking the state of the flexbox panel.");
-  ok(!content, "The flexbox panel content is not rendered.");
-  ok(
-    !Services.prefs.getBoolPref(FLEXBOX_OPENED_PREF),
-    `${FLEXBOX_OPENED_PREF} is pref off.`
-  );
+  is(content.style.display, "none", "The flexbox panel content is hidden.");
+  is(content.children.length, 0, "The flexbox panel content is not rendered.");
+  ok(!Services.prefs.getBoolPref(pref), `${pref} is pref off.`);
+}
+
+async function openLayoutViewAndSelectNode(target) {
+  const { inspector, flexboxInspector, toolbox } = await openLayoutView();
+  await selectNode(target, inspector);
+
+  return {
+    doc: flexboxInspector.document,
+    inspector,
+    target,
+    toolbox,
+  };
 }

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

@@ -27,7 +27,7 @@ add_task(async function() {
   let accordions = doc.querySelectorAll(".flex-accordion");
   is(accordions.length, 1, "There's only 1 accordion");
   ok(
-    accordions[0].classList.contains("container"),
+    accordions[0].classList.contains("flex-accordion-container"),
     "The accordion is the container type"
   );
 
@@ -41,11 +41,11 @@ add_task(async function() {
   accordions = doc.querySelectorAll(".flex-accordion");
   is(accordions.length, 2, "There are 2 accordions");
   ok(
-    accordions[0].classList.contains("container"),
+    accordions[0].classList.contains("flex-accordion-container"),
     "The first accordion is the container type"
   );
   ok(
-    accordions[1].classList.contains("item"),
+    accordions[1].classList.contains("flex-accordion-item"),
     "The second accordion is the item type"
   );
 
@@ -65,11 +65,11 @@ add_task(async function() {
   accordions = doc.querySelectorAll(".flex-accordion");
   is(accordions.length, 2, "There are 2 accordions again");
   ok(
-    accordions[0].classList.contains("item"),
+    accordions[0].classList.contains("flex-accordion-item"),
     "The first accordion is the item type"
   );
   ok(
-    accordions[1].classList.contains("container"),
+    accordions[1].classList.contains("flex-accordion-container"),
     "The second accordion is the container type"
   );
 });

devtools/client/inspector/flexbox/test/doc_flexbox_specific_cases.html

@@ -116,6 +116,6 @@
 </div>
 <div class="container" id="container-only">
   <div class="container" id="container-and-item">
-    <div>This item is inside a container-item element</div>
+    <div id="item-only">This item is inside a container-item element</div>
   </div>
 </div>

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

@@ -19,16 +19,26 @@ Services.scriptloader.loadSubScript(
   this
 );
 
-// Make sure only the flexbox layout accordion is opened, and the others are closed.
-Services.prefs.setBoolPref("devtools.layout.flexbox.opened", true);
-Services.prefs.setBoolPref("devtools.layout.boxmodel.opened", false);
-Services.prefs.setBoolPref("devtools.layout.grid.opened", false);
+const FLEXBOX_OPENED_PREF = "devtools.layout.flexbox.opened";
+const FLEX_CONTAINER_OPENED_PREF = "devtools.layout.flex-container.opened";
+const FLEX_ITEM_OPENED_PREF = "devtools.layout.flex-item.opened";
+const GRID_OPENED_PREF = "devtools.layout.grid.opened";
+const BOXMODEL_OPENED_PREF = "devtools.layout.boxmodel.opened";
+
+// Make sure only the flexbox layout accordions are opened, and the others are closed.
+Services.prefs.setBoolPref(FLEXBOX_OPENED_PREF, true);
+Services.prefs.setBoolPref(FLEX_CONTAINER_OPENED_PREF, true);
+Services.prefs.setBoolPref(FLEX_ITEM_OPENED_PREF, true);
+Services.prefs.setBoolPref(BOXMODEL_OPENED_PREF, false);
+Services.prefs.setBoolPref(GRID_OPENED_PREF, false);
 
 // Clear all set prefs.
 registerCleanupFunction(() => {
-  Services.prefs.clearUserPref("devtools.layout.flexbox.opened");
-  Services.prefs.clearUserPref("devtools.layout.boxmodel.opened");
-  Services.prefs.clearUserPref("devtools.layout.grid.opened");
+  Services.prefs.clearUserPref(FLEXBOX_OPENED_PREF);
+  Services.prefs.clearUserPref(FLEX_CONTAINER_OPENED_PREF);
+  Services.prefs.clearUserPref(FLEX_ITEM_OPENED_PREF);
+  Services.prefs.clearUserPref(BOXMODEL_OPENED_PREF);
+  Services.prefs.clearUserPref(GRID_OPENED_PREF);
 });
 
 /**

devtools/client/inspector/fonts/test/head.js

@@ -251,17 +251,3 @@ function getPropertyValue(viewDoc, name) {
 function isRemote(fontEl) {
   return fontEl.querySelector(".font-origin").classList.contains("remote");
 }
-
-/**
- * Wait for a predicate to return a result.
- *
- * @param  {Function} condition
- *         Invoked every 10ms for a maximum of 500 retries until it returns a truthy
- *         value.
- * @return {Promise}
- *         A promise that is resolved with the result of the condition.
- */
-async function waitFor(condition) {
-  await BrowserTestUtils.waitForCondition(condition, "waitFor", 10, 500);
-  return condition();
-}

devtools/client/inspector/grids/test/browser_grids_accordion-state.js

@@ -102,7 +102,7 @@ async function testAccordionStateAfterReopeningLayoutView(toolbox) {
   const gContent = doc.querySelector(".grid-pane ._content");
 
   info("Checking the state of the grid panel.");
-  ok(!gContent, "The grid panel content is not rendered.");
+  is(gContent.children.length, 0, "The grid panel content is not rendered.");
   ok(
     !Services.prefs.getBoolPref(GRID_OPENED_PREF),
     `${GRID_OPENED_PREF} is pref off.`

devtools/client/inspector/layout/components/Accordion.js

@@ -48,7 +48,7 @@ class Accordion extends PureComponent {
     }
 
     if (item.onToggled) {
-      item.onToggled();
+      item.onToggled(isOpened);
     }
 
     this.setState({
@@ -87,15 +87,15 @@ class Accordion extends PureComponent {
         span({ className: "truncate" }, item.header)
       ),
 
-      isCreated || isOpened
-        ? div(
-            {
-              className: "_content",
-              style: { display: isOpened ? "block" : "none" },
-            },
-            createElement(item.component, item.componentProps || {})
-          )
-        : null
+      div(
+        {
+          className: "_content",
+          style: { display: isOpened ? "block" : "none" },
+        },
+        isCreated || isOpened
+          ? createElement(item.component, item.componentProps || {})
+          : null
+      )
     );
   }
 

devtools/client/inspector/layout/components/LayoutApp.js

@@ -40,9 +40,11 @@ const BOXMODEL_L10N = new LocalizationHelper(BOXMODEL_STRINGS_URI);
 const LAYOUT_STRINGS_URI = "devtools/client/locales/layout.properties";
 const LAYOUT_L10N = new LocalizationHelper(LAYOUT_STRINGS_URI);
 
-const BOXMODEL_OPENED_PREF = "devtools.layout.boxmodel.opened";
 const FLEXBOX_OPENED_PREF = "devtools.layout.flexbox.opened";
+const FLEX_CONTAINER_OPENED_PREF = "devtools.layout.flex-container.opened";
+const FLEX_ITEM_OPENED_PREF = "devtools.layout.flex-item.opened";
 const GRID_OPENED_PREF = "devtools.layout.grid.opened";
+const BOXMODEL_OPENED_PREF = "devtools.layout.boxmodel.opened";
 
 class LayoutApp extends PureComponent {
   static get propTypes() {
@@ -78,24 +80,80 @@ class LayoutApp extends PureComponent {
     this.scrollToTop = this.scrollToTop.bind(this);
   }
 
-  getFlexboxHeader(flexContainer) {
+  getBoxModelSection() {
+    return {
+      component: BoxModel,
+      componentProps: this.props,
+      header: BOXMODEL_L10N.getStr("boxmodel.title"),
+      opened: Services.prefs.getBoolPref(BOXMODEL_OPENED_PREF),
+      onToggled: opened => {
+        Services.prefs.setBoolPref(BOXMODEL_OPENED_PREF, opened);
+      },
+    };
+  }
+
+  getFlexAccordionData(flexContainer) {
     if (!flexContainer.actorID) {
       // No flex container or flex item selected.
-      return LAYOUT_L10N.getStr("flexbox.header");
+      return {
+        pref: FLEXBOX_OPENED_PREF,
+        className: "flex-accordion flex-accordion-none",
+        header: LAYOUT_L10N.getStr("flexbox.header"),
+      };
     } else if (!flexContainer.flexItemShown) {
       // No flex item selected.
-      return LAYOUT_L10N.getStr("flexbox.flexContainer");
+      return {
+        pref: FLEX_CONTAINER_OPENED_PREF,
+        className: "flex-accordion flex-accordion-container",
+        header: LAYOUT_L10N.getStr("flexbox.flexContainer"),
+      };
     }
 
-    const grip = translateNodeFrontToGrip(flexContainer.nodeFront);
-    return LAYOUT_L10N.getFormatStr(
-      "flexbox.flexItemOf",
-      getSelectorFromGrip(grip)
+    return {
+      pref: FLEX_ITEM_OPENED_PREF,
+      className: "flex-accordion flex-accordion-item",
+      header: LAYOUT_L10N.getFormatStr(
+        "flexbox.flexItemOf",
+        getSelectorFromGrip(translateNodeFrontToGrip(flexContainer.nodeFront))
+      ),
+    };
+  }
+
+  getFlexSection(flexContainer) {
+    const { pref, className, header } = this.getFlexAccordionData(
+      flexContainer
     );
+
+    return {
+      className,
+      component: Flexbox,
+      componentProps: {
+        ...this.props,
+        flexContainer,
+        scrollToTop: this.scrollToTop,
+      },
+      header,
+      opened: Services.prefs.getBoolPref(pref),
+      onToggled: opened => {
+        Services.prefs.setBoolPref(pref, opened);
+      },
+    };
+  }
+
+  getGridSection() {
+    return {
+      component: Grid,
+      componentProps: this.props,
+      header: LAYOUT_L10N.getStr("layout.header"),
+      opened: Services.prefs.getBoolPref(GRID_OPENED_PREF),
+      onToggled: opened => {
+        Services.prefs.setBoolPref(GRID_OPENED_PREF, opened);
+      },
+    };
   }
 
   /**
-   * Scrolls to top of the layout container.
+   * Scrolls to the top of the layout container.
    */
   scrollToTop() {
     this.containerRef.current.scrollTop = 0;
@@ -105,45 +163,9 @@ class LayoutApp extends PureComponent {
     const { flexContainer, flexItemContainer } = this.props.flexbox;
 
     const items = [
-      {
-        className: `flex-accordion ${
-          flexContainer.flexItemShown ? "item" : "container"
-        }`,
-        component: Flexbox,
-        componentProps: {
-          ...this.props,
-          flexContainer,
-          scrollToTop: this.scrollToTop,
-        },
-        header: this.getFlexboxHeader(flexContainer),
-        opened: Services.prefs.getBoolPref(FLEXBOX_OPENED_PREF),
-        onToggled: () => {
-          Services.prefs.setBoolPref(
-            FLEXBOX_OPENED_PREF,
-            !Services.prefs.getBoolPref(FLEXBOX_OPENED_PREF)
-          );
-        },
-      },
-      {
-        component: Grid,
-        componentProps: this.props,
-        header: LAYOUT_L10N.getStr("layout.header"),
-        opened: Services.prefs.getBoolPref(GRID_OPENED_PREF),
-        onToggled: () => {
-          const opened = Services.prefs.getBoolPref(GRID_OPENED_PREF);
-          Services.prefs.setBoolPref(GRID_OPENED_PREF, !opened);
-        },
-      },
-      {
-        component: BoxModel,
-        componentProps: this.props,
-        header: BOXMODEL_L10N.getStr("boxmodel.title"),
-        opened: Services.prefs.getBoolPref(BOXMODEL_OPENED_PREF),
-        onToggled: () => {
-          const opened = Services.prefs.getBoolPref(BOXMODEL_OPENED_PREF);
-          Services.prefs.setBoolPref(BOXMODEL_OPENED_PREF, !opened);
-        },
-      },
+      this.getFlexSection(flexContainer),
+      this.getGridSection(),
+      this.getBoxModelSection(),
     ];
 
     // If the current selected node is both a flex container and flex item. Render
@@ -158,23 +180,7 @@ class LayoutApp extends PureComponent {
       items.splice(
         this.props.flexbox.initiatedByMarkupViewSelection ? 1 : 0,
         0,
-        {
-          className: "flex-accordion item",
-          component: Flexbox,
-          componentProps: {
-            ...this.props,
-            flexContainer: flexItemContainer,
-            scrollToTop: this.scrollToTop,
-          },
-          header: this.getFlexboxHeader(flexItemContainer),
-          opened: Services.prefs.getBoolPref(FLEXBOX_OPENED_PREF),
-          onToggled: () => {
-            Services.prefs.setBoolPref(
-              FLEXBOX_OPENED_PREF,
-              !Services.prefs.getBoolPref(FLEXBOX_OPENED_PREF)
-            );
-          },
-        }
+        this.getFlexSection(flexItemContainer)
       );
     }
 

devtools/client/inspector/markup/test/browser_markup_dom_mutation_breakpoints.js

@@ -7,21 +7,6 @@
 // Test inspector markup view handling DOM mutation breakpoints icons
 // The icon should display when a breakpoint exists for a given node
 
-async function waitFor(
-  condition,
-  message = "waitFor",
-  interval = 10,
-  maxTries = 500
-) {
-  await BrowserTestUtils.waitForCondition(
-    condition,
-    message,
-    interval,
-    maxTries
-  );
-  return condition();
-}
-
 function toggleMutationBreakpoint(inspector) {
   const allMenuItems = openContextMenuAndGetAllItems(inspector);
   const attributeMenuItem = allMenuItems.find(

devtools/client/inspector/test/head.js

@@ -742,6 +742,35 @@ var waitForTab = async function() {
   return tab;
 };
 
+/**
+ * Wait for a predicate to return a result.
+ *
+ * @param {Function} condition
+ *        Invoked once in a while until it returns a truthy value. This should be an
+ *        idempotent function, since we have to run it a second time after it returns
+ *        true in order to return the value.
+ * @param {String} message [optional]
+ *        A message to output if the condition fails.
+ * @param {Number} interval [optional]
+ *        How often the predicate is invoked, in milliseconds.
+ * @return {Object}
+ *         A promise that is resolved with the result of the condition.
+ */
+async function waitFor(
+  condition,
+  message = "waitFor",
+  interval = 10,
+  maxTries = 500
+) {
+  await BrowserTestUtils.waitForCondition(
+    condition,
+    message,
+    interval,
+    maxTries
+  );
+  return condition();
+}
+
 /**
  * Simulate the key input for the given input in the window.
  *