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

RESOLVED FIXED in Firefox 64

Status

enhancement
P3
normal
RESOLVED FIXED
10 months ago
8 days ago

People

(Reporter: pbro, Assigned: gl)

Tracking

(Blocks 1 bug, Regressed 1 bug)

unspecified
Firefox 64
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(21 attachments, 17 obsolete attachments)

23.90 KB, patch
Details | Diff | Splinter Review
18.53 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
8.24 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
11.54 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
6.88 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
7.60 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
21.30 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
14.13 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
13.13 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
10.40 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
11.50 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
16.39 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
5.58 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
854 bytes, patch
rcaliman
: review+
Details | Diff | Splinter Review
7.69 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
1.71 KB, patch
rcaliman
: review+
Details | Diff | Splinter Review
7.69 KB, patch
pbro
: review+
Details | Diff | Splinter Review
1.23 KB, patch
pbro
: review+
Details | Diff | Splinter Review
5.88 KB, patch
pbro
: review+
Details | Diff | Splinter Review
19.04 KB, patch
Details | Diff | Splinter Review
45.43 KB, patch
pbro
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 months ago
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.
(Reporter)

Comment 1

10 months ago
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.
Flags: needinfo?(gl)
(Assignee)

Updated

10 months ago
Assignee: nobody → gl
Flags: needinfo?(gl)
(Assignee)

Updated

10 months ago
Status: NEW → ASSIGNED
Priority: -- → P3
(Assignee)

Updated

9 months ago
Attachment #9002317 - Flags: review?(rcaliman)
(Assignee)

Comment 3

9 months ago
Attachment #9002317 - Attachment is obsolete: true
Attachment #9002350 - Flags: review?(rcaliman)
(Assignee)

Updated

9 months ago
Depends on: 1484483
(Assignee)

Comment 6

9 months ago
Attachment #9002351 - Attachment is obsolete: true
Attachment #9002351 - Flags: review?(rcaliman)
Attachment #9002353 - Flags: review?(rcaliman)
(Assignee)

Comment 7

9 months ago
Attachment #9002353 - Attachment is obsolete: true
Attachment #9002353 - Flags: review?(rcaliman)
Attachment #9002354 - Flags: review?(rcaliman)
(Assignee)

Comment 8

9 months ago
Attachment #9002352 - Attachment is obsolete: true
Attachment #9002352 - Flags: review?(rcaliman)
Attachment #9002355 - Flags: review?(rcaliman)
(Assignee)

Comment 9

9 months ago
Attachment #9002355 - Attachment is obsolete: true
Attachment #9002355 - Flags: review?(rcaliman)
Attachment #9002357 - Flags: review?(rcaliman)
(Assignee)

Comment 10

9 months ago
Attachment #9002357 - Attachment is obsolete: true
Attachment #9002357 - Flags: review?(rcaliman)
Attachment #9002368 - Flags: review?(rcaliman)
(Assignee)

Comment 11

9 months ago
Attachment #9002354 - Attachment is obsolete: true
Attachment #9002354 - Flags: review?(rcaliman)
Attachment #9003364 - Flags: review?(rcaliman)
(Assignee)

Updated

9 months ago
Attachment #9002350 - Flags: review?(rcaliman)
(Assignee)

Updated

9 months ago
Attachment #9002368 - Flags: review?(rcaliman)
(Assignee)

Updated

9 months ago
Attachment #9003364 - Flags: review?(rcaliman)
(Assignee)

Updated

9 months ago
Depends on: 1477614
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.
Attachment #9004119 - Flags: review?(rcaliman) → review+
Attachment #9004120 - Flags: review?(rcaliman) → review+
Attachment #9004121 - Flags: review?(rcaliman) → review+
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}`],
```
Attachment #9004123 - Flags: review?(rcaliman) → review+
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.
Attachment #9004124 - Flags: review?(rcaliman) → review+
(Assignee)

Updated

9 months ago
Keywords: leave-open

Comment 20

9 months ago
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
(Assignee)

Comment 22

9 months ago
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.
Attachment #9004754 - Flags: review?(rcaliman)
(Assignee)

Updated

9 months ago
Attachment #9004754 - Attachment description: art 5: Display the flex item sizing properties in the flexbox panel. [1.0] → Part 5: Display the flex item sizing properties in the flexbox panel. [1.0]
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.
Attachment #9004754 - Flags: review?(rcaliman) → review+

