Bug 1501207 - Don't say that an item was both set and not set to grow; r=jdescottes
authorPatrick Brosset <pbrosset@mozilla.com>
Wed, 24 Oct 2018 08:19:01 +0000
changeset 442736 c4123efb05361326c6ad4da81e8d826367efea36
parent 442735 0ce248abad81e801972662b8c56a726f039a9cf0
child 442737 f2a0505d9f54131436b6172a079a55a6aca22460
push id34921
push usershindli@mozilla.com
push dateWed, 24 Oct 2018 13:27:03 +0000
treeherdermozilla-central@d94d73fcec77 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdescottes
bugs1501207
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 1501207 - Don't say that an item was both set and not set to grow; r=jdescottes Differential Revision: https://phabricator.services.mozilla.com/D9504
devtools/client/inspector/flexbox/components/FlexItemSizingProperties.js
devtools/client/inspector/flexbox/test/browser.ini
devtools/client/inspector/flexbox/test/browser_flexbox_sizing_grow_and_not_grow.js
devtools/client/inspector/flexbox/test/doc_flexbox_specific_cases.html
--- a/devtools/client/inspector/flexbox/components/FlexItemSizingProperties.js
+++ b/devtools/client/inspector/flexbox/components/FlexItemSizingProperties.js
@@ -117,18 +117,19 @@ class FlexItemSizingProperties extends P
       // If not and width/height is defined, then that's what defines the base size.
       property = this.renderCssProperty(dimension, dimensionValue);
     } else {
       // Finally, if nothing is set, then the base size is the max-content size.
       reason = this.renderReasons(
         [getStr("flexbox.itemSizing.itemBaseSizeFromContent")]);
     }
 
