Based on a proposal I made recently to help solve the flex item sizing part of our flexbox tool: https://docs.google.com/document/d/163NIuKm40VlhtXDl0WR_EQ6-YFbUa3k-9wO4k-BEhYw/edit Latest mockups that show this UI: https://mozilla.invisionapp.com/share/36N5CTMQEWF#/screens/310617460 See screens 6 and 7. The goal is to add a little diagram/graph/widget in the flexbox layout sidebar that appears when a flex item is selected. This widget will represent the item in a simple form, and provide information about its min size, base size, main size and max size. It will explain what each of these sizes are and where they come from, and will explain why the item ended up being the size it currently is in the page.

First draft for this. This was set up in 1.5 day, so very early, dirty, incomplete, and full of FIXMEs. But I wanted to get it out there to illustrate my thoughts so far. Gabriel, as discussed, feel free to resume from here.

Comment on attachment 9004119 [details] [diff] [review] Part 0: Reorganize the flex container checkbox and flex container properties to match new designs. [1.0] Review of attachment 9004119 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/flexbox/components/FlexContainer.js @@ +113,2 @@ > dom.label({}, > + dom.input({ Move `{` to new like and also for Rep props to reflect format elsewhere in this file. ::: devtools/client/themes/common.css @@ +801,5 @@ > font-size: 12px; > } > + > +/* Checkbox Toggle: Restyle a native checkbox input to look like a toggle with a "thumb". > + * Build the decoration using solid shapes created with radial- and linear-gradient This comment is outdated. Update or remove it. @@ +807,5 @@ > +.devtools-checkbox-toggle { > + --x-pos: .15em; > + /* Reset native checkbox styling. */ > + -moz-appearance: none; > + background-color: var(--toggle-track-color); Need to bring over `--toggle-track-color` from fonts.css and remove it from there. @@ +840,5 @@ > + transform: translateY(.15em) translateX(var(--x-pos)); > + border-radius: 100%; > + display: block; > + content: ""; > + background-color: var(--toggle-thumb-color); Need to bring over `--toggle-thumb-color` from fonts.css and remove it from there. ::: devtools/client/themes/fonts.css @@ -364,5 @@ > -.font-value-toggle { > - --x-pos: .15em; > - /* Reset native checkbox styling. */ > - -moz-appearance: none; > - background-color: var(--toggle-track-color); Remove `--toggle-track-color` and `--toggle-thumb-color` declarations from the top of this file.

Comment on attachment 9004123 [details] [diff] [review] Part 3: Return the computed style properties and flex item sizing information from the FlexItemActor [5.0] Review of attachment 9004123 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/server/actors/layout.js @@ +154,5 @@ > > + const { flexDirection } = CssLogic.getComputedStyle(this.containerEl); > + const styles = CssLogic.getComputedStyle(this.element); > + const clientRect = this.element.getBoundingClientRect(); > + const widthOrHeight = flexDirection.startsWith("row") ? "Width" : "Height"; s/widthOrHeight/dimension You can assign lowercase "width" or "height". The result from CssLogic.getComputedStyle() returns properties in both camelCase and kebab-case. You don't have to use .toLowerCase() and can use this instead: ``` [`min-${dimension}`]: styles[`min-${dimension}`], ```

Comment on attachment 9004124 [details] [diff] [review] Part 4: Implement the methods to show the flex item highlighter from HighlightersOverlay. [1.0] Review of attachment 9004124 [details] [diff] [review]: ----------------------------------------------------------------- This logic doesn't seem to be used yet, but I assume you have a follow-up patch for that.

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/55ab0c5a32b0 Part 0: Reorganize the flex container checkbox and flex container properties to match new designs. r=rcaliman https://hg.mozilla.org/integration/mozilla-inbound/rev/f21e10fcd4fe Part 1: Implement a FlexItemActor to retrieve information about the flex items. r=rcaliman https://hg.mozilla.org/integration/mozilla-inbound/rev/58fb697a29d3 Part 2: Display an ordered list of flex items when the flex container is highlighted. r=rcaliman https://hg.mozilla.org/integration/mozilla-inbound/rev/4a5c07069012 Part 3: Return the computed style properties and flex item sizing information from the FlexItemActor. r=rcaliman https://hg.mozilla.org/integration/mozilla-inbound/rev/920027e02ee1 Part 4: Implement the methods to show the flex item highlighter from HighlightersOverlay. r=rcaliman

https://hg.mozilla.org/mozilla-central/rev/55ab0c5a32b0 https://hg.mozilla.org/mozilla-central/rev/f21e10fcd4fe https://hg.mozilla.org/mozilla-central/rev/58fb697a29d3 https://hg.mozilla.org/mozilla-central/rev/4a5c07069012 https://hg.mozilla.org/mozilla-central/rev/920027e02ee1

This doesn't fully match the design spec yet when it comes to the button selection for the flex items, but the point of this part is to display the flex item sizing properties so we can have people testing that section.

Comment on attachment 9004754 [details] [diff] [review] Part 5: Display the flex item sizing properties in the flexbox panel. [1.0] Review of attachment 9004754 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/flexbox/components/Flexbox.js @@ +96,5 @@ > onShowBoxModelHighlighterForNode, > onToggleFlexboxHighlighter, > setSelectedNode, > }), > + this.renderFlexItem(), Is a bit confusing that this method is called renderFlexItem() but can return either a FlexItemSizingProperties for a single flex item or a FlexItemList. Perhaps extract FlexItemList from renderFlexItem() and have the method return null when there's no selectedFlexItem. In the render() method, conditionally use the returned value or a FlexItemList. ::: devtools/client/inspector/flexbox/flexbox.js @@ +369,5 @@ > } > > flexItems.push({ > actorID: flexItemFront.actorID, > + checked: false, Did you mean `shown` instead of `checked`? ::: devtools/client/inspector/flexbox/reducers/flexbox.js @@ +36,5 @@ > + [TOGGLE_FLEX_ITEM_SHOWN](flexbox, { nodeFront }) { > + return Object.assign({}, flexbox, { > + flexItems: flexbox.flexItems.map(flexItem => { > + if (flexItem.nodeFront !== nodeFront) { > + return Object.assign({}, flexItem, { Here we're doing only a shallow copy of flexItem which means that values nested deeper like the ones in `properties` or `nodeFront` don't get copied over but get passed by reference instead. See: https://redux.js.org/recipes/structuringreducers/immutableupdatepatterns#updating-nested-objects We don't do anything with Redux yet where this could be an issue. But this mutation _may_ bite us at some point. Redux encourages storing primitives and shallow structures that are simple to serialize. Not a blocker now, but a thing to keep in mind if unexpected state bugs show up. nodeFront would be a prime candidate for plucking off only the required properties. ::: devtools/client/inspector/flexbox/types.js @@ +18,1 @@ > flexItemSizing: PropTypes.object, What do you think about defining a type in this file for `flexItemSizing`? I find types.js to be a useful quick reference of what's expected to be inside unfamiliar objects.

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b4dc5ede8f56 Part 5: Display the flex item sizing properties in the flexbox panel. r=rcaliman

Comment on attachment 9006323 [details] [diff] [review] Part 6: Refactor the styles in the layout panel. [1.0] Review of attachment 9006323 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/grids/components/GridItem.js @@ +38,5 @@ > this.setGridColor = this.setGridColor.bind(this); > } > > componentDidMount() { > + const swatchEl = findDOMNode(this).querySelector(".layout-color-swatch"); Could we use React.createRef() to store and reference the swatch? https://reactjs.org/docs/refs-and-the-dom.html#creating-refs If so, the ref could be reused in componentWillUnmount()

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/60c2e9b522a4 Part 6: Refactor the styles in the layout panel. r=rcaliman

Comment on attachment 9006984 [details] [diff] [review] Part 7: Add a flex item selector in the flexbox panel. [1.0] Review of attachment 9006984 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/flexbox/components/FlexContainer.js @@ +36,5 @@ > > constructor(props) { > super(props); > > this.colorValueEl = createRef(); Thanks for doing this! ::: devtools/client/themes/layout.css @@ +134,5 @@ > + left: -12px; > + top: 3px; > + content: ''; > + display: block; > + border-left: 0.5px solid var(--flex-item-selector-wrapper-border-color); Is this sub-pixel border width going to cause trouble on non-Hi-DPI monitors?

Comment on attachment 9006984 [details] [diff] [review] Part 7: Add a flex item selector in the flexbox panel. [1.0] Review of attachment 9006984 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/themes/layout.css @@ +134,5 @@ > + left: -12px; > + top: 3px; > + content: ''; > + display: block; > + border-left: 0.5px solid var(--flex-item-selector-wrapper-border-color); I haven't heard of any complaints yet about this, but we can revisit this. This styling is actually copied and modified from the overridden rules https://searchfox.org/mozilla-central/source/devtools/client/themes/rules.css#385.

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/123d63eccdb7 Part 7: Add a flex item selector in the flexbox panel. r=rcaliman https://hg.mozilla.org/integration/mozilla-inbound/rev/154fd1725864 Part 8: Refactor the flex item shown to prevent it from being hidden on reflow updates. r=rcaliman

https://hg.mozilla.org/mozilla-central/rev/123d63eccdb7 https://hg.mozilla.org/mozilla-central/rev/154fd1725864

If the current selected element in the markup view is a flex item, we will display its flex item sizing properties.

Comment on attachment 9009672 [details] [diff] [review] Part 9: Adjust the flexbox accordion header to match the design spec. [1.0] Review of attachment 9009672 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/shared/utils.js @@ +109,5 @@ > return elt; > } > > /** > + * Returns a selector label of the Element Rep from the grip. This is based on the We can remove the "label" from this comment and from the body of the function (rename `let label` to `let selector`). The output is a fully compliant selector anyway. The fact that it's used as a label is relevant only to the consumer. Given that this is a `utils` module, the selector may end up being used for more than just a label in other use cases. ::: devtools/client/locales/en-US/layout.properties @@ +10,3 @@ > flexbox.header=Flexbox > +flexbox.flexContainer=Flex Container > +flexbox.flexItemOf=Flex Item of %s The translators may want a note to know what %s will be in `Flex Item of %s` to understand the context.

Comment on attachment 9009733 [details] [diff] [review] Part 10: Adjust the flexbox header to match the design spec. [1.0] Review of attachment 9009733 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/flexbox/components/FlexItemSelector.js @@ +63,5 @@ > > render() { > const { flexItem } = this.props; > > + return createElement(Fragment, null, Is a Fragment wrapper needed here? It seems to wrap a single `dom.button`.

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/65cb9952c79d Part 9: Adjust the flexbox accordion header to match the design spec. r=rcaliman

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8a5ca81f8429 Part 10: Adjust the flexbox header to match the design spec. r=rcaliman https://hg.mozilla.org/integration/mozilla-inbound/rev/faf1ea0e1a15 Part 11: Add a prev button to navigate from the flex item sizing view to flex container view. r=rcaliman https://hg.mozilla.org/integration/mozilla-inbound/rev/b11a8565deff Part 12: Don't render the flex item list if there are no flex items. r=rcaliman https://hg.mozilla.org/integration/mozilla-inbound/rev/588d147e401b Part 13: Show the flex item sizing properties of the currently selected element in the markup view. r=rcaliman https://hg.mozilla.org/integration/mozilla-inbound/rev/c7db1d98d62b Part 14: Selecting a flex item in the flex item selector should select that element in the markup view. r=rcaliman

Backout by toros@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3655d882ad9c Backed out 5 changesets for failures on browser/base/content/test/static/browser_parsable_css.js on a CLOSED TREE

Backed out 5 changesets for failures on browser/base/content/test/static/browser_parsable_css.js Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/3655d882ad9c1762ffc8af04f8962cae986ee8f4 Push with the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c7db1d98d62b6fbf4e4920eb3f6c24b7136239b4 Failure log file: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c7db1d98d62b6fbf4e4920eb3f6c24b7136239b4&selectedJob=199948878

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8535a198a105 Part 10: Adjust the flexbox header to match the design spec. r=rcaliman https://hg.mozilla.org/integration/mozilla-inbound/rev/640618689bcb Part 11: Add a prev button to navigate from the flex item sizing view to flex container view. r=rcaliman https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd4d2d28e7d Part 12: Don't render the flex item list if there are no flex items. r=rcaliman https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a2efd0c5b3 Part 13: Show the flex item sizing properties of the currently selected element in the markup view. r=rcaliman https://hg.mozilla.org/integration/mozilla-inbound/rev/f10b145f9b74 Part 14: Selecting a flex item in the flex item selector should select that element in the markup view. r=rcaliman

https://hg.mozilla.org/mozilla-central/rev/8535a198a105 https://hg.mozilla.org/mozilla-central/rev/640618689bcb https://hg.mozilla.org/mozilla-central/rev/7bd4d2d28e7d https://hg.mozilla.org/mozilla-central/rev/a3a2efd0c5b3 https://hg.mozilla.org/mozilla-central/rev/f10b145f9b74

@pbro, I couldn't ping you for review for Part 15 because of your away setting, but please go ahead with reviewing it. I kinda reverted the arrow design to the old spec to make immediate progress. I could use some css to get the arrow seen in https://mozilla.invisionapp.com/d/main#/console/14988591/319386527/preview.

Comment on attachment 9009672 [details] [diff] [review] Part 9: Adjust the flexbox accordion header to match the design spec. [1.0] Review of attachment 9009672 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/locales/en-US/layout.properties @@ +10,3 @@ > flexbox.header=Flexbox > +flexbox.flexContainer=Flex Container > +flexbox.flexItemOf=Flex Item of %s Is %s lowercase correct here? I'm not exactly sure for devtools, but this would break in standard stringbundles

Comment on attachment 9010132 [details] [diff] [review] Part 15: Implement the flex item sizing outline in the flexbox panel. [2.0] Review of attachment 9010132 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Gabriel. Smart using CSS grid to position everything! Here are a few general remarks for things I noticed while testing: - In row direction, the outline has a fixed width which causes overflow if the sidebar is too thin, and doesn't use the space when the sidebar is wider. - Setting a super high max-width might draw the max point far far to the right, causing a large overflow too. - Setting a super high max-height in column mode will draw the max point far down, and in a way that it overlaps the rest of the UI below the outline. In fact it can be displayed over the flex container properties, or the grid inspector, or box model. In cases where the max value isn't very useful because clamping did not occur, it might be good to do one of these: - just don't draw the point - or draw it near the outline, even if that means the distance ratio isn't respected. Honestly, I would just not draw it at all. - In some cases, the basis and max labels can overlap and it's impossible to read what those labels say anymore. I don't think this can happen with min and final. I think we should orient these top labels depending on how they are positioned with respect to one another, so that they are always facing away from eachother, and never overlapping. - The clamp icon is sometimes incorrectly drawn and sometimes missing. I've seen an example where flex-basis:200px, flex:1, max-width:100px. In this case the final size is 100px, and the padlock should be displayed, right? It wasn't in my case. I've also seen a case where it was drawn at the incorrect place, but I can't remember the details. - Row-reverse and column-reverse directions aren't handled. Really the outline should face the other way in those cases. ::: devtools/client/inspector/flexbox/components/FlexItemSizingOutline.js @@ +18,5 @@ > + }; > + } > + > + renderBasisPoint(baseSize) { > + const isRow = this.props.flexDirection.startsWith("row"); You need to do this in 9 places in this file. Please create a getter on the class like: get isRow() { return this.props.flexDirection.startsWith("row"); } @@ +222,5 @@ > + let maxSize = 1; > + let minSize = 1; > + let deltaSize = 0; > + > + // Computed the relative size ratio to determine where to position the min, max, basis typo here: Compute instead of Computed. ::: devtools/client/themes/layout.css @@ +161,5 @@ > /** > + * Flex Item Sizing Outline > + */ > + > +#flex-item-sizing-outline-container { Probably unnecessary usage of an ID selector (here and in other selectors below). Knowing that it might be helpful to display several of these outlines in the future, let's use class selectors instead. @@ +174,5 @@ > + > +#flex-item-sizing-outline.row { > + grid-auto-rows: 35px; > + margin-bottom: 28px; > + width: 200px; A bit sad to be hard-coding this to 200px. Why not use a % value instead, so that the outline resizes with its sidebar? You could replace this with flex-basis:80% for instance, and maybe add a max-width if you think that after a certain point it becomes too wide and harder to read. @@ +180,5 @@ > + > +#flex-item-sizing-outline.column { > + grid-auto-columns: 35px; > + margin: 16px 0; > + height: 140px; In column mode, I guess we do have to set a hard-coded height. Resizing it based on the height of the sidebar will not make sense. @@ +185,5 @@ > + > +} > + > +#flex-item-sizing-outline-item { > + border: 1px solid #A744FF; We need to make this color match the color of the highlighter ideally. That's what we do for grids too. This would avoid hard-coding it here. @@ +195,5 @@ > + background-image: url(chrome://devtools/skin/images/lock.svg); > + background-size: 16px; > + background-repeat: no-repeat; > + display: inline-block; > + fill: #BD10E0; Same comment here. Can we derive this color from the highlighter color? @@ +201,5 @@ > + height: 16px; > + position: absolute; > + right: -8px; > + top: 8px; > + -moz-context-properties: fill; Can you please move this property to be next to the fill property (6 lines above)? It will help people understand how the fill is set to the svg element. @@ +205,5 @@ > + -moz-context-properties: fill; > +} > + > +#flex-item-sizing-outline-growth { > + border: 3px dotted rgba(167, 68, 255, 0.4); Same comment for this color here. We should use the highlighter color dynamically, and then apply the opacity. @@ +211,5 @@ > +} > + > +#flex-item-sizing-outline-delta { > + background-repeat: round; > + fill: #A744FF; The color is hard-coded here too. And in fact in multiple rules below too. Perhaps you could use the `currentColor` variable in all these rules, and then just set the `color` property once at the top level. @@ +231,5 @@ > +#flex-item-sizing-outline-delta.column.shrinking { > + background-image: url(chrome://devtools/skin/images/arrowhead-up.svg); > +} > + > +.point { All the selectors above are very specifically tied to the outline (with a long #flex-item-sizing-outline prefix), but starting here, this prefix is missing. All of the selectors below are therefore more risky to use in the inspector iframe, as they might accidentally apply to other elements in that iframe. Now, I don't think we have other elements with the point-top class for instance in the inspector, but we might very well have many elements with the class label. So, please scope these so we don't risk breaking other things. @@ +262,5 @@ > + position: relative; > + -moz-user-select: none; > +} > + > +.point-top .label::before { .point-top .label::before, .point-bottom .label::before, .point-left .label::before and .point-right .label::before share a good amount of CSS properties. Please make a rule with these 4 selectors to avoid repeating them (content, position, maybe border-color and thickness).

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c384ed897d2d Part 16: Cache the custom host colors and the overlay color for the current host to avoid unnecessary fetching. r=pbro https://hg.mozilla.org/integration/mozilla-inbound/rev/28b3efa55ebc Part 17: Replace %s with %S for flexbox.flexItemOf string in layout.properties. r=pbro

https://hg.mozilla.org/mozilla-central/rev/c384ed897d2d https://hg.mozilla.org/mozilla-central/rev/28b3efa55ebc

Comment on attachment 9011310 [details] [diff] [review] Part 19: Show both the flex container and flex item sizing properties when the selected element is both a flex item and container. [1.0] Review of attachment 9011310 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/flexbox/flexbox.js @@ +105,5 @@ > this.walker = null; > } > > + /** > + * The current selected node might be a flex item and flex container. It's very easy to get lost in this function. The terminology doesn't help. The element is a container, but it's parent is a container too. Honestly, I've read this comment and the function a couple of times and got lost every time. Do you mind trying to make this as self-explanatory as possible? Even if it means adding line comments in many places. @@ +115,5 @@ > + * 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 (containerNodeFront !== this.selection.nodeFront) { I think I understand the source of my confusion now. The tool used to work 1 way: if you selected an element and it was a container or an item, then the tool would show the information of the container (and some way to see any of its items sizing info). Now we're shifting away from this a bit, and I think the code should be refactored to show this more clearly. I don't think it does now. Let me know if you agree with this: - if the element is a container, then the accordion for containers is shown - if the element is an item, then the accordion for items is shown - if the user clicks on an item inside the item list of the accordion for containers, then the inspector.selection is changed to that item, and the sidebar refreshes - if the user clicks on the back arrow inside the accordion for items, then the inspector.selection is changed to go back to the container, and the sidebar refreshes - if the element is both a container and an item, then both accordions are shown So, with that simple logic, I don't really understand why we would check if containerNodeFront is the current selection. Why would we care about a container if it wasn't the current selection? Unless this is our only way to know that the current selection is an item. In which case, we should create a property like isItem instead (which could be defined as this.isItem = containerNodeFront !== this.selection.nodeFront) But at least having isItem would make some of the code easier to understand. ::: devtools/client/inspector/flexbox/reducers/flexbox.js @@ +16,3 @@ > // The color of the flexbox highlighter overlay. > color: "", > + // The flex container of the selected element. I think the structure of this store could be re-modeled after my last comment, to be simpler. Why would we care about the flex container of the selected element? If the selected element is indeed an item, then we show the item's accordion (with a back arrow to go back to the container, which when clicked would just select the container in the inspector). It could very well be that I'm mistaken about the way this is supposed to work. But this was my understanding, and how I thought others understood too. So, based on this, the data model could be something like: // If the selected element is a flex container flexContainer: { actorID, flexItems, properties } // If the selected element is a flex item flexItem: { actorID, properties } @@ +22,5 @@ > + // An array of flex items belonging to the selected flex container. > + flexItems: [], > + // The NodeFront actor ID of the flex item to display in the flex item sizing > + // properties. > + flexItemShown: null, Based on what I said, you wouldn't need to know which of the flex items is shown here, because that information is in the flexItem part of the data model. ::: devtools/server/actors/layout.js @@ +102,5 @@ > const flexItemActors = []; > > for (const line of flex.getLines()) { > for (const item of line.getItems()) { > + if (item.node.nodeType !== ELEMENT_NODE) { Hmm, looks like this will filter out text nodes and pseudo-elements, right? As discussed, we do want to show those in the UI, because they *are* flex items (well, not really, but gecko creates anonymous flex items around them still).

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e883cd321684 Part 18: Refactor the logic to get the flex items for a given flex container. r=pbro

Comment on attachment 9010132 [details] [diff] [review] Part 15: Implement the flex item sizing outline in the flexbox panel. [2.0] Review of attachment 9010132 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/flexbox/components/FlexItemSizingOutline.js @@ +18,5 @@ > + }; > + } > + > + renderBasisPoint(baseSize) { > + const isRow = this.props.flexDirection.startsWith("row"); Fixed. @@ +222,5 @@ > + let maxSize = 1; > + let minSize = 1; > + let deltaSize = 0; > + > + // Computed the relative size ratio to determine where to position the min, max, basis Fixed. ::: devtools/client/themes/layout.css @@ +161,5 @@ > /** > + * Flex Item Sizing Outline > + */ > + > +#flex-item-sizing-outline-container { Fixed. @@ +174,5 @@ > + > +#flex-item-sizing-outline.row { > + grid-auto-rows: 35px; > + margin-bottom: 28px; > + width: 200px; Fixed. Added flex-basis: 80% and max-width: 450px @@ +180,5 @@ > + > +#flex-item-sizing-outline.column { > + grid-auto-columns: 35px; > + margin: 16px 0; > + height: 140px; Nothing to do here. @@ +185,5 @@ > + > +} > + > +#flex-item-sizing-outline-item { > + border: 1px solid #A744FF; Fixed. @@ +195,5 @@ > + background-image: url(chrome://devtools/skin/images/lock.svg); > + background-size: 16px; > + background-repeat: no-repeat; > + display: inline-block; > + fill: #BD10E0; Fixed. @@ +201,5 @@ > + height: 16px; > + position: absolute; > + right: -8px; > + top: 8px; > + -moz-context-properties: fill; Fixed. @@ +205,5 @@ > + -moz-context-properties: fill; > +} > + > +#flex-item-sizing-outline-growth { > + border: 3px dotted rgba(167, 68, 255, 0.4); Fixed. @@ +211,5 @@ > +} > + > +#flex-item-sizing-outline-delta { > + background-repeat: round; > + fill: #A744FF; Fixed. @@ +231,5 @@ > +#flex-item-sizing-outline-delta.column.shrinking { > + background-image: url(chrome://devtools/skin/images/arrowhead-up.svg); > +} > + > +.point { Fixed. @@ +262,5 @@ > + position: relative; > + -moz-user-select: none; > +} > + > +.point-top .label::before { Fixed.