Comment 24

9 months ago
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()
Attachment #9006323 - Flags: review?(rcaliman) → review+
(Assignee)

Updated

9 months ago
Attachment #9006323 - Attachment description: Bug 1478397 - Part 6: Refactor the styles in the layout panel. [1.0] → Part 6: Refactor the styles in the layout panel. [1.0]

Comment 28

9 months ago
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
(Assignee)

Updated

9 months ago
Blocks: 1488885
(Assignee)

Updated

9 months ago
Blocks: 1488883
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?
Attachment #9006984 - Flags: review?(rcaliman) → review+
Attachment #9007280 - Flags: review?(rcaliman) → review+
(Assignee)

Comment 33

9 months ago
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

9 months ago
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
(Assignee)

Comment 40

8 months ago
If the current selected element in the markup view is a flex item, we will display its flex item sizing properties.
Attachment #9009825 - Flags: review?(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.
Attachment #9009672 - Flags: review?(rcaliman) → review+
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`.
Attachment #9009733 - Flags: review?(rcaliman) → review+
Attachment #9009775 - Flags: review?(rcaliman) → review+
Attachment #9009823 - Flags: review?(rcaliman) → review+
Attachment #9009825 - Flags: review?(rcaliman) → review+
Attachment #9009829 - Flags: review?(rcaliman) → review+

Comment 44

8 months ago
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

Comment 45

8 months ago
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

Comment 46

8 months ago
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
(Assignee)

Updated

8 months ago
Flags: needinfo?(gl)

Comment 48

8 months ago
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
(Assignee)

Comment 50

8 months ago
@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.
Flags: needinfo?(pbrosset)
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
(Assignee)

Updated

8 months ago
Attachment #9010132 - Flags: review?(pbrosset)
(Reporter)

Updated

8 months ago
Attachment #9010278 - Flags: review?(pbrosset) → review+
(Assignee)

Updated

8 months ago
Flags: needinfo?(pbrosset)
(Reporter)

Updated

8 months ago
Attachment #9010294 - Flags: review?(pbrosset) → review+
(Reporter)

Comment 55

8 months ago
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).
Attachment #9010132 - Flags: review?(pbrosset)

Comment 56

8 months ago
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
(Assignee)

Updated

8 months ago
Attachment #9009829 - Flags: checkin+
(Assignee)

Updated

8 months ago
Attachment #9010278 - Flags: checkin+
(Assignee)

Updated

8 months ago
Attachment #9010294 - Flags: checkin+
(Reporter)

Updated

8 months ago
Attachment #9011309 - Flags: review?(pbrosset) → review+
(Reporter)

Comment 60

8 months ago
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).
Attachment #9011310 - Flags: review?(pbrosset)

Comment 61

8 months ago
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
(Assignee)

Updated

8 months ago
Attachment #9011309 - Flags: checkin+
(Assignee)

Comment 63

8 months ago
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.
(Assignee)

Comment 64

8 months ago
Attachment #9010132 - Attachment is obsolete: true
Attachment #9011913 - Flags: review?(pbrosset)
(Reporter)

Comment 65

8 months ago
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?
Attachment #9011913 - Flags: review?(pbrosset)
(Reporter)

Updated

8 months ago
Blocks: 1495717
(Reporter)

Comment 68

8 months ago
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.
(Reporter)

Updated

8 months ago
Blocks: 1496439
(Assignee)

Updated

8 months ago
Keywords: leave-open
(Reporter)

Comment 70

8 months ago
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.
Attachment #9014469 - Flags: review?(pbrosset)
(Assignee)

Comment 71

8 months ago
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.
(Reporter)

Updated

8 months ago
Attachment #9014887 - Flags: review?(pbrosset) → review+

Comment 74

8 months ago
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
(Assignee)

Updated

8 months ago
Flags: needinfo?(gl)

Comment 76

8 months ago
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

Comment 77

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1522c7633aa8
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
(Reporter)

Updated

7 months ago
No longer blocks: 1488885
(Reporter)

Updated

6 months ago
No longer depends on: 1477614
Regressions: 1551114
You need to log in before you can comment on or make changes to this bug.