From 83af8e000a2a0c923eba24e869208d3da0328f76 Mon Sep 17 00:00:00 2001 From: Florens Verschelde Date: Wed, 25 Sep 2019 07:57:26 +0000 Subject: [PATCH] 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 | 4 + ...browser_boxmodel_layout-accordion-state.js | 6 +- .../test/browser_flexbox_accordion_state.js | 106 +++++++++----- ...xbox_container_and_item_accordion_state.js | 10 +- .../test/doc_flexbox_specific_cases.html | 2 +- .../client/inspector/flexbox/test/head.js | 24 +++- devtools/client/inspector/fonts/test/head.js | 14 -- .../test/browser_grids_accordion-state.js | 2 +- .../inspector/layout/components/Accordion.js | 20 +-- .../inspector/layout/components/LayoutApp.js | 136 +++++++++--------- ...browser_markup_dom_mutation_breakpoints.js | 15 -- devtools/client/inspector/test/head.js | 29 ++++ 12 files changed, 211 insertions(+), 157 deletions(-) diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js index f05866ce1bc3..bf4ee9371c12 100644 --- a/browser/app/profile/firefox.js +++ b/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); diff --git a/devtools/client/inspector/boxmodel/test/browser_boxmodel_layout-accordion-state.js b/devtools/client/inspector/boxmodel/test/browser_boxmodel_layout-accordion-state.js index 2d7fa52cc6fe..9f66defa5359 100644 --- a/devtools/client/inspector/boxmodel/test/browser_boxmodel_layout-accordion-state.js +++ b/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.` diff --git a/devtools/client/inspector/flexbox/test/browser_flexbox_accordion_state.js b/devtools/client/inspector/flexbox/test/browser_flexbox_accordion_state.js index 5c1fe031471f..812a9cd7ff29 100644 --- a/devtools/client/inspector/flexbox/test/browser_flexbox_accordion_state.js +++ b/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, + }; } diff --git a/devtools/client/inspector/flexbox/test/browser_flexbox_container_and_item_accordion_state.js b/devtools/client/inspector/flexbox/test/browser_flexbox_container_and_item_accordion_state.js index 948c538cacc3..7350dc3fa29f 100644 --- a/devtools/client/inspector/flexbox/test/browser_flexbox_container_and_item_accordion_state.js +++ b/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" ); }); diff --git a/devtools/client/inspector/flexbox/test/doc_flexbox_specific_cases.html b/devtools/client/inspector/flexbox/test/doc_flexbox_specific_cases.html index bc32f69a4655..f5a6aaad24c7 100644 --- a/devtools/client/inspector/flexbox/test/doc_flexbox_specific_cases.html +++ b/devtools/client/inspector/flexbox/test/doc_flexbox_specific_cases.html @@ -116,6 +116,6 @@
-
This item is inside a container-item element
+
This item is inside a container-item element
diff --git a/devtools/client/inspector/flexbox/test/head.js b/devtools/client/inspector/flexbox/test/head.js index 17f0f34753de..627a8e3ffd79 100644 --- a/devtools/client/inspector/flexbox/test/head.js +++ b/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); }); /** diff --git a/devtools/client/inspector/fonts/test/head.js b/devtools/client/inspector/fonts/test/head.js index 285ded545693..43402b8fd5f0 100644 --- a/devtools/client/inspector/fonts/test/head.js +++ b/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(); -} diff --git a/devtools/client/inspector/grids/test/browser_grids_accordion-state.js b/devtools/client/inspector/grids/test/browser_grids_accordion-state.js index 82e78cb7494e..700b1aa3d8ce 100644 --- a/devtools/client/inspector/grids/test/browser_grids_accordion-state.js +++ b/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.` diff --git a/devtools/client/inspector/layout/components/Accordion.js b/devtools/client/inspector/layout/components/Accordion.js index 2fa1ace5fd9e..06c6a2b8c9b3 100644 --- a/devtools/client/inspector/layout/components/Accordion.js +++ b/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 + ) ); } diff --git a/devtools/client/inspector/layout/components/LayoutApp.js b/devtools/client/inspector/layout/components/LayoutApp.js index 7c194d4d236d..8936aa79a993 100644 --- a/devtools/client/inspector/layout/components/LayoutApp.js +++ b/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) ); } diff --git a/devtools/client/inspector/markup/test/browser_markup_dom_mutation_breakpoints.js b/devtools/client/inspector/markup/test/browser_markup_dom_mutation_breakpoints.js index d2f19d087b59..015117731514 100644 --- a/devtools/client/inspector/markup/test/browser_markup_dom_mutation_breakpoints.js +++ b/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( diff --git a/devtools/client/inspector/test/head.js b/devtools/client/inspector/test/head.js index cb5ba881634a..474a06683da9 100644 --- a/devtools/client/inspector/test/head.js +++ b/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. *