Show the flex lines in the flexbox highlighter

RESOLVED FIXED in Firefox 59

Status

P3
normal
RESOLVED FIXED
11 months ago
6 months ago

People

(Reporter: gl, Assigned: mashuoyi111)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 59
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

11 months ago
Created attachment 8944076 [details] [diff] [review]
1431841.patch
Attachment #8944076 - Flags: review?(gl)
(Reporter)

Comment 2

11 months ago
Comment on attachment 8944076 [details] [diff] [review]
1431841.patch

Review of attachment 8944076 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/actors/highlighters/flexbox.js
@@ +31,1 @@
>      alpha: 1,

Global alpha actually defaults to 1. So, I think we can actually remove all the alpha and all the globalAlpha that we set.

@@ +31,3 @@
>      alpha: 1,
>    },
> +  "line": {

This won't be needed because of the above.

@@ +288,5 @@
>      this.ctx.stroke();
>      this.ctx.restore();
>    }
>  
> +  renderFlexContainerBorder() {

Move this function above renderFlexContainerFill. We actually order these functions alphabetically.

@@ +317,5 @@
> +
> +  renderFlexLines() {
> +    if (!this.currentQuads.content || !this.currentQuads.content[0]) {
> +      return;
> +    }

Add a new line after if statement blocks

@@ +319,5 @@
> +    if (!this.currentQuads.content || !this.currentQuads.content[0]) {
> +      return;
> +    }
> +    let container = this.currentNode;
> +    let containerStyle = getComputedStyle(container);

s/containerStyle/computedStyle

Just use this.currentNode instead of assigning it to "container.

@@ +321,5 @@
> +    }
> +    let container = this.currentNode;
> +    let containerStyle = getComputedStyle(container);
> +    let flexLines = container.getAsFlexContainer().getLines();
> +    if(!flexLines) {

Add a space after "if"

@@ +339,5 @@
> +    this.ctx.lineWidth = lineWidth;
> +    this.ctx.strokeStyle = DEFAULT_COLOR;
> +
> +    let { bounds } = this.currentQuads.content[0];
> +    let flexItems = this.currentNode.children;

This is never used.

@@ +341,5 @@
> +
> +    let { bounds } = this.currentQuads.content[0];
> +    let flexItems = this.currentNode.children;
> +
> +    if (containerStyle.getPropertyValue("flex-direction") === "column" || containerStyle.getPropertyValue("flex-direction") === "column-reverse") {

This is eslint error since it runs over 90 characters in a line.

@@ +342,5 @@
> +    let { bounds } = this.currentQuads.content[0];
> +    let flexItems = this.currentNode.children;
> +
> +    if (containerStyle.getPropertyValue("flex-direction") === "column" || containerStyle.getPropertyValue("flex-direction") === "column-reverse") {
> +      for (let flexLineItem of flexLines) {

s/flexLineItem/flexLine

@@ +343,5 @@
> +    let flexItems = this.currentNode.children;
> +
> +    if (containerStyle.getPropertyValue("flex-direction") === "column" || containerStyle.getPropertyValue("flex-direction") === "column-reverse") {
> +      for (let flexLineItem of flexLines) {
> +        clearRect(this.ctx, flexLineItem.crossStart, 0, flexLineItem.crossStart +  flexLineItem.crossSize, bounds.height, this.currentMatrix);

We can destructure flexLine and get the crossStart and crossSize.

const { crossStart, crossSize } = flexLine;

@@ +346,5 @@
> +      for (let flexLineItem of flexLines) {
> +        clearRect(this.ctx, flexLineItem.crossStart, 0, flexLineItem.crossStart +  flexLineItem.crossSize, bounds.height, this.currentMatrix);
> +
> +        drawRect(this.ctx, flexLineItem.crossStart, 0, flexLineItem.crossStart, bounds.height, this.currentMatrix);
> +        this.ctx.stroke();

I am wondering if we can just call stroke once. Double check for me.

@@ +351,5 @@
> +        drawRect(this.ctx, flexLineItem.crossStart +  flexLineItem.crossSize, 0, flexLineItem.crossStart +  flexLineItem.crossSize, bounds.height, this.currentMatrix);
> +        this.ctx.stroke();
> +       }
> +    } else {
> +      for (let flexLineItem of flexLines) {

We can probably just check the computedStyle.getPropertyValue("flex-direction") inside the for loop so we can just have one for loop

for (let flexLine of flexLines) {
 if (column) {
  ....
 } else {
  ....
 }
}

@@ +355,5 @@
> +      for (let flexLineItem of flexLines) {
> +        clearRect(this.ctx, 0,flexLineItem.crossStart, bounds.width, flexLineItem.crossStart +  flexLineItem.crossSize, this.currentMatrix);
> +
> +        drawRect(this.ctx, 0, flexLineItem.crossStart, bounds.width, flexLineItem.crossStart, this.currentMatrix);
> +        this.ctx.stroke();

Probably can just call stroke once() as above.

@@ +363,5 @@
> +    }
> +
> +    this.ctx.restore();
> +
> +    console.log(flexLines)

Remove this console.log
Attachment #8944076 - Flags: review?(gl)
(Assignee)

Comment 3

11 months ago
Created attachment 8944184 [details] [diff] [review]
1431841.patch
Attachment #8944184 - Flags: review?(gl)
(Assignee)

Updated

11 months ago
Attachment #8944076 - Attachment is obsolete: true

Comment 4

11 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82427fc508cd
Show the flex lines in the flexbox highlighter. r=gl
(Reporter)

Comment 5

11 months ago
Note to sheriff, I think the eslint errors in Bug 1431941.
(Reporter)

Updated

11 months ago
Attachment #8944184 - Flags: review?(gl) → review+

Comment 6

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/82427fc508cd
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59

Updated

6 months ago
Product: Firefox → DevTools
Blocks: 1470379
You need to log in before you can comment on or make changes to this bug.