1478397 - Add a "flex item sizing" panel in the Flexbox layout sidebar

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Patrick Brosset <:pbro>]

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.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: flex-item.diff

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 2

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Implement a FlexItemActor to retrieve information about the flex items.

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Implement a FlexItemActor to retrieve information about the flex items. [1.0]

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Display an ordered list of flex items when the flex container is highlighted. [1.0]

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Return the computed style properties and flex item sizing information from the FlexItemActor. [1.0]

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Display an ordered list of flex items when the flex container is highlighted. [2.0]

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Display an ordered list of flex items when the flex container is highlighted. [3.0]

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Return the computed style properties and flex item sizing information from the FlexItemActor. [2.0]

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Return the computed style properties and flex item sizing information from the FlexItemActor. [3.0]

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Return the computed style properties and flex item sizing information from the FlexItemActor. [4.0]

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Display an ordered list of flex items when the flex container is highlighted. [4.0]

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 0: Reorganize the flex container checkbox and flex container properties to match new designs. [1.0]

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Implement a FlexItemActor to retrieve information about the flex items. [2.0]

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Display an ordered list of flex items when the flex container is highlighted. [5.0]

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Return the computed style properties and flex item sizing information from the FlexItemActor [5.0]

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 4: Implement the methods to show the flex item highlighter from HighlightersOverlay. [1.0]

Comment 17

[Razvan Caliman [:rcaliman]]

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 18

[Razvan Caliman [:rcaliman]]

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 19

[Razvan Caliman [:rcaliman]]

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.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Andrei Ciure[:aciure]]

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

Comment 22

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 5: Display the flex item sizing properties in the flexbox panel. [1.0]

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 23

[Razvan Caliman [:rcaliman]]

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.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4dc5ede8f56
Part 5: Display the flex item sizing properties in the flexbox panel. r=rcaliman

Comment 25

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/b4dc5ede8f56

Comment 26

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 6: Refactor the styles in the layout panel. [1.0]

Comment 27

[Razvan Caliman [: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()

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60c2e9b522a4
Part 6: Refactor the styles in the layout panel. r=rcaliman

Comment 29

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/60c2e9b522a4

Comment 30

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 7: Add a flex item selector in the flexbox panel. [1.0]

Comment 31

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 8: Refactor the flex item shown to prevent it from being hidden on reflow updates. [1.0]

Comment 32

[Razvan Caliman [: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 33

[Gabriel Luong [:gl] (ΦωΦ)]

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.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
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

Comment 35

[Cristina Coroiu [:ccoroiu]]

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

Comment 36

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 9: Adjust the flexbox accordion header to match the design spec. [1.0]

Comment 37

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 10: Adjust the flexbox header to match the design spec. [1.0]

Comment 38

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 11: Add a prev button to navigate from the flex item sizing view to flex container view. [1.0]

Comment 39

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 12: Don't render the flex item list if there are no flex items. [1.0]

Comment 40

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 13: Show the flex item sizing properties of the currently selected element in the markup view. [1.0]

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

Comment 41

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 14: Selecting a flex item in the flex item selector should select that element in the markup view. [1.0]

Comment 42

[Razvan Caliman [:rcaliman]]

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 43

[Razvan Caliman [:rcaliman]]

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`.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65cb9952c79d
Part 9: Adjust the flexbox accordion header to match the design spec. r=rcaliman

Comment 45

[Pulsebot]

Pushed by [e-mail]:
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

Comment 46

[Pulsebot]

Backout by [e-mail]:
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

Comment 47

[Tiberius Oros[:tiberius_oros]]

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

Comment 48

[Pulsebot]

Pushed by [e-mail]:
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

Comment 49

[Arthur Iakab [arthur_iakab]]

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

Comment 50

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 15: Implement the flex item sizing outline in the flexbox panel. [1.0]

@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 51

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 15: Implement the flex item sizing outline in the flexbox panel. [2.0]

Comment 52

[Francesco Lodolo [:flod]]

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 53

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 16: Cache the custom host colors and the overlay color for the current host to avoid unnecessary fetching. [1.0]

Comment 54

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 17: Replace %s with %S for flexbox.flexItemOf string in layout.properties. [1.0]

Comment 55

[Patrick Brosset <: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]:
-----------------------------------------------------------------

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).

Comment 56

[Pulsebot]

Pushed by [e-mail]:
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

Comment 57

[Andreea Pavel [:apavel]]

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

Comment 58

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 18: Refactor the logic to get the flex items for a given flex container. [1.0]

Comment 59

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 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]

Comment 60

[Patrick Brosset <:pbro>]

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).

Comment 61

[Pulsebot]

Pushed by [e-mail]:
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 62

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e883cd321684

Comment 63

[Gabriel Luong [:gl] (ΦωΦ)]

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 64

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 15: Implement the flex item sizing outline in the flexbox panel. [3.0]

Comment 65

[Patrick Brosset <:pbro>]

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?

Comment 66

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 19: Show both the flex container and flex item sizing properties when the selected element is both a flex item and container. [2.0]

Comment 67

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 15: Implement the flex item sizing outline in the flexbox panel. [4.0]

Comment 68

[Patrick Brosset <:pbro>]

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 69

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 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]

Comment 70

[Patrick Brosset <:pbro>]

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 71

[Gabriel Luong [:gl] (ΦωΦ)]

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.

Comment 72

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 19: Show both the flex container and flex item sizing properties when the selected element is both a flex item and container. [4.0]

Comment 73

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 19: Show both the flex container and flex item sizing properties when the selected element is both a flex item and container. [5.0]

Comment 74

[Pulsebot]

Pushed by [e-mail]:
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

Comment 75

[Andreea Pavel [:apavel]]

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

Comment 76

[Pulsebot]

Pushed by [e-mail]:
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

Comment 77

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/1522c7633aa8