+    const className = "section base";
     return (
-      dom.li({ className: property ? "section" : "section no-property" },
+      dom.li({ className: className + (property ? "" : " no-property") },
         dom.span({ className: "name" },
           getStr("flexbox.itemSizing.baseSizeSectionHeader")
         ),
         dom.span({ className: "value theme-fg-color1" },
           this.getRoundedDimension(mainBaseSize)
         ),
         property,
         reason
@@ -145,17 +146,17 @@ class FlexItemSizingProperties extends P
     } = flexItemSizing;
 
     // Don't attempt to display anything useful if everything is 0.
     if (!mainFinalSize && !mainBaseSize && !mainDeltaSize) {
       return null;
     }
 
     const flexGrow = properties["flex-grow"];
-    const flexGrow0 = parseFloat(flexGrow) === 0;
+    const nonZeroFlexGrowDefined = flexGrow && parseFloat(flexGrow) !== 0;
     const flexShrink = properties["flex-shrink"];
     const flexShrink0 = parseFloat(flexShrink) === 0;
     const grew = mainDeltaSize > 0;
     const shrank = mainDeltaSize < 0;
     // TODO: replace this with the new clamping state that the API will return once bug
     // 1498273 is fixed.
     const wasClamped = mainDeltaSize + mainBaseSize !== mainFinalSize;
 
@@ -165,23 +166,23 @@ class FlexItemSizingProperties extends P
     // not on the line.
     if (lineGrowthState === "growing") {
       reasons.push(getStr("flexbox.itemSizing.extraRoomOnLine"));
     } else if (lineGrowthState === "shrinking") {
       reasons.push(getStr("flexbox.itemSizing.notEnoughRoomOnLine"));
     }
 
     // Then tell users whether the item was set to grow, shrink or none of them.
-    if (flexGrow && !flexGrow0 && lineGrowthState !== "shrinking") {
+    if (nonZeroFlexGrowDefined && lineGrowthState !== "shrinking") {
       reasons.push(getStr("flexbox.itemSizing.setToGrow"));
     }
     if (flexShrink && !flexShrink0 && lineGrowthState !== "growing") {
       reasons.push(getStr("flexbox.itemSizing.setToShrink"));
     }
-    if (!grew && !shrank && lineGrowthState === "growing") {
+    if (!nonZeroFlexGrowDefined && !grew && !shrank && lineGrowthState === "growing") {
       reasons.push(getStr("flexbox.itemSizing.notSetToGrow"));
     }
     if (!grew && !shrank && lineGrowthState === "shrinking") {
       reasons.push(getStr("flexbox.itemSizing.notSetToShrink"));
     }
 
     let property = null;
 
@@ -206,17 +207,17 @@ class FlexItemSizingProperties extends P
         property = this.renderCssProperty("flex-shrink", "1", true);
       }
 
       if (wasClamped) {
         // It might have wanted to shrink more (to accomodate all items) but couldn't
         // because it was later min-clamped.
         reasons.push(getStr("flexbox.itemSizing.shrinkAttemptWhenClamped"));
       }
-    } else if (lineGrowthState === "growing" && flexGrow && !flexGrow0) {
+    } else if (lineGrowthState === "growing" && nonZeroFlexGrowDefined) {
       // The item did not grow or shrink. There was room on the line and flex-grow was
       // set, other items have likely used up all of the space.
       property = this.renderCssProperty("flex-grow", flexGrow);
       reasons.push(getStr("flexbox.itemSizing.growthAttemptButSiblings"));
     } else if (lineGrowthState === "shrinking") {
       // The item did not grow or shrink and there wasn't enough room on the line.
       if (!flexShrink0) {
         // flex-shrink was set (either defined in CSS, or via its default value of 1).
@@ -239,18 +240,19 @@ class FlexItemSizingProperties extends P
       }
     }
 
     // Don't display the section at all if there's nothing useful to show users.
     if (!property && !reasons.length) {
       return null;
     }
 
+    const className = "section flexibility";
     return (
-      dom.li({ className: property ? "section" : "section no-property" },
+      dom.li({ className: className + (property ? "" : " no-property") },
         dom.span({ className: "name" },
           getStr("flexbox.itemSizing.flexibilitySectionHeader")
         ),
         dom.span({ className: "value theme-fg-color1" },
           this.getFlexibilityValueString(grew, mainDeltaSize)
         ),
         property,
         this.renderReasons(reasons)
@@ -266,17 +268,17 @@ class FlexItemSizingProperties extends P
     // TODO: replace this with the new clamping state that the API will return once bug
     // 1498273 is fixed.
     const minDimensionValue = properties[`min-${dimension}`];
     if (mainMinSize !== mainFinalSize || !minDimensionValue) {
       return null;
     }
 
     return (
-      dom.li({ className: "section" },
+      dom.li({ className: "section min" },
         dom.span({ className: "name" },
           getStr("flexbox.itemSizing.minSizeSectionHeader")
         ),
         dom.span({ className: "value theme-fg-color1" },
           this.getRoundedDimension(mainMinSize)
         ),
         this.renderCssProperty(`min-${dimension}`, minDimensionValue)
       )
@@ -288,31 +290,31 @@ class FlexItemSizingProperties extends P
     // 1498273 is fixed.
     if (mainMaxSize !== mainFinalSize) {
       return null;
     }
 
     const maxDimensionValue = properties[`max-${dimension}`];
 
     return (
-      dom.li({ className: "section" },
+      dom.li({ className: "section max" },
         dom.span({ className: "name" },
           getStr("flexbox.itemSizing.maxSizeSectionHeader")
         ),
         dom.span({ className: "value theme-fg-color1" },
           this.getRoundedDimension(mainMaxSize)
         ),
         this.renderCssProperty(`max-${dimension}`, maxDimensionValue)
       )
     );
   }
 
   renderFinalSizeSection({ mainFinalSize }) {
     return (
-      dom.li({ className: "section no-property" },
+      dom.li({ className: "section final no-property" },
         dom.span({ className: "name" },
           getStr("flexbox.itemSizing.finalSizeSectionHeader")
         ),
         dom.span({ className: "value theme-fg-color1" },
           this.getRoundedDimension(mainFinalSize)
         )
       )
     );
--- a/devtools/client/inspector/flexbox/test/browser.ini
+++ b/devtools/client/inspector/flexbox/test/browser.ini
@@ -1,27 +1,29 @@
 [DEFAULT]
 tags = devtools
 subsuite = devtools
 support-files =
+  doc_flexbox_pseudos.html
   doc_flexbox_simple.html
-  doc_flexbox_pseudos.html
+  doc_flexbox_specific_cases.html
   doc_flexbox_text_nodes.html
   head.js
   !/devtools/client/inspector/test/head.js
   !/devtools/client/inspector/test/shared-head.js
   !/devtools/client/shared/test/shared-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_highlighter_color_picker_on_ESC.js]
 [browser_flexbox_highlighter_color_picker_on_RETURN.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_rotates_for_column.js]
 [browser_flexbox_pseudo_elements_are_listed.js]
+[browser_flexbox_sizing_grow_and_not_grow.js]
 [browser_flexbox_sizing_info_exists.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_text_nodes_are_listed.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/flexbox/test/browser_flexbox_sizing_grow_and_not_grow.js
@@ -0,0 +1,33 @@
+/* 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";
+
+const { getStr } = require("devtools/client/inspector/layout/utils/l10n");
+
+// Non-regression for bug 1501207.
+// In this bug, an item that was set to grow, with a flex-basis larger than its max-width
+// was marked both as "set to grow" and "not set to grow" at the same time.
+// So this test checks that this does not happen anymore.
+
+const TEST_URI = URL_ROOT + "doc_flexbox_specific_cases.html";
+
+add_task(async function() {
+  await addTab(TEST_URI);
+  const { inspector, flexboxInspector } = await openLayoutView();
+  const { document: doc } = flexboxInspector;
+
+  info("Select the test item in the document and wait for the sizing info to render");
+  const onFlexibilityReasonsRendered = waitForDOM(doc,
+    "ul.flex-item-sizing .section.flexibility .reasons");
+  await selectNode("#grow-not-grow div:first-child", inspector);
+  const [reasonsList] = await onFlexibilityReasonsRendered;
+
+  info("Get the list of reasons in the flexibility section");
+  const [...reasons] = reasonsList.querySelectorAll("li");
+  ok(reasons.some(r => r.textContent === getStr("flexbox.itemSizing.setToGrow")),
+     "The 'set to grow' reason was found");
+  ok(reasons.every(r => r.textContent !== getStr("flexbox.itemSizing.notSetToGrow")),
+     "The 'not set to grow' reason was not found");
+});
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/flexbox/test/doc_flexbox_specific_cases.html
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<style>
+#grow-not-grow {
+  display:flex;
+  width:500px;
+  border:1px solid;
+}
+#grow-not-grow div:first-child {
+  flex-grow:1;
+  flex-basis:100px;
+  max-width:50px;
+}
+#grow-not-grow div:last-child {
+  flex-grow: 1;
+}
+</style>
+<div id="grow-not-grow">
+  <div>item 1</div>
+  <div>item 2</div>
+</div>