Closed Bug 1414362 Opened 2 years ago Closed 2 years ago

Outline the flex container and items in the flexbox highlighter

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

(Whiteboard: [designer-tools])

Attachments

(1 file)

No description provided.
Blocks: 1414275
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?
Flags: needinfo?(gl)
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.
Attachment #8926886 - Flags: review?(pbrosset)
Sorry, I had enabled the wrong pref.
Flags: needinfo?(gl)
Comment on attachment 8926886 [details]
Bug 1414362 - Outline the flex container and items in the flexbox highlighter.

https://reviewboard.mozilla.org/r/198136/#review204096
Attachment #8926886 - Flags: review?(pbrosset) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f2f2e48e66d
Outline the flex container and items in the flexbox highlighter. r=pbro
https://hg.mozilla.org/mozilla-central/rev/0f2f2e48e66d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
Blocks: 1470379
You need to log in before you can comment on or make changes to this bug.