Comment on attachment 9011913 [details] [diff] [review] Part 15: Implement the flex item sizing outline in the flexbox panel. [3.0] Review of attachment 9011913 [details] [diff] [review]: ----------------------------------------------------------------- Code changes look great. Thank you for addressing my comments. Testing the code locally, I can still see some problems with max-width/height. In fact, for some reason, I can't get the padlock icon to appear anymore at all. Here's an example: <div class="container"> <div>item</div> <div>item</div> <div>item</div> </div> .container { display: flex; } .container > div { flex-basis: 300px; flex-grow: 1; max-width: 200px; } Now in that case, I would expect the icon and max point to be displayed at the right end of the outline, right? Can you help me get a use case where max-width appears in the outline?

Gabriel, as discussed yesterday, let's proceed as follows: - let's drop part 15 from this bug. I'll file a new bug to land the outline. - let's land part 19 as soon as you think it's ready for a new review. - let's resolve this bug.

Comment on attachment 9014469 [details] [diff] [review] Part 19: Show both the flex container and flex item sizing properties when the selected element is both a flex item and container. [3.0] Review of attachment 9014469 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/flexbox/components/Flexbox.js @@ +114,1 @@ > FlexContainerProperties({ I think we agreed not to show the flexContainer properties in the flex item accordion. So I guess we should return null if flexItemShown is true. ::: devtools/client/inspector/layout/components/LayoutApp.js @@ +98,5 @@ > }, > ]; > > if (Services.prefs.getBoolPref(FLEXBOX_ENABLED_PREF)) { > + items.splice(0, 0, { Please add a comment before this splice to explain what it does to the items array. @@ +117,5 @@ > + // an accordion with another Flexbox component where the flexbox to show is the > + // parent flex container of the current selected node. > + if (this.props.flexbox.flexItemContainer && > + this.props.flexbox.flexItemContainer.actorID) { > + items.splice(1, 0, { Same comment here, please explain what this splice does here. @@ +126,5 @@ > + // Don't show the flexbox highlighter toggle. > + showFlexboxHighlighterToggle: false, > + }, > + header: this.getFlexboxHeader(this.props.flexbox.flexItemContainer), > + opened: true, We also need a pref to store the state of this accordion item. I don't know if it makes sense to use 2 different prefs though. Do we want the item accordion to preserve its closed/opened state differently than the container accordion? Probably not. Otherwise navigating from a container to an item inside the accordion might get weird (e.g. navigating from the container might lead you to a closed accordion). So we should probably just use the same pref here too. Which means when you select an element that is both a container and an item, then you always have both accordion either open or close. Now, I have to say I'm still confused by the dual state of the first accordion item. I know we've talked about that, and the rest of the code changes do look good to me now. But I would find this file so much easier to read if we had 1 accordion for containers, and 1 accordion for items, instead of the first one being able to display both types, and therefore having a getFlexboxHeader that also needs to encapsulate this complex logic. We already know the state of things when we're rendering. So to me, it would seem feasible to just separate those things out. If the code were a bit like this: - is this an item (even if it's also a container) - if yes display the item accordion - otherwise don't - is this a container (even if it's also an item) - if yes display the container accordion, - otherwise don't but those are always 2 different accordions. Not reusing the same for 2 different things. I'm going to cancel the review based on this for now. But let's discuss this later and agree on what makes the most sense. ::: devtools/server/actors/layout.js @@ +278,5 @@ > + * A flex container node. > + * @return {FlexboxActor|null} The FlexboxActor of the flex container of the given node. > + * Otherwise, returns null. > + */ > + getAsFlexItem(node) { I don't think this method name is correct. The method doesn't return the passed node as a flex item. It instead returns its flex container actor. Why don't we add a second param to getCurrentFlexbox instead. Like this: getCurrentFlexbox(node, onlyLookAtParents) This would be a boolean that forces this function to only start looking from the parent. So this way even if the node is itself a container, it isn't returned, it skips it and looks for a parent container.

Comment on attachment 9014469 [details] [diff] [review] Part 19: Show both the flex container and flex item sizing properties when the selected element is both a flex item and container. [3.0] Review of attachment 9014469 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/flexbox/components/Flexbox.js @@ +114,1 @@ > FlexContainerProperties({ Fixed. ::: devtools/client/inspector/layout/components/LayoutApp.js @@ +98,5 @@ > }, > ]; > > if (Services.prefs.getBoolPref(FLEXBOX_ENABLED_PREF)) { > + items.splice(0, 0, { Fixed. @@ +117,5 @@ > + // an accordion with another Flexbox component where the flexbox to show is the > + // parent flex container of the current selected node. > + if (this.props.flexbox.flexItemContainer && > + this.props.flexbox.flexItemContainer.actorID) { > + items.splice(1, 0, { Fixed. @@ +126,5 @@ > + // Don't show the flexbox highlighter toggle. > + showFlexboxHighlighterToggle: false, > + }, > + header: this.getFlexboxHeader(this.props.flexbox.flexItemContainer), > + opened: true, To be discussed. ::: devtools/server/actors/layout.js @@ +278,5 @@ > + * A flex container node. > + * @return {FlexboxActor|null} The FlexboxActor of the flex container of the given node. > + * Otherwise, returns null. > + */ > + getAsFlexItem(node) { Fixed.

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9f1fd88190e3 Part 19: Show both the flex container and flex item sizing properties when the selected element is both a flex item and container. r=pbro

Backed out for failing devtools at evtools/client/inspector/markup/test/browser_markup_flex_display_badge.js Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending,running,success,testfailed,busted,exception&selectedJob=203728445&revision=9f1fd88190e39614a71b069403b92169f38afc8e Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=203728445&repo=mozilla-inbound&lineNumber=8370 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/62a73eefbaac08eb73951548547eeb2f0847fc9a

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1522c7633aa8 Part 19: Show both the flex container and flex item sizing properties when the selected element is both a flex item and container. r=pbro