1414362 - Outline the flex container and items in the flexbox highlighter

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Comment 1

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

Attachment: Bug 1414362 - Outline the flex container and items in the flexbox highlighter.

Review commit: https://reviewboard.mozilla.org/r/198136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198136/

Comment 2

[Patrick Brosset <:pbro>]

Gabriel, I pulled this from mozreview and also applied the patches from bug 1414275 to test this, but couldn't get it to work. I couldn't see anything in the ruleview for display:flex elements. What is the best way for me to test this locally?

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8926886 [details]
Bug 1414362 - Outline the flex container and items in the flexbox highlighter.

https://reviewboard.mozilla.org/r/198136/#review203714

`_hasMoved` will probably need to be overridden in this class too, so that we can react to alignment and size changes (currently the highlighter does not).
This can lead to weird cases where the container resizes fine, but not the items. See this for instance: https://www.dropbox.com/s/obciw6vepdmtc9y/inline-flex-wrapping.png?dl=0

::: devtools/server/actors/highlighters/flexbox.js:25
(Diff revision 1)
> +  getAdjustedQuads,
> +  getDisplayPixelRatio,
>    setIgnoreLayoutChanges,
>  } = require("devtools/shared/layout/utils");
>  
> +const FLEXBOX_DEFAULT_COLOR = "#9400FF";

Let's take the opportunity to move this to a shared place so the grid highlighter can also use it. We try very hard to define colors in css variables for css, so it would be a pity if we started duplicating colors in JS now.

::: devtools/server/actors/highlighters/flexbox.js:202
(Diff revision 1)
> +    this.ctx.lineWidth = lineWidth;
> +    this.ctx.strokeStyle = FLEXBOX_DEFAULT_COLOR;
> +
> +    let { bounds } = this.currentQuads.content[0];
> +
> +    drawRect(this.ctx, 0, 0, bounds.right - bounds.left, bounds.bottom - bounds.top,

Can't we just use `bounds.width` and `bounds.height`?

::: devtools/server/actors/highlighters/flexbox.js:229
(Diff revision 1)
> +    this.ctx.globalAlpha = FLEXBOX_LINES_PROPERTIES.item.alpha;
> +    this.ctx.lineWidth = lineWidth;
> +    this.ctx.strokeStyle = FLEXBOX_DEFAULT_COLOR;
> +
> +    let { bounds } = this.currentQuads.content[0];
> +    let flexItems = this.currentNode.children;

can you add a TODO comment explaining that this will soon come from a platform API instead, because right now this doesn't work in all cases.

::: devtools/server/actors/highlighters/flexbox.js:232
(Diff revision 1)
> +
> +    let { bounds } = this.currentQuads.content[0];
> +    let flexItems = this.currentNode.children;
> +
> +    for (let flexItem of flexItems) {
> +      let quads = getAdjustedQuads(this.win, flexItem, "content");

And in fact, the platform API should return the "offset" for each item (much like we do for grid) so we don't have to cacuate geometry again here. Also, some items are not element nodes, so it may not always be possible for us to access the quads.

Comment 4

[Patrick Brosset <:pbro>]

Sorry, I had enabled the wrong pref.

Comment 5

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

Comment on attachment 8926886 [details]
Bug 1414362 - Outline the flex container and items in the flexbox highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198136/diff/1-2/

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8926886 [details]
Bug 1414362 - Outline the flex container and items in the flexbox highlighter.

https://reviewboard.mozilla.org/r/198136/#review204096

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2f2e48e66d
Outline the flex container and items in the flexbox highlighter. r=pbro

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/0f2f2e48e66d